Skip to content

Restore read-only shell to PR review (grep, cat, head, tail)#30

Merged
gobetti merged 1 commit into
mainfrom
chore/allow-readonly-shell-in-review
Jul 7, 2026
Merged

Restore read-only shell to PR review (grep, cat, head, tail)#30
gobetti merged 1 commit into
mainfrom
chore/allow-readonly-shell-in-review

Conversation

@gobetti

@gobetti gobetti commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Description

The review's --allowedTools was added in 834a453 (#12) to grant git commands for PR analysis — the commit describes it as "grants Claude access to git commands," and before it there was no --allowedTools at all, so the action ran on its permissive default set (which includes general Bash). But --allowedTools is an exclusive whitelist: the moment it's set, everything not listed is denied. So that change silently dropped grep, cat, and general Bash. The denial was a side effect of the flag's semantics, not a deliberate least-privilege decision.

The cost shows up now that reviews run on Sonnet 5: it still reaches for shell grep (including git show … | grep to search a specific blob version) when verifying findings across files, and each denied call burns a review turn plus a retry. On repos with a tight max_turns that waste contributes to error_max_turns failures.

This adds the read-only inspection commands back to the allow-list (grep, cat, head, tail, wc) so those calls execute instead of being denied. sed and find are intentionally left out (sed -i can write; find / can walk the whole filesystem). Builds, tests, installs, file edits, and network remain unlisted and therefore still denied — the blast radius is unchanged, since these commands only re-expose what the Read and Grep tools already permit over the same checkout. The prompt's now-contradictory "don't use shell grep" steer is dropped to match.

Ticket

N/A — ad hoc infra fix, not tied to a ticket.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Testing Done

Changes only the review workflow's allow-list and prompt (no functional or schema change). Reproducing failing reviews locally with the restricted tool-set showed Sonnet 5 making shell grep calls that the current allow-list denies (matching the permission_denials_count in those runs); the added commands remove that denial class. Will confirm on live PR reviews by watching denial counts drop.

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have added tests that prove my fix/feature works
  • New and existing tests pass locally
  • I have updated documentation as needed

The review's `--allowedTools` was added in 834a453 to grant git
commands for PR analysis, but `--allowedTools` is an exclusive
whitelist: setting it silently dropped the general Bash the action
allowed by default, so `grep`, `cat`, and friends are now denied.
That was a side effect of the flag, not a deliberate restriction —
the commit's intent was to enable analysis, not lock it down.

Sonnet 5 still reaches for `grep` (including `git show | grep`) to
verify findings across files, and each denied call burns a review
turn plus a retry. Add the read-only inspection commands back to
the allow-list so those calls run instead of being denied. Builds,
tests, installs, edits, and network stay unlisted and therefore
still denied, so the blast radius is unchanged — these commands
only re-expose what the Read and Grep tools already permit.

Also drops the now-contradictory "don't use shell grep" steer from
the prompt, since shell grep is allowed again.
@gobetti gobetti merged commit f92c5b9 into main Jul 7, 2026
1 check 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.

2 participants