v2.8.0 fix(eng): clear a rate-limited CodeRabbit failure, and add an audited operator bypass - #59
Conversation
mc_cr_failure_rate_limited answers one question: is a CodeRabbit commit status of "failure" on HEAD really a rate limit rather than a genuine CR objection? Two independent proofs, either sufficient: the status description says rate limit (case-insensitive substring, so "Review rate limited" and "Rate limit exceeded" both hit), or CR posted its existing "rate limited by coderabbit.ai" marker comment. This is the third rate-limit shape. The two the lib already had key on the marker comment and cover a MISSING status (CR never started) and a stuck PENDING one (CR started, then hit the limit mid-flight). Neither covers what CR does when an INCREMENTAL pass burns the limit: it resolves its per-commit status to failure with a rate-limit description and often posts no marker comment at all. Fails closed in every degraded direction (non-failure state, empty description, unparseable comments), so a genuine CR failure is never mistaken for a rate limit.
…operator bypass The gate reached its rate-limit helpers only for a missing or pending CodeRabbit status, so a failure status blocked unconditionally. On #58 CR had fully reviewed the PR and acked all five findings; its final incremental pass over a test-only commit tripped the limit and posted status=failure "Review rate limited". Every dimension rendered checked, the verdict was NOT CLEAR on that one line, and landing took the documented human workaround: move .merge-clearance.json aside, gh pr merge --admin, restore. That workaround is what this retires. Two paths clear a failure now, and BOTH require a current /eng:cr review on the head, so "never both reviewers down" still holds: - rate-limited failure: machine-detectable via mc_cr_failure_rate_limited (lazy status-description fetch, only on the failure branch, so the happy path makes no extra API call). Auto-satisfies with the backstop, no flag, exactly like the missing and stuck-pending shapes. - genuine failure: the new --override-cr-failure flag. Human judgment, so it is never inferred. Recorded in the checklist line, the JSON verdict, the stamp evidence and the posted status description. Also fixes the checklist mark: cr_mark ignored both the failure status and the reviewed-head blocker, so a PR blocked solely by either rendered CodeRabbit as a green tick while the verdict said NOT CLEAR. The one dimension actually blocking was the one shown as passing. pr-watcher's Step 4b/4h now name all three shapes in a table and tell the dispatcher to read the status description itself (the sensor does not carry it), so watcher and gate reach the same conclusion. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sion a tested unit Review (Codex plus the silent-failure and comment lenses, all three independently) found the shape-3 marker proof reused the LOOSE mc_cr_rate_limited, which matches the marker anywhere in a PR's history. So: CR is rate-limited on an early commit and posts its notice, later recovers, reviews, and fails HEAD for real. The gate read the stale marker, auto-cleared the genuine failure with no operator flag, and labelled the audit trail "rate-limited". That is the exact trap mc_cr_rate_limited_latest already exists to close for the stuck-pending shape; shape 3 now gets the same discipline. Reproduced before the fix, and pinned by a regression test. Also from the review: - the description classifier matched a bare "rate limit" substring, so "not a rate limit issue" qualified while the hyphenated "Rate-limited" did not. Now a positive rate[ -]?limit match with a negation guard. The residual (a trailing negation) is documented, not silently accepted. - the whole CR-failure decision moves into a pure mc_cr_failure_disposition in the lib, with its truth table as bats cases. It was the one line standing between --override-cr-failure and a bare merge bypass, and it was verifiable only by hand. It also fixes attribution: the rate-limit check runs first, so passing the flag defensively on a rate-limited failure records "rate-limited", not a human override that never happened, and a later grep for real overrides stays clean. - the checklist mark now reads a CR_BLOCKED flag set beside each blocker instead of re-deriving the same conditions in a second dialect. That mirroring is what produced #58; reading the blockers makes "green iff nothing is blocking" true by construction rather than by review. - STATUS_DESC accumulates ordered notes and truncates at GitHub's ~140 chars instead of last-wins overwrites. A docs-only PR cleared through a rate-limited CR failure posted only the bookkeeping note, dropping the security-relevant reason from the most durable audit surface while the local stamp still recorded it. - the marker proof is now genuinely lazy (description first, comments only if inconclusive), the flag warns instead of silently no-opping on a non-failure status, and several comments that the new behavior had made false are corrected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
Comment |
Production QA: verifiedDeployed and verified on the laptop (this repo's "production" is the installed plugin cache, which is what the hooks,
The posted clearance status records the path taken, which was the point: This PR landed through the exact mechanism it adds. CodeRabbit rate-limited itself here (status One cosmetic nit for a follow-up: the status description shows the backstop note twice, once for the failure-status dimension and once for reviewed-head. Both are true and it is not wrong, just redundant.
|
1 participant
Summary
The merge-clearance gate reached its CodeRabbit rate-limit escape hatches only for a
missingor stuck-pendingCR commit status. A status offailureblocked unconditionally, with no escape at all.That is what wedged #58. CodeRabbit had reviewed the PR fully across two rounds and posted "confirmed" acks on all five findings; its final incremental pass over a test-only, already-acked commit hit the rate limit and resolved its status to
failurewith the description "Review rate limited". CI was green, the local/eng:crstamp covered the head, the QA checklist was complete, and the land-deploy sentinel was valid. Every dimension rendered as checked and the verdict was still NOT CLEAR, on that one line. Landing it took the documented human workaround: move.merge-clearance.jsonaside,gh pr merge --admin, restore. This PR retires that workaround.Two paths now clear a CR failure, and both require a current
/eng:crreview on the head, so "never both reviewers down" still holds:missingand stuck-pendingshapes already get.--override-cr-failureflag. Human judgment, so it is never inferred, and it is recorded in the checklist, the--jsonverdict, the stamp evidence and the posted GitHub status description.What is in here
mc_cr_failure_rate_limited(pure, in the lib): is afailurestatus really a rate limit? Proven by the status description (rate[ -]?limit, case-insensitive, with a negation guard) or by CR's marker comment being its latest comment.mc_cr_failure_disposition(pure, in the lib): the entire CR-failure decision as one function with a bats truth table. This is the line standing between the override flag and a bare merge bypass, so it does not live as&&chains in the I/O script where it could only be hand-verified.--override-cr-failure, shaped like the existing--skip-review/--skip-qa, but never a bare bypass.CR_BLOCKEDflag set beside each blocker instead of re-deriving the same conditions (the mirroring is what made v2.7.0 fix(eng): pr-watcher v4 senses via a deterministic script, not a subagent #58 render CodeRabbit as a green tick while the verdict blocked), andSTATUS_DESCaccumulates ordered notes instead of last-wins overwrites (the bookkeeping note used to erase the CR-failure reason from the posted status while the local stamp still recorded it).pr-watcherSKILL.md: the three rate-limit shapes as a table, with the marker-strictness asymmetry called out, so watcher and gate tell one story.Review notes
/eng:crran at Major/risky tier (access-control escalation: this code decides what may merge). Six pr-review-toolkit lenses plus a Codex cross-model pass, then a Codex re-review of the fixes.Two blockers were found in my first cut and fixed in
f234305:mc_cr_rate_limited_latestalready existed to close for the stuck-pending shape.rate limitsubstring meantnot a rate limit issuequalified whileRate-limiteddid not (that second direction failing silently, telling the operator a rate-limited failure was genuine).Known residual, documented in the code: the negation guard is prefix-only, so a trailing negation (
rate limit was not the cause) would still classify as a rate limit. Bounded, since the failure still only degrades to "requires a current/eng:crreview".Tests
142/142 in
merge-clearance-lib.bats(16 new), pluspr-merge-gate,ship-watch-nudge-libandland-deploy-sentinelgreen. shellcheck clean,claude plugin validate --strictpasses. Full suites run in CI (this machine cannot run them).Beyond the unit tests, every end-to-end path was driven through the real script against a stubbed
gh: happy path, all three rate-limit shapes backstopped and unbackstopped, genuine failure with and without the flag, the flag without a backstop, the flag on a rate-limited failure (must record as rate-limited, not as an override), a negated description, and the flag on a non-failure status.QA
📄 Plan view: https://claude.ai/code/artifact/7fcf76a8-d2f1-4d73-bb23-6de78c716ace
QA driver: Claude (the building agent (this session)) - the whole surface is a local shell gate plus bats; this session can drive every check now and record evidence.
Standard (all green): unit tests · lint/types · CI ·
/eng:cr🖥️ Development
Before merge, we prove the gate on fake CodeRabbit inputs (unit tests) and on a real PR, so we know it lets a rate-limited-but-locally-reviewed PR through, still stops a genuinely broken one, and shouts about it either way.
mc_cr_failure_rate_limitedcases inmerge-clearance-lib.batsbats eng/hooks/tests/merge-clearance-lib.batsmerge-clearance-lib.batsfile for regressionsmc_cr_rate_limited/mc_cr_rate_limited_latestmerge-clearance.sh check --pr <this PR> --jsonagainst a real PR with a green CR statuscr_status_descriptionmust not fire when CR is not failingstate_of/ status description in a harness copy) withreview-skill-head== HEADreview-skill-headstale or absent/eng:crcr_markrendering fix--override-cr-failureand a current/eng:crstamp--override-cr-failurebut a stale/missing stampshellcheckboth changed scripts and re-read the pr-watcher Step 4b/4h wording against the new gate behavior🚀 Production
After merge, "production" for this repo is the copy of the gate that actually runs on this machine (the installed plugin cache), so we confirm the shipped copy is the code we merged and that it gates a live PR correctly.
bin/install, then diff the cache copy of both scripts againstmainmerge-clearance.sh check --pr <this PR>on the merged HEAD/land-and-deploywith the gate green and no marker-moving workaroundgh pr merge --adminand no.merge-clearance.jsonmoved asideProduction artifacts: all three Production rows exercise
~/.claude/plugins/cache/gstack-extensions/eng/<bumped-version>/hooks/scripts/merge-clearance.shand its siblingmerge-clearance-lib.shon this laptop, installed bybin/installfrom the mergedmain. That cache path is what the eng plugin hooks, the/land-and-deployskill and the merge-gate shim resolve and execute; the repo working tree is upstream of it and is never run in anger. Exercised by invoking the cache copy directly with an explicit--prand by watching the reallocal-review/merge-clearancecommit status this PR's own landing posts.Definition of Done:
/eng:cr); CodeRabbit rate-limited on this head, backstopped per the gate's own rule (see note below)where-things-run.jsonbumped if the deploy changed hosts (n/a: no host change)QA posture: Pre-merge -> state
QA_STATUS: dev_verified+EVIDENCE:once every Dev QA box and every Definition-of-Done box is checked. Post-deploy -> stateQA_STATUS: prod_verified+EVIDENCE:once the Prod QA rows are verified live.Dev QA evidence (2026-07-20)
merge-clearance-lib.bats(16 new), pluspr-merge-gate,ship-watch-nudge-lib,land-deploy-sentinelgreen. shellcheck clean,claude plugin validate --strictpasses.gh: green CR (no extra API call on the happy path), all three rate-limit shapes backstopped and unbackstopped, genuine failure with and without the flag, the flag without a backstop, the flag on a rate-limited failure (recorded as rate-limited, not as an override), a negated description, and the flag on a non-failure status. TheSTATUS_DESCaccumulator was exercised separately, including the bookkeeping-plus-failure case that previously dropped the security-relevant note.f234305isfailure/ "Review rate limited", with the marker comment also present, andreviewed-head=no. The gate returnedcoderabbit_failure_disposition: cleared-rate-limitedwith the/eng:crstamp backstopping it. That is the exact v2.7.0 fix(eng): pr-watcher v4 senses via a deterministic script, not a subagent #58 shape this PR exists to handle, reproduced live on the first try rather than in a harness.The one Definition-of-Done box left unchecked is deliberate: CodeRabbit has not reviewed this head at all, so "CodeRabbit addressed" is not yet true. See the note below.
Landed through its own escape hatch, deliberately
CodeRabbit rate-limited itself on this PR, so this change lands via the exact path it adds: a rate-limited CR plus a current
/eng:crreview on the head. That is the designed behavior, not an evasion of it. The rule this PR encodes is "never both reviewers down", and one reviewer is up: a Major-tier local review (six lenses plus two Codex passes) that found and fixed two blockers, including a genuine fail-open in the first cut.Treating this PR as a special case would have meant not believing the invariant while shipping it. The clearance is fully audited: the stamp evidence and the posted commit status both record
failure auto-satisfied: rate-limited, local /eng:cr review backstops.The "CodeRabbit addressed" Definition-of-Done box stays unchecked because CodeRabbit genuinely never reviewed this head. That box records reality; the rate-limit path is what makes an unchecked box landable.