Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions .github/actions/run-claude-review-attempt/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,18 @@
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
Expand Down Expand Up @@ -110,7 +122,7 @@
steps:
- name: Run Claude Code Review
id: run
uses: anthropics/claude-code-action@239e3a730883eeb5c53db12b0fc9573b3024b126 # v1.0.191

Check warning on line 125 in .github/actions/run-claude-review-attempt/action.yml

View workflow job for this annotation

GitHub Actions / lint-yaml

125:84 [comments] too few spaces before comment: expected 2
with:
claude_code_oauth_token: ${{ inputs.claude-code-oauth-token }}
anthropic_api_key: ${{ inputs.anthropic-api-key }}
Expand Down Expand Up @@ -366,6 +378,16 @@
### 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 <number> --repo
<owner>/<repo>` BARE -- no `>` redirect, no `|` pipe, no `;`/`&&`
chaining with `wc`, `grep`, or anything else, even to save or count
Expand Down
54 changes: 54 additions & 0 deletions .github/workflows/claude-code-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 }}
Expand Down Expand Up @@ -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 }}
Expand Down
26 changes: 26 additions & 0 deletions changelog.d/reviewer-pr-diff-file.fixed.md
Original file line number Diff line number Diff line change
@@ -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.
Loading