fix(workflows): grant the gh pr diff Bash command both review-lane prompts already instruct - #319
Merged
Merged
Conversation
…mpt already instructs The default review prompt tells the model to use `gh pr diff` to identify what changed, but the default allowedTools granted no Bash at all — only the inline-comment MCP tool. A live run (provisioning#235, run 30503910653) confirms the effective grant list omits it and recorded 4 permission denials; the review only worked there because the one changed file was reachable via Read/Grep on the checkout, which does not scale to a multi-file PR. Adds a scoped `Bash(gh pr diff:*)` grant, matching the minimal-grant pattern anthropics/claude-code-action's own security docs recommend for gh subcommands (`Bash(gh issue view:*)`). Read-only, so it carries no additional exposure for a fork PR's read-only token. Fixes #309 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C
|
Claude finished @kyle-sexton's task in 1m 30s —— View job Code review:
|
|
Claude finished @kyle-sexton's task in 2m 44s —— View job Security Review
Scope: FindingsNo CRITICAL or IMPORTANT security issues found in this diff. SUGGESTION (confidence: low) — Prefix-matched Everything else checks out:
|
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…s claude-review The security lane's default prompt gives the identical 'use gh pr diff' instruction with the same MCP-only default allowlist, so the agent hit the same denial; mirror the scoped read-only grant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STASxNiqEm7doy2Z65un4H
kyle-sexton
changed the title
|
Claude finished @kyle-sexton's task in 40s —— View job Security Review
Scope: This run covers the full PR diff as of FindingsNo CRITICAL or IMPORTANT security issues found in this diff. No new findings beyond what was already reported on the
The prefix-matched |
kyle-sexton
added a commit
that referenced
this pull request
Jul 30, 2026
…tion as the review's own (#320) ## Summary A posted review (provisioning#235, run 30503910653) closed with: "No findings — this is an accurate, well-scoped correction backed by verified evidence (\`gh variable list\`, \`gh api orgs/.../actions/variables\`, and direct \`ci.yml\` reads). Nothing blocking." ## Verification - The reviewed run's effective `allowedTools` (confirmed in #309's verification) grants no `Bash(gh:*)` at all, so the review could not have run either `gh` command itself. - The PR body's own Test plan section (melodic-software/provisioning#235) reads verbatim: - `gh variable list --repo melodic-software/provisioning` → empty. - `gh api orgs/melodic-software/actions/variables` → no `CI_PESTER_ENABLED`. - The posted review's closing sentence names exactly those two commands as "verified evidence" it is claiming credit for, despite having no tool grant to run them. It restated the author's self-reported verification as its own. ## Fix Added one instruction to the default review prompt: never restate a PR author's own claimed verification as the review's own conclusion; label anything not independently run as author-claimed and unverified. Kept minimal — no other prompt or grant changes (the companion tool-grant gap is #309 / #319). ## Test plan - `yq -e '.' .github/workflows/claude-review.yml` — valid YAML. - `node --test .github/scripts/claude-lane-incident.test.cjs .github/scripts/claude-lane-retry-gate.test.cjs .github/scripts/claude-review-superseded-guard.test.cjs` — 89/89 pass. ## Related - `melodic-software/provisioning`#235 (run 30503910653) — the reviewed PR whose posted review restated the author's claimed verification as its own. - #309 / #319 — the companion tool-grant gap surfaced by that same review run. Fixes #310 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Aug 3, 2026
…ne comments (#338) ## Summary The `claude-review` lane has posted **zero** line-anchored inline comments fleet-wide (~10,563 all-time `claude-review*` runs, 3,161 PRs), despite `mcp__github_inline_comment__create_inline_comment` being granted and reaching the agent — the effective `SDK options` echo lists it. The tool was installed and never called. **Why it was never called.** The assembled prompt never mentioned the tool, while the pinned action's own base prompt steers hard the other way: - `Never create new comments. Only update the existing comment using mcp__github_comment__update_claude_comment.` - `You communicate exclusively by editing your single comment - not through any other means.` - under **What You CANNOT Do**: `- Submit formal GitHub PR reviews` That text is authored by `anthropics/claude-code-action` and **cannot be changed here**. What can change is the lane's own prompt, which is appended after it and gets the last word. **What made the difference.** `claude-security-review.yml` carries the identical tool grant and *does* produce line-anchored comments. The only prompt-level delta is that its criteria body says *"Report findings as a pull request review."* This change gives the code-review lane the equivalent instruction and additionally **names the tool**, so the grant is discoverable rather than merely present. **Placement.** In the step's prompt wrapper, not the `prompt` input default. The input's documented contract is "just the instructions for what to look for", so reporting mechanics belong beside the structural header and permalink form — and the wrapper survives a caller that overrides the criteria body. Both the first attempt and the retry carry byte-identical prompt blocks. The pre-existing commit-permalink guidance is **scoped, not removed**: it now covers findings no changed line can carry, so it no longer competes with the inline-comment instruction for the same finding. ### Deliberate non-changes - `display-report` and `show_full_output` stay **off**. Public-repo log hygiene is untouched. (`show_full_output: false` is exactly why the logs cannot distinguish "never called" from "called and failed" — an accepted limitation, not a bug fixed here.) - No caller input is added or required. The fix lives entirely inside the reusable workflow, so the runner-policy contract (`allowedInputs` for this workflow) is unaffected. - `skip-actors` and `allowed_bots` are untouched. - **Nothing changes in `melodic-software/standards`.** `REVIEW.md` is the shared review-*criteria* prose and is also injected into Managed Code Review and local `review`-plugin runs; it says nothing about output form today, and lane plumbing (which MCP tool to call) does not belong there. Its downstream materialization in this repo is a managed sync target and was not touched. - The two lanes are left structurally asymmetric — the security lane's reporting instruction sits in its `prompt` input default, this one's sits in the wrapper. That is deliberate scope discipline, not an oversight; the wrapper is the correct home per the input's own contract, and normalizing the security lane is a separate change. ## Test plan A prompt change cannot be proven by unit test, so this was demonstrated live. **Setup.** Throwaway PR #337 (`demo/332-inline-comment-evidence`) based on **this branch**, adding one Node script with genuine, line-anchorable defects. `claude-review-self.yml` uses the local `./` ref, so the review ran against the **modified** reusable (`create_inline_comment` appears 3x in the demo tree's workflow vs 1x on `main`). The changed file matches none of the security lane's `paths` patterns, so `security-review` **skipped** — every `claude[bot]` inline comment on that PR is attributable to the code-review lane. **Result — 5 line-anchored inline comments where there were previously zero:** | Line | Severity | Comment | | --- | --- | --- | | 41 | 🔴 Important | #337 (comment) | | 51 | 🔴 Important | #337 (comment) | | 47 | 🟡 Nit | #337 (comment) | | 50 | 🟡 Nit | #337 (comment) | | 65 | 🟡 Nit | #337 (comment) | **Attribution controls:** - Run: https://github.com/melodic-software/ci-workflows/actions/runs/30768850693 — agent `init` 21:53:04Z → `result` `success` 21:55:13Z, 20 turns. All five comments landed 21:54:39Z–21:54:56Z, strictly inside that window. - `security-review / security-review` job: **skipped** (run 30768850685). - The five P1/P2-badged comments on #337 are `chatgpt-codex-connector[bot]`, a different reviewer, grouped under a single review id — not this lane. - Each of the five is its own single-comment review object, which is the shape `create_inline_comment` produces. **Note for future debugging:** the action still logs `No buffered inline comments` on this run even though five were posted. That step covers a different (batched) path; `create_inline_comment` posts immediately. The log line is **not** a usable signal for whether the tool was called. `actionlint` clean; both prompt blocks verified byte-identical. ## Review round Two threads, both addressed with code. **P2 (codex) — the inline grant did not survive a caller overriding `claude-args`.** Real, and worse than reported. The grant lived in the `claude-args` **input default**, which a caller replaces wholesale, so a consumer passing `claude-args` for an unrelated reason (selecting a model) dropped it silently — while the non-overridable wrapper prompt still asserted the tool was granted. And because this lane runs in **tag mode**, the action derives *which MCP servers to install* from these same args (`parseAllowedTools` → `prepareMcpConfig`), so the inline-comment server would not even be installed. Fixed in 923a2d2: `compose-args` appends the grant unconditionally, and it comes out of the overridable default. That step feeds both the first attempt and the retry, so one edit covers both. The invariant, now written into the file: **whatever the non-overridable wrapper prompt asserts must itself be non-overridable.** `Bash(gh pr diff:*)` deliberately stays overridable, because the instruction that uses it lives in the equally overridable `prompt` input. Safe because repeated `--allowedTools` flags accumulate and dedupe rather than overwrite (`ACCUMULATING_FLAGS`, both spellings, upstream-tested) — tag mode already relies on this, emitting its own `--allowedTools` before appending the caller's. Verified by executing the pinned action's real `parseAllowedTools` against the strings `compose-args` emits: the default path yields an effective tool set **identical to pre-fix** (no happy-path regression), every override scenario keeps the grant, and the pre-fix override reproduces the bug as an empty list. **SUGGESTION (claude) — widened trust surface.** Asked for no code change, only a `SECURITY MODEL` note; added. ### Corrections this round (8d7ab14) Independent verification caught two statements that were not true, in a file where the comments are the contract. - The `SECURITY MODEL` note repeated the review thread's claim that the action buffers and classifies inline comments before posting. **It filters nothing in this lane:** classification requires `ANTHROPIC_API_KEY`, and this lane passes only `claude_code_oauth_token`, so `classifyComments` returns `null` and every buffered comment posts. The error ran in the reassuring direction, describing a control that is not there. - The step comment claimed a granted tool the prompt never mentions "reads as unusable and goes uncalled". **This repo's own security lane disproves it** — that lane grants the tool, never names it, runs in the same tag mode under the same base prompt, and does post inline comments. The load-bearing part is instructing the agent to report findings *as a pull request review*; naming the tool is belt-and-braces, and the comment now says so. The empirical result the PR rests on is unaffected: the five line-anchored comments on #337 stand. Only the causal explanation was overstated. ### Follow-ups filed, deliberately not folded in - **#340** — `claude-security-review.yml` has the identical latent gap (same grant in its overridable default, forwarded verbatim at two call sites). It has no compose step, so closing it needs a new one. - **#341** — the first-attempt/retry prompt-block parity invariant is stated in a comment but enforced by nothing; the sibling lane already has that test. Checked by hand here (both blocks identical, 810 bytes) precisely because no test does it. ## Related Closes #332. Follow-ups from review: #340, #341. Related: #337 (throwaway evidence PR, `do-not-merge`, deleted after review). The `Bash(gh pr diff:*)` grant flagged during investigation is already fixed on `main` by ef63101 (#319) and is present in the effective `allowedTools` — the stale `fix/309-claude-review-gh-pr-diff-grant` remote branch is superseded. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The default review prompt (
claude-review.yml'spromptinput) tells the model "Use `gh pr diff` to identify what changed," but the defaultclaude-args--allowedToolsgrants no Bash at all — only the inline-comment MCP tool. On a large PR the model has no sanctioned way to enumerate the diff.claude-security-review.yml's default prompt gives the identical "use `gh pr diff`" instruction with the same MCP-only default allowlist, so the security lane hits the same denial.Verification
Fetched run 30503910653 (melodic-software/provisioning#235, reusable at c136b27, v0.9.1):
allowedTools:Glob, Grep, LS, Read, mcp__github_comment__update_claude_comment, mcp__github_ci__get_ci_status, mcp__github_ci__get_workflow_run_details, mcp__github_ci__download_job_log, mcp__github_inline_comment__create_inline_comment, Bash(git add:*), Bash(git commit:*), Bash(<git-push.sh>:*), Bash(git rm:*)— noBash(gh:*)of any kind."permission_denials_count": 4, consistent with the model attempting and being denied tool calls the prompt instructed it to make.gh pr diffin the currentmaindefault.anthropics/claude-code-action's owndocs/security.md(line 18) that the upstream-recommended pattern for exactly this situation is a scoped grant likeBash(gh issue view:*)— not rewriting the prompt to usegit diff, which would face its own problem: the checkout step usesfetch-depth: 1(single commit, no base ref available locally), sogit diff <base>...HEADwould not work today either even if granted.Fix
Added
Bash(gh pr diff:*)to the defaultclaude-args--allowedToolsof bothclaude-review.ymlandclaude-security-review.yml, aligning each lane's grant with what its prompt already instructs (the security lane was this PR's own review finding — same root cause, fixed here rather than deferred). Read-only command, so it adds no exposure for a fork PR's read-only token (fork PRs aren't reviewed at all per this repo'sCLAUDE.md).claude-e2e-verify.ymlis deliberately untouched: its defaultclaude-argssets no--allowedToolsat all, a different grant model whose defaults this fix does not own.Test plan
yq -e '.' .github/workflows/claude-review.yml/yq -e '.' .github/workflows/claude-security-review.yml— valid YAML.node --test .github/scripts/claude-lane-incident.test.cjs .github/scripts/claude-lane-retry-gate.test.cjs .github/scripts/claude-review-superseded-guard.test.cjs— 89/89 pass.Related
melodic-software/provisioningfix(link-check,queue-monitor-liveness,tool-version-drift-check): restrict tracking-issue adoption to the workflow's own token author #235 (run 30503910653) — the review run whose composed SDK options supplied theallowedToolsevidence above.Fixes #309
🤖 Generated with Claude Code
https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C