Support --effort on adversarial-review - #677
Conversation
`/codex:task` accepts `--effort`, but `/codex:adversarial-review` does not: `handleReviewCommand` omits it from `valueOptions`, so `lib/args.mjs` pushes the flag into positionals and `handleReviewCommand` joins positionals into the review's focus text. The flag is therefore not ignored -- it is injected as literal prose into the review prompt -- while `executeReviewRun` calls `runAppServerTurn` without an effort key and `turn/start` receives `effort: null`. The result is that reasoning effort is settable for tasks but not for reviews, where it falls back to `model_reasoning_effort` in config.toml. That file is global, so on a machine running several Claude Code sessions there is no way to raise effort for one review without changing every other session's reviews for the duration. This adds `"effort"` to the review path's `valueOptions`, normalizes it with the same `normalizeReasoningEffort` the task path uses, and threads it to the `turn/start` call that already accepts an `effort` parameter. Three object keys; no new machinery. Also documents the flag in the usage string, the command's argument-hint and the README, and adds a test asserting the parse, the threading, and the documentation stay in sync. Verified against `npm test`: 89 pass, 3 fail, with the same three failures present on an unmodified checkout (they concern `status` and `result` job bookkeeping and are unrelated). Note those runs need CLAUDE_PLUGIN_DATA pointed at a scratch directory -- the suite otherwise writes fixture registries into the developer's live plugin state, which is what a fourth apparent failure turned out to be.
![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: 225f6c3a69
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…path Addresses the review finding on openai#677. This PR advertises --model <model|spark> in the adversarial-review usage, but handleReviewCommand forwarded options.model unchanged, so turn/start received the literal "spark" instead of gpt-5.3-codex-spark. normalizeRequestedModel had exactly one call site, in handleTask, which is why the task path resolved the alias and the review path did not -- the same object literal already normalized effort but not model. The review path now calls normalizeRequestedModel the same way. This introduces no undefined-vs-null change: normalizeRequestedModel returns null for unset input, and runAppServerTurn already sends model: options.model ?? null. Tests: adds a behavioural test mirroring the existing task-path spark test, asserting turn/start receives gpt-5.3-codex-spark and effort "low" for adversarial-review --model spark --effort low. Verified non-vacuous against the unpatched tree, where it fails with actual 'spark'. That test also gives the --effort threading this PR adds its first behavioural coverage rather than source assertions alone.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0b7a09496
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…oring it Addresses the second review finding on openai#677. handleReviewCommand is shared by the native `review` subcommand and `adversarial-review`. Adding "effort" to its valueOptions therefore made `review --effort high` parse the flag as valid, and the native branch calls runAppServerReview without request.effort -- so the review silently ran at the configured default. Before this PR the unsupported flag stayed in focusText and validateNativeReviewRequest rejected it, so this was a regression that turned a loud failure into a silent one. The flag is still parsed, so the error can name it precisely, but a caller must now opt in with supportsEffort. Failing closed rather than keying off the review name means a future caller of this handler cannot inherit the silent drop by omission: if (options.effort !== undefined && !config.supportsEffort) throw ... The adversarial-review call site sets supportsEffort: true; the native one does not, and now reports: `/codex:review` maps directly to the built-in reviewer and does not support `--effort`. Retry with `/codex:adversarial-review --effort high` to choose a reasoning effort. Test mirrors the existing focus-text and staged-scope rejection tests. Verified non-vacuous against d0b7a09, where the flag is accepted and silently dropped.
|
@codex review 3750b7a is the current head and has not been reviewed — the previous passes covered 225f6c3 and d0b7a09. Both findings so far are fixed:
Worth scrutinising in this round: whether any other option parsed by |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Correction to my earlier test-suite claims on this PR. I reported 3 pre-existing failures on
function filterJobsForCurrentClaudeSession(jobs) {
const sessionId = getCurrentClaudeSessionId();
if (!sessionId) return jobs;
return jobs.filter((job) => job.sessionId === sessionId);
}The Unsetting the variable turns all three green, and every branch is fully green: So: there are no pre-existing failures on One thing this does suggest: since this plugin is for Claude Code, contributors are likely to run |
1 participant
/codex:taskaccepts--effort;/codex:adversarial-reviewdoes not.The behaviour today
handleReviewCommandomitseffortfromvalueOptions(codex-companion.mjs:714), solib/args.mjspushes the flag into positionals andhandleReviewCommandjoins positionals into the review's focus text. The flag isn't ignored — it's injected as literal prose into the review prompt. MeanwhileexecuteReviewRuncallsrunAppServerTurnwithout aneffortkey, soturn/startreceiveseffort: nulland the turn runs at whatevermodel_reasoning_effortsays inconfig.toml.Why it matters
config.tomlis global. On a machine running several Claude Code sessions concurrently, there is no way to raise effort for one sensitive review without changing every other session's reviews for its duration — and no job record captures what effort a review actually ran at, so the change isn't auditable afterwards.The change
Three object keys:
"effort"added to the review path'svalueOptionsnormalizeReasoningEffortthe task path already usesturn/startcall, which already accepts aneffortparameterNo new machinery — this makes the review path do what the task path does.
Also documents the flag in the usage string, the command's
argument-hint, and the README, plus a test asserting the parse, the threading, and the docs stay in sync.Verification
npm test: 89 pass, 3 fail — the same three failures occur on an unmodified checkout (status/resultjob bookkeeping, unrelated to this change).One note that may be worth a separate issue: those runs need
CLAUDE_PLUGIN_DATApointed at a scratch directory. Otherwise the suite writes fixture registries into the developer's live plugin state — on my machine an unisolated run left 144 of them, three carrying non-terminal jobs. A fourth apparent test failure turned out to be exactly that contamination.