pr-review: cycle cap counts approvals, ignores human re-requests, and escalation is unrecoverable #467

Summary

The PR-review cascade's MAX_REVIEW_CYCLES circuit breaker (default 3) escalated PR #458 to a human and permanently disengaged, even though the PR was actively converging (agent had approved on cycle 3, CodeRabbit approved, SonarCloud quality gate passed). Investigation found the cap counts the wrong thing and the documented recovery path does not work.

Evidence: #458 (comment)

What happened on #458

After the cycle-3 approval the PR kept evolving legitimately (dev-lead addressing CodeRabbit/Codex findings; blocked on dependency #463). When the cascade next ran, it counted 3 lifetime markers and escalated with "without converging on an approval-and-merge state" — factually wrong; cycle 3 did converge to an approval.

Defects

1. Lifetime counter; approvals count against the cap

scripts/review-one-pr.sh (cycle count): REVIEW_CYCLE is a grep -c of all <!-- pr-review-agent v1 sha=... --> markers ever posted (including approvals, and markers preserved inside superseded <details> blocks). It never resets on progress. The cap's purpose is to break fix-request ping-pong loops, but it fires on any PR that has simply been reviewed 3 times — including PRs that were approved and then legitimately evolved.

2. Explicit human re-request is silently swallowed

On #458 a human posted @donpetry-bot review twice (13:07, 14:11). Both times the bot acked "I'm on it — starting a fresh review now" — and no review ever appeared. FORCE_REVIEW=true (the mention path) only bypasses the already-reviewed-at-head idempotency check; the escalation-marker no-op and the cycle cap run after it, unconditionally. An explicit human request is precisely the human-in-the-loop the cap exists to obtain; it should bypass and reset the cap.

3. Documented re-engagement path does not work

The escalation comment says: "remove the needs-human-review label and the cascade can be re-engaged on the next push." But the no-op check greps comment bodies for <!-- pr-review-agent escalation --> — the label is never consulted, and no script ever removes or edits the escalation comment. Even if a human deletes the comment manually, REVIEW_CYCLE is still ≥ cap (superseded markers persist), so the next run immediately re-escalates. Once capped, a PR is permanently un-reviewable by the cascade. Every labeled PR that later merged (#453, #358, #357, #302, #153, #3) required a fully manual human merge.

When quitting IS valid

The cap guards a real failure mode: review requests fixes → dev-lead auto-applies → re-review requests more fixes → repeat. Two automated agents can oscillate indefinitely, burning tokens/API quota. A circuit breaker on consecutive non-converging fix-request cycles is correct and should stay.

Proposed fixes

1. Count consecutive fix-requested cycles since the last approval (markers carry decision= — count only fix-requested markers newer than the newest approved marker), instead of lifetime markers.
2. Mention-triggered runs (FORCE_REVIEW=true) bypass the escalation no-op and the cap, treating the human request as a cap reset.
3. Make re-engagement real: gate the escalation no-op on the marker and the needs-human-review label still being present, and count only markers newer than the latest escalation comment — so removing the label re-engages the cascade exactly as the escalation message promises.