Claude review runs finish green having posted nothing: the allowlist forbids every read tool the prompt invites, denial churn ends sessions without a verdict, and the job never checks that it posted

[erikdarlingdata]

Summary

Tonight at least seven Claude-review runs across three PRs completed green and posted nothing — no comment, no inline note, no verdict — so the review guard (#2229/#3492) failed each one as "do NOT read this PR as reviewed", and each cost a paid run (~$2.2–3.0) plus a guard cycle plus human diagnosis. On one PR the review was re-run five times on the same head with the same empty result; a fresh head then reviewed normally. The working theory on the floor was that the review is head-keyed and no-ops on a head it has already processed. That is refuted: the workflow (.github/workflows/claude-review.yml) carries no head-keyed skip, every run checks out the head and starts Claude fresh, and #3618's attempt 2 was a re-run on the same head that posted a real changes-requested. The mechanism is in the swallowed runs' own result blocks.

What the swallowed run says about itself (VERIFIED from its log)

"type": "result", "subtype": "success", "is_error": false,
"num_turns": 50, "permission_denials_count": 18, "total_cost_usd": 2.19

Fifty turns, eighteen tool-permission denials, ended "success", zero GitHub posts. The successful run on the same PR's fresh head: 65 turns, 13 denials, posted. So 50 is not a cap — Claude ended the session itself, after a run dominated by denials, without executing the verdict protocol the prompt calls mandatory.

The denials have an obvious source (VERIFIED): the prompt tells Claude "The PR branch is already checked out in the working directory" and asks for a correctness/parity/security review — an invitation to read code — while --allowedTools permits only mcp__github_inline_comment__create_inline_comment, Bash(gh pr comment:*), Bash(gh pr diff:*), Bash(gh pr view:*), Bash(gh pr review:*). No Read, Grep, Glob, no git. Every attempt to open a file the diff touches, find a symbol's other callers, or check a parity twin is a denial. Thirteen to eighteen per run is the reviewer fighting its own harness. HYPOTHESIS (the per-turn transcript is written to claude-execution-output.json on the runner and not retained, so this cannot be proven from tonight's logs): after enough denials Claude sometimes gives up and ends with a plain-text summary — which the prompt forbids but cannot prevent — instead of gh pr review. Stochastic, so a re-run may work (as #3618's did) and may not five times running (as #3642's didn't); the empty commit "worked" as a fresh draw, not as a cache-buster.

Fix shape

1. Let the reviewer read what the prompt says it can: add read-only tools to the allowlist — Read, Grep, Glob, Bash(git diff:*), Bash(git log:*), Bash(git show:*). Fewer denials, better reviews (parity checks actually read the twin), fewer give-ups. Nothing that writes.
2. The review job fails when it posted no verdict. A post-step that counts claude[bot] reviews on the PR with submitted_at ≥ this run's start and exits non-zero with the transcript tail when the count is zero. Then a green review job means a verdict exists; a red one carries its own diagnosis; the guard becomes the second line rather than the first. (The guard stays required — it also enforces newest-verdict-wins and the drift arm.)
3. Retain the transcript: upload claude-execution-output.json as a run artifact (7-day retention) so the next swallowed run is diagnosable in one click rather than by inference.
4. Operational rule (brief templates, tonight): a swallowed review is a stochastic give-up, not a stale-head no-op. Re-run once; if it repeats, the PR is likely too large for one review pass — re-run after the allowlist fix lands, or split. Do not push empty commits to "refresh" the head.

Sequencing constraint — this is why it is not laned tonight

The guard's drift arm (#2229, kept on purpose per #3492) hard-fails every PR while dev's claude-review.yml differs from main's — silent repo-wide unreview must scream. So a PR editing this workflow on dev would red the guard on every other open PR until the next dev → main sync. It has to land either (a) on main and dev together, or (b) as the last thing before a release, with the guard requirement lifted for the window per the pre-release exception. Erik's call on which; the change itself is small.

Evidence: run 35391054185 (attempt 5, head 1deff83e, swallowed) vs the fresh-head run on the same PR; #3618's same-head re-run that posted; the workflow source at dev. Cost tonight: ≥7 × ~$2.5 in review runs, ~7 guard cycles, three seats' attention. Credit: the monitoring seat for the pattern and the cost framing.

[erikdarlingdata]

Second independent confirmation, same night, from a different seat and PR: #3642's review was swallowed six times across two heads. Its run logs: num_turns: 18, permission_denials_count: 17, then a success result with no post — on a 37-file diff. #3618's swallowed attempt was 39 turns, 21 denials. The pattern is now three PRs, ~thirteen paid runs, and the ratio is the tell: denials ≈ turns. The reviewer spends essentially every turn asking to open a file in the tree the prompt told it was checked out, is refused, and ends without executing the verdict protocol. Small diffs get lucky trajectories (a review from gh pr diff alone); a 37-file diff does not. The fix shape stands: read-only tools in the allowlist, and the job fails when it posted nothing. Sequencing still Erik's (drift arm). Operational cost tonight: one PR (#3642) is being drafted to clear the board rather than merged on its earlier clean LGTM — a paid review that could not complete is now costing a merge.

[erikdarlingdata]

Status, so the open state reads on its face: open by design, sequencing-blocked — the fix edits .github/workflows/claude-review.yml, and the review guard's drift arm (#2229, kept by #3492) fails every PR while dev's copy differs from main's. It lands either with the next dev → main release or on main and dev together; the maintainer's sequencing call is pending. Three confirmations tonight (#3618, #3642 ×6, #3646), fix shape unchanged; brief templates already carry the interim rule (one re-run, then stop).

[erikdarlingdata]

Fix built and live-verified; landing paused on a main protection the sequencing plan did not see.

Two PRs carry the same commit (c39de4c8, one file, byte-identical): #3656 → main (lands first, so dev's twin never drifts) and #3657 → dev (closes this). Read-only tools (Read, Grep, Glob, git diff|log|show) join the allowlist; a post-step fails the job when claude[bot] submitted no non-empty-bodied review since the run's start stamp (the empty-bodied inline-comment carriers are excluded — #3647 has two); the transcript is uploaded as claude-review-transcript (7 days). Neither PR has auto-merge armed.

The post-step already fired for real on both PRs (runs 35402780264, 35402811321): the action's own annotation reads "Skipping action due to workflow validation: the workflow file must have identical content to the version on the default branch" — cause 1, live — and the new step reds the (non-required) review job with "Claude never ran", while the guard reports "Expected: this PR edits the workflow." Exactly the shape designed.

Blocker: main carries an active ruleset ("Protect main", id 12793430, no bypass actors) requiring the check-branches status, and check-pr-branch.yml fails it by design for any main PR whose head is not dev. The classic branch-protection API — the surface the plan verified (build + Darling PostgreSQL tests, admins not enforced) — does not show ruleset-sourced requirements; gh api repos/…/rules/branches/main does. #3656 is mergeStateStatus: BLOCKED with build and Darling PostgreSQL tests green. Per the plan: stopped, #3657 is not merged alone (that would open the repo-wide refusal window with no way to close it before a release).

Maintainer's call, two shapes: (a) relax the ruleset for the minutes the landing takes — e.g. gh api -X PUT repos/erikdarlingdata/PerformanceMonitor/rulesets/12793430 -F enforcement=disabled, merge #3656, verify main == fix/3650-lane for the file, merge #3657 the moment its checks are green, -F enforcement=active — or (b) the release train: #3657 lands last before the next dev → main sync under the pre-release guard exception, #3656 closes. Both PRs are safe to leave open meanwhile: nothing on dev or main has changed.

[erikdarlingdata]

Closed by the merge of #3657 (its body names this issue with a closing keyword; keywords do not fire against dev, so the watcher closes it).