From d22037c7517729bf0c5fa09ab21f6e90a603ffc3 Mon Sep 17 00:00:00 2001 From: Kent Date: Sun, 17 May 2026 17:58:16 +0800 Subject: [PATCH] fix: raise adversarial review inline diff limits and expose overrides Default inline-diff caps (2 files / 256KB) were tuned for early-2024 context windows and caused /codex:adversarial-review to fail on normal-sized PRs. The self-collect path tells Codex to read the diff itself with git commands, but the same turn enforces outputSchema with sandbox: read-only and no shell tool exposed, so Codex emits a tool-call JSON that fails schema validation. Raise defaults to 50 files / 1 MB so the contradictory self-collect path is rarely reached in practice, and add --max-inline-files / --max-inline-bytes overrides on /codex:adversarial-review for users who hit edge cases or want to force the lightweight prompt for debugging. --- plugins/codex/commands/adversarial-review.md | 3 +- plugins/codex/scripts/codex-companion.mjs | 24 ++++++- plugins/codex/scripts/lib/git.mjs | 4 +- tests/commands.test.mjs | 4 +- tests/git.test.mjs | 44 ++++++++++++- tests/runtime.test.mjs | 68 +++++++++++++++++++- 6 files changed, 137 insertions(+), 10 deletions(-) diff --git a/plugins/codex/commands/adversarial-review.md b/plugins/codex/commands/adversarial-review.md index da440ab4..b24d3b56 100644 --- a/plugins/codex/commands/adversarial-review.md +++ b/plugins/codex/commands/adversarial-review.md @@ -1,6 +1,6 @@ --- description: Run a Codex review that challenges the implementation approach and design choices -argument-hint: '[--wait|--background] [--base ] [--scope auto|working-tree|branch] [focus ...]' +argument-hint: '[--wait|--background] [--base ] [--scope auto|working-tree|branch] [--max-inline-files ] [--max-inline-bytes ] [focus ...]' disable-model-invocation: true allowed-tools: Read, Glob, Grep, Bash(node:*), Bash(git:*), AskUserQuestion --- @@ -43,6 +43,7 @@ Argument handling: - It supports working-tree review, branch review, and `--base `. - It does not support `--scope staged` or `--scope unstaged`. - Unlike `/codex:review`, it can still take extra focus text after the flags. +- `--max-inline-files ` and `--max-inline-bytes ` override the inline diff limits when the diff is too large or too small for the defaults; pass `0` to force the lightweight self-collect prompt. Foreground flow: - Run: diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index 35222fd5..3f1d81aa 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -76,7 +76,7 @@ function printUsage() { "Usage:", " node scripts/codex-companion.mjs setup [--enable-review-gate|--disable-review-gate] [--json]", " node scripts/codex-companion.mjs review [--wait|--background] [--base ] [--scope ]", - " node scripts/codex-companion.mjs adversarial-review [--wait|--background] [--base ] [--scope ] [focus text]", + " node scripts/codex-companion.mjs adversarial-review [--wait|--background] [--base ] [--scope ] [--max-inline-files ] [--max-inline-bytes ] [focus text]", " node scripts/codex-companion.mjs task [--background] [--write] [--resume-last|--resume|--fresh] [--model ] [--effort ] [prompt]", " node scripts/codex-companion.mjs status [job-id] [--all] [--json]", " node scripts/codex-companion.mjs result [job-id] [--json]", @@ -157,6 +157,17 @@ function sleep(ms) { return new Promise((resolve) => setTimeout(resolve, ms)); } +function parseNonNegativeIntegerOption(rawValue, optionLabel) { + if (rawValue === undefined || rawValue === null || rawValue === "") { + return undefined; + } + const parsed = Number(rawValue); + if (!Number.isFinite(parsed) || parsed < 0 || !Number.isInteger(parsed)) { + throw new Error(`${optionLabel} must be a non-negative integer.`); + } + return parsed; +} + function shorten(text, limit = 96) { const normalized = String(text ?? "").trim().replace(/\s+/g, " "); if (!normalized) { @@ -403,7 +414,10 @@ async function executeReviewRun(request) { }; } - const context = collectReviewContext(request.cwd, target); + const context = collectReviewContext(request.cwd, target, { + maxInlineFiles: request.maxInlineFiles, + maxInlineDiffBytes: request.maxInlineDiffBytes + }); const prompt = buildAdversarialReviewPrompt(context, focusText); const result = await runAppServerTurn(context.repoRoot, { prompt, @@ -681,7 +695,7 @@ function enqueueBackgroundTask(cwd, job, request) { async function handleReviewCommand(argv, config) { const { options, positionals } = parseCommandInput(argv, { - valueOptions: ["base", "scope", "model", "cwd"], + valueOptions: ["base", "scope", "model", "cwd", "max-inline-files", "max-inline-bytes"], booleanOptions: ["json", "background", "wait"], aliasMap: { m: "model" @@ -695,6 +709,8 @@ async function handleReviewCommand(argv, config) { base: options.base, scope: options.scope }); + const maxInlineFiles = parseNonNegativeIntegerOption(options["max-inline-files"], "--max-inline-files"); + const maxInlineDiffBytes = parseNonNegativeIntegerOption(options["max-inline-bytes"], "--max-inline-bytes"); config.validateRequest?.(target, focusText); const metadata = buildReviewJobMetadata(config.reviewName, target); @@ -716,6 +732,8 @@ async function handleReviewCommand(argv, config) { model: options.model, focusText, reviewName: config.reviewName, + maxInlineFiles, + maxInlineDiffBytes, onProgress: progress }), { json: options.json } diff --git a/plugins/codex/scripts/lib/git.mjs b/plugins/codex/scripts/lib/git.mjs index 1749cfc8..5967ca0b 100644 --- a/plugins/codex/scripts/lib/git.mjs +++ b/plugins/codex/scripts/lib/git.mjs @@ -5,8 +5,8 @@ import { isProbablyText } from "./fs.mjs"; import { formatCommandFailure, runCommand, runCommandChecked } from "./process.mjs"; const MAX_UNTRACKED_BYTES = 24 * 1024; -const DEFAULT_INLINE_DIFF_MAX_FILES = 2; -const DEFAULT_INLINE_DIFF_MAX_BYTES = 256 * 1024; +const DEFAULT_INLINE_DIFF_MAX_FILES = 50; +const DEFAULT_INLINE_DIFF_MAX_BYTES = 1024 * 1024; function git(cwd, args, options = {}) { return runCommand("git", args, { cwd, ...options }); diff --git a/tests/commands.test.mjs b/tests/commands.test.mjs index 3724ffa4..30905bb6 100644 --- a/tests/commands.test.mjs +++ b/tests/commands.test.mjs @@ -49,7 +49,9 @@ test("adversarial review command uses AskUserQuestion and background Bash while assert.match(source, /```bash/); assert.match(source, /```typescript/); assert.match(source, /adversarial-review "\$ARGUMENTS"/); - assert.match(source, /\[--scope auto\|working-tree\|branch\] \[focus \.\.\.\]/); + assert.match(source, /\[--scope auto\|working-tree\|branch\][^\n]*\[focus \.\.\.\]/); + assert.match(source, /\[--max-inline-files \] \[--max-inline-bytes \]/); + assert.match(source, /override the inline diff limits/i); assert.match(source, /run_in_background:\s*true/); assert.match(source, /command:\s*`node "\$\{CLAUDE_PLUGIN_ROOT\}\/scripts\/codex-companion\.mjs" adversarial-review "\$ARGUMENTS"`/); assert.match(source, /description:\s*"Codex adversarial review"/); diff --git a/tests/git.test.mjs b/tests/git.test.mjs index 14ff2576..ba9231eb 100644 --- a/tests/git.test.mjs +++ b/tests/git.test.mjs @@ -132,7 +132,7 @@ test("collectReviewContext falls back to lightweight context for larger adversar fs.writeFileSync(path.join(cwd, "c.js"), 'export const value = "SELF_COLLECT_MARKER_C";\n'); const target = resolveReviewTarget(cwd, {}); - const context = collectReviewContext(cwd, target); + const context = collectReviewContext(cwd, target, { maxInlineFiles: 2 }); assert.equal(context.inputMode, "self-collect"); assert.equal(context.fileCount, 3); @@ -173,7 +173,7 @@ test("collectReviewContext keeps untracked file content in lightweight working t fs.writeFileSync(path.join(cwd, "new-risk.js"), 'export const value = "UNTRACKED_RISK_MARKER";\n'); const target = resolveReviewTarget(cwd, {}); - const context = collectReviewContext(cwd, target); + const context = collectReviewContext(cwd, target, { maxInlineFiles: 2 }); assert.equal(context.inputMode, "self-collect"); assert.equal(context.fileCount, 3); @@ -181,3 +181,43 @@ test("collectReviewContext keeps untracked file content in lightweight working t assert.match(context.content, /## Untracked Files/); assert.match(context.content, /UNTRACKED_RISK_MARKER/); }); + +test("collectReviewContext keeps inline diffs for medium-sized adversarial reviews under default limits", () => { + const cwd = makeTempDir(); + initGitRepo(cwd); + const fileNames = Array.from({ length: 11 }, (_, i) => `mod-${i}.js`); + for (const name of fileNames) { + fs.writeFileSync(path.join(cwd, name), `export const value = "${name}-v1";\n`); + } + run("git", ["add", ...fileNames], { cwd }); + run("git", ["commit", "-m", "init"], { cwd }); + for (const name of fileNames) { + fs.writeFileSync(path.join(cwd, name), `export const value = "INLINE_DEFAULT_${name}";\n`); + } + + const target = resolveReviewTarget(cwd, {}); + const context = collectReviewContext(cwd, target); + + assert.equal(context.inputMode, "inline-diff"); + assert.equal(context.fileCount, 11); + assert.match(context.collectionGuidance, /primary evidence/i); + assert.match(context.content, /INLINE_DEFAULT_mod-0\.js/); +}); + +test("collectReviewContext respects maxInlineFiles override forcing self-collect", () => { + const cwd = makeTempDir(); + initGitRepo(cwd); + fs.writeFileSync(path.join(cwd, "a.js"), "export const value = 'v1';\n"); + fs.writeFileSync(path.join(cwd, "b.js"), "export const value = 'v1';\n"); + run("git", ["add", "a.js", "b.js"], { cwd }); + run("git", ["commit", "-m", "init"], { cwd }); + fs.writeFileSync(path.join(cwd, "a.js"), "export const value = 'OVERRIDE_MARKER_A';\n"); + fs.writeFileSync(path.join(cwd, "b.js"), "export const value = 'OVERRIDE_MARKER_B';\n"); + + const target = resolveReviewTarget(cwd, {}); + const context = collectReviewContext(cwd, target, { maxInlineFiles: 1 }); + + assert.equal(context.inputMode, "self-collect"); + assert.equal(context.fileCount, 2); + assert.doesNotMatch(context.content, /OVERRIDE_MARKER_[AB]/); +}); diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 90408372..a58f17f4 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -288,7 +288,7 @@ test("adversarial review asks Codex to inspect larger diffs itself", () => { fs.writeFileSync(path.join(repo, "src", "b.js"), 'export const value = "PROMPT_SELF_COLLECT_B";\n'); fs.writeFileSync(path.join(repo, "src", "c.js"), 'export const value = "PROMPT_SELF_COLLECT_C";\n'); - const result = run("node", [SCRIPT, "adversarial-review"], { + const result = run("node", [SCRIPT, "adversarial-review", "--max-inline-files", "2"], { cwd: repo, env: buildEnv(binDir) }); @@ -300,6 +300,72 @@ test("adversarial review asks Codex to inspect larger diffs itself", () => { assert.doesNotMatch(state.lastTurnStart.prompt, /PROMPT_SELF_COLLECT_[ABC]/); }); +test("adversarial review keeps medium-sized diffs inline under default limits", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + initGitRepo(repo); + fs.mkdirSync(path.join(repo, "src")); + const fileNames = Array.from({ length: 10 }, (_, i) => `mod-${i}.js`); + for (const name of fileNames) { + fs.writeFileSync(path.join(repo, "src", name), `export const value = "${name}-v1";\n`); + } + run("git", ["add", ...fileNames.map((name) => `src/${name}`)], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + for (const name of fileNames) { + fs.writeFileSync(path.join(repo, "src", name), `export const value = "INLINE_DEFAULT_${name}";\n`); + } + + const result = run("node", [SCRIPT, "adversarial-review"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + const state = JSON.parse(fs.readFileSync(path.join(binDir, "fake-codex-state.json"), "utf8")); + assert.match(state.lastTurnStart.prompt, /primary evidence/i); + assert.match(state.lastTurnStart.prompt, /INLINE_DEFAULT_mod-0\.js/); +}); + +test("adversarial review --max-inline-bytes forces self-collect on large diffs", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "app.js"), "export const value = 'v1';\n"); + run("git", ["add", "app.js"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "app.js"), `export const value = '${"x".repeat(512)}';\n`); + + const result = run("node", [SCRIPT, "adversarial-review", "--max-inline-bytes", "128"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.equal(result.status, 0, result.stderr); + const state = JSON.parse(fs.readFileSync(path.join(binDir, "fake-codex-state.json"), "utf8")); + assert.match(state.lastTurnStart.prompt, /lightweight summary/i); +}); + +test("adversarial review rejects negative --max-inline-files", () => { + const repo = makeTempDir(); + const binDir = makeTempDir(); + installFakeCodex(binDir); + initGitRepo(repo); + fs.writeFileSync(path.join(repo, "app.js"), "v1\n"); + run("git", ["add", "app.js"], { cwd: repo }); + run("git", ["commit", "-m", "init"], { cwd: repo }); + fs.writeFileSync(path.join(repo, "app.js"), "v2\n"); + + const result = run("node", [SCRIPT, "adversarial-review", "--max-inline-files", "-1"], { + cwd: repo, + env: buildEnv(binDir) + }); + + assert.notEqual(result.status, 0); + assert.match(result.stderr, /--max-inline-files must be a non-negative integer/); +}); + test("review includes reasoning output when the app server returns it", () => { const repo = makeTempDir(); const binDir = makeTempDir();