From 8da7c96c41257718a00f78b1546fdf85ddbc51a9 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Wed, 14 Oct 2026 14:47:33 -0700 Subject: [PATCH 1/2] refactor(config): fold duplicate flag spellings into single parse paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The exec/run/-p trio, the resume family, the yolo spellings, and the telemetry kill-switches each carried parallel handling for one operator knob apiece; internal alias maps now own the surface (--pick/--list stay flow-specific errors everywhere else). OAuth provider/model selection collapses three projection helpers into one ordered chain at loadConfig; OTEL environment overrides now win wholesale over settings resource attributes — including a set-but-empty value, which races nothing in from settings (documented in PERFTRACE). topP is documented deprecated-but-honored next to temperature. The always-overridden defaults turned out to be live fallbacks or single definitions and stay, recorded. Implementation documents the settings precedence; perfectrace documents the OTEL rule. --- docs/IMPLEMENTATION.md | 44 +++++----- docs/PERFTRACE.md | 6 +- src/config.test.ts | 15 ++++ src/config/index.ts | 137 ++++++++++++++++++++----------- src/config/oauth-catalog.test.ts | 69 ++++++++++++---- src/config/settings.ts | 9 +- src/perf/otel-config.test.ts | 49 ++++++++++- src/perf/otel-config.ts | 41 +++++---- src/telemetry/index.ts | 7 +- 9 files changed, 264 insertions(+), 113 deletions(-) diff --git a/docs/IMPLEMENTATION.md b/docs/IMPLEMENTATION.md index c14f7dc4f..5a7ec7d5d 100644 --- a/docs/IMPLEMENTATION.md +++ b/docs/IMPLEMENTATION.md @@ -174,7 +174,7 @@ Intent defaults: `intent=implement` → director `coder`; `explore` → `explore ### Auto Mode -Auto mode defaults **on** (`config.auto = true` from `loadConfig`; pass `--no-auto` to start off, or `--auto` to force on). It is toggled only via those CLI flags — there is currently no in-session key bound to it. The permission gate reads the flag (`getAuto`/`setAuto` in `src/permission/gate.ts`) on the next tool call. `--dangerously-skip-permissions` and its `--yolo` alias force skip-permissions for this process only; skip-permissions wins when combined with auto, so `--auto --yolo` runs in yolo mode. `/yolo [on|off|toggle]` (bare `/yolo` also toggles) persists the skip-permissions setting to the active settings file and wires `getSkipPermissions`/`setSkipPermissions` so the gate and pre-gate sandboxes honor the change on the next tool call without rebuilding plugins. The active source defaults to the user-global `~/.corbits/settings.json`, making that default machine-wide. Explicit `--config ` selects that file instead, so `/yolo` writes the custom file; a later ordinary TUI launch without the same `--config` uses the user-global source and does not modify the custom file. Secret-guard and authz still apply — and the active file itself is runtime-denylisted for path-keyed tools, reads and writes, even under skip-permissions, so the persisted skip cannot be silently leveraged; the startup notice remains as disclosure, not the enforcement. `loadConfig` tracks `skipPermissionsFromSettings` (true only when the effective value came from persisted settings, not either CLI flag) so `runTUI` can show a startup notice and `exec` can print an equivalent stderr warning for the otherwise-silent persisted setting. +Auto mode defaults **on** (`config.auto = true` from `loadConfig`; pass `--no-auto` to start off, or `--auto` to force on). It is toggled only via those CLI flags — there is currently no in-session key bound to it. `--auto` remains an explicit re-assertion of that default instead of a pruned no-op flag (CL-10672 default decision: auto stays on; the flag is kept because sequence order is meaningful — `--no-auto --auto` re-enables). The permission gate reads the flag (`getAuto`/`setAuto` in `src/permission/gate.ts`) on the next tool call. `--dangerously-skip-permissions` and its `--yolo` alias force skip-permissions for this process only; skip-permissions wins when combined with auto, so `--auto --yolo` runs in yolo mode. `/yolo [on|off|toggle]` (bare `/yolo` also toggles) persists the skip-permissions setting to the active settings file and wires `getSkipPermissions`/`setSkipPermissions` so the gate and pre-gate sandboxes honor the change on the next tool call without rebuilding plugins. The active source defaults to the user-global `~/.corbits/settings.json`, making that default machine-wide. Explicit `--config ` selects that file instead, so `/yolo` writes the custom file; a later ordinary TUI launch without the same `--config` uses the user-global source and does not modify the custom file. Secret-guard and authz still apply — and the active file itself is runtime-denylisted for path-keyed tools, reads and writes, even under skip-permissions, so the persisted skip cannot be silently leveraged; the startup notice remains as disclosure, not the enforcement. `loadConfig` tracks `skipPermissionsFromSettings` (true only when the effective value came from persisted settings, not either CLI flag) so `runTUI` can show a startup notice and `exec` can print an equivalent stderr warning for the otherwise-silent persisted setting. When auto is on, the gate auto-allows workspace file tools in `AUTO_ALLOWED_TOOLS` — which also includes `web_fetch` and `web_search` — and any `run_shell` that does not match the auto-shell policy. With auto off, `web_fetch` and `web_search` still ask. The policy (`autoShellRuleForCall` / `AUTO_SHELL_RULES` in `src/permission/auto-shell-policy.ts`) peels wrappers via `expandShellSubjects` (`bash`/`sh`/`zsh -c`, `xargs`, transparent prefixes), then applies: @@ -398,6 +398,8 @@ Profiles supply per-project or named-profile overrides for `model`, `systemPromp Providers and credentials are read exclusively from settings files: the global `~/.corbits/settings.json` (definitions + credentials) and the per-repo `.corbits/settings.json` (selection only). There are no `OPENAI_COMPATIBLE_*` environment-variable overrides, and `index.ts` does not load `.env` files — a deliberately stale or exported key can no longer shadow the configured provider. +**Sampling knobs.** `temperature` (0..2) is the canonical per-provider sampling knob. `topP` (0..1) is a deprecated but honored spelling kept for existing files: it loads and flows to the wire as `top_p`, and the two are mutually exclusive — when both are set, load fails; never both are sent (CL-10672 marks `topP` for deprecation pacing; removal would be a breaking rename and has not happened). + **Models-first connect.** There is no standalone `/login` command. `/model` opens on a flat **models-only** list (Recent, Favorites, then connected provider/model rows) built by `buildModelsFirstList` (`src/tui/model-picker.ts`); type-to-filter owns printable keys. Selecting a row runs `applyLiveModelSwitch` (`src/session/live-model-switch.ts`) so inference sources, permission-gate identity, grant persistence identity, and advertised tool schemas cut over together. **Alt+A** or `/connect` opens Connect via `addProviderSelectorChoices` (`src/tui/provider-setup.ts`), which lists every first-class kind including Custom — never bare `c` / Ctrl+A, and never in-list “connect →” rows. First-class API-key rows use a named-instance + auth-only form (instance name, key; catalog base URL is display-only); Custom keeps the full manual form. **Alt+F** toggles favorites; recent/favorite pairs live in global settings (`recentModels` / `favoriteModels`). **Alt+D** sets the default via `setDefaultModel` (global `defaultProvider` + that provider's `defaultModel`) plus `persistConnectedSelection` without switching the live session. First-class providers ship from `packages/first-class-providers` (corbits-agnostic defs) and `packages/opencode-go` (Go catalog, auth validate, multi-protocol endpoints, usage). OAuth providers open the existing browser login modal with a named account step; API-key providers share the same multi-instance naming and pre-seed models on save so selection works without restart. Both OAuth and API-key (including Custom) connects share `persistConnectedSelection` in `provider-setup-submit.ts` so project-local provider/model selection is written alongside global credentials. OpenCode Go forces `OPENCODE_GO_BASE_URL` when `opencodeGo` is set so subscription traffic is not billed as Zen PAYG. ### Inference authentication recovery @@ -423,26 +425,26 @@ This release changes no settings, OAuth-store, or session persistence format and Printed by `corbits --help` / `-h` from `CLI_HELP_TEXT` in `src/config/index.ts` (that constant is the source of truth; keep this table in sync when flags change). -| Verb / Flag | Default | Description | -| ------------------------------------------ | -------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| _(no verb)_ | — | Interactive session; optional trailing task text | -| `exec` / `run` / `-p` | — | Run a prompt (non-interactive / one-shot). `-p` is the same path as `exec` and may appear in any flag position. | -| `resume` / `continue` | — | Open the session picker for this folder (project-keyed to this checkout's git toplevel). Lists the 10 most recently persisted sessions, completed included. Type to filter. | -| `--resume []` | — | Interactive: open the session picker, or reopen a specific session when an id is given. With `exec` / `-p` the id is required, that session is continued, and the new prompt is sent (no picker). | -| `resume ` | — | Reopen a specific session in the TUI (does not auto-send a prompt) | -| `exec --resume ` | — | Headless: load that session and send ``, then exit. Missing or unreadable ids error and do not create a session. `--resume` without an id errors. | -| `-p --resume ` | — | Same headless continue path as `exec --resume` | -| `resume --pick` / `--list` | — | Interactive session picker | -| `--cwd ` | `process.cwd()` | Working directory | -| `--config ` | `~/.corbits/settings.json` | Active settings file for provider definitions and TUI persistence such as `/yolo`; composes with (does not exclude) home-level codex/xai OAuth credentials | -| `--provider ` | from settings | Select a configured provider | -| `--model ` | provider default | Select a model for the active provider | -| `--profile ` | — | Settings profile | -| `--dangerously-skip-permissions`, `--yolo` | false | Process-only aliases that auto-allow anything not denied by the authorization layer (gate + pre-gate workspace sandboxes; secret-guard / authz hard denies remain). `/yolo [on\|off\|toggle]` persists to the active settings file instead. | -| `--auto` | true (default) | Force auto mode on (workspace writes + unconstrained shell without prompts) | -| `--no-auto` | false | Start with auto mode off (ask on every consequential action); no in-session key toggles it | -| `--help`, `-h` | — | Show help (exit 0 via `CliHelpError`) | -| `--version`, `-V` | — | Print `corbits v` plus the running `package.json` version (exit 0 via `CliVersionError`) | +| Verb / Flag | Default | Description | +| ------------------------------------------ | -------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| _(no verb)_ | — | Interactive session; optional trailing task text | +| `exec` / `run` / `-p` | — | Run a prompt (non-interactive / one-shot). `exec` is the canonical verb (`run` / `-p` are kept aliases); `-p` is the same path and may appear in any flag position. | +| `resume` / `continue` | — | Open the session picker for this folder (project-keyed to this checkout's git toplevel). Lists the 10 most recently persisted sessions, completed included. Type to filter. `resume` is the canonical continue verb (`continue` is a kept alias). | +| `--resume []` | — | Interactive: open the session picker, or reopen a specific session when an id is given. With `exec` / `-p` the id is required, that session is continued, and the new prompt is sent (no picker). | +| `resume ` | — | Reopen a specific session in the TUI (does not auto-send a prompt) | +| `exec --resume ` | — | Headless: load that session and send ``, then exit. Missing or unreadable ids error and do not create a session. `--resume` without an id errors. | +| `-p --resume ` | — | Same headless continue path as `exec --resume` | +| `resume --pick` / `--list` | — | Interactive session picker (kept aliases of the bare `--resume` picker flow) | +| `--cwd ` | `process.cwd()` | Working directory | +| `--config ` | `~/.corbits/settings.json` | Active settings file for provider definitions and TUI persistence such as `/yolo`; composes with (does not exclude) home-level codex/xai OAuth credentials | +| `--provider ` | from settings | Select a configured provider | +| `--model ` | provider default | Select a model for the active provider | +| `--profile ` | — | Settings profile | +| `--dangerously-skip-permissions`, `--yolo` | false | Process-only aliases that auto-allow anything not denied by the authorization layer (gate + pre-gate workspace sandboxes; secret-guard / authz hard denies remain). `/yolo [on\|off\|toggle]` persists to the active settings file instead. | +| `--auto` | true (default) | Force auto mode on (workspace writes + unconstrained shell without prompts) | +| `--no-auto` | false | Start with auto mode off (ask on every consequential action); no in-session key toggles it | +| `--help`, `-h` | — | Show help (exit 0 via `CliHelpError`) | +| `--version`, `-V` | — | Print `corbits v` plus the running `package.json` version (exit 0 via `CliVersionError`) | Positional arguments after flags are joined into the optional initial task delivered when the TUI mounts. With no positional task, the operator starts from an empty prompt. diff --git a/docs/PERFTRACE.md b/docs/PERFTRACE.md index 984ace052..273f6e0e8 100644 --- a/docs/PERFTRACE.md +++ b/docs/PERFTRACE.md @@ -66,7 +66,11 @@ Precedence: `resourceAttributes["service.name"]` > `corbits-code`. After merge, `resourceAttributes["service.name"]` is always set to the resolved name so the two never diverge. -- **resourceAttributes:** settings merged with env; env wins on key conflict +- **resourceAttributes:** env list replaces settings wholesale when + `OTEL_RESOURCE_ATTRIBUTES` is set (no per-key merge); otherwise settings + apply alone. Set-but-empty counts as set: `OTEL_RESOURCE_ATTRIBUTES=""` + replaces the settings attributes with nothing (and does not re-apply the + settings map) — the same set-but-empty rule as `OTEL_EXPORTER_OTLP_HEADERS`. - **enabling export:** setting an endpoint (settings or env) enables export; `otel.enabled` need not be set. Service name or resource attributes alone do not enable export. diff --git a/src/config.test.ts b/src/config.test.ts index 06b3bb79a..9842ab4bf 100644 --- a/src/config.test.ts +++ b/src/config.test.ts @@ -572,6 +572,10 @@ describe("loadConfig", () => { { argv: ["resume", "--pick"] }, { argv: ["resume"] }, { argv: ["continue", "--list"] }, + // CL-10672: --pick/--list fold into the single --resume picker verb; both + // aliases remain accepted in any resume-flow position. + { argv: ["resume", "--list"] }, + { argv: ["resume", "--pick", "--list"] }, ] as const)("$argv opens the session picker", async ({ argv }) => { const cwd = await emptyCwd(); const config = await loadFor(cwd, argv, { home: await tempHome() }); @@ -581,6 +585,17 @@ describe("loadConfig", () => { expect(config.skipInitialTask).toBe(true); }); + test("CL-10672 keeps --pick/--list out of flow as unrecognized flags", async () => { + // Without the resume verb these spellings were never recognized; the + // alias fold preserves that fallthrough instead of quietly opening a picker. + await expect( + loadConfig(["--pick"], { globalSettingsPath: NO_SETTINGS }), + ).rejects.toThrow("unrecognized flag: --pick"); + await expect( + loadConfig(["--list"], { globalSettingsPath: NO_SETTINGS }), + ).rejects.toThrow("unrecognized flag: --list"); + }); + test("resume reopens a known session and skips the initial task", async () => { const cwd = await emptyCwd(); const home = await tempHome(); diff --git a/src/config/index.ts b/src/config/index.ts index 6d14de5a5..aad457354 100644 --- a/src/config/index.ts +++ b/src/config/index.ts @@ -109,7 +109,9 @@ import { resolveProfile } from "./profiles.js"; // Per-call token ceiling for inference sources, shared by agent creation // (runner.ts) and live provider switching (the /agent modal) so a live switch -// cannot silently revert it. +// cannot silently revert it. CL-10672 review: kept as the shared fallback — +// it is NOT an always-overridden default; ProviderSettings.maxTokens is the +// optional override and an unset value resolves to this ceiling. export const SOURCE_MAX_TOKENS = 16384; // Credential-cell placeholder for keyless local providers (e.g. Ollama). The @@ -160,8 +162,9 @@ function applyPersistedOAuthDefaults( // OAuth entries in settings.json carry no credentials; they work only while a // matching auth-store profile exists. Drop orphans in memory so a removed -// profile does not pin resolution to an unauthenticatable provider. -export function dropOrphanedOAuthEntries( +// profile does not pin resolution to an unauthenticatable provider. Called +// only from applyOAuthProviderProjections (plus its tests). +function dropOrphanedOAuthEntries( settings: Settings | null, projected: Record, ): Settings | null { @@ -197,8 +200,8 @@ function isHandNamedProviderEntry( } // Overlay live OAuth profile projections onto settings for runtime provider -// resolution. Exported for tests; loadConfig is the only production caller. -export function overlayOAuthProjections( +// resolution. Called only from applyOAuthProviderProjections (plus its tests). +function overlayOAuthProjections( settings: Settings | null, projected: Record, ): Settings | null { @@ -213,6 +216,28 @@ export function overlayOAuthProjections( }; } +// The one ordered OAuth projection chain for runtime provider resolution, +// folding the three dedicated helpers at the layer that owns them +// (loadConfig): (1) persisted defaultModel defaults merge into the live +// projections, (2) credential-less orphan entries are dropped from settings +// (only when the projections came from the home auth stores), (3) the +// projections overlay onto the surviving settings without overwriting +// hand-named API-key rows. Exposed for tests. +export function applyOAuthProviderProjections( + settings: Settings | null, + projectedProviderSettings: Record, + options: { dropOrphans: boolean }, +): Settings | null { + const projected = applyPersistedOAuthDefaults( + settings, + projectedProviderSettings, + ); + const liveSettings = options.dropOrphans + ? dropOrphanedOAuthEntries(settings, projected) + : settings; + return overlayOAuthProjections(liveSettings, projected); +} + function hasExaEntry(servers: MCPServerSettingsEntry[] | undefined): boolean { return ( servers?.some((server) => server.name === EXA_MCP_SERVER_NAME) === true @@ -705,6 +730,15 @@ export interface UnconfiguredConfig { settingsDiagnostics?: SettingsLoadDiagnostic[]; } +// Skip-permissions spellings. `--dangerously-skip-permissions` is the +// canonical process-only flag; `--yolo` is a kept alias (`/yolo` is the +// persistent settings writer — a separate surface). One flag, one setting +// (`dangerouslySkipPermissions`); the parse consults one alias set. +const SKIP_PERMISSIONS_FLAGS: ReadonlySet = new Set([ + "--dangerously-skip-permissions", + "--yolo", +]); + /** Printed for `corbits --help` / `-h`. Keep in sync with docs/IMPLEMENTATION.md. */ export const CLI_HELP_TEXT = `corbits — coding agent CLI @@ -833,20 +867,21 @@ export async function loadConfig( let resumeMode: "id" | "pick" | undefined; let resumeSessionId: string | undefined; const leading = args[0]; + // One exec verb: `exec` is canonical; `run` is a kept alias (help names + // both). One resume verb: `resume` is canonical; `continue` is a kept + // alias of the same flow. if (leading === "exec" || leading === "run") { command = "exec"; args.shift(); } else if (leading === "resume" || leading === "continue") { - command = "tui"; args.shift(); // Bare resume opens the list; a session id is the only direct-resume path. // Invalid non-flag positionals error instead of falling through (a // free-form token would otherwise become task while skipInitialTask is set). + // A following --pick/--list falls through to the loop's single resume + // branch, where it is a kept alias of the bare --resume picker flow. const next = args.slice()[0]; - if (next === "--pick" || next === "--list") { - resumeMode = "pick"; - args.shift(); - } else if (next !== undefined && !isFlagToken(next)) { + if (next !== undefined && !isFlagToken(next)) { if (!isSessionId(next)) { throw new Error( `'${next}' is not a session id. Use a UUID session id or \`corbits resume\` to choose.`, @@ -860,9 +895,10 @@ export async function loadConfig( } } - // `-p` is the exec one-shot path in any position (`-p --provider …`, + // `-p` is the exec one-shot alias in any position (`-p --provider …`, // `--model … -p …`). Detect it before value flags so exec-only options - // that appear before `-p` still see exec mode. + // that appear before `-p` still see exec mode; the loop keeps the token + // out of the task positionals. if (args.includes("-p")) { if (leading === "resume" || leading === "continue") { throw new Error( @@ -934,11 +970,13 @@ export async function loadConfig( continue; } - if (arg === "--dangerously-skip-permissions" || arg === "--yolo") { + if (SKIP_PERMISSIONS_FLAGS.has(arg)) { dangerouslySkipPermissions = true; continue; } if (arg === "--auto") { + // Explicit re-assertion of the on-by-default auto mode. Kept because + // sequence order still matters (`--no-auto --auto` re-enables). auto = true; continue; } @@ -947,41 +985,45 @@ export async function loadConfig( continue; } if (arg === "-p") { - command = "exec"; + // `-p` alias of exec: the pre-scan already fixed command="exec"; the + // token must stay out of the task positionals. continue; } - if (arg === "--resume") { - if (resumeMode === "id") { - throw new Error("cannot combine a session id with --resume"); - } - // Optional session id on the interactive path: `corbits --resume ` - // reopens that session (alias for `corbits resume `); bare - // `--resume` opens the picker. Exec / `-p` require the id — headless has - // no picker. A non-id token errors instead of leaking into task text. - const next = args[i + 1]; - if (next !== undefined && !isFlagToken(next)) { - if (!isSessionId(next)) { + if (arg === "--resume" || arg === "--pick" || arg === "--list") { + // One resume verb: `--resume` carries the optional session id (exec + // requires it; interactive bare `--resume` opens the picker, the alias + // for `resume `). `--pick`/`--list` are kept aliases of that + // bare picker flow and may not follow a session id. + if (arg === "--resume") { + if (resumeMode === "id") { + throw new Error("cannot combine a session id with --resume"); + } + const next = args[i + 1]; + if (next !== undefined && !isFlagToken(next)) { + if (!isSessionId(next)) { + throw new Error( + `'${next}' is not a session id. Use a UUID session id or \`corbits resume\` to choose.`, + ); + } + resumeMode = "id"; + resumeSessionId = next; + i++; + } else if (command === "exec") { throw new Error( - `'${next}' is not a session id. Use a UUID session id or \`corbits resume\` to choose.`, + `--resume requires a session id in exec mode. Use \`${COMMAND_NAME} resume\` to choose a session.`, ); + } else { + resumeMode = "pick"; } - resumeMode = "id"; - resumeSessionId = next; - i++; - } else if (command === "exec") { - throw new Error( - `--resume requires a session id in exec mode. Use \`${COMMAND_NAME} resume\` to choose a session.`, - ); - } else { - resumeMode = "pick"; - } - continue; - } - if ((arg === "--pick" || arg === "--list") && resumeMode !== undefined) { - if (resumeMode === "id") { + } else if (resumeMode === "id") { throw new Error("cannot combine a session id with --pick/--list"); + } else if (resumeMode !== undefined) { + resumeMode = "pick"; + } else { + // Without the resume verb these spellings are not in flow; preserve + // the previous fallthrough as an unrecognized flag. + throw new Error(`unrecognized flag: ${arg}`); } - resumeMode = "pick"; continue; } if (arg.startsWith("--")) { @@ -1017,7 +1059,7 @@ export async function loadConfig( useOAuthProfiles ? await Promise.all([listCodexProfiles(), listXaiProfiles()]) : [[], []]; - let projectedOAuthProviders = { + const projectedOAuthProviders = { ...codexProvidersAsSettings(codexProfiles), ...xaiProvidersAsSettings(xaiProfiles), }; @@ -1047,16 +1089,11 @@ export async function loadConfig( settings?.dangerouslySkipPermissions === true; dangerouslySkipPermissions = dangerouslySkipPermissions || settings?.dangerouslySkipPermissions === true; - projectedOAuthProviders = applyPersistedOAuthDefaults( + // Single ordered projection chain (see applyOAuthProviderProjections). + const settingsForResolution: Settings | null = applyOAuthProviderProjections( settings, projectedOAuthProviders, - ); - const liveSettings = useOAuthProfiles - ? dropOrphanedOAuthEntries(settings, projectedOAuthProviders) - : settings; - const settingsForResolution: Settings | null = overlayOAuthProjections( - liveSettings, - projectedOAuthProviders, + { dropOrphans: useOAuthProfiles }, ); // The per-repo selection file still applies on top of --config: that file diff --git a/src/config/oauth-catalog.test.ts b/src/config/oauth-catalog.test.ts index 74d280703..f94ce5ea1 100644 --- a/src/config/oauth-catalog.test.ts +++ b/src/config/oauth-catalog.test.ts @@ -8,9 +8,8 @@ import { codexProvidersAsSettings, } from "./codex-providers.js"; import { - dropOrphanedOAuthEntries, + applyOAuthProviderProjections, mergeOAuthCatalog, - overlayOAuthProjections, providerCatalogToSettings, runtimeSettingsWithCatalog, } from "./index.js"; @@ -141,34 +140,38 @@ describe("CL-6728: OAuth projections do not overwrite hand-named provider entrie const liveProjected = () => codexProvidersAsSettings([liveMine]); const liveCatalog = () => codexProfilesToCatalogEntries([liveMine]); - test("overlayOAuthProjections keeps a hand-named codex/ API-key entry", () => { - const overlaid = overlayOAuthProjections( + test("the projection chain keeps a hand-named codex/ API-key entry", () => { + const overlaid = applyOAuthProviderProjections( settingsWith({ "codex/mine": handNamed() }), liveProjected(), + { dropOrphans: true }, ); expect(overlaid?.providers["codex/mine"]?.apiKey).toBe("hand-named-key"); }); - test("overlayOAuthProjections still applies the live token over a credential-less OAuth placeholder", () => { - const overlaid = overlayOAuthProjections( + test("the projection chain still applies the live token over a credential-less OAuth placeholder", () => { + const overlaid = applyOAuthProviderProjections( settingsWith({ "codex/mine": codexEntry() }), liveProjected(), + { dropOrphans: true }, ); expect(overlaid?.providers["codex/mine"]?.apiKey).toBe("live-token"); }); - test("dropOrphanedOAuthEntries never drops a hand-named API-key entry", () => { - const kept = dropOrphanedOAuthEntries( + test("the chain never drops a hand-named API-key entry (orphan sweep)", () => { + const kept = applyOAuthProviderProjections( settingsWith({ "codex/mine": handNamed() }), {}, + { dropOrphans: true }, ); expect(kept?.providers["codex/mine"]?.apiKey).toBe("hand-named-key"); }); - test("dropOrphanedOAuthEntries still drops a credential-less orphan", () => { - const dropped = dropOrphanedOAuthEntries( + test("the chain still drops a credential-less orphan", () => { + const dropped = applyOAuthProviderProjections( settingsWith({ "codex/mine": codexEntry() }), {}, + { dropOrphans: true }, ); expect(dropped?.providers["codex/mine"]).toBeUndefined(); }); @@ -274,8 +277,8 @@ describe("CL-6728: OAuth projections do not overwrite hand-named provider entrie expect(runtime.providers["codex/mine"]?.apiKey).toBeUndefined(); }); - test("overlayOAuthProjections treats a whitespace apiKey as hand-named", () => { - const overlaid = overlayOAuthProjections( + test("the chain treats a whitespace apiKey as hand-named", () => { + const overlaid = applyOAuthProviderProjections( settingsWith({ "codex/mine": { baseURL: "https://hand-named.example.com/v1", @@ -284,12 +287,13 @@ describe("CL-6728: OAuth projections do not overwrite hand-named provider entrie }, }), liveProjected(), + { dropOrphans: true }, ); expect(overlaid?.providers["codex/mine"]?.apiKey).toBe(" "); }); - test("overlayOAuthProjections treats an empty-string apiKey as a placeholder", () => { - const overlaid = overlayOAuthProjections( + test("the chain treats an empty-string apiKey as a placeholder", () => { + const overlaid = applyOAuthProviderProjections( settingsWith({ "codex/mine": { baseURL: "https://hand-named.example.com/v1", @@ -298,7 +302,44 @@ describe("CL-6728: OAuth projections do not overwrite hand-named provider entrie }, }), liveProjected(), + { dropOrphans: true }, ); expect(overlaid?.providers["codex/mine"]?.apiKey).toBe("live-token"); }); }); + +describe("applyOAuthProviderProjections: one ordered chain (CL-10672)", () => { + const liveMine: CodexProfile = { + name: "mine", + tokens: { access: "live-token", refresh: "r", expiresAt: 1 }, + createdAt: 0, + }; + test("persisted defaults feed the projections before the orphan sweep and overlay", () => { + // settings carry a persisted defaultModel on the credential-less + // placeholder; the projection must inherit it (defaults step) while the + // orphan sweep honors the live profile presence (drop guard) and the + // overlay delivers the live token. + const settings = settingsWith({ + "codex/mine": { ...codexEntry(), defaultModel: "chosen-model" }, + }); + const resolvedChain = applyOAuthProviderProjections( + settings, + codexProvidersAsSettings([liveMine]), + { dropOrphans: true }, + ); + const row = resolvedChain?.providers["codex/mine"]; + expect(row?.apiKey).toBe("live-token"); + expect(row?.models).toContain("chosen-model"); + expect(row?.defaultModel).toBe("chosen-model"); + }); + + test("dropOrphans: false skips the orphan sweep verbatim (programmatic settings load)", () => { + const placeholder = codexEntry(); + const kept = applyOAuthProviderProjections( + settingsWith({ "codex/mine": placeholder }), + {}, + { dropOrphans: false }, + ); + expect(kept?.providers["codex/mine"]).toBe(placeholder); + }); +}); diff --git a/src/config/settings.ts b/src/config/settings.ts index 419c6caeb..4e3685dcd 100644 --- a/src/config/settings.ts +++ b/src/config/settings.ts @@ -42,11 +42,12 @@ export interface ProviderSettings { contextWindow?: number; // Per-call output cap, overriding SOURCE_MAX_TOKENS when set. maxTokens?: number; - // Sampling temperature (0..2, OpenAI-compatible); mutually exclusive with - // topP — never send both on the wire. + // Sampling temperature (0..2, OpenAI-compatible); the canonical sampling + // knob. Mutually exclusive with topP — never send both on the wire. temperature?: number; - // Sampling top-p (0..1, OpenAI-compatible); mutually exclusive with - // temperature. + // Deprecated non-canonical sampling spelling; honored as-is (loads, flows + // to top_p) so existing files keep working. Mutually exclusive with + // temperature; new config should use temperature. topP?: number; // Bifrost virtual key (sk-bf-...): routes through the Bifrost adapter // (injects the x-bf-vk header) and enables /v1/models auto-discovery. diff --git a/src/perf/otel-config.test.ts b/src/perf/otel-config.test.ts index aadd1095a..6fb62d06b 100644 --- a/src/perf/otel-config.test.ts +++ b/src/perf/otel-config.test.ts @@ -197,7 +197,7 @@ describe("resolveOtelExportConfig", () => { } }); - test("resource attributes merge with env winning on conflict", () => { + test("env resource attributes replace settings wholesale (CL-10672)", () => { const result = resolveOtelExportConfig( baseSettings({ endpoint: "https://c.example", @@ -213,7 +213,52 @@ describe("resolveOtelExportConfig", () => { expect(result.config.resourceAttributes["deployment.environment"]).toBe( "prod", ); - expect(result.config.resourceAttributes.team).toBe("corbits"); + // Env wins wholesale: settings-only keys do not survive the merge. + expect(result.config.resourceAttributes.team).toBeUndefined(); + expect(result.config.resourceAttributes["service.name"]).toBe( + DEFAULT_OTEL_SERVICE_NAME, + ); + } + }); + + test("settings resource attributes can carry a service.name the env never overrides", () => { + const result = resolveOtelExportConfig( + baseSettings({ + endpoint: "https://c.example", + resourceAttributes: { + "deployment.environment": "settings-only", + team: "corbits", + }, + }), + { [OTEL_ENV.resourceAttributes]: "service.name=from-env" }, + ); + expect(result.ok).toBe(true); + if (result.ok && result.config.enabled) { + expect(result.config.resourceAttributes.team).toBeUndefined(); + expect(result.config.serviceName).toBe("from-env"); + } + }); + + test("set-but-empty env resource attributes replace with nothing (regression: no settings fallback)", () => { + // OTEL_RESOURCE_ATTRIBUTES="" counts as set for the wholesale rule, matching + // the same edge on OTEL_EXPORTER_OTLP_HEADERS: the settings map is dropped, + // leaving only the service.name sync. + const result = resolveOtelExportConfig( + baseSettings({ + endpoint: "https://c.example", + resourceAttributes: { + team: "corbits", + "deployment.environment": "dev", + }, + }), + { [OTEL_ENV.resourceAttributes]: "" }, + ); + expect(result.ok).toBe(true); + if (result.ok && result.config.enabled) { + expect(result.config.resourceAttributes.team).toBeUndefined(); + expect( + result.config.resourceAttributes["deployment.environment"], + ).toBeUndefined(); expect(result.config.resourceAttributes["service.name"]).toBe( DEFAULT_OTEL_SERVICE_NAME, ); diff --git a/src/perf/otel-config.ts b/src/perf/otel-config.ts index 2b3114d93..69bcc40f5 100644 --- a/src/perf/otel-config.ts +++ b/src/perf/otel-config.ts @@ -36,7 +36,8 @@ export interface OtelSettings { /** Resource service.name. Env OTEL_SERVICE_NAME overrides. */ serviceName?: string; /** - * Extra resource attributes (merged; env keys win). Non-secret labels only. + * Extra resource attributes. Non-secret labels only. Replaced wholesale by + * OTEL_RESOURCE_ATTRIBUTES when the env var is set; never merged per-key. */ resourceAttributes?: Record; } @@ -208,12 +209,13 @@ function validateStringMap( } /** - * Precedence (env over settings): + * Precedence (env wins wholesale per field): * - endpoint: env > settings * - headers: env list fully replaces settings when set; else settings * - serviceName: env > settings.serviceName > attrs["service.name"] > default - * - resourceAttributes: settings then env (env wins on conflict); after merge, - * attrs["service.name"] is synced to the resolved serviceName + * - resourceAttributes: env list fully replaces settings when OTEL_RESOURCE_ATTRIBUTES + * is set; else settings. After resolution, attrs["service.name"] is synced + * to the resolved serviceName * * No endpoint → disabled (ok); invalid or malformed → not ok. */ @@ -294,20 +296,10 @@ export function resolveOtelExportConfig( } let resourceAttributes: Record = {}; - const settingsAttrs = validateStringMap( - otel?.resourceAttributes, - "settings.otel.resourceAttributes", - ); - if (!settingsAttrs.ok) { - return { - ok: false, - code: OTEL_CONFIG_INVALID, - message: settingsAttrs.message, - }; - } - resourceAttributes = { ...settingsAttrs.value }; - if (envResourceAttrsRaw !== undefined) { + // Env wins wholesale: a set OTEL_RESOURCE_ATTRIBUTES replaces settings + // attrs entirely, matching the headers rule. An invalid settings map + // never blocks a fully-specified env list. const parsed = parseOtelKeyValueList( envResourceAttrsRaw, OTEL_ENV.resourceAttributes, @@ -315,7 +307,20 @@ export function resolveOtelExportConfig( if (!parsed.ok) { return { ok: false, code: OTEL_CONFIG_INVALID, message: parsed.message }; } - resourceAttributes = { ...resourceAttributes, ...parsed.value }; + resourceAttributes = parsed.value; + } else { + const settingsAttrs = validateStringMap( + otel?.resourceAttributes, + "settings.otel.resourceAttributes", + ); + if (!settingsAttrs.ok) { + return { + ok: false, + code: OTEL_CONFIG_INVALID, + message: settingsAttrs.message, + }; + } + resourceAttributes = settingsAttrs.value; } // Sync attrs["service.name"] to the resolved name so the two never diverge. diff --git a/src/telemetry/index.ts b/src/telemetry/index.ts index c4cef6396..8e1b0aae6 100644 --- a/src/telemetry/index.ts +++ b/src/telemetry/index.ts @@ -236,9 +236,10 @@ export function generationSampleRate( return Math.min(1, Math.max(0, parsed)); } -// Env kills win over settings and need none — callers skip settings writes -// entirely. Falsy CORBITS_TELEMETRY and truthy DO_NOT_TRACK share the same -// flag parsing, so the two switches agree on what counts as "off". +// Env kill switches, one canonical switch plus a kept alias: DO_NOT_TRACK is +// canonical; a falsy CORBITS_TELEMETRY is an accepted alias. Env kills win +// over settings and need none — callers skip settings writes entirely. The +// two spellings share this one falsy parse so "off" agrees across the pair. export function telemetryDisabledByEnv( env: NodeJS.ProcessEnv = process.env, ): boolean { From 7ad873e5287453bc650e360df9c92b2e4d1424b9 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 15 Oct 2026 14:47:00 -0700 Subject: [PATCH 2/2] refactor(config): drop the OAuth projection wrapper and flag narration Inline applyOAuthProviderProjections back into loadConfig and remove comments that only restate alias spellings. Flag parse results are unchanged. --- docs/IMPLEMENTATION.md | 44 +++++++++--------- docs/PERFTRACE.md | 7 +-- src/config.test.ts | 4 -- src/config/index.ts | 79 +++++++------------------------- src/config/oauth-catalog.test.ts | 69 ++++++---------------------- src/config/settings.ts | 9 ++-- src/perf/otel-config.test.ts | 4 -- src/perf/otel-config.ts | 5 +- src/telemetry/index.ts | 7 ++- 9 files changed, 62 insertions(+), 166 deletions(-) diff --git a/docs/IMPLEMENTATION.md b/docs/IMPLEMENTATION.md index 5a7ec7d5d..c14f7dc4f 100644 --- a/docs/IMPLEMENTATION.md +++ b/docs/IMPLEMENTATION.md @@ -174,7 +174,7 @@ Intent defaults: `intent=implement` → director `coder`; `explore` → `explore ### Auto Mode -Auto mode defaults **on** (`config.auto = true` from `loadConfig`; pass `--no-auto` to start off, or `--auto` to force on). It is toggled only via those CLI flags — there is currently no in-session key bound to it. `--auto` remains an explicit re-assertion of that default instead of a pruned no-op flag (CL-10672 default decision: auto stays on; the flag is kept because sequence order is meaningful — `--no-auto --auto` re-enables). The permission gate reads the flag (`getAuto`/`setAuto` in `src/permission/gate.ts`) on the next tool call. `--dangerously-skip-permissions` and its `--yolo` alias force skip-permissions for this process only; skip-permissions wins when combined with auto, so `--auto --yolo` runs in yolo mode. `/yolo [on|off|toggle]` (bare `/yolo` also toggles) persists the skip-permissions setting to the active settings file and wires `getSkipPermissions`/`setSkipPermissions` so the gate and pre-gate sandboxes honor the change on the next tool call without rebuilding plugins. The active source defaults to the user-global `~/.corbits/settings.json`, making that default machine-wide. Explicit `--config ` selects that file instead, so `/yolo` writes the custom file; a later ordinary TUI launch without the same `--config` uses the user-global source and does not modify the custom file. Secret-guard and authz still apply — and the active file itself is runtime-denylisted for path-keyed tools, reads and writes, even under skip-permissions, so the persisted skip cannot be silently leveraged; the startup notice remains as disclosure, not the enforcement. `loadConfig` tracks `skipPermissionsFromSettings` (true only when the effective value came from persisted settings, not either CLI flag) so `runTUI` can show a startup notice and `exec` can print an equivalent stderr warning for the otherwise-silent persisted setting. +Auto mode defaults **on** (`config.auto = true` from `loadConfig`; pass `--no-auto` to start off, or `--auto` to force on). It is toggled only via those CLI flags — there is currently no in-session key bound to it. The permission gate reads the flag (`getAuto`/`setAuto` in `src/permission/gate.ts`) on the next tool call. `--dangerously-skip-permissions` and its `--yolo` alias force skip-permissions for this process only; skip-permissions wins when combined with auto, so `--auto --yolo` runs in yolo mode. `/yolo [on|off|toggle]` (bare `/yolo` also toggles) persists the skip-permissions setting to the active settings file and wires `getSkipPermissions`/`setSkipPermissions` so the gate and pre-gate sandboxes honor the change on the next tool call without rebuilding plugins. The active source defaults to the user-global `~/.corbits/settings.json`, making that default machine-wide. Explicit `--config ` selects that file instead, so `/yolo` writes the custom file; a later ordinary TUI launch without the same `--config` uses the user-global source and does not modify the custom file. Secret-guard and authz still apply — and the active file itself is runtime-denylisted for path-keyed tools, reads and writes, even under skip-permissions, so the persisted skip cannot be silently leveraged; the startup notice remains as disclosure, not the enforcement. `loadConfig` tracks `skipPermissionsFromSettings` (true only when the effective value came from persisted settings, not either CLI flag) so `runTUI` can show a startup notice and `exec` can print an equivalent stderr warning for the otherwise-silent persisted setting. When auto is on, the gate auto-allows workspace file tools in `AUTO_ALLOWED_TOOLS` — which also includes `web_fetch` and `web_search` — and any `run_shell` that does not match the auto-shell policy. With auto off, `web_fetch` and `web_search` still ask. The policy (`autoShellRuleForCall` / `AUTO_SHELL_RULES` in `src/permission/auto-shell-policy.ts`) peels wrappers via `expandShellSubjects` (`bash`/`sh`/`zsh -c`, `xargs`, transparent prefixes), then applies: @@ -398,8 +398,6 @@ Profiles supply per-project or named-profile overrides for `model`, `systemPromp Providers and credentials are read exclusively from settings files: the global `~/.corbits/settings.json` (definitions + credentials) and the per-repo `.corbits/settings.json` (selection only). There are no `OPENAI_COMPATIBLE_*` environment-variable overrides, and `index.ts` does not load `.env` files — a deliberately stale or exported key can no longer shadow the configured provider. -**Sampling knobs.** `temperature` (0..2) is the canonical per-provider sampling knob. `topP` (0..1) is a deprecated but honored spelling kept for existing files: it loads and flows to the wire as `top_p`, and the two are mutually exclusive — when both are set, load fails; never both are sent (CL-10672 marks `topP` for deprecation pacing; removal would be a breaking rename and has not happened). - **Models-first connect.** There is no standalone `/login` command. `/model` opens on a flat **models-only** list (Recent, Favorites, then connected provider/model rows) built by `buildModelsFirstList` (`src/tui/model-picker.ts`); type-to-filter owns printable keys. Selecting a row runs `applyLiveModelSwitch` (`src/session/live-model-switch.ts`) so inference sources, permission-gate identity, grant persistence identity, and advertised tool schemas cut over together. **Alt+A** or `/connect` opens Connect via `addProviderSelectorChoices` (`src/tui/provider-setup.ts`), which lists every first-class kind including Custom — never bare `c` / Ctrl+A, and never in-list “connect →” rows. First-class API-key rows use a named-instance + auth-only form (instance name, key; catalog base URL is display-only); Custom keeps the full manual form. **Alt+F** toggles favorites; recent/favorite pairs live in global settings (`recentModels` / `favoriteModels`). **Alt+D** sets the default via `setDefaultModel` (global `defaultProvider` + that provider's `defaultModel`) plus `persistConnectedSelection` without switching the live session. First-class providers ship from `packages/first-class-providers` (corbits-agnostic defs) and `packages/opencode-go` (Go catalog, auth validate, multi-protocol endpoints, usage). OAuth providers open the existing browser login modal with a named account step; API-key providers share the same multi-instance naming and pre-seed models on save so selection works without restart. Both OAuth and API-key (including Custom) connects share `persistConnectedSelection` in `provider-setup-submit.ts` so project-local provider/model selection is written alongside global credentials. OpenCode Go forces `OPENCODE_GO_BASE_URL` when `opencodeGo` is set so subscription traffic is not billed as Zen PAYG. ### Inference authentication recovery @@ -425,26 +423,26 @@ This release changes no settings, OAuth-store, or session persistence format and Printed by `corbits --help` / `-h` from `CLI_HELP_TEXT` in `src/config/index.ts` (that constant is the source of truth; keep this table in sync when flags change). -| Verb / Flag | Default | Description | -| ------------------------------------------ | -------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| _(no verb)_ | — | Interactive session; optional trailing task text | -| `exec` / `run` / `-p` | — | Run a prompt (non-interactive / one-shot). `exec` is the canonical verb (`run` / `-p` are kept aliases); `-p` is the same path and may appear in any flag position. | -| `resume` / `continue` | — | Open the session picker for this folder (project-keyed to this checkout's git toplevel). Lists the 10 most recently persisted sessions, completed included. Type to filter. `resume` is the canonical continue verb (`continue` is a kept alias). | -| `--resume []` | — | Interactive: open the session picker, or reopen a specific session when an id is given. With `exec` / `-p` the id is required, that session is continued, and the new prompt is sent (no picker). | -| `resume ` | — | Reopen a specific session in the TUI (does not auto-send a prompt) | -| `exec --resume ` | — | Headless: load that session and send ``, then exit. Missing or unreadable ids error and do not create a session. `--resume` without an id errors. | -| `-p --resume ` | — | Same headless continue path as `exec --resume` | -| `resume --pick` / `--list` | — | Interactive session picker (kept aliases of the bare `--resume` picker flow) | -| `--cwd ` | `process.cwd()` | Working directory | -| `--config ` | `~/.corbits/settings.json` | Active settings file for provider definitions and TUI persistence such as `/yolo`; composes with (does not exclude) home-level codex/xai OAuth credentials | -| `--provider ` | from settings | Select a configured provider | -| `--model ` | provider default | Select a model for the active provider | -| `--profile ` | — | Settings profile | -| `--dangerously-skip-permissions`, `--yolo` | false | Process-only aliases that auto-allow anything not denied by the authorization layer (gate + pre-gate workspace sandboxes; secret-guard / authz hard denies remain). `/yolo [on\|off\|toggle]` persists to the active settings file instead. | -| `--auto` | true (default) | Force auto mode on (workspace writes + unconstrained shell without prompts) | -| `--no-auto` | false | Start with auto mode off (ask on every consequential action); no in-session key toggles it | -| `--help`, `-h` | — | Show help (exit 0 via `CliHelpError`) | -| `--version`, `-V` | — | Print `corbits v` plus the running `package.json` version (exit 0 via `CliVersionError`) | +| Verb / Flag | Default | Description | +| ------------------------------------------ | -------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| _(no verb)_ | — | Interactive session; optional trailing task text | +| `exec` / `run` / `-p` | — | Run a prompt (non-interactive / one-shot). `-p` is the same path as `exec` and may appear in any flag position. | +| `resume` / `continue` | — | Open the session picker for this folder (project-keyed to this checkout's git toplevel). Lists the 10 most recently persisted sessions, completed included. Type to filter. | +| `--resume []` | — | Interactive: open the session picker, or reopen a specific session when an id is given. With `exec` / `-p` the id is required, that session is continued, and the new prompt is sent (no picker). | +| `resume ` | — | Reopen a specific session in the TUI (does not auto-send a prompt) | +| `exec --resume ` | — | Headless: load that session and send ``, then exit. Missing or unreadable ids error and do not create a session. `--resume` without an id errors. | +| `-p --resume ` | — | Same headless continue path as `exec --resume` | +| `resume --pick` / `--list` | — | Interactive session picker | +| `--cwd ` | `process.cwd()` | Working directory | +| `--config ` | `~/.corbits/settings.json` | Active settings file for provider definitions and TUI persistence such as `/yolo`; composes with (does not exclude) home-level codex/xai OAuth credentials | +| `--provider ` | from settings | Select a configured provider | +| `--model ` | provider default | Select a model for the active provider | +| `--profile ` | — | Settings profile | +| `--dangerously-skip-permissions`, `--yolo` | false | Process-only aliases that auto-allow anything not denied by the authorization layer (gate + pre-gate workspace sandboxes; secret-guard / authz hard denies remain). `/yolo [on\|off\|toggle]` persists to the active settings file instead. | +| `--auto` | true (default) | Force auto mode on (workspace writes + unconstrained shell without prompts) | +| `--no-auto` | false | Start with auto mode off (ask on every consequential action); no in-session key toggles it | +| `--help`, `-h` | — | Show help (exit 0 via `CliHelpError`) | +| `--version`, `-V` | — | Print `corbits v` plus the running `package.json` version (exit 0 via `CliVersionError`) | Positional arguments after flags are joined into the optional initial task delivered when the TUI mounts. With no positional task, the operator starts from an empty prompt. diff --git a/docs/PERFTRACE.md b/docs/PERFTRACE.md index 273f6e0e8..f631cb673 100644 --- a/docs/PERFTRACE.md +++ b/docs/PERFTRACE.md @@ -66,11 +66,8 @@ Precedence: `resourceAttributes["service.name"]` > `corbits-code`. After merge, `resourceAttributes["service.name"]` is always set to the resolved name so the two never diverge. -- **resourceAttributes:** env list replaces settings wholesale when - `OTEL_RESOURCE_ATTRIBUTES` is set (no per-key merge); otherwise settings - apply alone. Set-but-empty counts as set: `OTEL_RESOURCE_ATTRIBUTES=""` - replaces the settings attributes with nothing (and does not re-apply the - settings map) — the same set-but-empty rule as `OTEL_EXPORTER_OTLP_HEADERS`. +- **resourceAttributes:** when `OTEL_RESOURCE_ATTRIBUTES` is set, it fully + replaces settings attributes (including set-but-empty) - **enabling export:** setting an endpoint (settings or env) enables export; `otel.enabled` need not be set. Service name or resource attributes alone do not enable export. diff --git a/src/config.test.ts b/src/config.test.ts index 9842ab4bf..72a92d167 100644 --- a/src/config.test.ts +++ b/src/config.test.ts @@ -572,8 +572,6 @@ describe("loadConfig", () => { { argv: ["resume", "--pick"] }, { argv: ["resume"] }, { argv: ["continue", "--list"] }, - // CL-10672: --pick/--list fold into the single --resume picker verb; both - // aliases remain accepted in any resume-flow position. { argv: ["resume", "--list"] }, { argv: ["resume", "--pick", "--list"] }, ] as const)("$argv opens the session picker", async ({ argv }) => { @@ -586,8 +584,6 @@ describe("loadConfig", () => { }); test("CL-10672 keeps --pick/--list out of flow as unrecognized flags", async () => { - // Without the resume verb these spellings were never recognized; the - // alias fold preserves that fallthrough instead of quietly opening a picker. await expect( loadConfig(["--pick"], { globalSettingsPath: NO_SETTINGS }), ).rejects.toThrow("unrecognized flag: --pick"); diff --git a/src/config/index.ts b/src/config/index.ts index aad457354..79d91d2b4 100644 --- a/src/config/index.ts +++ b/src/config/index.ts @@ -109,9 +109,7 @@ import { resolveProfile } from "./profiles.js"; // Per-call token ceiling for inference sources, shared by agent creation // (runner.ts) and live provider switching (the /agent modal) so a live switch -// cannot silently revert it. CL-10672 review: kept as the shared fallback — -// it is NOT an always-overridden default; ProviderSettings.maxTokens is the -// optional override and an unset value resolves to this ceiling. +// cannot silently revert it. export const SOURCE_MAX_TOKENS = 16384; // Credential-cell placeholder for keyless local providers (e.g. Ollama). The @@ -162,9 +160,8 @@ function applyPersistedOAuthDefaults( // OAuth entries in settings.json carry no credentials; they work only while a // matching auth-store profile exists. Drop orphans in memory so a removed -// profile does not pin resolution to an unauthenticatable provider. Called -// only from applyOAuthProviderProjections (plus its tests). -function dropOrphanedOAuthEntries( +// profile does not pin resolution to an unauthenticatable provider. +export function dropOrphanedOAuthEntries( settings: Settings | null, projected: Record, ): Settings | null { @@ -200,8 +197,8 @@ function isHandNamedProviderEntry( } // Overlay live OAuth profile projections onto settings for runtime provider -// resolution. Called only from applyOAuthProviderProjections (plus its tests). -function overlayOAuthProjections( +// resolution. Exported for tests; loadConfig is the only production caller. +export function overlayOAuthProjections( settings: Settings | null, projected: Record, ): Settings | null { @@ -216,28 +213,6 @@ function overlayOAuthProjections( }; } -// The one ordered OAuth projection chain for runtime provider resolution, -// folding the three dedicated helpers at the layer that owns them -// (loadConfig): (1) persisted defaultModel defaults merge into the live -// projections, (2) credential-less orphan entries are dropped from settings -// (only when the projections came from the home auth stores), (3) the -// projections overlay onto the surviving settings without overwriting -// hand-named API-key rows. Exposed for tests. -export function applyOAuthProviderProjections( - settings: Settings | null, - projectedProviderSettings: Record, - options: { dropOrphans: boolean }, -): Settings | null { - const projected = applyPersistedOAuthDefaults( - settings, - projectedProviderSettings, - ); - const liveSettings = options.dropOrphans - ? dropOrphanedOAuthEntries(settings, projected) - : settings; - return overlayOAuthProjections(liveSettings, projected); -} - function hasExaEntry(servers: MCPServerSettingsEntry[] | undefined): boolean { return ( servers?.some((server) => server.name === EXA_MCP_SERVER_NAME) === true @@ -730,15 +705,6 @@ export interface UnconfiguredConfig { settingsDiagnostics?: SettingsLoadDiagnostic[]; } -// Skip-permissions spellings. `--dangerously-skip-permissions` is the -// canonical process-only flag; `--yolo` is a kept alias (`/yolo` is the -// persistent settings writer — a separate surface). One flag, one setting -// (`dangerouslySkipPermissions`); the parse consults one alias set. -const SKIP_PERMISSIONS_FLAGS: ReadonlySet = new Set([ - "--dangerously-skip-permissions", - "--yolo", -]); - /** Printed for `corbits --help` / `-h`. Keep in sync with docs/IMPLEMENTATION.md. */ export const CLI_HELP_TEXT = `corbits — coding agent CLI @@ -867,9 +833,6 @@ export async function loadConfig( let resumeMode: "id" | "pick" | undefined; let resumeSessionId: string | undefined; const leading = args[0]; - // One exec verb: `exec` is canonical; `run` is a kept alias (help names - // both). One resume verb: `resume` is canonical; `continue` is a kept - // alias of the same flow. if (leading === "exec" || leading === "run") { command = "exec"; args.shift(); @@ -878,8 +841,6 @@ export async function loadConfig( // Bare resume opens the list; a session id is the only direct-resume path. // Invalid non-flag positionals error instead of falling through (a // free-form token would otherwise become task while skipInitialTask is set). - // A following --pick/--list falls through to the loop's single resume - // branch, where it is a kept alias of the bare --resume picker flow. const next = args.slice()[0]; if (next !== undefined && !isFlagToken(next)) { if (!isSessionId(next)) { @@ -895,10 +856,9 @@ export async function loadConfig( } } - // `-p` is the exec one-shot alias in any position (`-p --provider …`, + // `-p` is the exec one-shot path in any position (`-p --provider …`, // `--model … -p …`). Detect it before value flags so exec-only options - // that appear before `-p` still see exec mode; the loop keeps the token - // out of the task positionals. + // that appear before `-p` still see exec mode. if (args.includes("-p")) { if (leading === "resume" || leading === "continue") { throw new Error( @@ -970,13 +930,11 @@ export async function loadConfig( continue; } - if (SKIP_PERMISSIONS_FLAGS.has(arg)) { + if (arg === "--dangerously-skip-permissions" || arg === "--yolo") { dangerouslySkipPermissions = true; continue; } if (arg === "--auto") { - // Explicit re-assertion of the on-by-default auto mode. Kept because - // sequence order still matters (`--no-auto --auto` re-enables). auto = true; continue; } @@ -985,15 +943,9 @@ export async function loadConfig( continue; } if (arg === "-p") { - // `-p` alias of exec: the pre-scan already fixed command="exec"; the - // token must stay out of the task positionals. continue; } if (arg === "--resume" || arg === "--pick" || arg === "--list") { - // One resume verb: `--resume` carries the optional session id (exec - // requires it; interactive bare `--resume` opens the picker, the alias - // for `resume `). `--pick`/`--list` are kept aliases of that - // bare picker flow and may not follow a session id. if (arg === "--resume") { if (resumeMode === "id") { throw new Error("cannot combine a session id with --resume"); @@ -1020,8 +972,6 @@ export async function loadConfig( } else if (resumeMode !== undefined) { resumeMode = "pick"; } else { - // Without the resume verb these spellings are not in flow; preserve - // the previous fallthrough as an unrecognized flag. throw new Error(`unrecognized flag: ${arg}`); } continue; @@ -1059,7 +1009,7 @@ export async function loadConfig( useOAuthProfiles ? await Promise.all([listCodexProfiles(), listXaiProfiles()]) : [[], []]; - const projectedOAuthProviders = { + let projectedOAuthProviders = { ...codexProvidersAsSettings(codexProfiles), ...xaiProvidersAsSettings(xaiProfiles), }; @@ -1089,11 +1039,16 @@ export async function loadConfig( settings?.dangerouslySkipPermissions === true; dangerouslySkipPermissions = dangerouslySkipPermissions || settings?.dangerouslySkipPermissions === true; - // Single ordered projection chain (see applyOAuthProviderProjections). - const settingsForResolution: Settings | null = applyOAuthProviderProjections( + projectedOAuthProviders = applyPersistedOAuthDefaults( settings, projectedOAuthProviders, - { dropOrphans: useOAuthProfiles }, + ); + const liveSettings = useOAuthProfiles + ? dropOrphanedOAuthEntries(settings, projectedOAuthProviders) + : settings; + const settingsForResolution: Settings | null = overlayOAuthProjections( + liveSettings, + projectedOAuthProviders, ); // The per-repo selection file still applies on top of --config: that file diff --git a/src/config/oauth-catalog.test.ts b/src/config/oauth-catalog.test.ts index f94ce5ea1..74d280703 100644 --- a/src/config/oauth-catalog.test.ts +++ b/src/config/oauth-catalog.test.ts @@ -8,8 +8,9 @@ import { codexProvidersAsSettings, } from "./codex-providers.js"; import { - applyOAuthProviderProjections, + dropOrphanedOAuthEntries, mergeOAuthCatalog, + overlayOAuthProjections, providerCatalogToSettings, runtimeSettingsWithCatalog, } from "./index.js"; @@ -140,38 +141,34 @@ describe("CL-6728: OAuth projections do not overwrite hand-named provider entrie const liveProjected = () => codexProvidersAsSettings([liveMine]); const liveCatalog = () => codexProfilesToCatalogEntries([liveMine]); - test("the projection chain keeps a hand-named codex/ API-key entry", () => { - const overlaid = applyOAuthProviderProjections( + test("overlayOAuthProjections keeps a hand-named codex/ API-key entry", () => { + const overlaid = overlayOAuthProjections( settingsWith({ "codex/mine": handNamed() }), liveProjected(), - { dropOrphans: true }, ); expect(overlaid?.providers["codex/mine"]?.apiKey).toBe("hand-named-key"); }); - test("the projection chain still applies the live token over a credential-less OAuth placeholder", () => { - const overlaid = applyOAuthProviderProjections( + test("overlayOAuthProjections still applies the live token over a credential-less OAuth placeholder", () => { + const overlaid = overlayOAuthProjections( settingsWith({ "codex/mine": codexEntry() }), liveProjected(), - { dropOrphans: true }, ); expect(overlaid?.providers["codex/mine"]?.apiKey).toBe("live-token"); }); - test("the chain never drops a hand-named API-key entry (orphan sweep)", () => { - const kept = applyOAuthProviderProjections( + test("dropOrphanedOAuthEntries never drops a hand-named API-key entry", () => { + const kept = dropOrphanedOAuthEntries( settingsWith({ "codex/mine": handNamed() }), {}, - { dropOrphans: true }, ); expect(kept?.providers["codex/mine"]?.apiKey).toBe("hand-named-key"); }); - test("the chain still drops a credential-less orphan", () => { - const dropped = applyOAuthProviderProjections( + test("dropOrphanedOAuthEntries still drops a credential-less orphan", () => { + const dropped = dropOrphanedOAuthEntries( settingsWith({ "codex/mine": codexEntry() }), {}, - { dropOrphans: true }, ); expect(dropped?.providers["codex/mine"]).toBeUndefined(); }); @@ -277,8 +274,8 @@ describe("CL-6728: OAuth projections do not overwrite hand-named provider entrie expect(runtime.providers["codex/mine"]?.apiKey).toBeUndefined(); }); - test("the chain treats a whitespace apiKey as hand-named", () => { - const overlaid = applyOAuthProviderProjections( + test("overlayOAuthProjections treats a whitespace apiKey as hand-named", () => { + const overlaid = overlayOAuthProjections( settingsWith({ "codex/mine": { baseURL: "https://hand-named.example.com/v1", @@ -287,13 +284,12 @@ describe("CL-6728: OAuth projections do not overwrite hand-named provider entrie }, }), liveProjected(), - { dropOrphans: true }, ); expect(overlaid?.providers["codex/mine"]?.apiKey).toBe(" "); }); - test("the chain treats an empty-string apiKey as a placeholder", () => { - const overlaid = applyOAuthProviderProjections( + test("overlayOAuthProjections treats an empty-string apiKey as a placeholder", () => { + const overlaid = overlayOAuthProjections( settingsWith({ "codex/mine": { baseURL: "https://hand-named.example.com/v1", @@ -302,44 +298,7 @@ describe("CL-6728: OAuth projections do not overwrite hand-named provider entrie }, }), liveProjected(), - { dropOrphans: true }, ); expect(overlaid?.providers["codex/mine"]?.apiKey).toBe("live-token"); }); }); - -describe("applyOAuthProviderProjections: one ordered chain (CL-10672)", () => { - const liveMine: CodexProfile = { - name: "mine", - tokens: { access: "live-token", refresh: "r", expiresAt: 1 }, - createdAt: 0, - }; - test("persisted defaults feed the projections before the orphan sweep and overlay", () => { - // settings carry a persisted defaultModel on the credential-less - // placeholder; the projection must inherit it (defaults step) while the - // orphan sweep honors the live profile presence (drop guard) and the - // overlay delivers the live token. - const settings = settingsWith({ - "codex/mine": { ...codexEntry(), defaultModel: "chosen-model" }, - }); - const resolvedChain = applyOAuthProviderProjections( - settings, - codexProvidersAsSettings([liveMine]), - { dropOrphans: true }, - ); - const row = resolvedChain?.providers["codex/mine"]; - expect(row?.apiKey).toBe("live-token"); - expect(row?.models).toContain("chosen-model"); - expect(row?.defaultModel).toBe("chosen-model"); - }); - - test("dropOrphans: false skips the orphan sweep verbatim (programmatic settings load)", () => { - const placeholder = codexEntry(); - const kept = applyOAuthProviderProjections( - settingsWith({ "codex/mine": placeholder }), - {}, - { dropOrphans: false }, - ); - expect(kept?.providers["codex/mine"]).toBe(placeholder); - }); -}); diff --git a/src/config/settings.ts b/src/config/settings.ts index 4e3685dcd..419c6caeb 100644 --- a/src/config/settings.ts +++ b/src/config/settings.ts @@ -42,12 +42,11 @@ export interface ProviderSettings { contextWindow?: number; // Per-call output cap, overriding SOURCE_MAX_TOKENS when set. maxTokens?: number; - // Sampling temperature (0..2, OpenAI-compatible); the canonical sampling - // knob. Mutually exclusive with topP — never send both on the wire. + // Sampling temperature (0..2, OpenAI-compatible); mutually exclusive with + // topP — never send both on the wire. temperature?: number; - // Deprecated non-canonical sampling spelling; honored as-is (loads, flows - // to top_p) so existing files keep working. Mutually exclusive with - // temperature; new config should use temperature. + // Sampling top-p (0..1, OpenAI-compatible); mutually exclusive with + // temperature. topP?: number; // Bifrost virtual key (sk-bf-...): routes through the Bifrost adapter // (injects the x-bf-vk header) and enables /v1/models auto-discovery. diff --git a/src/perf/otel-config.test.ts b/src/perf/otel-config.test.ts index 6fb62d06b..60db9604c 100644 --- a/src/perf/otel-config.test.ts +++ b/src/perf/otel-config.test.ts @@ -213,7 +213,6 @@ describe("resolveOtelExportConfig", () => { expect(result.config.resourceAttributes["deployment.environment"]).toBe( "prod", ); - // Env wins wholesale: settings-only keys do not survive the merge. expect(result.config.resourceAttributes.team).toBeUndefined(); expect(result.config.resourceAttributes["service.name"]).toBe( DEFAULT_OTEL_SERVICE_NAME, @@ -240,9 +239,6 @@ describe("resolveOtelExportConfig", () => { }); test("set-but-empty env resource attributes replace with nothing (regression: no settings fallback)", () => { - // OTEL_RESOURCE_ATTRIBUTES="" counts as set for the wholesale rule, matching - // the same edge on OTEL_EXPORTER_OTLP_HEADERS: the settings map is dropped, - // leaving only the service.name sync. const result = resolveOtelExportConfig( baseSettings({ endpoint: "https://c.example", diff --git a/src/perf/otel-config.ts b/src/perf/otel-config.ts index 69bcc40f5..25370d765 100644 --- a/src/perf/otel-config.ts +++ b/src/perf/otel-config.ts @@ -209,7 +209,7 @@ function validateStringMap( } /** - * Precedence (env wins wholesale per field): + * Precedence (env over settings): * - endpoint: env > settings * - headers: env list fully replaces settings when set; else settings * - serviceName: env > settings.serviceName > attrs["service.name"] > default @@ -297,9 +297,6 @@ export function resolveOtelExportConfig( let resourceAttributes: Record = {}; if (envResourceAttrsRaw !== undefined) { - // Env wins wholesale: a set OTEL_RESOURCE_ATTRIBUTES replaces settings - // attrs entirely, matching the headers rule. An invalid settings map - // never blocks a fully-specified env list. const parsed = parseOtelKeyValueList( envResourceAttrsRaw, OTEL_ENV.resourceAttributes, diff --git a/src/telemetry/index.ts b/src/telemetry/index.ts index 8e1b0aae6..c4cef6396 100644 --- a/src/telemetry/index.ts +++ b/src/telemetry/index.ts @@ -236,10 +236,9 @@ export function generationSampleRate( return Math.min(1, Math.max(0, parsed)); } -// Env kill switches, one canonical switch plus a kept alias: DO_NOT_TRACK is -// canonical; a falsy CORBITS_TELEMETRY is an accepted alias. Env kills win -// over settings and need none — callers skip settings writes entirely. The -// two spellings share this one falsy parse so "off" agrees across the pair. +// Env kills win over settings and need none — callers skip settings writes +// entirely. Falsy CORBITS_TELEMETRY and truthy DO_NOT_TRACK share the same +// flag parsing, so the two switches agree on what counts as "off". export function telemetryDisabledByEnv( env: NodeJS.ProcessEnv = process.env, ): boolean {