From 66dcf2d6c97e99b18becaddf3504e8b57112fd48 Mon Sep 17 00:00:00 2001 From: taur-us <93385619+taur-us@users.noreply.github.com> Date: Sun, 6 Sep 2026 22:55:56 +1000 Subject: [PATCH] Support --effort on adversarial-review, and surface unrecognised options `/codex:adversarial-review --model gpt-6-astra --effort xhigh` silently does something other than what it says. `handleReviewCommand` parses valueOptions ["base","scope","model","cwd"], with no "effort" - only `handleTask` parses it. `parseArgs` then demotes any unrecognised long option to a positional, and `handleReviewCommand` builds the review's focus text with `positionals.join(" ")`. So the two tokens `--effort` and `xhigh` are concatenated into the prompt handed to the reviewer, the run uses whatever `model_reasoning_effort` the config holds, and nothing warns. The run looks like it did what was asked. That is two separate problems, fixed separately. 1. The review commands now accept `--effort`, normalised through the existing `normalizeReasoningEffort` and threaded into the `runAppServerTurn` call that the adversarial path already makes. `runAppServerTurn` already accepts an `effort` option, so this is plumbing rather than new capability. The usage line is updated to match, including the `--model` flag it already supported but did not advertise. 2. `parseArgs` now returns `unknownOptions` alongside `options` and `positionals`, and `parseCommandInput` warns on stderr for each one. Behaviour is deliberately unchanged - the token still reaches positionals, because commands such as adversarial-review take free-form focus text and a hard error would break a prose word that happens to start with two dashes. The point is only that the demotion stops being silent. This generalises past `--effort`: any future flag typo on any command currently ends up pasted into a prompt with no indication. Tests: five in tests/args.test.mjs, covering the unknown-option report, the clean case, the `--` passthrough boundary, `--effort` staying out of the focus text under the real review config, and quoted focus phrases. Mutation-tested by reverting the `unknownOptions.push` and confirming the first fails, then restoring it. Full suite on Windows goes from 79 passing / 12 failing to 84 passing / 12 failing. Those 12 fail identically on clean main - they are platform tests (Unix sockets, temp-backed state dirs) and are untouched by this change. --- plugins/codex/scripts/codex-companion.mjs | 21 ++++++-- plugins/codex/scripts/lib/args.mjs | 4 +- tests/args.test.mjs | 65 +++++++++++++++++++++++ 3 files changed, 86 insertions(+), 4 deletions(-) create mode 100644 tests/args.test.mjs diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index 83df468ad..f72b24967 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -78,7 +78,7 @@ function printUsage() { "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 adversarial-review [--wait|--background] [--base ] [--scope ] [--model ] [--effort ] [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]", @@ -139,13 +139,25 @@ function normalizeArgv(argv) { } function parseCommandInput(argv, config = {}) { - return parseArgs(normalizeArgv(argv), { + const parsed = parseArgs(normalizeArgv(argv), { ...config, aliasMap: { C: "cwd", ...(config.aliasMap ?? {}) } }); + + // An unrecognised long option is still treated as a positional, because some + // commands take free-form text. Say so on stderr rather than swallowing it: + // a mistyped or unsupported flag would otherwise be silently folded into a + // prompt, and the run would look like it did what was asked. + for (const token of parsed.unknownOptions ?? []) { + console.warn( + `Warning: unrecognised option ${token}; treating it as text. It will be passed through verbatim, not interpreted as a flag.` + ); + } + + return parsed; } function resolveCommandCwd(options = {}) { @@ -411,6 +423,7 @@ async function executeReviewRun(request) { const result = await runAppServerTurn(context.repoRoot, { prompt, model: request.model, + effort: request.effort, sandbox: "read-only", outputSchema: readOutputSchema(REVIEW_SCHEMA), onProgress: request.onProgress @@ -711,7 +724,7 @@ function enqueueBackgroundTask(cwd, job, request) { async function handleReviewCommand(argv, config) { const { options, positionals } = parseCommandInput(argv, { - valueOptions: ["base", "scope", "model", "cwd"], + valueOptions: ["base", "scope", "model", "effort", "cwd"], booleanOptions: ["json", "background", "wait"], aliasMap: { m: "model" @@ -720,6 +733,7 @@ async function handleReviewCommand(argv, config) { const cwd = resolveCommandCwd(options); const workspaceRoot = resolveCommandWorkspace(options); + const effort = normalizeReasoningEffort(options.effort); const focusText = positionals.join(" ").trim(); const target = resolveReviewTarget(cwd, { base: options.base, @@ -744,6 +758,7 @@ async function handleReviewCommand(argv, config) { base: options.base, scope: options.scope, model: options.model, + effort, focusText, reviewName: config.reviewName, onProgress: progress diff --git a/plugins/codex/scripts/lib/args.mjs b/plugins/codex/scripts/lib/args.mjs index 6b1518502..3454e65c3 100644 --- a/plugins/codex/scripts/lib/args.mjs +++ b/plugins/codex/scripts/lib/args.mjs @@ -4,6 +4,7 @@ export function parseArgs(argv, config = {}) { const aliasMap = config.aliasMap ?? {}; const options = {}; const positionals = []; + const unknownOptions = []; let passthrough = false; for (let index = 0; index < argv.length; index += 1) { @@ -45,6 +46,7 @@ export function parseArgs(argv, config = {}) { continue; } + unknownOptions.push(token); positionals.push(token); continue; } @@ -70,7 +72,7 @@ export function parseArgs(argv, config = {}) { positionals.push(token); } - return { options, positionals }; + return { options, positionals, unknownOptions }; } export function splitRawArgumentString(raw) { diff --git a/tests/args.test.mjs b/tests/args.test.mjs new file mode 100644 index 000000000..4f41b2924 --- /dev/null +++ b/tests/args.test.mjs @@ -0,0 +1,65 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { parseArgs, splitRawArgumentString } from "../plugins/codex/scripts/lib/args.mjs"; + +const REVIEW_CONFIG = { + valueOptions: ["base", "scope", "model", "effort", "cwd"], + booleanOptions: ["json", "background", "wait"], + aliasMap: { m: "model" } +}; + +test("parseArgs reports an unrecognised long option instead of silently demoting it", () => { + const { options, positionals, unknownOptions } = parseArgs( + ["--model", "gpt-6-astra", "--nonsense", "value"], + { valueOptions: ["model"], booleanOptions: [] } + ); + + assert.equal(options.model, "gpt-6-astra"); + // Behaviour is unchanged: the token still reaches positionals, because some + // commands take free-form text after their flags. + assert.deepEqual(positionals, ["--nonsense", "value"]); + // But it is now reported, so a caller can warn rather than swallow it. + assert.deepEqual(unknownOptions, ["--nonsense"]); +}); + +test("parseArgs reports nothing when every option is recognised", () => { + const { unknownOptions } = parseArgs(["--model", "gpt-6-astra", "--json"], { + valueOptions: ["model"], + booleanOptions: ["json"] + }); + + assert.deepEqual(unknownOptions, []); +}); + +test("parseArgs does not treat text after -- as an unrecognised option", () => { + const { positionals, unknownOptions } = parseArgs(["--", "--not-a-flag"], { + valueOptions: [], + booleanOptions: [] + }); + + assert.deepEqual(positionals, ["--not-a-flag"]); + assert.deepEqual(unknownOptions, []); +}); + +test("review commands accept --effort rather than folding it into the focus text", () => { + const { options, positionals, unknownOptions } = parseArgs( + ["--model", "gpt-6-astra", "--effort", "xhigh", "focus", "on", "auth"], + REVIEW_CONFIG + ); + + assert.equal(options.model, "gpt-6-astra"); + assert.equal(options.effort, "xhigh"); + assert.deepEqual(unknownOptions, []); + // The regression this guards: --effort and xhigh used to land here and be + // joined into the prompt the reviewer was given. + assert.deepEqual(positionals, ["focus", "on", "auth"]); + assert.equal(positionals.join(" "), "focus on auth"); +}); + +test("splitRawArgumentString keeps a quoted focus phrase together", () => { + assert.deepEqual( + splitRawArgumentString('--model gpt-6-astra --effort xhigh "the auth path"'), + ["--model", "gpt-6-astra", "--effort", "xhigh", "the auth path"] + ); +});