diff --git a/.github/actions/run-claude-review-attempt/action.yml b/.github/actions/run-claude-review-attempt/action.yml index 64eb36ed..e53f047c 100644 --- a/.github/actions/run-claude-review-attempt/action.yml +++ b/.github/actions/run-claude-review-attempt/action.yml @@ -43,6 +43,18 @@ inputs: pr-state above. See pr-state's description. required: false default: '' + pr-diff-file: + description: >- + 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 + 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 +378,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}`, 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 + 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..4fc9a561 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -494,6 +494,58 @@ 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_NAME: .claude-review-pr.diff + run: | + set -uo pipefail + target="$GITHUB_WORKSPACE/$DIFF_NAME" + if gh pr diff "$PR_NUMBER" --repo "$REPO" > "$target" && [ -s "$target" ]; then + # 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 $target; 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 +783,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 +891,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 }} 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.