Skip to content

Commit 8da7c96

Browse files
committed
refactor(config): fold duplicate flag spellings into single parse paths
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.
1 parent 1334a97 commit 8da7c96

9 files changed

Lines changed: 264 additions & 113 deletions

File tree

‎docs/IMPLEMENTATION.md‎

Lines changed: 23 additions & 21 deletions
Large diffs are not rendered by default.

‎docs/PERFTRACE.md‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,11 @@ Precedence:
6666
`resourceAttributes["service.name"]` > `corbits-code`. After merge,
6767
`resourceAttributes["service.name"]` is always set to the resolved name so
6868
the two never diverge.
69-
- **resourceAttributes:** settings merged with env; env wins on key conflict
69+
- **resourceAttributes:** env list replaces settings wholesale when
70+
`OTEL_RESOURCE_ATTRIBUTES` is set (no per-key merge); otherwise settings
71+
apply alone. Set-but-empty counts as set: `OTEL_RESOURCE_ATTRIBUTES=""`
72+
replaces the settings attributes with nothing (and does not re-apply the
73+
settings map) — the same set-but-empty rule as `OTEL_EXPORTER_OTLP_HEADERS`.
7074
- **enabling export:** setting an endpoint (settings or env) enables export;
7175
`otel.enabled` need not be set. Service name or resource attributes alone do
7276
not enable export.

‎src/config.test.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -572,6 +572,10 @@ describe("loadConfig", () => {
572572
{ argv: ["resume", "--pick"] },
573573
{ argv: ["resume"] },
574574
{ argv: ["continue", "--list"] },
575+
// CL-10672: --pick/--list fold into the single --resume picker verb; both
576+
// aliases remain accepted in any resume-flow position.
577+
{ argv: ["resume", "--list"] },
578+
{ argv: ["resume", "--pick", "--list"] },
575579
] as const)("$argv opens the session picker", async ({ argv }) => {
576580
const cwd = await emptyCwd();
577581
const config = await loadFor(cwd, argv, { home: await tempHome() });
@@ -581,6 +585,17 @@ describe("loadConfig", () => {
581585
expect(config.skipInitialTask).toBe(true);
582586
});
583587

588+
test("CL-10672 keeps --pick/--list out of flow as unrecognized flags", async () => {
589+
// Without the resume verb these spellings were never recognized; the
590+
// alias fold preserves that fallthrough instead of quietly opening a picker.
591+
await expect(
592+
loadConfig(["--pick"], { globalSettingsPath: NO_SETTINGS }),
593+
).rejects.toThrow("unrecognized flag: --pick");
594+
await expect(
595+
loadConfig(["--list"], { globalSettingsPath: NO_SETTINGS }),
596+
).rejects.toThrow("unrecognized flag: --list");
597+
});
598+
584599
test("resume <id> reopens a known session and skips the initial task", async () => {
585600
const cwd = await emptyCwd();
586601
const home = await tempHome();

‎src/config/index.ts‎

Lines changed: 87 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,9 @@ import { resolveProfile } from "./profiles.js";
109109

110110
// Per-call token ceiling for inference sources, shared by agent creation
111111
// (runner.ts) and live provider switching (the /agent modal) so a live switch
112-
// cannot silently revert it.
112+
// cannot silently revert it. CL-10672 review: kept as the shared fallback —
113+
// it is NOT an always-overridden default; ProviderSettings.maxTokens is the
114+
// optional override and an unset value resolves to this ceiling.
113115
export const SOURCE_MAX_TOKENS = 16384;
114116

115117
// Credential-cell placeholder for keyless local providers (e.g. Ollama). The
@@ -160,8 +162,9 @@ function applyPersistedOAuthDefaults(
160162

161163
// OAuth entries in settings.json carry no credentials; they work only while a
162164
// matching auth-store profile exists. Drop orphans in memory so a removed
163-
// profile does not pin resolution to an unauthenticatable provider.
164-
export function dropOrphanedOAuthEntries(
165+
// profile does not pin resolution to an unauthenticatable provider. Called
166+
// only from applyOAuthProviderProjections (plus its tests).
167+
function dropOrphanedOAuthEntries(
165168
settings: Settings | null,
166169
projected: Record<string, ProviderSettings>,
167170
): Settings | null {
@@ -197,8 +200,8 @@ function isHandNamedProviderEntry(
197200
}
198201

199202
// Overlay live OAuth profile projections onto settings for runtime provider
200-
// resolution. Exported for tests; loadConfig is the only production caller.
201-
export function overlayOAuthProjections(
203+
// resolution. Called only from applyOAuthProviderProjections (plus its tests).
204+
function overlayOAuthProjections(
202205
settings: Settings | null,
203206
projected: Record<string, ProviderSettings>,
204207
): Settings | null {
@@ -213,6 +216,28 @@ export function overlayOAuthProjections(
213216
};
214217
}
215218

219+
// The one ordered OAuth projection chain for runtime provider resolution,
220+
// folding the three dedicated helpers at the layer that owns them
221+
// (loadConfig): (1) persisted defaultModel defaults merge into the live
222+
// projections, (2) credential-less orphan entries are dropped from settings
223+
// (only when the projections came from the home auth stores), (3) the
224+
// projections overlay onto the surviving settings without overwriting
225+
// hand-named API-key rows. Exposed for tests.
226+
export function applyOAuthProviderProjections(
227+
settings: Settings | null,
228+
projectedProviderSettings: Record<string, ProviderSettings>,
229+
options: { dropOrphans: boolean },
230+
): Settings | null {
231+
const projected = applyPersistedOAuthDefaults(
232+
settings,
233+
projectedProviderSettings,
234+
);
235+
const liveSettings = options.dropOrphans
236+
? dropOrphanedOAuthEntries(settings, projected)
237+
: settings;
238+
return overlayOAuthProjections(liveSettings, projected);
239+
}
240+
216241
function hasExaEntry(servers: MCPServerSettingsEntry[] | undefined): boolean {
217242
return (
218243
servers?.some((server) => server.name === EXA_MCP_SERVER_NAME) === true
@@ -705,6 +730,15 @@ export interface UnconfiguredConfig {
705730
settingsDiagnostics?: SettingsLoadDiagnostic[];
706731
}
707732

733+
// Skip-permissions spellings. `--dangerously-skip-permissions` is the
734+
// canonical process-only flag; `--yolo` is a kept alias (`/yolo` is the
735+
// persistent settings writer — a separate surface). One flag, one setting
736+
// (`dangerouslySkipPermissions`); the parse consults one alias set.
737+
const SKIP_PERMISSIONS_FLAGS: ReadonlySet<string> = new Set([
738+
"--dangerously-skip-permissions",
739+
"--yolo",
740+
]);
741+
708742
/** Printed for `corbits --help` / `-h`. Keep in sync with docs/IMPLEMENTATION.md. */
709743
export const CLI_HELP_TEXT = `corbits — coding agent CLI
710744
@@ -833,20 +867,21 @@ export async function loadConfig(
833867
let resumeMode: "id" | "pick" | undefined;
834868
let resumeSessionId: string | undefined;
835869
const leading = args[0];
870+
// One exec verb: `exec` is canonical; `run` is a kept alias (help names
871+
// both). One resume verb: `resume` is canonical; `continue` is a kept
872+
// alias of the same flow.
836873
if (leading === "exec" || leading === "run") {
837874
command = "exec";
838875
args.shift();
839876
} else if (leading === "resume" || leading === "continue") {
840-
command = "tui";
841877
args.shift();
842878
// Bare resume opens the list; a session id is the only direct-resume path.
843879
// Invalid non-flag positionals error instead of falling through (a
844880
// free-form token would otherwise become task while skipInitialTask is set).
881+
// A following --pick/--list falls through to the loop's single resume
882+
// branch, where it is a kept alias of the bare --resume picker flow.
845883
const next = args.slice()[0];
846-
if (next === "--pick" || next === "--list") {
847-
resumeMode = "pick";
848-
args.shift();
849-
} else if (next !== undefined && !isFlagToken(next)) {
884+
if (next !== undefined && !isFlagToken(next)) {
850885
if (!isSessionId(next)) {
851886
throw new Error(
852887
`'${next}' is not a session id. Use a UUID session id or \`corbits resume\` to choose.`,
@@ -860,9 +895,10 @@ export async function loadConfig(
860895
}
861896
}
862897

863-
// `-p` is the exec one-shot path in any position (`-p --provider …`,
898+
// `-p` is the exec one-shot alias in any position (`-p --provider …`,
864899
// `--model … -p …`). Detect it before value flags so exec-only options
865-
// that appear before `-p` still see exec mode.
900+
// that appear before `-p` still see exec mode; the loop keeps the token
901+
// out of the task positionals.
866902
if (args.includes("-p")) {
867903
if (leading === "resume" || leading === "continue") {
868904
throw new Error(
@@ -934,11 +970,13 @@ export async function loadConfig(
934970
continue;
935971
}
936972

937-
if (arg === "--dangerously-skip-permissions" || arg === "--yolo") {
973+
if (SKIP_PERMISSIONS_FLAGS.has(arg)) {
938974
dangerouslySkipPermissions = true;
939975
continue;
940976
}
941977
if (arg === "--auto") {
978+
// Explicit re-assertion of the on-by-default auto mode. Kept because
979+
// sequence order still matters (`--no-auto --auto` re-enables).
942980
auto = true;
943981
continue;
944982
}
@@ -947,41 +985,45 @@ export async function loadConfig(
947985
continue;
948986
}
949987
if (arg === "-p") {
950-
command = "exec";
988+
// `-p` alias of exec: the pre-scan already fixed command="exec"; the
989+
// token must stay out of the task positionals.
951990
continue;
952991
}
953-
if (arg === "--resume") {
954-
if (resumeMode === "id") {
955-
throw new Error("cannot combine a session id with --resume");
956-
}
957-
// Optional session id on the interactive path: `corbits --resume <uuid>`
958-
// reopens that session (alias for `corbits resume <uuid>`); bare
959-
// `--resume` opens the picker. Exec / `-p` require the id — headless has
960-
// no picker. A non-id token errors instead of leaking into task text.
961-
const next = args[i + 1];
962-
if (next !== undefined && !isFlagToken(next)) {
963-
if (!isSessionId(next)) {
992+
if (arg === "--resume" || arg === "--pick" || arg === "--list") {
993+
// One resume verb: `--resume` carries the optional session id (exec
994+
// requires it; interactive bare `--resume` opens the picker, the alias
995+
// for `resume <session-id>`). `--pick`/`--list` are kept aliases of that
996+
// bare picker flow and may not follow a session id.
997+
if (arg === "--resume") {
998+
if (resumeMode === "id") {
999+
throw new Error("cannot combine a session id with --resume");
1000+
}
1001+
const next = args[i + 1];
1002+
if (next !== undefined && !isFlagToken(next)) {
1003+
if (!isSessionId(next)) {
1004+
throw new Error(
1005+
`'${next}' is not a session id. Use a UUID session id or \`corbits resume\` to choose.`,
1006+
);
1007+
}
1008+
resumeMode = "id";
1009+
resumeSessionId = next;
1010+
i++;
1011+
} else if (command === "exec") {
9641012
throw new Error(
965-
`'${next}' is not a session id. Use a UUID session id or \`corbits resume\` to choose.`,
1013+
`--resume requires a session id in exec mode. Use \`${COMMAND_NAME} resume\` to choose a session.`,
9661014
);
1015+
} else {
1016+
resumeMode = "pick";
9671017
}
968-
resumeMode = "id";
969-
resumeSessionId = next;
970-
i++;
971-
} else if (command === "exec") {
972-
throw new Error(
973-
`--resume requires a session id in exec mode. Use \`${COMMAND_NAME} resume\` to choose a session.`,
974-
);
975-
} else {
976-
resumeMode = "pick";
977-
}
978-
continue;
979-
}
980-
if ((arg === "--pick" || arg === "--list") && resumeMode !== undefined) {
981-
if (resumeMode === "id") {
1018+
} else if (resumeMode === "id") {
9821019
throw new Error("cannot combine a session id with --pick/--list");
1020+
} else if (resumeMode !== undefined) {
1021+
resumeMode = "pick";
1022+
} else {
1023+
// Without the resume verb these spellings are not in flow; preserve
1024+
// the previous fallthrough as an unrecognized flag.
1025+
throw new Error(`unrecognized flag: ${arg}`);
9831026
}
984-
resumeMode = "pick";
9851027
continue;
9861028
}
9871029
if (arg.startsWith("--")) {
@@ -1017,7 +1059,7 @@ export async function loadConfig(
10171059
useOAuthProfiles
10181060
? await Promise.all([listCodexProfiles(), listXaiProfiles()])
10191061
: [[], []];
1020-
let projectedOAuthProviders = {
1062+
const projectedOAuthProviders = {
10211063
...codexProvidersAsSettings(codexProfiles),
10221064
...xaiProvidersAsSettings(xaiProfiles),
10231065
};
@@ -1047,16 +1089,11 @@ export async function loadConfig(
10471089
settings?.dangerouslySkipPermissions === true;
10481090
dangerouslySkipPermissions =
10491091
dangerouslySkipPermissions || settings?.dangerouslySkipPermissions === true;
1050-
projectedOAuthProviders = applyPersistedOAuthDefaults(
1092+
// Single ordered projection chain (see applyOAuthProviderProjections).
1093+
const settingsForResolution: Settings | null = applyOAuthProviderProjections(
10511094
settings,
10521095
projectedOAuthProviders,
1053-
);
1054-
const liveSettings = useOAuthProfiles
1055-
? dropOrphanedOAuthEntries(settings, projectedOAuthProviders)
1056-
: settings;
1057-
const settingsForResolution: Settings | null = overlayOAuthProjections(
1058-
liveSettings,
1059-
projectedOAuthProviders,
1096+
{ dropOrphans: useOAuthProfiles },
10601097
);
10611098

10621099
// The per-repo selection file still applies on top of --config: that file

0 commit comments

Comments
 (0)