From a9b9481e97e42b5788941c7186a5304ea46b8572 Mon Sep 17 00:00:00 2001 From: axisrow Date: Sat, 18 Jul 2026 18:17:50 +0800 Subject: [PATCH 1/3] fix(review): accept positional focus text for parity with adversarial-review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #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 #471 base (4 pre-existing fails unchanged). Co-Authored-By: Claude --- plugins/codex/commands/review.md | 2 +- plugins/codex/scripts/codex-companion.mjs | 14 +++++++------- tests/commands.test.mjs | 3 ++- tests/runtime.test.mjs | 12 ++++++++---- 4 files changed, 18 insertions(+), 13 deletions(-) diff --git a/plugins/codex/commands/review.md b/plugins/codex/commands/review.md index fb70a4876..bbd462240 100644 --- a/plugins/codex/commands/review.md +++ b/plugins/codex/commands/review.md @@ -36,7 +36,7 @@ Argument handling: - Do not strip `--wait` or `--background` yourself. - Do not add extra review instructions or rewrite the user's intent. - The companion script parses `--wait` and `--background`, but Claude Code's `Bash(..., run_in_background: true)` is what actually detaches the run. -- `/codex:review` is native-review only. It does not support staged-only review, unstaged-only review, or extra focus text. +- `/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. - If the user needs custom review instructions or more adversarial framing, they should use `/codex:adversarial-review`. Foreground flow: diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index 83df468ad..cc8f4717e 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -268,13 +268,13 @@ function buildNativeReviewTarget(target) { return null; } -function validateNativeReviewRequest(target, focusText) { - if (focusText.trim()) { - throw new Error( - `\`/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.` - ); - } - +function validateNativeReviewRequest(target, _focusText) { + // Parity with /codex:adversarial-review: positional focus text no longer + // aborts the native review. The native reviewer (review/start) does not + // consume focus text, so leftover positional words are silently ignored + // rather than rejecting the invocation. This keeps `/codex:review --model sol` + // usable when a host forwards residual positional text alongside flags, + // and removes an interface-parity gap with /codex:adversarial-review (#522). const nativeTarget = buildNativeReviewTarget(target); if (!nativeTarget) { throw new Error("This `/codex:review` target is not supported by the built-in reviewer. Retry with `/codex:adversarial-review` for custom targeting."); diff --git a/tests/commands.test.mjs b/tests/commands.test.mjs index c34b06059..598e3bd47 100644 --- a/tests/commands.test.mjs +++ b/tests/commands.test.mjs @@ -36,7 +36,8 @@ test("review command uses AskUserQuestion and background Bash while staying revi assert.match(source, /Claude Code's `Bash\(..., run_in_background: true\)` is what actually detaches the run/i); assert.match(source, /When in doubt, run the review/i); assert.match(source, /\(Recommended\)/); - assert.match(source, /does not support staged-only review, unstaged-only review, or extra focus text/i); + assert.match(source, /does not support staged-only review or unstaged-only review/i); + assert.match(source, /Positional focus text is ignored by the native reviewer/i); }); test("adversarial review command uses AskUserQuestion and background Bash while staying review-only", () => { diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 8f276835b..27dbc0ee7 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -969,7 +969,7 @@ test("task --background enqueues a detached worker and exposes per-job status", assert.match(resultPayload.storedJob.rendered, /Handled the requested task/); }); -test("review rejects focus text because it is native-review only", () => { +test("review accepts (ignores) positional focus text for parity with adversarial-review", () => { const repo = makeTempDir(); const binDir = makeTempDir(); installFakeCodex(binDir); @@ -984,9 +984,13 @@ test("review rejects focus text because it is native-review only", () => { env: buildEnv(binDir) }); - assert.equal(result.status > 0, true); - assert.match(result.stderr, /does not support custom focus text/i); - assert.match(result.stderr, /\/codex:adversarial-review focus on auth/i); + // Parity with /codex:adversarial-review: positional args no longer abort the + // native review. The native reviewer (review/start) does not consume focus + // text, so leftover words are ignored rather than rejecting the invocation. + // This keeps `/codex:review --model sol` usable when a host (e.g. Claude) + // forwards residual positional text alongside flags. + assert.equal(result.status, 0); + assert.doesNotMatch(result.stderr, /does not support custom focus text/i); }); test("review rejects staged-only scope because it is native-review only", () => { From b9115b92538764185354192dd74619050159385a Mon Sep 17 00:00:00 2001 From: axisrow Date: Sat, 18 Jul 2026 21:03:36 +0800 Subject: [PATCH 2/3] refactor(review): drop dead focusText from the native-review path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- plugins/codex/scripts/codex-companion.mjs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index cc8f4717e..e737d2288 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -268,7 +268,7 @@ function buildNativeReviewTarget(target) { return null; } -function validateNativeReviewRequest(target, _focusText) { +function validateNativeReviewRequest(target) { // Parity with /codex:adversarial-review: positional focus text no longer // aborts the native review. The native reviewer (review/start) does not // consume focus text, so leftover positional words are silently ignored @@ -363,10 +363,9 @@ async function executeReviewRun(request) { base: request.base, scope: request.scope }); - const focusText = request.focusText?.trim() ?? ""; const reviewName = request.reviewName ?? "Review"; if (reviewName === "Review") { - const reviewTarget = validateNativeReviewRequest(target, focusText); + const reviewTarget = validateNativeReviewRequest(target); const result = await runAppServerReview(request.cwd, { target: reviewTarget, model: request.model, @@ -407,6 +406,7 @@ async function executeReviewRun(request) { } const context = collectReviewContext(request.cwd, target); + const focusText = request.focusText?.trim() ?? ""; const prompt = buildAdversarialReviewPrompt(context, focusText); const result = await runAppServerTurn(context.repoRoot, { prompt, From 41e40fad9b1a2f7d03235eddc109dc892fc1f5bd Mon Sep 17 00:00:00 2001 From: axisrow Date: Sun, 19 Jul 2026 00:39:13 +0800 Subject: [PATCH 3/3] refactor(review): drop dead focusText arg from validateRequest call 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 --- plugins/codex/scripts/codex-companion.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index e737d2288..e2418aa4b 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -726,7 +726,7 @@ async function handleReviewCommand(argv, config) { scope: options.scope }); - config.validateRequest?.(target, focusText); + config.validateRequest?.(target); const metadata = buildReviewJobMetadata(config.reviewName, target); const job = createCompanionJob({ prefix: "review",