home/.claude/contrib/prompts/claude-review.md drove 70 review rounds on a single PR (evansenter/agent-event-bus#135) in under 24 hours. CI was green the entire time. The PR merged in the end with findings still open, because the loop had no way to end on its own.
This is a prompt-design problem, not a reviewer-behavior problem — the reviewer followed the prompt correctly every round.
Evidence
- ~70 rounds, each producing 1–5 findings, each answered with a fix + push, each push triggering the next round.
- Two reviews of the same commit (
f5a37e1) disagreed: one APPROVE ("I found no correctness defects"), one REQUEST_CHANGES with 2 Important — submitted minutes apart.
- A substantial share of Important findings were regressions introduced by the previous round's fix. The singleton lock was reworked across four consecutive rounds; IPv6 host normalization across two.
- Late rounds were blocking on: a missing explanatory comment, two 400 responses sharing an error string, and a correct-but-untested code path.
Root causes in the prompt
1. The test-coverage rule is a ratchet (§4, §5, §7).
"New public functions/methods without tests → Important" and "New code paths (branches, error handling) without tests → Important", combined with §7's HARD CONSTRAINT (any Important ⇒ REQUEST_CHANGES). Every round's fix adds code — which is new, by definition untested-at-review-time surface for the next round. The rule generates its own next finding indefinitely.
2. §6 sets "always find something" as the expectation.
"REQUEST_CHANGES is the normal outcome for thorough reviews" and "Report them freely" have no counterweight for diminishing returns. There is no instruction that a converged PR is a success state.
3. The "Previously Addressed" filter cannot converge (§3).
It matches findings semantically against prior rounds, which stops literal repeats — but a new finding on newly added code never matches anything, so the filter has no effect on the actual growth mechanism.
4. "Violation of project conventions (check CLAUDE.md)" promotes nits to Important (§5).
In a repo whose CLAUDE.md encodes comment/docstring conventions (agent-event-bus does), a missing comment becomes a convention violation ⇒ Important ⇒ REQUEST_CHANGES. Several late rounds blocked on comment wording.
5. No maturity/scope calibration.
The PR was an explicitly experimental prototype behind an RFC. It was reviewed at production-hardening depth: DNS rebinding, symlinks planted in shared temp dirs, split-brain across launch contexts, HTTP Host rewriting by reverse proxies. All real, none proportionate.
6. Verdict is non-deterministic, so there is no fixed point.
Given #5 above, whether a given commit gets APPROVE or REQUEST_CHANGES is partly a coin flip. A loop that terminates on APPROVE cannot reliably terminate.
Suggested changes
- Add a convergence rule. Pass the round count into the prompt. After round N (3? 5?), only findings with a concrete failure scenario block; everything else is posted as non-blocking. State explicitly that a PR reaching a polish-only steady state should be approved.
- Require Important to carry a failure scenario — concrete inputs/state → wrong output. "Missing comment", "reused error string", "named behavior without a test" are Suggestions by construction.
- Narrow the missing-tests rule to untested new user-facing behavior, not every new branch or internal helper.
- Discount self-inflicted surface. A finding against code that exists only because of a prior round's fix should be a Suggestion unless it is a genuine regression.
- Let the PR declare maturity. Honor a marker in the PR body (
experimental / prototype / RFC link) and calibrate hardening depth to it.
- Consider not re-reviewing on every push, or rate-limiting rounds per PR — the current setup makes the answer to a review the trigger for the next one.
The narrowest single change that would have stopped this: §7's HARD CONSTRAINT should not be reachable from a missing test, a missing comment, or a style/convention nit.
home/.claude/contrib/prompts/claude-review.mddrove 70 review rounds on a single PR (evansenter/agent-event-bus#135) in under 24 hours. CI was green the entire time. The PR merged in the end with findings still open, because the loop had no way to end on its own.This is a prompt-design problem, not a reviewer-behavior problem — the reviewer followed the prompt correctly every round.
Evidence
f5a37e1) disagreed: oneAPPROVE("I found no correctness defects"), oneREQUEST_CHANGESwith 2 Important — submitted minutes apart.Root causes in the prompt
1. The test-coverage rule is a ratchet (§4, §5, §7).
"New public functions/methods without tests → Important" and "New code paths (branches, error handling) without tests → Important", combined with §7's HARD CONSTRAINT (any Important ⇒
REQUEST_CHANGES). Every round's fix adds code — which is new, by definition untested-at-review-time surface for the next round. The rule generates its own next finding indefinitely.2. §6 sets "always find something" as the expectation.
"REQUEST_CHANGES is the normal outcome for thorough reviews" and "Report them freely" have no counterweight for diminishing returns. There is no instruction that a converged PR is a success state.
3. The "Previously Addressed" filter cannot converge (§3).
It matches findings semantically against prior rounds, which stops literal repeats — but a new finding on newly added code never matches anything, so the filter has no effect on the actual growth mechanism.
4. "Violation of project conventions (check CLAUDE.md)" promotes nits to Important (§5).
In a repo whose CLAUDE.md encodes comment/docstring conventions (agent-event-bus does), a missing comment becomes a convention violation ⇒ Important ⇒
REQUEST_CHANGES. Several late rounds blocked on comment wording.5. No maturity/scope calibration.
The PR was an explicitly experimental prototype behind an RFC. It was reviewed at production-hardening depth: DNS rebinding, symlinks planted in shared temp dirs, split-brain across launch contexts, HTTP
Hostrewriting by reverse proxies. All real, none proportionate.6. Verdict is non-deterministic, so there is no fixed point.
Given #5 above, whether a given commit gets
APPROVEorREQUEST_CHANGESis partly a coin flip. A loop that terminates onAPPROVEcannot reliably terminate.Suggested changes
experimental/prototype/ RFC link) and calibrate hardening depth to it.The narrowest single change that would have stopped this: §7's HARD CONSTRAINT should not be reachable from a missing test, a missing comment, or a style/convention nit.