fix(claude-code-review): allow WebFetch/curl for addendum preview checks - #202
Conversation
A repo's prompt-addendum can tell the reviewer to verify computed values and figures against the PR-preview site (rme does). No fetch tool was granted, so each of the code-review plugin's 4 parallel sub-agents hit a permission denial per fetch attempt — 36 denials on rme#954 — and the run finished is_error:false but never emitted a verdict, tripping the gha#173 stub-review guard. Allowlist WebFetch and Bash(curl:*). Both are read-only GETs and add no exfiltration capability beyond Bash(python3:*), already granted, which can make arbitrary network calls via urllib. Refs #199.
|
Claude finished review — View run Code review: #202PR: "fix(claude-code-review): allow WebFetch/curl for addendum preview checks" — adds No Findings1. (Blocking) The new tool grant is self-defeating — the untouched system prompt still tells the agent the tools don't exist and forbids using them.
2. The new "read-only (GET); no write verbs" security justification is inaccurate for
Non-blocking note (not independently validated): one reviewer flagged that this grant is unscoped Nothing else stood out: the changelog fragment is correctly named/formatted per VerdictNeeds more work — finding 1 is a genuine, self-confirming bug: the PR grants the fetch tools but leaves the reviewer's own system prompt instructing it not to use them, which defeats the fix's stated purpose. Finding 2 is a smaller factual-accuracy fix to the new comment/changelog text. |
Address review of #202: 1. (blocking) The --append-system-prompt still told the agent "no network-fetch tools enabled; do not attempt to fetch external URLs", which contradicted and defeated the WebFetch/curl grant. Replace that text with an instruction that the fetch tools ARE available for verifying values/figures against rendered output. 2. Correct the "read-only (GET); no write verbs" claim: WebFetch is GET-only, but Bash(curl:*) is a prefix match that also allows write verbs (curl -X POST / --data / -T). Reworded in the workflow comment and the changelog fragment; the no-new-capability-beyond-python3 argument is unchanged and still holds.
|
Thanks — finding 1 was a real miss on my part. Addressed both in 1. (Blocking) System prompt contradicted the grant — Addressed. You're right: granting 2. "read-only (GET); no write verbs" inaccurate for curl — Addressed. Correct — Non-blocking (unscoped Generated by Claude Code |
|
Working on this — paws off until I'm done. (Rebasing onto Generated by Claude Code |
…sis-docs-0b39md # Conflicts: # .github/workflows/claude-code-review.yml
|
Merged Base is now Generated by Claude Code |
|
Claude finished review — View run Code Review: #202PR: "fix(claude-code-review): allow WebFetch/curl for addendum preview checks" — allowlists This PR was already reviewed once by Claude (comment: #202 (comment), verdict Needs more work, two findings). Since then, new commits landed — Finding 1 (was blocking) — Fixed, verified. The Finding 2 (factual accuracy) — Fixed, verified. The inline comment and changelog fragment no longer claim Non-blocking note (unscoped Rebase/relocation check. Confirmed the merge is clean: diffing against pre-PR No CLAUDE.md doc-sync rules apply here — no new No new issues found beyond what the prior review already caught, and both of those are now correctly resolved in the current code. VerdictReady for merge — the prior review's blocking finding (self-defeating system prompt) and its factual-accuracy finding (curl write-verb claim) are both fixed and verified in the current post-rebase code; the non-blocking scoping note was reasonably deferred by the author. (Note: per this run's configuration, no |
Review finding on #205: the parenthetical implied #202's diff was fixing the same stub-review bug #201 fixed, and that both PRs' reviews were touching claude-code-review.yml right up until merge. Neither is quite right: #202 fixed a different (related) allowlist gap and only hit #185's stub-review signature as a bystander while it still edited claude-code-review.yml directly, before a rebase onto #201 moved that edit into the new run-claude-review-attempt composite action. Verified the underlying fact via the actual failing run's execution output (permission_denials_count:1, no verdict) rather than dropping the citation outright. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B2aHbEXwTUA13vYMuQXqas
…-merge (#205) * docs: note that fixes to claude-code-review.yml can't self-verify pre-merge Learned across gha#201/#202: this repo's own claude-review.yml pins claude-code-review.yml@v2 (the released tag), not a local ref, so a PR fixing a bug in that file hits the pre-fix version on its own automatic review until after merge. Both PRs saw their own claude-review check fail with the exact signature they were fixing, right up until merge. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B2aHbEXwTUA13vYMuQXqas * fix(docs): correct overstated gha#202 citation in self-verify note Review finding on #205: the parenthetical implied #202's diff was fixing the same stub-review bug #201 fixed, and that both PRs' reviews were touching claude-code-review.yml right up until merge. Neither is quite right: #202 fixed a different (related) allowlist gap and only hit #185's stub-review signature as a bystander while it still edited claude-code-review.yml directly, before a rebase onto #201 moved that edit into the new run-claude-review-attempt composite action. Verified the underlying fact via the actual failing run's execution output (permission_denials_count:1, no verdict) rather than dropping the citation outright. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B2aHbEXwTUA13vYMuQXqas --------- Co-authored-by: Claude <noreply@anthropic.com>
…loses #266) Add WebSearch to run-claude-review-attempt's --allowedTools and update the system prompt in lockstep, so the reviewer can locate authoritative sources (docs, specs, papers) for the claims, citations, and APIs the review guidelines tell it to verify, instead of only fetching URLs it already has (from #202's WebFetch/curl grant). No new egress capability: python3/curl already permit arbitrary direct requests (see #240), and WebSearch queries are served through the Anthropic API. Also reword the three stale 'no network-fetch tools' claims left behind by #202 (check-latex-macros input description, its prompt block, and the reference page): the macro checks stay local-checkout-only, now justified by the pinned submodule version being authoritative rather than by a tool gap that no longer exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMTBtWqifJeeDRJHVkQZua
) * start: grant reviewer WebSearch access for fact-checking (closes #266) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMTBtWqifJeeDRJHVkQZua * claude-code-review: grant the reviewer WebSearch for fact-checking (closes #266) Add WebSearch to run-claude-review-attempt's --allowedTools and update the system prompt in lockstep, so the reviewer can locate authoritative sources (docs, specs, papers) for the claims, citations, and APIs the review guidelines tell it to verify, instead of only fetching URLs it already has (from #202's WebFetch/curl grant). No new egress capability: python3/curl already permit arbitrary direct requests (see #240), and WebSearch queries are served through the Anthropic API. Also reword the three stale 'no network-fetch tools' claims left behind by #202 (check-latex-macros input description, its prompt block, and the reference page): the macro checks stay local-checkout-only, now justified by the pinned submodule version being authoritative rather than by a tool gap that no longer exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CMTBtWqifJeeDRJHVkQZua --------- Co-authored-by: Claude <noreply@anthropic.com>
Follow-up to #200 (refs #199).
Problem
#200 fixed the CAS-tool denials, but a math review on
rme#954still failed with no verdict — and denials went up (33 → 36) over 81 turns. So the CAS tools weren't the main blocker.The real remaining gap is the addendum's second instruction: verify computed values and figures against the PR-preview site (
rme's addendum: "check any computed value or figure the prose describes against the PR-preview site … rather than trusting the prose's own description"). No fetch tool was granted, so:code-reviewplugin fans out to 4 parallel sub-agents.WebFetch, nocurl).is_error:falsebut never emits a### Verdict— tripping the same gha#173 / stub-review guard.Fix
Allowlist
WebFetchandBash(curl:*).Security: both are read-only GETs. They add no exfiltration capability beyond
Bash(python3:*)(already granted), which can already make arbitrary network calls viaurllib— this just lets the reviewer fetch directly instead of scripting it.After merge
Move
v2(auto viaslide-major-tag.yml), then re-trigger the review onrme#954.Testing
yaml.safe_load)._selftest.ymlexercises the workflow end-to-end on this PR.🤖 Generated with Claude Code
Generated by Claude Code