Skip to content

ci: scope claude review/mention bots to read-only git tools#1069

Merged
shane-moore merged 1 commit into
sigp:stablefrom
shane-moore:fix/claude-review-git-read-tools
May 30, 2026
Merged

ci: scope claude review/mention bots to read-only git tools#1069
shane-moore merged 1 commit into
sigp:stablefrom
shane-moore:fix/claude-review-git-read-tools

Conversation

@shane-moore

@shane-moore shane-moore commented May 29, 2026

Copy link
Copy Markdown
Member

Problem, Evidence, and Context (Required)

Change Overview (Required)

  • claude-pr-review: extend the existing allowlist with read-only git (status/diff/show/log/rev-parse/merge-base/grep).
  • claude-mentions: introduce an allowlist (Glob/Grep/LS/Read, the comment + inline-comment + CI MCP tools, read-only git) and an explicit --disallowedTools deny for file edits and commit-producing git operations (Edit/MultiEdit/Write/NotebookEdit, git add/commit/rm).
  • The inline-comment tool is included so the bot can follow review.md's "use inline comments" instruction instead of stalling on a denied tool.
  • Intentionally unchanged: --max-turns 50, the model, and the prompt-loading step.

Risks, Trade-offs, and Mitigations (Required)

  • Blast radius is limited to the two Claude bot workflows; no product code.
  • Trade-off: introducing an allowlist on claude-mentions also restricts it to the listed tools. Mitigated by including the inline + sticky comment tools so it retains a working output path.
  • claude-mentions remains review-oriented: it can inspect code and comment, but cannot edit files or create commits. The action's version-pinned push wrapper is not explicitly denied to avoid a brittle path-specific entry; with edits and commits denied, there should be nothing new to push.

Validation (Required)

  • pull_request_target/mention workflows load from the default branch, so this can only be exercised once merged to stable. First real test is an @claude mention post-merge.

Rollback (Required for behavior or runtime changes; optional otherwise)

  • Revert the commit; no config, data, or runtime state involved.

@shane-moore

Copy link
Copy Markdown
Member Author

@claude, please take a look

@claude-code-actions-sigp

claude-code-actions-sigp Bot commented May 29, 2026

Copy link
Copy Markdown

Claude finished @shane-moore's task in 3m 14s —— View job


Review

  • Read the two changed workflow files
  • Cross-check the diff vs origin/stable
  • Verify the git-push.sh action-version pin claim
  • Check the prompt expectations in review.md
  • Post review feedback

No actionable issues found. The change is narrow, well-scoped, and the design choices match the stated rationale.

A few observations (non-blocking):

1. Allowlist is the primary defense; the deny list is genuinely belt-and-suspenders. Claude Code's Bash permissions are deny-by-default — anything not in --allowedTools already requires user approval, which doesn't exist in CI. So if v1.0.127 is bumped and the absolute path in --disallowedTools (.github/workflows/claude-mentions.yml:96) goes stale, the deny becomes a no-op but the allowlist still blocks git-push.sh (it's not listed). That makes the sync comment at line 91 a quality-of-defense reminder rather than a correctness requirement, which seems right.

2. Asymmetry between the two workflows is intentional and consistent.

  • claude-pr-review.yml keeps Bash(gh pr comment|diff|view:*) because it relies on gh to fetch the diff and post the summary comment.
  • claude-mentions.yml deliberately omits gh and routes comment output through mcp__github_comment__update_claude_comment / mcp__github_inline_comment__create_inline_comment, plus uses git diff/show/log for repo introspection.

Both end up read-only by construction. Worth being aware that the mentions bot now has no gh CLI access at all — if a future mention requires e.g. gh run view for a CI question, the MCP CI tools (mcp__github_ci__*, already in the allowlist) cover it, so this should be fine.

3. Bash(git fetch:*) is not on either allowlist. Both workflows do their own git fetch in earlier steps (Read review prompt, line 79 / line 100; and setupBranch() inside the action), so the bot inherits an origin/stable ref without needing to fetch. If a future use case requires fetching an additional ref, it will fail closed — which is the safer default.

4. --max-turns 50 retained. The PR description correctly identifies the failure mode (turn exhaustion from re-fetching context) as a symptom of missing git tools, not a turn-budget problem. Adding the tools rather than raising the budget is the right call.

LGTM.
fix/claude-review-git-read-tools

The claude-mentions bot ran with no tool allowlist, and neither bot had
git read tools. Add an explicit allowlist so both bots can orient with
read-only git (diff/show/log/merge-base/rev-parse/grep/status):

- claude-pr-review: extend the existing allowlist with read-only git.
- claude-mentions: introduce an allowlist (Glob/Grep/LS/Read, the
  comment + inline-comment + CI MCP tools, read-only git) and an explicit
  disallowedTools deny for Edit/Write and git write/push, keeping the
  mention bot strictly read-only.

The inline-comment tool is included so the bot can follow review.md's
"use inline comments" instruction instead of stalling on a denied tool.
@shane-moore shane-moore force-pushed the fix/claude-review-git-read-tools branch from fc1b05f to fe3fd1c Compare May 29, 2026 19:20
@shane-moore

shane-moore commented May 29, 2026

Copy link
Copy Markdown
Member Author

Thanks @claude, this matches my read overall.

  1. Agree that claude-mentions should be review-oriented, not fix/apply mode. I removed the write/push allowlist entries and kept an explicit deny for edits plus git add/commit/rm. One nuance: in tag mode the deny list is more than belt-and-suspenders because the action injects write-capable defaults and allowedTools are accumulated.

  2. I’m intentionally keeping gh out of claude-mentions for now. It has mcp__github_comment__update_claude_comment for the tracking comment, mcp__github_inline_comment__create_inline_comment for review feedback, and mcp__github_ci__* for CI questions. The git read commands cover local repo/diff inspection, so gh would be extra shell surface without a current need.

  3. I removed the version-pinned git-push.sh deny to avoid maintaining an action-path-specific rule. With edits and commits denied, there should be nothing new to push.

  4. Agree on the caveat: the hidden transcript means we can’t prove the exact denied commands. This PR fixes the visible mismatch between the review prompt and the allowed tools; if the next failure has no permission-denial churn, then we should look at prompt scope or --max-turns.

@claude-code-actions-sigp

claude-code-actions-sigp Bot commented May 29, 2026

Copy link
Copy Markdown

Claude finished @shane-moore's task in 1m 32s —— View job


Reply

Acknowledging the corrections — all four points land.

1. You're right that the deny list is load-bearing in tag mode, not belt-and-suspenders. I was reasoning from stock Claude Code semantics (deny-by-default for Bash, no implicit Edit/Write), but claude-code-action@v1.0.127 tag mode injects write-capable defaults into the effective allowlist and accumulates user-supplied --allowedTools on top. Without the explicit --disallowedTools line at .github/workflows/claude-mentions.yml:95 denying Edit,MultiEdit,Write,NotebookEdit,Bash(git add:*),Bash(git commit:*),Bash(git rm:*), those write paths would remain enabled by the injected defaults. Withdrawing my "deny is redundant" framing — the deny list is the actual enforcement boundary here.

2. Keeping gh out of claude-mentions — agreed. The MCP tools (mcp__github_comment__update_claude_comment, mcp__github_inline_comment__create_inline_comment, mcp__github_ci__*) plus the read-only git verbs cover the review-and-comment surface without adding shell-level GitHub API access. If a future use case actually needs gh, adding it then is cheaper than carrying the surface preemptively.

3. Dropping the path-pinned git-push.sh deny — agreed. With Bash(git add:*) and Bash(git commit:*) denied, there's no new commit to push, so a push entry would be deny-list theater that goes stale on every action version bump. The remaining deny entries are version-independent.

4. Diagnostic plan — agreed. This PR closes the visible mismatch between the review.md prompt's "orient against the diff" expectation and the missing read-only git tools. If the next non-trivial review still hits the 50-turn ceiling without permission denials in the trace, the next-most-likely culprits are prompt scope (e.g., the watchdog vs. mention prompts pulling for too much context) or the turn budget itself — but those should only get tuned with evidence, not preemptively.

LGTM.

  • Verify "tag mode injects write-capable defaults and allowedTools accumulate" claim
  • Re-check current claude-mentions.yml allow/deny lists vs. the response
  • Post follow-up
    fix/claude-review-git-read-tools

@shane-moore shane-moore merged commit e65ec5e into sigp:stable May 30, 2026
14 of 18 checks passed
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.

3 participants