fix(claude-code-review): tell the reviewer not to redirect/pipe gh pr diff

[d-morrison]

Closes #541.

What

On a large PR, the reviewer would try to chunk gh pr diff's output by
saving it to a file (> /tmp/pr.diff, or a mkdired checkout subdir) and
then wc/greping it. Both mechanisms are dead ends in this sandbox:

1. A compound command combining an allowed pattern (Bash(gh pr diff:*))
   with ;/&&/|/a redirect is denied as a whole command, even
   though the allowed part matches exactly.
2. Writing to a file anywhere — /tmp, or a subdirectory created with
   mkdir inside the checkout — is a hard block in this sandbox, not
   a permission prompt, so no allowlist entry can satisfy it.

The reviewer tried 15+ variants of this before exhausting its denial
budget, per the issue's own downloaded execution artifact (ucdavis/win#78,
run 32415477507): permission_denials_count:33, total_cost_usd:2.29, no
verdict ever produced.

Fix

This needs no new tool grant — Bash(gh pr diff:*) is already allowlisted;
the failure is in how the reviewer used it. A bare, unredirected
gh pr diff <n> --repo <owner>/<repo> call already returns the whole diff
as the Bash tool's own result text, with nothing written to disk at all.
Added an explicit instruction to run-claude-review-attempt's
--append-system-prompt telling the reviewer to always call gh pr diff
bare and read the result inline, and to switch to that form immediately
(rather than trying another redirect/pipe variant) if a chained attempt is
denied. Documented the mechanism as a fourth stub-review cause alongside
the existing gha#173/#218/#392 comments in the same file.

Test plan

• YAML parses (python3 -c "import yaml; yaml.safe_load(open(...))")
• Confirmed the new prompt paragraph reads correctly once YAML-folded
  (no broken quoting/escaping)
• No offline selftest coverage is possible here — run-claude-review-attempt
  wraps a live anthropics/claude-code-action call (per this repo's own
  CLAUDE.md), so this is validated the same way the rest of that file's
  prompt fixes have been: by real reviews once released. Will watch the
  next few dispatched reviews on large-diff PRs for the previous
  redirect/pipe denial signature.

🤖 Generated with Claude Code

[d-morrison]

Both items from this review round are addressed:

• The confirmed correctness bug ("however large it is" not holding past ~30K chars) was already fixed in 0c0abf3, pushed before this review's write-up posted — the inline thread is replied-to and resolved.
• The non-blocking overclaim ("anywhere ... no retry ... will get past" reading as universal rather than sandbox-observed) is now fixed in 72b7dda: reworded to state what was actually tried and observed blocked (/tmp, a mkdir'd subdirectory) rather than asserting a universal property.

Reviewed commit for this write-up was 17de2c2; current head is 72b7dda.

[d-morrison]

Verdict on 72b7dda was Ready for merge with no blocking findings. Pushed one more cosmetic alignment (f7e8697) for the changelog-fragment wording explicitly flagged as "not raising this as a finding" in that review — waiting for review on this head before merging.

[github-actions]

Claude finished review — View run

---

Code Review: Morrison-Lab/gha PR #542

What changed since the last review

The last review round (commit 72b7dda) reached Ready for merge (posted 2026-08-20T23:20:10Z), having confirmed both the original bug fix (the false "however large it is" claim) and a prior non-blocking overclaim note were addressed.

One new commit has landed since then, bringing the head to f7e8697:

• f7e8697 — "fix: scope changelog fragment's hard-block claim, matching action.yml": edits changelog.d/reviewer-diff-redirect-stub.fixed.md only. It changes "writing to any file -- /tmp included -- is a hard sandbox block, not a permission prompt" to "writing to any file -- /tmp and a mkdir-created directory were both tried and both blocked -- is a hard sandbox block, not a permission prompt" — adding the "tried and observed" framing the prior review's non-blocking note pointed at.

This is the exact, explicitly-non-blocking item the last review round flagged for awareness ("not raising this as a finding") and did not require for the "Ready for merge" verdict.

Verification

I compared this commit's wording against action.yml's already-verified scoped language (from 72b7dda, reviewed and confirmed clean last round): "...has not worked in this sandbox" (past-tense, explicitly scoped to what was observed).

The new changelog wording adds the "both tried and both blocked" clause matching that observational framing, but its main clause still reads as a categorical present-tense assertion — "is a hard sandbox block, not a permission prompt" — rather than fully mirroring action.yml's scoped "has not worked in this sandbox" phrasing. So the alignment the commit message claims ("matching action.yml") is partial, not exact.

This doesn't rise to a blocking finding: as the prior round noted, this text is a historical changelog entry, not prompt text that governs the live reviewer agent's behavior, and the substantive fix (the false inline-output claim that does govern live behavior) was already verified fixed. I'm not re-raising it as a new finding — it's the same non-blocking observation as before, now partially rather than fully addressed.

No other files changed in this commit, and no new issues were introduced.

Verdict

Ready for merge — the prior verdict (2026-08-20T23:20:10Z, reviewing commit 72b7dda) still stands. The one commit added since then (f7e8697) only tweaks changelog wording on an item that round already treated as non-blocking, and introduces no new issues.

Reviewed commit: f7e8697

[github-actions]

💰 Cost: $1.1364 (review) — run