diff --git a/packages/coding-agent/src/cli/args.ts b/packages/coding-agent/src/cli/args.ts index d5af659e6..735740399 100644 --- a/packages/coding-agent/src/cli/args.ts +++ b/packages/coding-agent/src/cli/args.ts @@ -6,6 +6,13 @@ import { APP_NAME, CONFIG_DIR_NAME, logger } from "@oh-my-pi/pi-utils"; import chalk from "chalk"; import { parseEffort } from "../thinking"; import { BUILTIN_TOOLS } from "../tools"; +import { + OPTIONAL_FLAGS, + OPTIONAL_VALUE_FLAGS, + type ParseDeps, + STRING_SETTERS, + STRING_VALUE_FLAGS, +} from "./flag-tables"; export type Mode = "text" | "json" | "rpc" | "acp" | "rpc-ui"; @@ -56,6 +63,19 @@ export interface Args { unknownFlags: Map; } +/** + * Runtime dependencies the data-driven setters need. Constructed once at + * module load and passed to every {@link STRING_SETTERS} call so the + * setter table itself can stay free of `@oh-my-pi/pi-utils` runtime imports + * (which would otherwise trip the profile bootstrap's env-init ordering). + */ +const PARSE_DEPS: ParseDeps = { + logger, + parseEffort, + BUILTIN_TOOLS, + THINKING_EFFORTS, +}; + export function parseArgs(args: string[], extensionFlags?: Map): Args { const result: Args = { messages: [], @@ -75,6 +95,23 @@ export function parseArgs(args: string[], extensionFlags?: Map s.trim()); } else if (arg === "--no-tools") { result.noTools = true; } else if (arg === "--no-lsp") { result.noLsp = true; } else if (arg === "--no-pty") { result.noPty = true; - } else if (arg === "--tools" && i + 1 < args.length) { - const toolNames = args[++i] - .split(",") - .map(s => s.trim().toLowerCase()) - .filter(Boolean); - const validTools: string[] = []; - for (const name of toolNames) { - if (name in BUILTIN_TOOLS) { - validTools.push(name); - } else { - logger.warn("Unknown tool passed to --tools", { - tool: name, - validTools: Object.keys(BUILTIN_TOOLS), - }); - } - } - result.tools = validTools; - } else if (arg === "--thinking" && i + 1 < args.length) { - const rawThinking = args[++i]; - const thinking = parseEffort(rawThinking); - if (thinking !== undefined) { - result.thinking = thinking; - } else { - logger.warn("Invalid thinking level passed to --thinking", { - level: rawThinking, - validThinkingLevels: THINKING_EFFORTS, - }); - } } else if (arg === "--print" || arg === "-p") { result.print = true; - } else if (arg === "--export" && i + 1 < args.length) { - result.export = args[++i]; - } else if (arg === "--hook" && i + 1 < args.length) { - result.hooks = result.hooks ?? []; - result.hooks.push(args[++i]); - } else if ((arg === "--extension" || arg === "-e") && i + 1 < args.length) { - result.extensions = result.extensions ?? []; - result.extensions.push(args[++i]); - } else if (arg === "--plugin-dir" && i + 1 < args.length) { - result.pluginDirs = result.pluginDirs ?? []; - result.pluginDirs.push(args[++i]); } else if (arg === "--no-extensions") { result.noExtensions = true; } else if (arg === "--no-skills") { @@ -186,30 +150,10 @@ export function parseArgs(args: string[], extensionFlags?: Map s.trim()); - } else if (arg === "--list-models") { - // Check if next arg is a search pattern (not a flag or file arg) - if (i + 1 < args.length && !args[i + 1].startsWith("-") && !args[i + 1].startsWith("@")) { - result.listModels = args[++i]; - } else { - result.listModels = true; - } } else if (arg.startsWith("@")) { - result.fileArgs.push(arg.slice(1)); // Remove @ prefix + result.fileArgs.push(arg.slice(1)); } else if (arg.startsWith("--") && extensionFlags) { - // Check if it's an extension-registered flag + // Extension-registered flags: dispatched dynamically via the runtime map. const flagName = arg.slice(2); const extFlag = extensionFlags.get(flagName); if (extFlag) { @@ -219,7 +163,7 @@ export function parseArgs(args: string[], extensionFlags?: Map) => void }; + parseEffort: (value: string | null | undefined) => Effort | undefined; + BUILTIN_TOOLS: Record; + THINKING_EFFORTS: readonly string[]; +} + +export type StringSetter = (result: Args, value: string, deps: ParseDeps) => void; + +/** + * Setter for a flag that may or may not consume the next argv token. + * Receives `undefined` for the bare form (`--resume` with no value, + * `--list-models` without a search pattern, etc.). + */ +export type OptionalSetter = (result: Args, value: string | undefined) => void; + +/** + * Per-flag optional-value consumption policy. + * + * Every optional flag always rejects tokens that start with `-` — that shared + * rule lives in the dispatch site. These booleans capture the *additional* + * per-flag quirks that previously lived inline in `args.ts`: + * + * - `rejectEmpty`: treat `""` like “no value provided”. Needed for + * `--resume` / `-r` / `--session`, which historically used a truthiness + * check (`next && !next.startsWith("-")`). Without this, an empty string + * gets consumed as the session prefix and downstream resolution can match + * every session. + * - `rejectAtPrefix`: reject `@foo` as a value. Used only by + * `--list-models`, which reserves `@...` for file arguments. + */ +export interface OptionalFlagConfig { + set: OptionalSetter; + rejectEmpty?: boolean; + rejectAtPrefix?: boolean; +} + +// Shared setters for flags that alias the same field. +const setExtension: StringSetter = (result, value) => { + result.extensions = result.extensions ?? []; + result.extensions.push(value); +}; + +const setResume: OptionalSetter = (result, value) => { + result.resume = value !== undefined ? value : true; +}; + +/** + * Setters for flags that ALWAYS consume the next argv token, even when that + * token starts with `-`. Mirrors the + * `arg === "--xxx" && i + 1 < args.length ? args[++i]` pattern in the old + * `parseArgs`. + */ +export const STRING_SETTERS: Record = { + "--mode": (result, value) => { + if (value === "text" || value === "json" || value === "rpc" || value === "acp" || value === "rpc-ui") { + result.mode = value; + } + }, + "--fork": (result, value) => { + result.fork = value; + }, + "--provider": (result, value) => { + result.provider = value; + }, + "--model": (result, value) => { + result.model = value; + }, + "--smol": (result, value) => { + result.smol = value; + }, + "--slow": (result, value) => { + result.slow = value; + }, + "--plan": (result, value) => { + result.plan = value; + }, + "--api-key": (result, value) => { + result.apiKey = value; + }, + "--system-prompt": (result, value) => { + result.systemPrompt = value; + }, + "--append-system-prompt": (result, value) => { + result.appendSystemPrompt = value; + }, + "--provider-session-id": (result, value) => { + result.providerSessionId = value; + }, + "--session-dir": (result, value) => { + result.sessionDir = value; + }, + "--models": (result, value) => { + result.models = value.split(",").map(s => s.trim()); + }, + "--tools": (result, value, deps) => { + const names = value + .split(",") + .map(s => s.trim().toLowerCase()) + .filter(Boolean); + const valid: string[] = []; + for (const name of names) { + if (name in deps.BUILTIN_TOOLS) { + valid.push(name); + } else { + deps.logger.warn("Unknown tool passed to --tools", { + tool: name, + validTools: Object.keys(deps.BUILTIN_TOOLS), + }); + } + } + result.tools = valid; + }, + "--thinking": (result, value, deps) => { + const thinking = deps.parseEffort(value); + if (thinking !== undefined) { + result.thinking = thinking; + } else { + deps.logger.warn("Invalid thinking level passed to --thinking", { + level: value, + validThinkingLevels: deps.THINKING_EFFORTS, + }); + } + }, + "--export": (result, value) => { + result.export = value; + }, + "--hook": (result, value) => { + result.hooks = result.hooks ?? []; + result.hooks.push(value); + }, + "--extension": setExtension, + "-e": setExtension, + "--plugin-dir": (result, value) => { + result.pluginDirs = result.pluginDirs ?? []; + result.pluginDirs.push(value); + }, + "--skills": (result, value) => { + result.skills = value.split(",").map(s => s.trim()); + }, + "--approval-mode": (result, value, deps) => { + if (value === "always-ask" || value === "write" || value === "yolo") { + result.approvalMode = value; + } else { + deps.logger.warn("Invalid value passed to --approval-mode", { + value, + validValues: ["always-ask", "write", "yolo"], + }); + } + }, +}; + +/** + * Optional-value flags. Setters receive `undefined` for the bare form. + * + * The dispatch in `args.ts` applies the shared "doesn't start with `-`" + * check for every flag, then consults the per-flag booleans below for the + * remaining quirks. + */ +export const OPTIONAL_FLAGS: Record = { + "--resume": { set: setResume, rejectEmpty: true }, + "-r": { set: setResume, rejectEmpty: true }, + "--session": { set: setResume, rejectEmpty: true }, + "--list-models": { + set: (result, value) => { + result.listModels = value !== undefined ? value : true; + }, + rejectAtPrefix: true, + }, +}; + +/** + * Derived from {@link STRING_SETTERS}. A flag is in this set if and only if + * it has a setter — by construction, drift between "the bootstrap thinks + * this flag consumes a value" and "the launch parser actually consumes one" + * is structurally impossible. + */ +export const STRING_VALUE_FLAGS: ReadonlySet = new Set(Object.keys(STRING_SETTERS)); + +/** + * Derived from {@link OPTIONAL_FLAGS}. Same single-source contract as + * {@link STRING_VALUE_FLAGS}. + */ +export const OPTIONAL_VALUE_FLAGS: ReadonlySet = new Set(Object.keys(OPTIONAL_FLAGS)); diff --git a/packages/coding-agent/src/cli/profile-bootstrap.ts b/packages/coding-agent/src/cli/profile-bootstrap.ts index d30830459..5f0d52340 100644 --- a/packages/coding-agent/src/cli/profile-bootstrap.ts +++ b/packages/coding-agent/src/cli/profile-bootstrap.ts @@ -16,47 +16,11 @@ * instead of passing the literal `--profile` to the system prompt and `foo` * as a positional message (issue raised by code review). * - * Keep these tables in sync with `packages/coding-agent/src/cli/args.ts`. Any - * flag added there that consumes a value must be mirrored here, otherwise the - * preparser can corrupt user-visible CLI interpretation. + * The shared classification lives in {@link ./flag-tables}, imported below, + * so the bootstrap and `args.ts` reference one source of truth instead of + * maintaining parallel constants. */ - -/** - * Flags that always consume the next argv token, even when that token starts - * with `-`. Mirrors the `arg === "--xxx" && i + 1 < args.length ? args[++i]` - * pattern in `args.ts`. - */ -const STRING_VALUE_FLAGS: ReadonlySet = new Set([ - "--mode", - "--fork", - "--provider", - "--model", - "--smol", - "--slow", - "--plan", - "--api-key", - "--system-prompt", - "--append-system-prompt", - "--provider-session-id", - "--session-dir", - "--models", - "--tools", - "--thinking", - "--export", - "--hook", - "--extension", - "-e", - "--plugin-dir", - "--skills", - "--approval-mode", -]); - -/** - * Flags that consume the next argv token only when it does not look like - * another flag. Mirrors the `if (next && !next.startsWith("-")) args[++i]` - * pattern in `args.ts`. - */ -const OPTIONAL_VALUE_FLAGS: ReadonlySet = new Set(["--resume", "-r", "--session", "--list-models"]); +import { OPTIONAL_FLAGS, OPTIONAL_VALUE_FLAGS, STRING_VALUE_FLAGS } from "./flag-tables"; export interface ProfileBootstrapResult { argv: string[]; @@ -144,9 +108,14 @@ export function extractProfileFlags(argv: readonly string[]): ProfileBootstrapRe if (OPTIONAL_VALUE_FLAGS.has(arg)) { stripped.push(arg); + const config = OPTIONAL_FLAGS[arg]; const next = argv[index + 1]; - // `--list-models` also rejects `@` prefixes (treated as file args by args.ts). - if (next !== undefined && !next.startsWith("-") && !next.startsWith("@")) { + if ( + next !== undefined && + !next.startsWith("-") && + !(config.rejectAtPrefix === true && next.startsWith("@")) && + !(config.rejectEmpty === true && next.length === 0) + ) { stripped.push(next); index += 1; } diff --git a/packages/coding-agent/test/flag-tables.test.ts b/packages/coding-agent/test/flag-tables.test.ts new file mode 100644 index 000000000..50d84a6d6 --- /dev/null +++ b/packages/coding-agent/test/flag-tables.test.ts @@ -0,0 +1,75 @@ +import { describe, expect, it } from "bun:test"; +import { parseArgs } from "../src/cli/args"; +import { OPTIONAL_VALUE_FLAGS, STRING_VALUE_FLAGS } from "../src/cli/flag-tables"; + +/** + * Catches the set → args.ts direction of drift between + * `cli/flag-tables.ts` and `cli/args.ts`: + * + * - If `STRING_VALUE_FLAGS` claims a flag consumes a value but + * `parseArgs` treats it as boolean (or doesn't handle it), then + * ` --profile work` would leave `--profile` standing — and + * parseArgs would activate the profile branch. We assert + * `result.profile` is undefined: the only way that's true is if the + * flag actually swallowed `--profile` as its value. + * + * - If `OPTIONAL_VALUE_FLAGS` claims a flag releases `-`-prefixed + * tokens but `parseArgs` swallows them anyway, then + * ` --profile work` would suppress the profile activation. We + * assert `result.profile === "work"`: the flag must NOT have eaten + * `--profile`, so parseArgs sees and activates it. + * + * The reverse direction (args.ts handler missing from the set) cannot + * be reflected on without parsing args.ts source — it's covered by + * per-flag regression tests in `profile-bootstrap.test.ts` and by + * user-facing scenarios in `profile-cli.test.ts`. + */ +describe("STRING_VALUE_FLAGS table is honored by args.ts parseArgs", () => { + for (const flag of STRING_VALUE_FLAGS) { + it(`${flag} consumes the next token unconditionally`, () => { + const result = parseArgs([flag, "--profile", "work"]); + expect( + result.profile, + `parseArgs should treat --profile as the value of ${flag}, not as a profile activation`, + ).toBeUndefined(); + }); + } +}); + +describe("OPTIONAL_VALUE_FLAGS table is honored by args.ts parseArgs", () => { + for (const flag of OPTIONAL_VALUE_FLAGS) { + it(`${flag} releases tokens that start with -`, () => { + const result = parseArgs([flag, "--profile", "work"]); + expect( + result.profile, + `parseArgs should release --profile back to its own handler when it follows ${flag}`, + ).toBe("work"); + }); + } +}); + +describe("OPTIONAL_FLAGS per-flag quirks", () => { + it("treats empty string as bare resume for --resume", () => { + const result = parseArgs(["--resume", ""]); + expect(result.resume).toBe(true); + expect(result.messages).toEqual([""]); + }); + + it("treats empty string as bare resume for -r", () => { + const result = parseArgs(["-r", ""]); + expect(result.resume).toBe(true); + expect(result.messages).toEqual([""]); + }); + + it("treats empty string as bare resume for --session", () => { + const result = parseArgs(["--session", ""]); + expect(result.resume).toBe(true); + expect(result.messages).toEqual([""]); + }); + + it("preserves existing empty-string behavior for --list-models", () => { + const result = parseArgs(["--list-models", ""]); + expect(result.listModels).toBe(""); + expect(result.messages).toEqual([]); + }); +}); diff --git a/packages/coding-agent/test/profile-bootstrap.test.ts b/packages/coding-agent/test/profile-bootstrap.test.ts index 6c75ea6fb..4d68e0605 100644 --- a/packages/coding-agent/test/profile-bootstrap.test.ts +++ b/packages/coding-agent/test/profile-bootstrap.test.ts @@ -61,6 +61,15 @@ describe("extractProfileFlags", () => { expect(filePrefixed.profile).toBe("work"); }); + it("does not consume empty-string resume values before a trailing profile", () => { + // Shared OPTIONAL_FLAGS metadata drives the bootstrap too. Empty string is + // "no value" for resume/session aliases, so the bootstrap must release it + // and still activate the trailing --profile. + const result = extractProfileFlags(["--resume", "", "--profile", "work"]); + expect(result.argv).toEqual(["--resume", ""]); + expect(result.profile).toBe("work"); + }); + it("honors `--` and stops scanning for flags", () => { const result = extractProfileFlags(["--", "--profile", "foo", "--alias", "bar"]); expect(result.profile).toBeUndefined();