Skip to content

fix(claude-code-review): don't let a denied tool call stub the review - #187

Merged
d-morrison merged 1 commit into
mainfrom
claude/gha-issue-185-2merhb
Jul 2, 2026
Merged

fix(claude-code-review): don't let a denied tool call stub the review#187
d-morrison merged 1 commit into
mainfrom
claude/gha-issue-185-2merhb

Conversation

@d-morrison

Copy link
Copy Markdown
Collaborator

Root cause

#173 (closed) tracked the symptom — a claude-review run going green with only a stub, no verdict — and #172/#174/#176 added and tested a guard (check-review-execution.sh) that now correctly fails the check when this happens. #185 is about the root cause that guard was correctly catching, which this PR fixes.

Reproduced 3/3 times on PR #180 (a single-file CLAUDE.md prose addition) with an identical fingerprint: is_error: false, num_turns: 4, permission_denials_count: 1, no verdict written. The SDK call itself succeeded — this isn't the quota-exhaustion case (total_cost_usd==0 && num_turns==1).

Job logs for those runs show claude-code-action logs only the init/final result messages by default (show_full_output: false hides everything in between "for security"), so the exact denied tool call isn't visible in the log — but the evidence converges on the same explanation:

  • The reviewer's computed allowedTools for this run was ["mcp__github_inline_comment__create_inline_comment", "Bash(python3:*)"] (plus the disallowed git-write/python3 -c/-m list) — no WebFetch/WebSearch. Per this file's own comments, agent mode's default allowlist covers only Read/Glob/Grep, comment-update + CI MCP tools, and git Bash.
  • CLAUDE.md's own review guideline Smoke test: @claude agent workflow #4 (fact-check prose against external sources) and the review prompt's hallucination-detection instructions both tell the reviewer to verify claims "against the codebase and its dependencies" / an external source — but grant it no tool to do so.
  • PR CLAUDE.md: a rendered docs site 403 doesn't mean the source is inaccessible #180's diff was specifically about WebFetch-ing external URLs (raw.githubusercontent.com, a GitHub Pages doc site) — exactly the kind of content that would prompt the reviewer to attempt the denied call.
  • Two other runs on the same PR (an earlier commit) completed normally with full verdicts, and a separate run (28612906160) explicitly noted "network access was blocked" and still produced a full verdict — so a denial doesn't always derail the agent, just inconsistently.

Fix

Rather than widen the sandbox (granting broad WebFetch access raises its own question — an agent with review-only secrets access fetching attacker-influenceable URLs from a diff is a prompt-injection/exfiltration surface worth deciding deliberately, not as a side effect of this bug fix), this PR targets the actual failure mode: the reviewer sometimes stops instead of finishing when a tool call is denied.

claude-code-review.yml's --append-system-prompt now tells the reviewer up front that network-fetch tools aren't available in this sandbox (so it fact-checks from the repo's own content and its own knowledge instead of attempting the fetch, noting when something can't be independently verified) — and, more generally, that a denied tool call is never a reason to stop the review early; it must always finish with its findings and the explicit ### Verdict line.

Follow-up worth a separate decision

If live fact-checking against external docs (matching CLAUDE.md guideline #4) is wanted badly enough to justify the tradeoff, a follow-up could grant a domain-scoped WebFetch(domain:...) allowlist instead of leaving it fully disabled. Flagging rather than deciding here since it's a security-posture call for a workflow shared across every consumer repo, some of which may be private.

Testing

  • python3 -c "import yaml; yaml.safe_load(open('.github/workflows/claude-code-review.yml'))" — parses cleanly.
  • Added a changelog fragment per changelog.d/README.md.

Closes #185.


Generated by Claude Code

check-review-execution.sh's stub-review guard (gha#172/#176) correctly fails
a claude-review run that ends with no verdict, but the root cause of those
stub runs was still open (gha#173, tracked further in gha#185): the reviewer's
own fact-checking/hallucination-detection instructions can prompt it to
attempt a WebFetch/WebSearch call that agent mode's default allowedTools
doesn't grant, and on denial the agent sometimes stopped instead of finishing
with a verdict. Reproduced 3/3 times on PR #180 with an identical
permission_denials_count:1/num_turns:4 fingerprint.

Tell the reviewer up front that network-fetch tools aren't available in this
sandbox (so it fact-checks from the repo and its own knowledge instead of
attempting the fetch) and that a denied tool call is never a reason to stop
the review early.

Closes #185.
Copilot AI review requested due to automatic review settings July 2, 2026 21:50
@github-actions
github-actions Bot removed the request for review from Copilot July 2, 2026 21:50
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Claude finished reviewView run


Review: PR #187fix(claude-code-review): don't let a denied tool call stub the review

Scope of the diff: purely additive text appended to the existing --append-system-prompt block in .github/workflows/claude-code-review.yml (14 additions / 1 deletion — just repositioning the closing quote), plus a new changelog fragment changelog.d/review-continue-past-tool-denial.fixed.md.

What I checked

  • YAML/shell correctness: parsed the file with python3 -c "import yaml; yaml.safe_load(...)" and printed the folded claude_args string directly — it resolves cleanly, no unbalanced quotes, no shell-breaking characters introduced. The new blank line inside the >- folded scalar produces a single \n paragraph break (standard YAML folding), not a bug.
  • Factual accuracy of the new prompt claim ("no network-fetch tools enabled"): confirmed against the workflow's own --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(python3:*)" (line 365) and its accompanying pre-existing comment that the action's base allowlist is Read/Glob/Grep + comment/CI MCP tools + git Bash only — WebFetch/WebSearch are genuinely absent. Accurate.
  • Consistency with CLAUDE.md's general "try a raw fetch" guidance: no real conflict — that guidance applies to general Claude Code sessions; this system-prompt addition is narrowly scoped to this one hard-restricted reviewer job, where the tools genuinely aren't grantable.
  • Referenced issues/PRs (claude-review stub-review flake reproduces 3x with identical permission_denials_count: 1 signature #185, fix(claude-code-review): fail the check on stub/placeholder reviews #172, Add fixture-based test coverage for claude-code-review's fail-check guard logic #174, Add fixture-based test coverage for claude-code-review's fail-check guard logic #176, CLAUDE.md: a rendered docs site 403 doesn't mean the source is inaccessible #180): all verified to exist and match the described history via gh issue view/gh pr view.
  • Changelog fragment: filename (review-continue-past-tool-denial.fixed.md) and category (fixed) match changelog.d/README.md's <slug>.<category>.md convention; no ### Fixed heading in the body (correct — assemble.sh adds it); content matches keepachangelog-style bullet format used by sibling fragments.
  • CLAUDE.md doc-sync rules: the "new workflow_call input" and "composite/example-stub sync" rules don't apply — no input was added, and claude-code-review.yml has no backing composite/example page requiring a matching edit.
  • Broad "continue past any denial" wording: not a real risk — this is a review-only, non-interactive CI agent whose only allowed tools are the inline-comment tool and python3; a denial can't hide a mutating side effect, and the sandbox (not the prompt) is what actually enforces every tool denial regardless of what the prompt says.

Four independent review passes (2× CLAUDE.md compliance, 2× bug/security scan) all converged on the same conclusion: no defects found. Nothing required validation in a follow-up pass.

Verdict

Ready for merge. No issues found. Checked for bugs, YAML/shell correctness, factual accuracy of the new prompt text, and CLAUDE.md compliance (changelog fragment format, doc-sync rules).

Note: this run was not invoked with --comment, so per the code-review skill's process this summary is reported here only — no GitHub comment was posted.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the claude-code-review reusable workflow against a flaky failure mode where a mid-review tool permission denial can derail the agent and result in a green run that posts only a stub (missing an explicit verdict), which downstream guard logic then correctly fails.

Changes:

  • Extend claude-code-review.yml’s appended system prompt to pre-declare that WebFetch/WebSearch are unavailable and to require continuing the review after any denied tool call, finishing with an explicit verdict line.
  • Add a changelog fragment documenting the fix and its motivation/root cause (#185).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
.github/workflows/claude-code-review.yml Updates the reviewer’s system prompt to avoid derailing on denied tool calls and to set expectations about lack of network-fetch tools.
changelog.d/review-continue-past-tool-denial.fixed.md Adds a Keep a Changelog–style fragment describing the fix for the stub-review failure mode.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@d-morrison
d-morrison merged commit e088a47 into main Jul 2, 2026
19 checks passed
@d-morrison
d-morrison deleted the claude/gha-issue-185-2merhb branch July 2, 2026 22:58
d-morrison added a commit that referenced this pull request Jul 3, 2026
…erdict (#201)

* fix(claude-code-review): retry once when a review completes with no verdict

Reopened #185 after a fresh reproduction post-#187: the low-denial
stub-review signature (is_error:false, permission_denials_count:1, no
verdict) still recurs even with #187's "don't stop on a denied tool call"
system-prompt fix in place.

check-review-execution.sh now surfaces this specific, retryable case (real
non-empty text, no SDK error, no verdict) as a stub_review output, distinct
from a hard SDK error or genuinely empty output. claude-code-review.yml
retries the same review prompt once when it fires, with an added instruction
that the retry must end with a verdict regardless of what gets denied along
the way, before failing the check for real.

The anthropics/claude-code-action call moved into a new
run-claude-review-attempt composite action so the retry doesn't duplicate
that ~100-line step. The raw execution output is now also uploaded as a
workflow artifact on every attempt, so a future recurrence has a
downloadable transcript to diagnose instead of needing show-full-output
pre-enabled and a lucky re-trigger.

Closes #185.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B2aHbEXwTUA13vYMuQXqas

* fix(claude-code-review): gate stub-review retry on a denial-count threshold

Review finding on #201: the retry's header comment and PR description
claimed gha#198's high-denial-count no-verdict pattern was excluded from
the retry, but check-review-execution.sh never actually checked
permission_denials_count — stub_review fired purely on "no verdict",
which is textually identical between #185 (denials: 1) and #198
(denials: 17-35). Add an actual permission_denials_count <=
STUB_RETRY_MAX_DENIALS (default 5) gate, a fixture modeling #198's
pattern expecting plain `fail` (not `fail-stub`), and correct every
comment that claimed the exclusion without implementing it.

Also fixes a second finding: "Resolve final review outcome"'s failure
message always attributed a double-failure to attempt 1's stub_review
flag, even when the retry failed for a different reason. It now inspects
which attempt actually ran and why.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B2aHbEXwTUA13vYMuQXqas

* refactor(claude-code-review): dedupe resolve+upload steps into a composite

Review nit on #201: the attempt-1 and retry "Resolve execution file
path"/"Upload review execution output" step pairs duplicated the same
fallback-path shell logic and upload-artifact shape almost verbatim -
the same class of duplication run-claude-review-attempt was extracted
to avoid, just smaller. Folded both into a new upload-review-execution
composite action.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B2aHbEXwTUA13vYMuQXqas

---------

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

claude-review stub-review flake reproduces 3x with identical permission_denials_count: 1 signature

3 participants