fix(claude-code-review): hand the reviewer the PR diff as a file - #567
Conversation
Closes #543. The reviewer is not attempting something forbidden. It wants the diff in a FILE so it can chunk or count a large one, and it has no route to one: `Bash(gh pr diff:*)` is allowed, but chaining it with a redirect or pipe is rejected as a whole command, and the agent cannot write a file itself -- /tmp and a mkdir-created directory are both hard blocks in that sandbox rather than permission prompts. So it retries variants of the same redirect until the denial count crosses the stub-retry threshold and the run ends with no verdict. Measured three times: ucdavis/win#78 run 32415477507 15+ variants, 33 denials, $2.29 gha#543 repro 6 denials, $3.42 gha#555 run 32526051617 8 denials, $4.95 The third is why this is a step rather than another rewording: the prompt paragraph forbidding exactly that construction (#542) was already live at @v2 when it happened, which is CLAUDE.md's "a prompt instruction is a request; a permission rule is a constraint" landing on the other foot -- here the constraint exists and the capability the reviewer wants is missing. So supply it. A new step writes the diff to the workspace before the agent starts, and the prompt names that path and tells the reviewer to read it, to ignore it when listing changed files, and to fall back to `gh pr diff` only for something the file does not contain. Not fatal on failure, deliberately: the reviewer keeps the bare command it has today, so the worst case is current behaviour rather than a red check. Verified against a stub `gh` across four shapes -- gh succeeds -> path set, file present gh writes nothing -> path empty, no file gh fails -> path empty, no file, exit 0 gh writes then fails -> path empty, PARTIAL FILE REMOVED, exit 0 -- the last being the one that matters, since a truncated diff read as a complete one is worse than no file at all. Written into the workspace rather than $RUNNER_TEMP because the agent reads relative to its working directory, and dot-prefixed so it reads as tooling rather than repository content. Retracts the fix I proposed on #543 twice, which was to drop `Bash(gh pr diff:*)` from the allowlist "so the pre-written diff file is the only path". There was no such file: the harness saves one only after a SUCCESSFUL bare call exceeds the inline ceiling, so removing the grant would have deleted the reviewer's only route to the diff. Also worth recording on the issue rather than only here: #540's tool-name logging has made #543's own open question answerable, and the answer is not the `Task` fan-out it hypothesised. gha#555's denials were named `Bashx8`, all redirect-chained, with no `Task` denials at all -- and that run's own review reported working via four parallel agents.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Review round 1 on #567. FINDING -- missing changelog fragment. Correct and quotable: CLAUDE.md requires one for user-facing behaviour, and #541's own reviewer-diff-redirect-stub.fixed.md is direct precedent for a change of exactly this shape. Added changelog.d/reviewer-pr-diff-file.fixed.md. Also acted on one of the two observations the review explicitly did NOT flag as blocking, because it decides whether the fix works at all: the step emitted a workspace-relative path while Claude Code's Read tool requires an absolute `file_path`. A reviewer handed a relative path would have to resolve it before reading the file this change exists to put in front of it -- and a failed Read is exactly the kind of dead end that sends it back to redirect variants. Now emits "$GITHUB_WORKSPACE/ $DIFF_NAME", with the prompt and the input description saying absolute and why. Left the other observation alone, deliberately. The step uses `set -uo pipefail` rather than the file's `set +e -uo pipefail` idiom for always-exit-0 steps; the review confirmed no command in either branch can fail, and the `if` already handles the only failure that matters. Adding `+e` would suggest a hazard that is not there.
Round 1 addressed in
|
|
Claude finished review — View run Review: Morrison-Lab/gha PR #567Status checked live: open, unmerged (head SummaryThis is a confirming round. The prior review (posted 2026-08-21T22:33:34Z, #567 (comment)) reviewed head Commit
No other lines changed. I confirm via VerdictReady for merge — the prior round's sole finding is fixed, one of its non-blocking observations was proactively addressed as well, and no new issues were introduced in this round. Reviewed commit: ee5cbe6 |
2 participants
Closes #543.
The diagnosis this changes
The reviewer is not attempting something forbidden. It wants the diff in a file so it can chunk or count a large one, and it has no route to one:
Bash(gh pr diff:*)is allowed, but a command chaining it with>,|, or;is rejected as a whole, even though the allowed part matches exactly./tmpand amkdir-created directory are both hard blocks in that sandbox, not permission prompts.So it retries variants of the same redirect until the denial count crosses the stub-retry threshold, the run ends with no verdict, and
require-reviewgoes red.Measured three times:
ucdavis/win#78, run 32415477507The third is why this is a step rather than another rewording. #542's prompt paragraph forbids exactly that construction, in detail, and was already live at
@v2when it happened. That is CLAUDE.md's "a prompt instruction is a request; a permission rule is a constraint" landing on the other foot: here the constraint exists and the capability the reviewer wants is missing.The change
A step writes the diff to the workspace before the agent starts, and the prompt names that path — telling the reviewer to read it, to ignore it when listing changed files, and to fall back to
gh pr diffonly for something the file does not contain (a different base, a single path).That removes the motivation rather than the capability.
Bash(gh pr diff:*)stays allowed, so a denial afterwards means something has genuinely gone wrong — which is what the starvation threshold is trying to measure.Not fatal on failure, deliberately. The reviewer keeps the bare command it has today, so the worst case is the behaviour that exists now rather than a red check. Verified against a stub
ghacross four shapes:ghbehaviourpathoutputThe last row is the one that matters: a truncated diff read as a complete one is worse than no file at all.
Written into the workspace rather than
$RUNNER_TEMPbecause the agent reads relative to its working directory, and dot-prefixed so it reads as tooling rather than repository content.Retracting what I proposed on the issue
I twice proposed dropping
Bash(gh pr diff:*)from the allowlist "so the pre-written diff file is the only path". There is no pre-written diff file — I read "a file already saved for you" in the action's own comment as a standing artifact when it is a consequence of the successful call. That change would have deleted the reviewer's only route to the diff. Retracted on the issue.#543's open question is now answerable, and the answer is not
TaskThe body says it could not confirm its own hypothesis because the log recorded only a count — which is #540, since shipped. The first named instance is #555's:
All
Bash, noTask. Consistent with that, the same PR's round-1 review reported working via four parallel agents and produced two well-founded findings, so those calls were being permitted. This does not retroactively explainwai#83's 12 denials, whose run predates the naming.Cannot be verified on this PR
claude-review.ymlpinsclaude-code-review.yml@v2, so this PR's own review runs the pre-fix workflow — the documented self-verify gap. The review should still run (theself_modguard keys onclaude-review.yml, which this diff does not touch), it just cannot exercise the change. Verification is a consumer PR after the next tag slide.Passing
pr-diff-filetorun-claude-review-attempt@v2before the tag moves is safe: an undeclared input on a composite action is a warning, unlike a reusable workflow'swith:, which fails the call gate.Deferred
The save step is inline shell rather than a tested script. Extracting it would mean a new composite action, since a reusable workflow cannot reference this repo's
scripts/by path (the consumer's checkout is whatactions/checkoutproduces — gha#191/#196), and a new composite cannot gain its first@v2caller in the PR that introduces it (gha#326/#327). Filed as #568 rather than forced in here; the four-shape stub run above is the coverage in the meantime.