Skip to content

Stop wasting review turns retrying tools the sandbox denies#28

Merged
gobetti merged 1 commit into
mainfrom
fix/claude-review-sonnet5-turn-waste
Jul 6, 2026
Merged

Stop wasting review turns retrying tools the sandbox denies#28
gobetti merged 1 commit into
mainfrom
fix/claude-review-sonnet5-turn-waste

Conversation

@gobetti

@gobetti gobetti commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Description

Claude Code exposes its full standard toolset to the model regardless of --allowedTools scoping; an allow-list only gates execution at call time, it doesn't hide the tool from the model's context (that requires a bare deny rule, which we don't use here). So the model has no way to know in advance which of its visible tools will actually work in this sandboxed review — it only finds out by attempting the call and eating a permission denial, one turn at a time.

That was cheap and mostly invisible on Sonnet 4.6. Now that the org default is Sonnet 5, reviews retry disallowed tools noticeably more (build/lint/test attempts, LS, a WebFetch for the org CLAUDE.md link), and on larger diffs the wasted turns tip the review past max_turns entirely — ht-android hit three straight max-turns failures on one PR today, while a same-day, same-config sibling PR with a slightly smaller diff passed with turns to spare.

Fix, without touching max_turns:

  • Tell the prompt directly which tools are actually available, so the model stops discovering the boundary by trial and error.
  • Add LS to the allow-list — already granted one step later, in the sticky-comment update.
  • Fold the Mobile and Infrastructure rubric sections — previously reachable only via a WebFetch to the org CLAUDE.md — directly into the prompt, since the rest of that file was already duplicated here. Removes the WebFetch attempt instead of just tolerating its denial.

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

Only changes the shared review workflow's prompt and tool allow-list — no functional or schema change. Not yet run against a live PR; will confirm by watching turn counts and permission-denial counts on the next few PR reviews across repos.

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

Claude Code exposes its full standard toolset to the model
regardless of --allowedTools scoping; an allow-list only gates
execution at call time, it doesn't hide the tool from the model's
context (that requires a bare deny rule, which we don't use here).
So the model has no way to know in advance which of its visible
tools will actually work in this sandboxed review — it only finds
out by attempting the call and eating a permission denial, one
turn at a time.

That was cheap and mostly invisible on Sonnet 4.6. Now that the org
default is Sonnet 5, reviews retry disallowed tools noticeably more
(build/lint/test attempts, LS, a WebFetch for the org CLAUDE.md
link), and on larger diffs the wasted turns tip the review past
max_turns entirely — ht-android hit three straight max-turns
failures on one PR today, while a same-day, same-config sibling
PR with a slightly smaller diff passed with turns to spare.

Fix, without touching max_turns:
- Tell the prompt directly which tools are actually available, so
  the model stops discovering the boundary by trial and error.
- Add LS to the allow-list — already granted one step later, in
  the sticky-comment update.
- Fold the Mobile and Infrastructure rubric sections — previously
  reachable only via a WebFetch to the org CLAUDE.md — directly
  into the prompt, since the rest of that file was already
  duplicated here. Removes the WebFetch attempt instead of just
  tolerating its denial.
@gobetti gobetti merged commit 4748e34 into main Jul 6, 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