diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index 83df468ad..c334a506e 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -72,20 +72,102 @@ const VALID_REASONING_EFFORTS = new Set(["none", "minimal", "low", "medium", "hi const MODEL_ALIASES = new Map([["spark", "gpt-5.3-codex-spark"]]); const STOP_REVIEW_TASK_MARKER = "Run a stop-gate review of the previous Claude turn."; -function printUsage() { - console.log( - [ - "Usage:", - " node scripts/codex-companion.mjs setup [--enable-review-gate|--disable-review-gate] [--json]", - " node scripts/codex-companion.mjs review [--wait|--background] [--base ] [--scope ]", - " node scripts/codex-companion.mjs adversarial-review [--wait|--background] [--base ] [--scope ] [focus text]", - " node scripts/codex-companion.mjs task [--background] [--write] [--resume-last|--resume|--fresh] [--model ] [--effort ] [prompt]", - " node scripts/codex-companion.mjs transfer [--source ] [--json]", - " node scripts/codex-companion.mjs status [job-id] [--all] [--json]", - " node scripts/codex-companion.mjs result [job-id] [--json]", - " node scripts/codex-companion.mjs cancel [job-id] [--json]" - ].join("\n") - ); +const USAGE_LINES = new Map([ + ["setup", " node scripts/codex-companion.mjs setup [--enable-review-gate|--disable-review-gate] [--json]"], + ["review", " node scripts/codex-companion.mjs review [--wait|--background] [--base ] [--scope ]"], + ["adversarial-review", " node scripts/codex-companion.mjs adversarial-review [--wait|--background] [--base ] [--scope ] [focus text]"], + ["task", " node scripts/codex-companion.mjs task [--background] [--write] [--resume-last|--resume|--fresh] [--model ] [--effort ] [prompt]"], + ["transfer", " node scripts/codex-companion.mjs transfer [--source ] [--json]"], + ["status", " node scripts/codex-companion.mjs status [job-id] [--all] [--json]"], + ["result", " node scripts/codex-companion.mjs result [job-id] [--json]"], + ["cancel", " node scripts/codex-companion.mjs cancel [job-id] [--json]"] +]); + +// A help request must never become a dispatch. Every subcommand parser treats an +// unrecognised token as a positional, so `adversarial-review --help` was joined into the +// review's focus text and ran a full review -- minutes of wall clock and a model turn, +// for someone who asked what the flags were. +// positionalsAreFreeForm marks the subcommands whose positionals are arbitrary user +// prose, which is the only case where a leftover positional should block a help request: +// there, a --help token may be something the user meant literally. It is a predicate over +// the parsed options rather than a flag, because for task it depends on whether +// --prompt-file displaced the positional. Everything else -- a structured job id, or a +// positional the handler discards -- must let help win. +const REVIEW_PARSE_OPTIONS = { + valueOptions: ["base", "scope", "model", "cwd"], + booleanOptions: ["json", "background", "wait"], + aliasMap: { m: "model" } +}; + +// One schema per subcommand, read by BOTH the handler and help detection. Keeping a +// separate hand-maintained list for help detection is what broke it: an option the help +// parser did not know (`--wait`) became a positional, help was not detected, and the real +// parser then consumed the option and reviewed `--help` as focus text. Any option added +// here is automatically known to both, so the two can never disagree again. +const COMMAND_OPTION_SCHEMAS = new Map([ + ["setup", { valueOptions: ["cwd"], booleanOptions: ["json", "enable-review-gate", "disable-review-gate"] }], + // Same parsing, different positional semantics. adversarial-review takes focus text, so + // a --help token there may be prose. Native review rejects ALL focus text in + // validateNativeReviewRequest, so it has no free-form use and help can win over any + // positional -- otherwise `review "--scope working-tree focus --help"` answers a help + // request with a confusing complaint about custom focus text. + ["review", REVIEW_PARSE_OPTIONS], + ["adversarial-review", { ...REVIEW_PARSE_OPTIONS, positionalsAreFreeForm: () => true }], + [ + "task", + { + valueOptions: ["model", "effort", "cwd", "prompt-file"], + booleanOptions: ["json", "write", "resume-last", "resume", "fresh", "background"], + aliasMap: { m: "model" }, + // Only when the positional is actually the prompt. readTaskPrompt returns the file + // unconditionally when --prompt-file is given, so a positional alongside it is + // discarded -- there is no literal prompt text to protect, and suppressing help + // over it starts a real turn for someone who asked what the flags were. + positionalsAreFreeForm: (options) => !options["prompt-file"] + } + ], + ["transfer", { valueOptions: ["cwd", "source"], booleanOptions: ["json"] }], + ["status", { valueOptions: ["cwd", "timeout-ms", "poll-interval-ms"], booleanOptions: ["json", "all", "wait"] }], + ["result", { valueOptions: ["cwd"], booleanOptions: ["json"] }], + ["cancel", { valueOptions: ["cwd"], booleanOptions: ["json"] }] +]); + +// Parse with the subcommand's real schema, then treat it as help only when the flag is +// present AND nothing else was asked for. Scanning tokens cannot work here: argv is +// normalized first, so focus text is split into tokens too, and a scan would match +// `adversarial-review "why does --help start a review"` and make that review impossible +// to run. Parsing also gets `--` right for free -- anything after it is a positional, so +// a literal `--help` in focus text stays focus text. +function isHelpRequest(subcommand, argv) { + const schema = COMMAND_OPTION_SCHEMAS.get(subcommand); + if (!schema) { + return false; + } + // Detect through parseCommandInput, not parseArgs: the handlers reach the parser that + // way, so this inherits the shared `-C` alias and the argv normalization instead of + // restating them. Calling parseArgs directly is what missed `-C --help` -- the + // alias was unknown here, so the flag and its value became positionals, help was not + // detected, and the handler then dispatched with --help left as input. + const { options, positionals } = parseCommandInput(argv, { + ...schema, + booleanOptions: [...(schema.booleanOptions ?? []), "help"], + aliasMap: { ...(schema.aliasMap ?? {}), h: "help" } + }); + if (options.help !== true) { + return false; + } + // A positional only blocks help when it is genuinely free-form input the user may have + // meant literally. Where it is a structured job id (`cancel "job-1 --help"`), or where + // the handler discards it anyway (`task --prompt-file f.txt ignored`), it is not a + // reason to dispatch. + const freeForm = + typeof schema.positionalsAreFreeForm === "function" && schema.positionalsAreFreeForm(options); + return !freeForm || positionals.length === 0; +} + +function printUsage(subcommand) { + const line = subcommand ? USAGE_LINES.get(subcommand) : null; + console.log(["Usage:", ...(line ? [line] : USAGE_LINES.values())].join("\n")); } function outputResult(value, asJson) { @@ -214,8 +296,7 @@ async function buildSetupReport(cwd, actionsTaken = []) { async function handleSetup(argv) { const { options } = parseCommandInput(argv, { - valueOptions: ["cwd"], - booleanOptions: ["json", "enable-review-gate", "disable-review-gate"] + ...COMMAND_OPTION_SCHEMAS.get("setup") }); if (options["enable-review-gate"] && options["disable-review-gate"]) { @@ -711,11 +792,7 @@ function enqueueBackgroundTask(cwd, job, request) { async function handleReviewCommand(argv, config) { const { options, positionals } = parseCommandInput(argv, { - valueOptions: ["base", "scope", "model", "cwd"], - booleanOptions: ["json", "background", "wait"], - aliasMap: { - m: "model" - } + ...REVIEW_PARSE_OPTIONS }); const cwd = resolveCommandCwd(options); @@ -761,11 +838,7 @@ async function handleReview(argv) { async function handleTask(argv) { const { options, positionals } = parseCommandInput(argv, { - valueOptions: ["model", "effort", "cwd", "prompt-file"], - booleanOptions: ["json", "write", "resume-last", "resume", "fresh", "background"], - aliasMap: { - m: "model" - } + ...COMMAND_OPTION_SCHEMAS.get("task") }); const cwd = resolveCommandCwd(options); @@ -824,8 +897,7 @@ async function handleTask(argv) { async function handleTransfer(argv) { const { options } = parseCommandInput(argv, { - valueOptions: ["cwd", "source"], - booleanOptions: ["json"] + ...COMMAND_OPTION_SCHEMAS.get("transfer") }); const cwd = resolveCommandCwd(options); @@ -882,8 +954,7 @@ async function handleTaskWorker(argv) { async function handleStatus(argv) { const { options, positionals } = parseCommandInput(argv, { - valueOptions: ["cwd", "timeout-ms", "poll-interval-ms"], - booleanOptions: ["json", "all", "wait"] + ...COMMAND_OPTION_SCHEMAS.get("status") }); const cwd = resolveCommandCwd(options); @@ -909,8 +980,7 @@ async function handleStatus(argv) { function handleResult(argv) { const { options, positionals } = parseCommandInput(argv, { - valueOptions: ["cwd"], - booleanOptions: ["json"] + ...COMMAND_OPTION_SCHEMAS.get("result") }); const cwd = resolveCommandCwd(options); @@ -962,8 +1032,7 @@ function handleTaskResumeCandidate(argv) { async function handleCancel(argv) { const { options, positionals } = parseCommandInput(argv, { - valueOptions: ["cwd"], - booleanOptions: ["json"] + ...COMMAND_OPTION_SCHEMAS.get("cancel") }); const cwd = resolveCommandCwd(options); @@ -1023,11 +1092,21 @@ async function handleCancel(argv) { async function main() { const [subcommand, ...argv] = process.argv.slice(2); - if (!subcommand || subcommand === "help" || subcommand === "--help") { + if (!subcommand || subcommand === "help" || subcommand === "--help" || subcommand === "-h") { printUsage(); return; } + // Normalize first. The plugin commands pass "$ARGUMENTS" as ONE quoted argument, so + // `/codex:adversarial-review --base main --help` arrives here as a single string and a + // raw token comparison misses the flag -- the handler would then split it itself and + // start a full review with --help as focus text, which is exactly what this prevents. + // Checked before the switch, so help can never reach a handler that would dispatch. + if (USAGE_LINES.has(subcommand) && isHelpRequest(subcommand, argv)) { + printUsage(subcommand); + return; + } + switch (subcommand) { case "setup": await handleSetup(argv); diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 8f276835b..55e979912 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -969,6 +969,312 @@ test("task --background enqueues a detached worker and exposes per-job status", assert.match(resultPayload.storedJob.rendered, /Handled the requested task/); }); +test("adversarial-review --help prints usage without dispatching a review", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const statePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "README.md"), "hello again\n"); + + const result = run("node", [SCRIPT, "adversarial-review", "--help"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /^Usage:/); + assert.match(result.stdout, /adversarial-review \[--wait\|--background\]/); + + // The point of the change: an unrecognised flag used to become focus text, so asking + // for help started a real review. Nothing may reach the model. + const state = fs.existsSync(statePath) ? JSON.parse(fs.readFileSync(statePath, "utf8")) : {}; + assert.equal(state.lastTurnStart ?? null, null, "a help request started a Codex turn"); +}); + +test("help is detected when the plugin passes all arguments as one string", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const statePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "README.md"), "hello again\n"); + + // The plugin commands invoke the companion with "$ARGUMENTS" as a SINGLE quoted + // argument, so this is the shape real usage takes. Comparing raw tokens misses the + // flag here, and the handler then splits the string itself and reviews with --help as + // focus text. + const result = run("node", [SCRIPT, "adversarial-review", "--base main --help"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /^Usage:/); + const state = fs.existsSync(statePath) ? JSON.parse(fs.readFileSync(statePath, "utf8")) : {}; + assert.equal(state.lastTurnStart ?? null, null, "a help request started a Codex turn"); +}); + +test("task honours help when --prompt-file makes the positional irrelevant", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const statePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "prompt.txt"), "the real prompt\n"); + + // readTaskPrompt returns the file unconditionally when --prompt-file is set, so the + // positional is discarded. Treating it as protected prompt text suppressed help and + // started a real turn using prompt.txt. + const result = run("node", [SCRIPT, "task", "--prompt-file prompt.txt ignored --help"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /^Usage:/); + const state = fs.existsSync(statePath) ? JSON.parse(fs.readFileSync(statePath, "utf8")) : {}; + assert.equal(state.lastTurnStart ?? null, null, "a help request started a Codex turn"); +}); + +test("task still protects a literal prompt that mentions --help", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const statePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + + // Without --prompt-file the positional IS the prompt, so it must still block help. + const result = run("node", [SCRIPT, "task", "explain what --help prints"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + assert.equal(/^Usage:/.test(result.stdout), false, "a literal prompt printed usage"); + const state = JSON.parse(fs.readFileSync(statePath, "utf8")); + assert.ok(state.lastTurnStart, "the task never started"); + assert.match(state.lastTurnStart.prompt, /what --help prints/); +}); + +test("native review honours help even when a positional is present", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const statePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "README.md"), "hello again\n"); + + // Native review rejects all focus text in validateNativeReviewRequest, so unlike + // adversarial-review it has no free-form positional use. Sharing the free-form + // classification made a help request answered with a complaint about focus text. + const result = run("node", [SCRIPT, "review", "--scope working-tree focus --help"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /^Usage:/); + assert.equal(/custom focus text/.test(result.stderr), false, "help was answered with the focus-text error"); + const state = fs.existsSync(statePath) ? JSON.parse(fs.readFileSync(statePath, "utf8")) : {}; + assert.equal(state.lastTurnStart ?? null, null, "a help request started a Codex turn"); +}); + +test("cancel with a job id and --help prints usage without cancelling the job", () => { + const workspace = makeTempDir(); + const stateDir = resolveStateDir(workspace); + fs.mkdirSync(path.join(stateDir, "jobs"), { recursive: true }); + fs.writeFileSync( + path.join(stateDir, "state.json"), + `${JSON.stringify( + { + version: 1, + config: { stopReviewGate: false }, + jobs: [ + { + id: "task-live", + kind: "task", + kindLabel: "task", + status: "running", + title: "Codex Task", + jobClass: "task", + summary: "A job that must survive a help request", + createdAt: "2026-03-18T15:30:00.000Z", + updatedAt: "2026-03-18T15:30:03.000Z" + } + ] + }, + null, + 2 + )}\n`, + "utf8" + ); + + // cancel takes a structured job id, not free-form text, so a leftover positional is no + // reason to dispatch. Applying the free-form rule here made `cancel "task-live --help"` + // cancel task-live instead of explaining the command. + const result = run("node", [SCRIPT, "cancel", "task-live --help"], { cwd: workspace }); + + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /^Usage:/); + + const after = JSON.parse(fs.readFileSync(path.join(stateDir, "state.json"), "utf8")); + assert.equal(after.jobs[0].status, "running", "a help request cancelled the job"); +}); + +test("help is detected when combined with the shared -C alias", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const statePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "README.md"), "hello again\n"); + + // -C is injected by parseCommandInput, not by any subcommand schema. Detecting help + // with a bare parseArgs call misses it, so -C and its value become positionals, the + // no-focus-text rule fails, and the handler dispatches with --help still in the input. + const result = run("node", [SCRIPT, "adversarial-review", `-C ${repo} --help`], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /^Usage:/); + const state = fs.existsSync(statePath) ? JSON.parse(fs.readFileSync(statePath, "utf8")) : {}; + assert.equal(state.lastTurnStart ?? null, null, "a help request started a Codex turn"); +}); + +test("help is detected when it follows another recognized flag", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const statePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "README.md"), "hello\n"); + run("git", ["add", "README.md"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "README.md"), "hello again\n"); + + // Help detection must parse with the subcommand's real schema. A separate list would + // not know --wait, which would land in positionals, defeat the no-focus-text rule, and + // let the real parser consume --wait and review "--help" as focus text. + const result = run("node", [SCRIPT, "adversarial-review", "--wait --help"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /^Usage:/); + const state = fs.existsSync(statePath) ? JSON.parse(fs.readFileSync(statePath, "utf8")) : {}; + assert.equal(state.lastTurnStart ?? null, null, "a help request started a Codex turn"); +}); + +test("a quoted focus argument keeps its help-looking words as focus text", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const statePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir); + initGitRepo(repo); + fs.mkdirSync(path.join(repo, "src")); + fs.writeFileSync(path.join(repo, "src", "app.js"), "export const value = items[0];\n"); + run("git", ["add", "src/app.js"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "src", "app.js"), "export const value = items[0].id;\n"); + + const result = run("node", [SCRIPT, "adversarial-review", "review --help handling"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + assert.equal(/^Usage:/.test(result.stdout), false, "a focused review printed usage instead"); + const state = JSON.parse(fs.readFileSync(statePath, "utf8")); + assert.ok(state.lastTurnStart, "the review never started"); + assert.match(state.lastTurnStart.prompt, /handling/); +}); + +test("focus text containing --help is reviewed, not swallowed as a help request", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const statePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir); + initGitRepo(repo); + fs.mkdirSync(path.join(repo, "src")); + fs.writeFileSync(path.join(repo, "src", "app.js"), "export const value = items[0];\n"); + run("git", ["add", "src/app.js"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "src", "app.js"), "export const value = items[0].id;\n"); + + // Normalization splits focus text into tokens, so a token scan would see --help here and + // make this review impossible to run. Help means the flag AND nothing else asked for. + const result = run("node", [SCRIPT, "adversarial-review", "why does --help start a review"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + assert.equal(/^Usage:/.test(result.stdout), false, "focus text containing --help printed usage"); + const state = JSON.parse(fs.readFileSync(statePath, "utf8")); + assert.ok(state.lastTurnStart, "the review never started"); + assert.match(state.lastTurnStart.prompt, /start a review/); +}); + +test("focus text mentioning help still runs the review", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + const statePath = path.join(binDir, "fake-codex-state.json"); + installFakeCodex(binDir); + initGitRepo(repo); + fs.mkdirSync(path.join(repo, "src")); + fs.writeFileSync(path.join(repo, "src", "app.js"), "export const value = items[0];\n"); + run("git", ["add", "src/app.js"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "src", "app.js"), "export const value = items[0].id;\n"); + + // Because argv is normalized before the check, a bare "help" token appears in ordinary + // focus text. Matching it would silently swap the user's review for a usage dump. + const result = run("node", [SCRIPT, "adversarial-review", "review the help system"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + assert.equal(/^Usage:/.test(result.stdout), false, "focus text containing 'help' printed usage"); + const state = JSON.parse(fs.readFileSync(statePath, "utf8")); + assert.ok(state.lastTurnStart, "the review never started"); + assert.match(state.lastTurnStart.prompt, /help system/); +}); + +test("subcommand help accepts -h and prints only that subcommand", () => { + const binDir = makeTempDir(); + installFakeCodex(binDir); + + const result = run("node", [SCRIPT, "task", "-h"], { cwd: makeTempDir(), env: buildEnv(binDir) }); + + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /codex-companion\.mjs task \[--background\]/); + // Scoped, not the whole usage block. + assert.equal(/adversarial-review/.test(result.stdout), false, "task -h printed other subcommands"); +}); + test("review rejects focus text because it is native-review only", () => { const repo = makeTempDir(); const binDir = makeTempDir();