fix(review): accept positional focus text for parity with adversarial-review#523
fix(review): accept positional focus text for parity with adversarial-review#523axisrow wants to merge 3 commits into
Conversation
…-review Closes openai#522. `/codex:review` unconditionally rejected any positional argument as "custom focus text" (validateNativeReviewRequest), while `/codex:adversarial-review` accepts it. That interface-parity gap also blocked non-English model/effort entry: even when a host (e.g. Claude) rewrites a natural-language phrase into `--model`/`--effort`, any residual positional word still aborted the native review. Remove the unconditional focus-text rejection. The native reviewer (review/start) does not consume focus text, so leftover positional words are now silently ignored rather than rejecting the invocation. Review semantics are unchanged (still native structured-review); only the spurious abort is gone. Companion does no language/alias recognition — that is the host/.md layer's job. This fix is purely about parity: review no longer fails where adversarial-review succeeds. Tested: runtime parity test (red before, green after), commands.test.mjs assert updated to the new review.md wording. 0 regressions vs the openai#471 base (4 pre-existing fails unchanged). Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9b9481e97
ℹ️ 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".
| ); | ||
| } | ||
|
|
||
| function validateNativeReviewRequest(target, _focusText) { |
There was a problem hiding this comment.
Reject unknown switches before ignoring focus text
Because parseArgs treats any unrecognized --... token as a positional, ignoring all focusText here also silently accepts typos or unsupported flags such as /codex:review --bas main or /codex:review --scoope branch; in those cases the command now runs a review against the auto-selected target instead of failing, which can give users a successful review of the wrong changes. Please distinguish real positional focus text from option-looking positionals before dropping it.
Useful? React with 👍 / 👎.
🔍 Local review (cycle 1)Reviewed locally (Claude subagent + Codex companion), no bots pinged. Critical-only bar.
Codex companion: Claude subagent: 0 critical findings after verifying 6 claims against the code (focusText cannot leak into No |
After the parity fix, focusText was computed unconditionally and passed to validateNativeReviewRequest, which no longer reads it — dead in the native branch (review/start ignores focus text). Scope focusText to the adversarial branch (the only place it is used, via buildAdversarialReviewPrompt) and drop the now-unused validator parameter. No behavior change. Co-Authored-By: Claude <noreply@anthropic.com>
🔍 Local review (cycle 1, round 2 after cleanup commit)Reviewed locally (Claude subagent + Codex companion), no bots pinged. Critical-only bar.
Codex companion: Claude subagent: 0 critical after verifying 6 claims against the code (native path never reads focusText; cleanup commit did NOT break the adversarial path — focusText still reaches No |
validateNativeReviewRequest(target) takes one arg after the parity fix, but the call site still passed focusText as a second argument. Harmless (JS drops it), but dead. Simplify to the single-arg call. Co-Authored-By: Claude <noreply@anthropic.com>
…rom openai#523) Port the openai#523 review cleanups to fork-main: validateNativeReviewRequest takes a single arg (the parity fix made focusText dead in the native branch), the validateRequest call site no longer passes the dead 2nd arg, and focusText is now scoped to the adversarial branch (the only reader, via buildAdversarialReviewPrompt). No behavior change. Co-Authored-By: Claude <noreply@anthropic.com>
What
Removes the unconditional focus-text rejection in
/codex:reviewso it no longer aborts where/codex:adversarial-reviewsucceeds.Closes #522.
Why
/codex:reviewrejected any positional argument as "custom focus text" (validateNativeReviewRequest), while/codex:adversarial-reviewaccepts it. Two sibling review commands behaved differently in a way the user only discovered by hitting the error, and the suggested escape hatch (adversarial-review) changes the review semantics — not a substitute for "let me name the model in my own words."This also blocks non-English model/effort entry: even when a host (e.g. Claude Code reading the command's
.md) rewrites a natural-language phrase into--model/--effort, any residual positional word still aborted the native review (see the Japanese example in #522).Change
Remove the focus-text branch from
validateNativeReviewRequest. The native reviewer (review/start) does not consume focus text, so leftover positional words are now silently ignored rather than rejecting the invocation. Review semantics are unchanged (still native structured-review viareview/start); only the spurious abort is gone.This PR does not add language/alias recognition in the companion — that stays the host's job. The fix is purely about interface parity: review no longer fails where adversarial-review succeeds.
Scope
This is independent of #471 — the two touch different concerns (#471 is model/effort negotiation + broker refresh; this is a review-command parity fix). Based on
mainso it can land on its own.Testing
review accepts (ignores) positional focus text for parity with adversarial-review(red before, green after).review rejects focus texttest and thecommands.test.mjsassertion that pinned the old wording.main(the pre-existing suite failures are unchanged).🤖 Generated with Claude Code