Skip to content

Commit 7de747f

Browse files
axisrowclaude
andcommitted
fix(review): accept positional focus text for parity with adversarial-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>
1 parent 1cc7545 commit 7de747f

4 files changed

Lines changed: 18 additions & 13 deletions

File tree

plugins/codex/commands/review.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ Argument handling:
3636
- `--model` and `--effort` select the Codex runtime for the review and are not focus text.
3737
- Do not strip `--wait` or `--background` yourself.
3838
- The companion script parses `--wait` and `--background`, but Claude Code's `Bash(..., run_in_background: true)` is what actually detaches the run.
39-
- `/codex:review` is native-review only. It does not support staged-only review, unstaged-only review, or extra focus text.
39+
- `/codex:review` is native-review only. It does not support staged-only review or unstaged-only review. Positional focus text is ignored by the native reviewer (for interface parity with `/codex:adversarial-review`); it does not abort the review.
4040
- If the user needs custom review instructions or more adversarial framing, they should use `/codex:adversarial-review`.
4141

4242
Model/effort recognition (natural-language phrases):

plugins/codex/scripts/codex-companion.mjs

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -272,13 +272,13 @@ function buildNativeReviewTarget(target) {
272272
return null;
273273
}
274274

275-
function validateNativeReviewRequest(target, focusText) {
276-
if (focusText.trim()) {
277-
throw new Error(
278-
`\`/codex:review\` now maps directly to the built-in reviewer and does not support custom focus text. Retry with \`/codex:adversarial-review ${focusText.trim()}\` for focused review instructions.`
279-
);
280-
}
281-
275+
function validateNativeReviewRequest(target, _focusText) {
276+
// Parity with /codex:adversarial-review: positional focus text no longer
277+
// aborts the native review. The native reviewer (review/start) does not
278+
// consume focus text, so leftover positional words are silently ignored
279+
// rather than rejecting the invocation. This keeps `/codex:review --model sol`
280+
// usable when a host forwards residual positional text alongside flags,
281+
// and removes an interface-parity gap with /codex:adversarial-review (#522).
282282
const nativeTarget = buildNativeReviewTarget(target);
283283
if (!nativeTarget) {
284284
throw new Error("This `/codex:review` target is not supported by the built-in reviewer. Retry with `/codex:adversarial-review` for custom targeting.");

tests/commands.test.mjs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,8 @@ test("review command uses AskUserQuestion and background Bash while staying revi
3939
assert.match(source, /Claude Code's `Bash\(..., run_in_background: true\)` is what actually detaches the run/i);
4040
assert.match(source, /When in doubt, run the review/i);
4141
assert.match(source, /\(Recommended\)/);
42-
assert.match(source, /does not support staged-only review, unstaged-only review, or extra focus text/i);
42+
assert.match(source, /does not support staged-only review or unstaged-only review/i);
43+
assert.match(source, /Positional focus text is ignored by the native reviewer/i);
4344
assert.match(source, /Preserve the user's INTENT exactly/i);
4445
assert.match(source, /rewrite a natural-language model or effort mention into the corresponding/i);
4546
assert.match(source, /Model\/effort recognition \(natural-language phrases\):/i);

tests/runtime.test.mjs

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1321,7 +1321,7 @@ test("task --background enqueues a detached worker and exposes per-job status",
13211321
assert.equal(fakeState.lastTurnStart.effort, "max");
13221322
});
13231323

1324-
test("review rejects focus text because it is native-review only", () => {
1324+
test("review accepts (ignores) positional focus text for parity with adversarial-review", () => {
13251325
const repo = makeTempDir();
13261326
const binDir = makeTempDir();
13271327
installFakeCodex(binDir);
@@ -1336,9 +1336,13 @@ test("review rejects focus text because it is native-review only", () => {
13361336
env: buildEnv(binDir)
13371337
});
13381338

1339-
assert.equal(result.status > 0, true);
1340-
assert.match(result.stderr, /does not support custom focus text/i);
1341-
assert.match(result.stderr, /\/codex:adversarial-review focus on auth/i);
1339+
// Parity with /codex:adversarial-review: positional args no longer abort the
1340+
// native review. The native reviewer (review/start) does not consume focus
1341+
// text, so leftover words are ignored rather than rejecting the invocation.
1342+
// This keeps `/codex:review --model sol` usable when a host (e.g. Claude)
1343+
// forwards residual positional text alongside flags.
1344+
assert.equal(result.status, 0);
1345+
assert.doesNotMatch(result.stderr, /does not support custom focus text/i);
13421346
});
13431347

13441348
test("review rejects staged-only scope because it is native-review only", () => {

0 commit comments

Comments
 (0)