fix(review): give the reviewer the diff instead of expecting it to find one - #375
Conversation
…nd one The gate's own output caught this. A CI run returned `revise` whose feedback began: "I cannot determine the actual diff between the current state and commit 47b9837 because the .git directory is not accessible" — and then delivered a verdict anyway, on files read in isolation. `buildReviewerPrompt` gave direct mode only a list of changed filenames. In working-tree mode an agent can recover the rest with bash. Under `--read-only` it cannot, and in `--base` mode there is nothing in the working tree to recover: reading a file shows the result of the change, never the change. Those two flags together are exactly what the GitHub Action passes, so the flagship CI path was reviewing blind. The diff now travels in the prompt. It is bounded at 200KB and says so when it truncates, because a silently partial diff invites a verdict on a change the reviewer only partly saw. A git failure degrades to the old file-list behaviour rather than ending the review, and omits the section rather than showing an empty one, since a heading with nothing under it reads as "nothing changed". The diff is appended as plain text: the template already wraps the whole worker report in its untrusted-data block. My first version added a second fence with a comment claiming it stopped the diff from escaping — and the test I wrote for it passed with that escaping removed, because the outer block was doing the work. Removed, and the test now bisects against the escaper that actually protects it.
![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: e4fe9d8500
ℹ️ 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".
| const report = options.diff | ||
| ? `- **Files changed (${files.length}):** ${filesSummary}\n- **Diff under review:**\n${options.diff}` | ||
| : `- **Files changed (${files.length}):** ${filesSummary}`; | ||
| return getPrompts().buildReviewerPrompt({ | ||
| taskTitle: options.taskTitle, | ||
| taskDescription: options.taskDescription, | ||
| workerReport: `- **Files changed (${files.length}):** ${filesSummary}`, | ||
| workerReport: report, |
There was a problem hiding this comment.
When a diff exceeds roughly 20,000 characters, passing it as workerReport causes promptDataBlock() to truncate it at MAX_PROMPT_DATA_CHARS in src/locale/prompts/en.ts:9-17 (and the Korean equivalent), long before getDiffText()'s advertised 200,000-character limit. In read-only CI, a large early patch can therefore hide every later patch; when more than 20 files changed, those later filenames are also absent from filesSummary, so the reviewer cannot even determine which files to inspect without Git access. Give direct-review diffs a compatible bound or otherwise preserve the remaining changed-file identities and truncation guidance.
Useful? React with 👍 / 👎.
| mode: 'direct', | ||
| priorReviewContext: history.context, | ||
| readOnly: opts.readOnly, | ||
| diff: await deps.getDiff?.(c, opts.base) ?? await defaultGetDiff(c, opts.base), |
There was a problem hiding this comment.
When a repository does not ignore .openswarm/ (the case the explicit filter at lines 287-290 supports) and it has both a real source change and generated review-history/audit changes, changed excludes the OpenSwarm state but this call obtains an unfiltered repository-wide Git diff. The prompt consequently includes files that the command deliberately removed from its review scope, allowing self-generated reports to consume prompt space and influence the next verdict. Generate the supplied diff from the filtered paths or apply the same .openswarm exclusion to it.
Useful? React with 👍 / 👎.
| const diff = await runGitCommand(projectPath, args); | ||
| if (diff.length <= maxBytes) return diff; | ||
| return `${diff.slice(0, maxBytes)}\n\n[diff truncated at ${maxBytes} bytes of ${diff.length}; read the files directly for the rest]`; |
There was a problem hiding this comment.
For a very large text diff, such as an attacker-authored CI change containing generated files, runGitCommand() concatenates the complete Git output before this code checks maxBytes and slices it. The advertised 200 KB bound therefore does not bound memory or collection time, and a sufficiently large diff can exhaust the Node process before the reviewer starts. Stream and count the output while retaining only the allowed prefix instead of materializing the entire patch.
Useful? React with 👍 / 👎.
1 participant
The gate's own output caught this. A CI run returned
revisewhose feedback opened with:…and then delivered a verdict anyway, based on files read in isolation.
Why
buildReviewerPromptgave direct mode only a list of changed filenames. Its own comment says direct mode "reviews a Git diff supplied by a user/CI checkout" — but no diff was ever supplied.In working-tree mode an agent can recover the rest with
bash. Under--read-onlyit has no bash, and in--basemode there is nothing in the working tree to recover: reading a file shows the result of the change, never the change.--read-only --baseis exactly what the GitHub Action passes. The flagship CI path was reviewing blind and still emitting confident verdicts.What
The diff travels in the prompt now. Bounded at 200KB and says so when it truncates — a silently partial diff invites a verdict on a change the reviewer only partly saw. A git failure degrades to the old file-list behaviour rather than ending the review, and omits the section rather than rendering an empty one, since a heading with nothing under it reads as "nothing changed".
A correction I made mid-change
The diff is appended as plain text, because the template already wraps the whole worker report in its untrusted-data block.
My first version added a second fence, with a comment claiming it stopped a malicious diff from escaping. Then the test I wrote for it passed with that escaping removed — the outer block had been doing the work all along. The redundant fence is gone, the comment says what is actually true, and the test now bisects against the escaper that really protects it (removing
DATA_BLOCK_CLOSEescaping in the template turns it red).npm run lint·npx tsc --noEmit— cleannpm run test:coverage— 266 files, 3574 pass, statements 90.14%