Support --effort on adversarial-review, and surface unrecognised options - #746
Open
taur-us wants to merge 1 commit into
Open
Support --effort on adversarial-review, and surface unrecognised options#746taur-us wants to merge 1 commit into
taur-us wants to merge 1 commit into
Conversation
`/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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66dcf2d6c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| " node scripts/codex-companion.mjs setup [--enable-review-gate|--disable-review-gate] [--json]", | ||
| " node scripts/codex-companion.mjs review [--wait|--background] [--base <ref>] [--scope <auto|working-tree|branch>]", | ||
| " node scripts/codex-companion.mjs adversarial-review [--wait|--background] [--base <ref>] [--scope <auto|working-tree|branch>] [focus text]", | ||
| " node scripts/codex-companion.mjs adversarial-review [--wait|--background] [--base <ref>] [--scope <auto|working-tree|branch>] [--model <model|spark>] [--effort <none|minimal|low|medium|high|xhigh>] [focus text]", |
There was a problem hiding this comment.
When a user follows this newly advertised --model spark form, handleReviewCommand forwards the raw options.model to the app-server instead of applying normalizeRequestedModel (unlike task). The app-server therefore receives the literal model name spark rather than gpt-5.3-codex-spark, so this documented invocation can fail model selection instead of running the adversarial review.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
/codex:adversarial-review --model gpt-6-astra --effort xhighsilently does something other than what it says, with no error and no warning.Two independent causes compound:
--effortis not a flag on the review commands.handleReviewCommand(codex-companion.mjs#L713-L719) parsesvalueOptions: ["base", "scope", "model", "cwd"]. OnlyhandleTask(#L763-L769) parseseffort.Unrecognised long options are silently demoted to positionals.
args.mjs#L48falls through topositionals.push(token)after the known-flag branches, andhandleReviewCommandthen builds the review's focus text withpositionals.join(" ")(#L723).So the literal tokens
--effortandxhighget concatenated into the prompt handed to the reviewer, the run uses whatevermodel_reasoning_effortthe config holds, and the invocation looks like it worked.The second cause is the more general one. It is not specific to
--effort: any mistyped or unsupported flag on any command currently ends up pasted verbatim into a prompt with nothing said about it.This is not hypothetical for us. Our own always-loaded team instructions had been recommending exactly that invocation as the standard escalation, so every session following them was getting a corrupted prompt at the wrong reasoning effort. We only noticed by reading the parser.
The fix
1. Review commands accept
--effort. Normalised through the existingnormalizeReasoningEffort, then threaded into therunAppServerTurncall the adversarial path already makes.runAppServerTurnalready accepts aneffortoption, so this is plumbing rather than new capability. The usage line is updated to match, including the--modelflag it already supported but never advertised.2.
parseArgsreports what it demoted. It now returnsunknownOptionsalongsideoptionsandpositionals, andparseCommandInputwarns on stderr for each.Behaviour is deliberately unchanged: the token still reaches
positionals. Commands likeadversarial-reviewtake free-form focus text, so a hard error would break a prose word that happens to start with two dashes, and existing callers that destructure{ options, positionals }are unaffected. The only change is that the demotion stops being silent.I am happy to make it a hard rejection instead if you would prefer that — it is a smaller change, and arguably the right one for the strictly-flagged commands — but a warning seemed the safer default given the free-text commands.
Tests
Five new tests in
tests/args.test.mjs, covering the unknown-option report, the clean case, the--passthrough boundary,--effortstaying out of the focus text under the real review config, and quoted focus phrases.Mutation-tested rather than merely written: reverting the
unknownOptions.pushline makes the first test fail, and restoring it makes it pass.Suite status
Run on Windows,
node --test tests/*.test.mjs:mainThe 12 failures are identical on clean
mainand are untouched by this change — they are platform tests (Unix sockets, temp-backed state dirs) that do not pass on Windows. Happy to open a separate issue for those if useful.🤖 Generated with Claude Code
https://claude.ai/code/session_01MbutZdFgSLna89DcLEhe7Z