From f043cfd7d3479c483e2de2d90e9020d54fd3fe8c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:18:52 +0000 Subject: [PATCH 1/3] start: supply the PR diff as a file so the reviewer stops redirecting (#543) From 828cb1fa6ca06f883ff9a3377b2c941cb7350f38 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:21:45 +0000 Subject: [PATCH 2/3] fix(claude-code-review): hand the reviewer the PR diff as a file 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. --- .../run-claude-review-attempt/action.yml | 21 ++++++++ .github/workflows/claude-code-review.yml | 50 +++++++++++++++++++ 2 files changed, 71 insertions(+) diff --git a/.github/actions/run-claude-review-attempt/action.yml b/.github/actions/run-claude-review-attempt/action.yml index 64eb36ed..a4a90e83 100644 --- a/.github/actions/run-claude-review-attempt/action.yml +++ b/.github/actions/run-claude-review-attempt/action.yml @@ -43,6 +43,17 @@ inputs: pr-state above. See pr-state's description. required: false default: '' + pr-diff-file: + description: >- + Workspace-relative path of a file holding the PR's diff, written by + claude-code-review.yml before this action runs. Naming it in the prompt + is the point: the reviewer wants the diff in a file so it can chunk or + count a large one, and its only route to that was a redirect the sandbox + rejects, so it spent whole runs retrying variants until the denial count + starved it (gha#541, gha#543). Empty when the write failed, in which + case the prompt falls back to the bare-command instructions below. + required: false + default: '' latexmacros-guidance: description: >- LaTeX-macro review guidance, from claude-code-review.yml's Compose @@ -366,6 +377,16 @@ runs: ### Verdict line required below -- do not end the run silently or wait for approval. + ${{ inputs.pr-diff-file != '' && format('The complete PR diff has + already been saved for you at `{0}`, relative to your working + directory. READ THAT FILE. It is the whole diff, it costs no Bash + call, and you can read it in parts if it is large. The workflow + wrote it before you started, so it is not yours to create and it is + not part of the change under review -- ignore it when listing + changed files. Reach for the `gh pr diff` command below only if you + need something this file does not contain, such as a diff against a + different base.', inputs.pr-diff-file) || '' }} + To read the PR diff, call `gh pr diff --repo /` BARE -- no `>` redirect, no `|` pipe, no `;`/`&&` chaining with `wc`, `grep`, or anything else, even to save or count diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index a802b9f0..ec9d3ac3 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -494,6 +494,54 @@ jobs: apt-packages: ${{ inputs.apt-packages }} pip-packages: ${{ inputs.pip-packages }} + # Hand the reviewer the diff as a FILE, so it never has to build one. + # + # `Bash(gh pr diff:*)` is allowed, but a command chaining it with a + # redirect or pipe is rejected as a whole, and the agent cannot write a + # file itself -- /tmp and a mkdir-created directory are both hard blocks + # in that sandbox, not permission prompts. So a reviewer that wants the + # diff in a file to chunk or count it has no route to one, and what it + # does instead is retry 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's own reproduction (6 denials, + # $3.42), and gha#555 run 32526051617 (8 denials, $4.95) -- the last of + # these with the prompt paragraph forbidding exactly that construction + # already live at @v2, which is why this is a step rather than another + # rewording (gha#541, gha#543; CLAUDE.md's "A prompt instruction is a + # request; a permission rule is a constraint"). + # + # 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 as repository content; the prompt tells the + # reviewer to ignore it when listing changed files. + # + # A failure here is NOT fatal: the reviewer keeps the bare `gh pr diff` + # route it has today, so the worst case is the behaviour that exists now + # rather than a red check. The partial file is removed first, so a + # truncated diff can never be read as a complete one, and `path` stays + # empty so the prompt falls back instead of naming a file that is not + # there. + - name: Save the PR diff for the reviewer + id: prdiff + if: steps.selfmod.outputs.self_mod != 'true' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number || inputs.pr-number }} + DIFF_PATH: .claude-review-pr.diff + run: | + set -uo pipefail + target="$GITHUB_WORKSPACE/$DIFF_PATH" + if gh pr diff "$PR_NUMBER" --repo "$REPO" > "$target" && [ -s "$target" ]; then + echo "path=$DIFF_PATH" >> "$GITHUB_OUTPUT" + echo "Saved the PR diff for the reviewer: $DIFF_PATH ($(wc -c < "$target") bytes)." + else + rm -f "$target" + echo "path=" >> "$GITHUB_OUTPUT" + echo "::warning::Could not save the PR diff to $DIFF_PATH; the reviewer will fall back to calling gh pr diff itself." + fi + # Clear the requested reviewers (human and AI, e.g. Copilot) before # Claude runs so they aren't notified mid-run, then re-add them when # Claude finishes so the re-add fires a fresh notification — letting a @@ -731,6 +779,7 @@ jobs: latexmacros-guidance: ${{ steps.latexmacros.outputs.guidance }} pr-state: ${{ steps.stash.outputs.pr_state }} pr-merged: ${{ steps.stash.outputs.pr_merged }} + pr-diff-file: ${{ steps.prdiff.outputs.path }} prompt-addendum: ${{ inputs.prompt-addendum }} track-progress: ${{ inputs.track-progress && github.event_name == 'pull_request' && 'true' || 'false' }} show-full-output: ${{ inputs.show-full-output }} @@ -838,6 +887,7 @@ jobs: latexmacros-guidance: ${{ steps.latexmacros.outputs.guidance }} pr-state: ${{ steps.stash.outputs.pr_state }} pr-merged: ${{ steps.stash.outputs.pr_merged }} + pr-diff-file: ${{ steps.prdiff.outputs.path }} prompt-addendum: ${{ inputs.prompt-addendum }} track-progress: ${{ inputs.track-progress && github.event_name == 'pull_request' && 'true' || 'false' }} show-full-output: ${{ inputs.show-full-output }} From ee5cbe6447edf14af8c79b16f2622e491e717b9d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 22:36:00 +0000 Subject: [PATCH 3/3] fix(claude-code-review): absolute diff path, and the changelog fragment 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. --- .../run-claude-review-attempt/action.yml | 9 ++++--- .github/workflows/claude-code-review.yml | 14 ++++++---- changelog.d/reviewer-pr-diff-file.fixed.md | 26 +++++++++++++++++++ 3 files changed, 40 insertions(+), 9 deletions(-) create mode 100644 changelog.d/reviewer-pr-diff-file.fixed.md diff --git a/.github/actions/run-claude-review-attempt/action.yml b/.github/actions/run-claude-review-attempt/action.yml index a4a90e83..e53f047c 100644 --- a/.github/actions/run-claude-review-attempt/action.yml +++ b/.github/actions/run-claude-review-attempt/action.yml @@ -45,8 +45,9 @@ inputs: default: '' pr-diff-file: description: >- - Workspace-relative path of a file holding the PR's diff, written by - claude-code-review.yml before this action runs. Naming it in the prompt + Absolute path of a file holding the PR's diff, written by + claude-code-review.yml before this action runs. Absolute because Claude + Code's Read tool requires an absolute `file_path`. Naming it in the prompt is the point: the reviewer wants the diff in a file so it can chunk or count a large one, and its only route to that was a redirect the sandbox rejects, so it spent whole runs retrying variants until the denial count @@ -378,8 +379,8 @@ runs: wait for approval. ${{ inputs.pr-diff-file != '' && format('The complete PR diff has - already been saved for you at `{0}`, relative to your working - directory. READ THAT FILE. It is the whole diff, it costs no Bash + already been saved for you at `{0}`, an absolute path you can + pass straight to Read. READ THAT FILE. It is the whole diff, it costs no Bash call, and you can read it in parts if it is large. The workflow wrote it before you started, so it is not yours to create and it is not part of the change under review -- ignore it when listing diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index ec9d3ac3..4fc9a561 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -529,17 +529,21 @@ jobs: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} REPO: ${{ github.repository }} PR_NUMBER: ${{ github.event.pull_request.number || inputs.pr-number }} - DIFF_PATH: .claude-review-pr.diff + DIFF_NAME: .claude-review-pr.diff run: | set -uo pipefail - target="$GITHUB_WORKSPACE/$DIFF_PATH" + target="$GITHUB_WORKSPACE/$DIFF_NAME" if gh pr diff "$PR_NUMBER" --repo "$REPO" > "$target" && [ -s "$target" ]; then - echo "path=$DIFF_PATH" >> "$GITHUB_OUTPUT" - echo "Saved the PR diff for the reviewer: $DIFF_PATH ($(wc -c < "$target") bytes)." + # ABSOLUTE, because Claude Code's Read tool requires an absolute + # `file_path`. A workspace-relative path would cost the reviewer a + # resolution step to reach the file this whole change exists to put + # in front of it (gha#567 review). + echo "path=$target" >> "$GITHUB_OUTPUT" + echo "Saved the PR diff for the reviewer: $target ($(wc -c < "$target") bytes)." else rm -f "$target" echo "path=" >> "$GITHUB_OUTPUT" - echo "::warning::Could not save the PR diff to $DIFF_PATH; the reviewer will fall back to calling gh pr diff itself." + echo "::warning::Could not save the PR diff to $target; the reviewer will fall back to calling gh pr diff itself." fi # Clear the requested reviewers (human and AI, e.g. Copilot) before diff --git a/changelog.d/reviewer-pr-diff-file.fixed.md b/changelog.d/reviewer-pr-diff-file.fixed.md new file mode 100644 index 00000000..1784d77b --- /dev/null +++ b/changelog.d/reviewer-pr-diff-file.fixed.md @@ -0,0 +1,26 @@ +- **`claude-code-review` now hands the reviewer the PR diff as a file** + (#543, #567). + #541 told the reviewer, in the system prompt, to call `gh pr diff` bare + rather than redirecting or piping it. + That instruction was live at `@v2` and the reviewer redirected anyway, + eight times, on gha#555 -- + spending $4.95 on a run that produced no verdict. + The reason is that the instruction removed the only route to what the + reviewer actually wanted: + the diff in a file, so it can chunk or count a large one. + A compound command is denied as a whole, + and the agent cannot write a file itself, + so there was nothing left to reach for. + A new step now writes the diff to the workspace before the agent starts, + and the prompt names that absolute path -- + absolute because Claude Code's `Read` tool requires one -- + telling 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. + `Bash(gh pr diff:*)` stays allowed, + so a denial afterwards means something has genuinely gone wrong, + which is what the stub-retry threshold is trying to measure. + Failure to save the diff is not fatal: + the partial file is removed, the path is left empty, + and the reviewer keeps the route it has today.