diff --git a/eng/.claude-plugin/plugin.json b/eng/.claude-plugin/plugin.json index a2c5ca4..e7b9f8e 100644 --- a/eng/.claude-plugin/plugin.json +++ b/eng/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "eng", - "version": "2.7.0", + "version": "2.8.0", "description": "Engineer Ernie, the engineering persona. eng:cr is his master code-review skill and the single local review path the ~/dev merge gate keys on: it risk-tiers depth, runs the pr-review-toolkit lenses, and mints the merge-clearance stamp. It routes to cr-teammate (review someone else's PR and post a comment) and to address-pr-feedback / pr-watcher (respond to review feedback). He also spikes the riskiest unknown before building, generates CodeRabbit config, and builds macOS Shortcuts. The plugin also SHIPS Ernie's PR-lifecycle enforcement hooks (hooks/hooks.json): the ship-PR gate (PRs only via /ship), the merge-clearance gate (no merge without the cleared gauntlet AND a /land-and-deploy sentinel, making /land-and-deploy the single CLI merge path), the /ship and /land-and-deploy sentinels, the review stamp recorder, and an after-ship CodeRabbit-watcher nudge (after a genuine /ship opens a PR, points the agent at /eng:pr-watcher, or when CodeRabbit is rate-limited routes to /land-and-deploy if a current /eng:cr review backstops the head, else to /eng:cr and then /land-and-deploy), active in opted-in repos (.ship-gate.json / .merge-clearance.json) under ~/dev. Skills: eng:cr, eng:cr-teammate, eng:address-pr-feedback, eng:pr-watcher, eng:spike, eng:coderabbit-config, eng:shortcut.", "author": { "name": "Mujtaba Badat", diff --git a/eng/hooks/scripts/merge-clearance-lib.sh b/eng/hooks/scripts/merge-clearance-lib.sh index 93a5b97..b7c1116 100755 --- a/eng/hooks/scripts/merge-clearance-lib.sh +++ b/eng/hooks/scripts/merge-clearance-lib.sh @@ -393,6 +393,135 @@ mc_cr_rate_limited_latest() { echo "no"; return 1 } +# mc_cr_failure_rate_limited [comments_json] +# Decide whether a CodeRabbit commit status of "failure" on HEAD is really a RATE +# LIMIT rather than a genuine CodeRabbit objection or CR-side error. This is the +# third rate-limit shape, and the one the two functions above miss: +# 1. status MISSING + marker comment -> mc_cr_rate_limited +# 2. status PENDING (stuck) + marker as CR's LATEST comment +# -> mc_cr_rate_limited_latest +# 3. status FAILURE, description "Review rate limited" -> THIS function +# Shape 3 is what CodeRabbit posts when it burns its limit on an INCREMENTAL pass: +# it has already reviewed the PR (often posting acks on every finding), then the +# final pass over a trailing commit trips the limit and CR resolves its per-commit +# status to failure with a rate-limit description. In that flavour CR posts NO +# marker comment at all, so keying only on "rate limited by coderabbit.ai" (as the +# two functions above do) leaves the PR wedged behind a hard failure blocker. The +# marker is therefore the SECONDARY proof here, for the residual case where CR +# posts both; the description is the primary one. +# +# cr_status_state: the already-folded CR commit-status state on HEAD +# (state_of "CodeRabbit"): success | failure | pending | missing. Only "failure" +# can be a shape-3 rate limit; every other state echoes "no" (the caller handles +# missing / pending through the two functions above). +# cr_status_description: the description string on that CodeRabbit status +# (e.g. "Review rate limited"). Matched case-insensitively against +# "rate[ -]?limit", so "Review rate limited", "Rate limit exceeded", +# "Rate-limited" and "ratelimit" all hit. A plain substring test missed the +# hyphenated spelling, and that miss would be SILENT: the operator would be +# told the failure is genuine when it is not. +# A NEGATED phrasing ("not rate limited", "no rate limit hit") is explicitly +# excluded, so a description that mentions rate limiting only to deny it +# cannot buy the escape hatch. This is a prefix-negation guard, NOT a semantic +# parser: a trailing negation ("rate limit was not the cause") would still +# classify as a rate limit. Accepted, and bounded: even then the failure only +# degrades from a hard block to "requires a current local /eng:cr review", +# which is the same deal the other two shapes get. Widen the guard if CR's +# wording ever makes that theoretical case real. +# comments_json (optional): the PR's issue comments, SAME shape as +# mc_cr_rate_limited. Checked as a SECOND, independent proof, via the STRICT +# mc_cr_rate_limited_latest: the marker must be CR's LATEST comment. The loose +# mc_cr_rate_limited would match a marker anywhere in the PR's history, so a +# stale rate-limit notice from an early commit would let a LATER, genuine CR +# failure auto-clear while the audit trail called it a rate limit. That is the +# same stale-evidence trap mc_cr_rate_limited_latest already exists to close +# for the stuck-pending shape; shape 3 gets the same discipline. +# Defaults to [] so a description-only caller works. +# +# Echoes "yes" iff the state is "failure" AND (the description says rate limit, +# non-negated, OR the marker is CR's latest comment); else "no". Return code +# mirrors the verdict. Fails CLOSED in every degraded direction: a non-failure +# state, an unreadable description, and an unparseable comments array all yield +# "no", so a genuine CR failure is never mistaken for a rate limit. Like the other +# two shapes this only ever RELAXES the gate in combination with a current local +# /eng:cr review; mc_cr_failure_disposition owns that interlock. +mc_cr_failure_rate_limited() { + local state="$1" desc="${2:-}" comments="${3:-[]}" + [ "$state" = "failure" ] || { echo "no"; return 1; } + # Positive match, minus negations. grep -i keeps the case-insensitivity without + # a tr round-trip, and -E gives the optional space/hyphen between the words. + if printf '%s' "$desc" | grep -qiE 'rate[ -]?limit' \ + && ! printf '%s' "$desc" | grep -qiE '(not|no|never|isn.t|wasn.t)[^.]{0,20}rate[ -]?limit'; then + echo "yes"; return 0 + fi + # Second proof: the marker as CR's LATEST comment. mc_cr_rate_limited_latest + # returns rc 1 on "no", but it is read here as a string in $(...), so only its + # stdout token is authoritative. + if [ "$(mc_cr_rate_limited_latest "$comments")" = "yes" ]; then echo "yes"; return 0; fi + echo "no"; return 1 +} + +# mc_cr_failure_disposition +# The gate's whole decision about a CodeRabbit FAILURE status on HEAD, as one +# pure function. This lives here rather than as shell control flow in +# merge-clearance.sh because it is the single most security-relevant decision the +# gate makes: it is what stands between "--override-cr-failure" and a bare merge +# bypass. As inline `&&` chains in the I/O script it could only ever be verified +# by hand; here every row of its truth table is a bats case. +# +# Arguments are all already-computed scalars (no network, no git): +# cr_status_state: success | failure | pending | missing (state_of "CodeRabbit") +# failure_rate_limited: yes | no (mc_cr_failure_rate_limited) +# override_flag: 1 | 0 (the --override-cr-failure flag) +# review_state: current | stale | missing | n/a (the /eng:cr stamp vs HEAD) +# +# Echoes exactly one disposition token; rc 0 for the non-blocking ones, 1 for the +# blocking ones so callers can branch on either: +# n/a - not a failure status; nothing to decide (rc 0) +# override-inert - flag passed on a non-failure status; it does +# nothing, and the caller should SAY so (rc 0) +# cleared-rate-limited - rate limit + current local review (rc 0) +# cleared-override - genuine failure + explicit flag + current +# local review (rc 0) +# block-rate-limited-unbackstopped- rate limit but no current local review (rc 1) +# block-override-needs-review - flag passed but no current local review (rc 1) +# block-genuine - genuine CR failure, no flag (rc 1) +# +# The invariants this function EXISTS to enforce, none of which may be relaxed: +# 1. NOTHING clears a failure without review_state == "current". Not the rate +# limit, not the operator flag. "Never both reviewers down." +# 2. review_state is read as given. The caller must NOT fold --skip-review or +# the bookkeeping fast lane into it: those waive the review DIMENSION, they +# do not conjure a review that can backstop a broken CodeRabbit. +# 3. The rate-limit path is checked BEFORE the override path, so a failure the +# machine can classify never gets ATTRIBUTED to human judgment. Passing the +# flag defensively on a rate-limited failure yields cleared-rate-limited, so +# a later grep for real operator overrides stays free of false positives. +# 4. The default is to block. Any state that is not explicitly cleared above +# falls through to block-genuine. +mc_cr_failure_disposition() { + local state="$1" rate_limited="${2:-no}" override="${3:-0}" review="${4:-}" + + if [ "$state" != "failure" ]; then + [ "$override" = "1" ] && { echo "override-inert"; return 0; } + echo "n/a"; return 0 + fi + + # Invariant 3: machine-detectable rate limit wins over the human flag. + if [ "$rate_limited" = "yes" ]; then + [ "$review" = "current" ] && { echo "cleared-rate-limited"; return 0; } + echo "block-rate-limited-unbackstopped"; return 1 + fi + + if [ "$override" = "1" ]; then + [ "$review" = "current" ] && { echo "cleared-override"; return 0; } + echo "block-override-needs-review"; return 1 + fi + + # Invariant 4: default deny. + echo "block-genuine"; return 1 +} + # mc_head_cr_unreviewable # Decide whether the INCREMENTAL change at a PR HEAD is something CodeRabbit # legitimately cannot (or will not) review. This is the ONLY condition under diff --git a/eng/hooks/scripts/merge-clearance.sh b/eng/hooks/scripts/merge-clearance.sh index 96c4bc2..3f1200f 100755 --- a/eng/hooks/scripts/merge-clearance.sh +++ b/eng/hooks/scripts/merge-clearance.sh @@ -6,11 +6,17 @@ # "Clear" means, on the PR's CURRENT head commit: # - CI required checks are green # - CodeRabbit has finished reviewing and left nothing unresolved -# (escape hatch: if CR posts its rate-limit notice on a HEAD it never -# finished - whether its commit status is "missing" (never started) or stuck -# "pending" (started, then hit the limit mid-flight) - a CURRENT local -# /eng:cr review backstops it: the "CR reviewed HEAD" / in-progress dimension -# auto-satisfies, fully audited. Never both reviewers down.) +# (escape hatch: if CR signals a rate limit on this HEAD - whether its commit +# status is "missing" (never started), stuck "pending" (started, then hit the +# limit mid-flight), or resolved to "failure" with a rate-limit description +# (an incremental pass burned the limit, often AFTER CR had already reviewed +# HEAD) - a CURRENT local /eng:cr review backstops it. The dimension that +# auto-satisfies differs by shape: reviewed-head for missing/pending, the +# failure status itself for the failure shape. Fully audited either way. +# Never both reviewers down. +# A GENUINE CR failure has no such auto-satisfy; it takes the explicit +# --override-cr-failure operator flag, which ALSO requires the current local +# review and is recorded just as loudly.) # - a local /review was recorded (gstack review-skill stamp) [soft, --skip-review] # - the PR body's QA checklist has no unchecked boxes [soft, --skip-qa] # @@ -67,6 +73,16 @@ usage: merge-clearance [options] --ttl SECONDS clearance stamp TTL for 'clear' (default: $DEFAULT_TTL) --skip-review do not require a recorded local /review --skip-qa do not require the PR body QA checklist to be complete + --override-cr-failure + operator override for a GENUINE CodeRabbit failure status on + this head. Still requires a CURRENT local /eng:cr review + (never a bare bypass) and is recorded in the checklist, the + --json verdict, the stamp evidence and the posted status + description. Without that current review the flag does not + clear: it swaps the generic failure blocker for one telling + you to run /eng:cr first. A rate-limited failure needs no + flag (it auto-satisfies on the same backstop, and is recorded + as rate-limited rather than as an override). --json also emit a machine-readable JSON verdict (check only) enable (repo-level - opt a repo into the gate in one command): @@ -79,7 +95,7 @@ EOF esac PR=""; REPO=""; TTL="$DEFAULT_TTL"; SKIP_REVIEW=0; SKIP_QA=0; WANT_JSON=0 -BASE_ARG=""; CHECKS_ARG=""; APPLY_PROTECTION=0 +BASE_ARG=""; CHECKS_ARG=""; APPLY_PROTECTION=0; OVERRIDE_CR_FAILURE=0 while [ $# -gt 0 ]; do case "$1" in --pr) PR="${2:-}"; shift 2 ;; @@ -90,6 +106,7 @@ while [ $# -gt 0 ]; do --apply-protection) APPLY_PROTECTION=1; shift ;; --skip-review) SKIP_REVIEW=1; shift ;; --skip-qa) SKIP_QA=1; shift ;; + --override-cr-failure) OVERRIDE_CR_FAILURE=1; shift ;; --json) WANT_JSON=1; shift ;; *) die "unknown option: $1" ;; esac @@ -313,9 +330,11 @@ STATUSES=$(gh api "repos/$REPO/commits/$HEAD/statuses" \ | to_entries | map({name:.key, state:.value})' || echo '[]') CHECKMAP=$(jq -cn --argjson a "$CHECKRUNS" --argjson b "$STATUSES" '$a + $b') -# state_of -> success|failure|pending|missing (success conclusions -# from check-runs are "success"; a completed status is "success"; everything not -# yet conclusive is "pending"; absent is "missing"). +# state_of -> success|failure|pending|missing. A commit status whose +# state is failure/error, or a check-run whose conclusion is timed_out / cancelled / +# action_required, all fold to "failure" (the fold the CodeRabbit failure branch +# below is built on); any other completed state is "success"; everything not yet +# conclusive is "pending"; absent is "missing". state_of() { printf '%s' "$CHECKMAP" | jq -r --arg n "$1" ' [ .[] | select(.name==$n) | .state ] as $s @@ -365,6 +384,27 @@ cr_issue_comments() { [ -n "$out" ] && [ "$out" != "null" ] && printf '%s' "$out" || echo '[]' } +# cr_status_description -> the description string on CodeRabbit's NEWEST commit +# status for HEAD (e.g. "Review rate limited"). The CHECKMAP above deliberately +# carries only name+state, because the description is needed for exactly one +# question: is a "failure" state really a rate limit? So this refetches lazily and +# is called ONLY from the failure branch below (mirroring head_changed_files / +# cr_issue_comments - the happy path pays for no extra API call). The statuses +# endpoint returns newest-first, so `first` is the newest CodeRabbit COMMIT STATUS, +# which is what produces the folded state today (CR publishes a commit status, not a +# check-run). Note the asymmetry: state_of folds check-runs AND statuses, while this +# reads statuses only, so if CR ever also emitted a check-run named "CodeRabbit" the +# description could go empty while the state stayed "failure". That direction fails +# CLOSED (the failure keeps blocking), it just makes the hatch stop firing. Echoes "" on any failure (fail closed: an unreadable description feeds +# mc_cr_failure_rate_limited, which then reports "not a rate limit" and the failure +# keeps blocking - the safe direction). +cr_status_description() { + local out + out=$(gh api "repos/$REPO/commits/$HEAD/statuses" \ + -q 'first(.[] | select(.context=="CodeRabbit") | (.description // ""))' 2>/dev/null) || out="" + printf '%s' "$out" +} + # ---- evaluate each dimension ----------------------------------------------- # 1) CI required checks @@ -399,6 +439,7 @@ CR_INPROGRESS=0 # (unresolved threads / changes-requested) still blocks regardless. CR_HEAD_UNREVIEWABLE=no CR_RATE_LIMITED=no +CR_FAILURE_RATE_LIMITED=no if [ "$CR_STATUS_STATE" != "pending" ] && [ "$CR_REVIEWED_HEAD" = "no" ] && [ "$CR_STATUS_STATE" = "missing" ]; then CR_HEAD_UNREVIEWABLE=$(mc_head_cr_unreviewable "$(head_changed_files "$HEAD")" "$CR_UNREVIEWABLE_GLOBS") # Second auto-satisfy path for the same "CR silent on HEAD" gap: CodeRabbit @@ -426,6 +467,25 @@ elif [ "$CR_STATUS_STATE" = "pending" ] && [ "$CR_REVIEWED_HEAD" = "no" ]; then # is deliberately NOT offered here: a pending status means CR did start, so there # was reviewable content - silence is "couldn't finish", never "nothing to review". CR_RATE_LIMITED=$(mc_cr_rate_limited_latest "$(cr_issue_comments)") +elif [ "$CR_STATUS_STATE" = "failure" ]; then + # Third rate-limit shape: CR RESOLVED its per-commit status to failure with a + # rate-limit description ("Review rate limited"). This is what an incremental pass + # that burns the limit looks like - CR may have fully reviewed the PR already + # (reviewed-head can be "yes" here, unlike the two branches above), then tripped + # the limit on a trailing commit. In this flavour CR often posts NO marker comment, + # so the description is the primary signal and the marker the secondary one; both + # live in mc_cr_failure_rate_limited. Deliberately NOT conditioned on + # CR_REVIEWED_HEAD: the failure status blocks on its own (see the verdict section), + # independently of whether CR reviewed HEAD. Like the other two shapes this only + # RELAXES the gate together with a current local /eng:cr review (CR_RL_BACKSTOPPED). + # Two-step so the marker proof stays LAZY: the description alone settles the + # common case, and only an inconclusive description pays for the paginated + # comments fetch. (Passing both as arguments would expand each $(...) up front, + # so the comments call would always run.) + CR_FAILURE_RATE_LIMITED=$(mc_cr_failure_rate_limited "$CR_STATUS_STATE" "$(cr_status_description)" '[]') + [ "$CR_FAILURE_RATE_LIMITED" = "yes" ] \ + || CR_FAILURE_RATE_LIMITED=$(mc_cr_failure_rate_limited "$CR_STATUS_STATE" "" "$(cr_issue_comments)") + CR_RATE_LIMITED="$CR_FAILURE_RATE_LIMITED" fi # 3) local /review stamp (best-effort; keyed to the local checkout's git dir) @@ -445,19 +505,57 @@ fi QA_STATE=$(mc_qa_state "$BODY" "$REQUIRE_QA_PLAN") # ---- verdict ---------------------------------------------------------------- -# Hard blockers: CI not green, CR not clear, CR mid-review, CR hasn't seen HEAD. +# Hard blockers: CI not green, CR not clear, CR mid-review, CR hasn't seen HEAD, +# CR status failure (unless rate-limited + backstopped, or operator-overridden). # Soft blockers (block unless --skip-*): /review missing-or-stale, QA incomplete. -# CR rate-limit escape hatch (gated): when CodeRabbit signalled rate-limit on a -# HEAD it never reviewed, fall back to the local /eng:cr review IFF that review is -# current for this HEAD. Never lose both reviewers - CR down + no local review +# CR rate-limit escape hatch (gated): when CodeRabbit signalled rate-limit on this +# HEAD (never started, stuck mid-flight, or a failure-status limit on an incremental +# pass - the last of which can happen on a HEAD CR DID review), fall back to the +# local /eng:cr review IFF that review is current for this HEAD. Never lose both reviewers - CR down + no local review # still blocks. Computed here because it needs REVIEW_STATE (section 3). CR_RL_BACKSTOPPED=no [ "$CR_RATE_LIMITED" = "yes" ] && [ "$REVIEW_STATE" = "current" ] && CR_RL_BACKSTOPPED=yes +# Operator override for a GENUINE CodeRabbit failure (one with no rate-limit +# evidence). This is the human-judgment counterpart to the machine-detectable +# rate-limit auto-satisfy above, and it exists so a legitimate override never +# requires moving the repo's .merge-clearance.json marker aside or reaching for +# `gh pr merge --admin`. Two invariants keep it honest: +# - it is NEVER the default: nothing sets OVERRIDE_CR_FAILURE but the explicit +# --override-cr-failure flag (the bookkeeping fast lane does not touch it); +# - it still requires a CURRENT local /eng:cr review on this exact head, so the +# "never both reviewers down" rule holds here too. --skip-review does NOT +# satisfy it: REVIEW_STATE is read independently of that hatch. +# It is recorded in the checklist, the JSON verdict, the stamp evidence and the +# posted GitHub status description, so an override is always greppable after the fact. +# +# The decision itself lives in mc_cr_failure_disposition (merge-clearance-lib.sh), +# not in `&&` chains here, because it is the one place a wrong edit turns the flag +# into a bare merge bypass. As a pure function every row of its truth table is a +# bats case; as inline shell it could only ever be verified by hand. +CR_FAILURE_DISPOSITION=$(mc_cr_failure_disposition \ + "$CR_STATUS_STATE" "$CR_FAILURE_RATE_LIMITED" "$OVERRIDE_CR_FAILURE" "$REVIEW_STATE") +CR_FAILURE_OVERRIDDEN=no +[ "$CR_FAILURE_DISPOSITION" = "cleared-override" ] && CR_FAILURE_OVERRIDDEN=yes +# Deriving the flag FROM the disposition (rather than recomputing it) is what keeps +# the audit trail honest when both escapes apply at once: the disposition checks the +# rate limit first, so passing the flag defensively on a rate-limited failure records +# "rate-limited", not a human override that was never needed. A later grep for real +# operator overrides then has no false positives. +[ "$CR_FAILURE_DISPOSITION" = "override-inert" ] \ + && err "merge-clearance: --override-cr-failure ignored (CodeRabbit status is ${CR_STATUS_STATE}, not failure)" + BLOCKERS=() +# CR_BLOCKED is set alongside EVERY CodeRabbit-dimension blocker below, and is the +# ONLY input to the CodeRabbit checklist mark. The mark used to re-derive the same +# conditions in a second dialect, which is exactly how #58 happened: the expression +# omitted the failure status, so the one dimension actually blocking rendered as a +# green tick. Reading the blockers instead of mirroring them makes +# "mark is green iff nothing is blocking" true by construction. +CR_BLOCKED=0 [ "$CI_STATE" = "success" ] || BLOCKERS+=("CI is ${CI_STATE} (${CI_DETAIL})") -if [ "$CR_RC" -ne 0 ]; then BLOCKERS+=("CodeRabbit ${CR_VERDICT}"); fi +if [ "$CR_RC" -ne 0 ]; then BLOCKERS+=("CodeRabbit ${CR_VERDICT}"); CR_BLOCKED=1; fi # CodeRabbit in-progress (pending commit status). A genuine pending is "review # running, wait". But a pending that is really CR stuck after posting its rate-limit # notice (CR_RATE_LIMITED, set above only when that notice is CR's latest comment) is @@ -466,17 +564,36 @@ if [ "$CR_RC" -ne 0 ]; then BLOCKERS+=("CodeRabbit ${CR_VERDICT}"); fi # "in progress". if [ "$CR_INPROGRESS" -eq 1 ]; then if [ "$CR_RATE_LIMITED" = "yes" ]; then - [ "$CR_RL_BACKSTOPPED" = "yes" ] || BLOCKERS+=("CodeRabbit is rate-limited (status stuck pending) and no current local review backstops it (run /eng:cr on this head, then land)") + [ "$CR_RL_BACKSTOPPED" = "yes" ] || { BLOCKERS+=("CodeRabbit is rate-limited (status stuck pending) and no current local review backstops it (run /eng:cr on this head, then land)"); CR_BLOCKED=1; } else - BLOCKERS+=("CodeRabbit review in progress on this head") + BLOCKERS+=("CodeRabbit review in progress on this head"); CR_BLOCKED=1 fi fi # A CodeRabbit commit status of "failure" (state_of folds error/timed_out/ -# cancelled/action_required into "failure") is CR actively signaling a problem, -# not silence, so it blocks unconditionally - it is NOT relaxed by the -# CR-unreviewable auto-satisfy below, which only covers the "CR posted nothing" -# (missing) case. The reviewed-head blocker then handles only the missing case. -[ "$CR_STATUS_STATE" != "failure" ] || BLOCKERS+=("CodeRabbit status is failure on this head") +# cancelled/action_required into "failure") is CR actively signaling a problem, not +# silence, so it blocks - it is NOT relaxed by the CR-unreviewable auto-satisfy +# below, which only covers the "CR posted nothing" (missing) case. The reviewed-head +# blocker then handles only the missing case. Two, and only two, things clear a +# failure, and BOTH require a current local /eng:cr review on this head: +# - the failure is really a RATE LIMIT (CR_FAILURE_RATE_LIMITED, detected from the +# status description and/or CR's marker comment). Machine-detectable, so it +# auto-satisfies with the backstop and needs no flag - the same deal the missing +# and stuck-pending rate-limit shapes already get. +# - the operator explicitly passed --override-cr-failure for a GENUINE failure. +# Human judgment, so it is never inferred. +case "$CR_FAILURE_DISPOSITION" in + cleared-rate-limited|cleared-override|n/a|override-inert) ;; + block-rate-limited-unbackstopped) + BLOCKERS+=("CodeRabbit is rate-limited (status failure on this head) and no current local review backstops it (run /eng:cr on this head, then land)") + CR_BLOCKED=1 ;; + block-override-needs-review) + BLOCKERS+=("--override-cr-failure passed but the local /review is ${REVIEW_STATE} for this head; the override still requires a current /eng:cr review (run it, then retry)") + CR_BLOCKED=1 ;; + *) + # Default deny: block-genuine, and any token a future lib change might add. + BLOCKERS+=("CodeRabbit status is failure on this head (genuine CR failure: fix it, or pass --override-cr-failure with a current /eng:cr review)") + CR_BLOCKED=1 ;; +esac if [ "$CR_STATUS_STATE" != "pending" ] && [ "$CR_REVIEWED_HEAD" = "no" ] && [ "$CR_STATUS_STATE" = "missing" ] \ && [ "$CR_HEAD_UNREVIEWABLE" != "yes" ] && [ "$CR_RL_BACKSTOPPED" != "yes" ]; then if [ "$CR_RATE_LIMITED" = "yes" ]; then @@ -486,6 +603,7 @@ if [ "$CR_STATUS_STATE" != "pending" ] && [ "$CR_REVIEWED_HEAD" = "no" ] && [ "$ else BLOCKERS+=("CodeRabbit has not reviewed the current head yet") fi + CR_BLOCKED=1 fi if [ "$SKIP_REVIEW" -eq 0 ] && { [ "$REVIEW_STATE" = "missing" ] || [ "$REVIEW_STATE" = "stale" ]; }; then BLOCKERS+=("local /review ${REVIEW_STATE} for this head (run /review, or pass --skip-review)") @@ -505,7 +623,14 @@ CLEAR=0; [ "${#BLOCKERS[@]}" -eq 0 ] && CLEAR=1 mark() { case "$1" in ok) printf '✅';; warn) printf '⚠️ ';; bad) printf '❌';; esac; } ci_mark=bad; [ "$CI_STATE" = success ] && ci_mark=ok -cr_mark=bad; [ "$CR_RC" -eq 0 ] && { [ "$CR_INPROGRESS" -eq 0 ] || [ "$CR_RL_BACKSTOPPED" = "yes" ]; } && cr_mark=ok +# CodeRabbit mark: green iff nothing in the CodeRabbit family is blocking. It reads +# CR_BLOCKED (set beside every CR blocker above) rather than re-deriving those +# conditions, so the mark cannot disagree with the verdict. An operator override +# renders ⚠️ (warn), matching how --skip-review / --skip-qa surface: cleared, but +# visibly not on the strength of the machine check. +cr_mark=bad +[ "$CR_BLOCKED" -eq 0 ] && cr_mark=ok +[ "$CR_FAILURE_OVERRIDDEN" = "yes" ] && [ "$CR_BLOCKED" -eq 0 ] && cr_mark=warn rev_mark=bad; case "$REVIEW_STATE" in current) rev_mark=ok;; n/a) rev_mark=warn;; esac [ "$SKIP_REVIEW" -eq 1 ] && rev_mark=warn qa_mark=bad; case "$QA_STATE" in complete) qa_mark=ok;; n/a) qa_mark=warn;; esac @@ -519,10 +644,22 @@ qa_mark=bad; case "$QA_STATE" in complete) qa_mark=ok;; n/a) qa_mark=warn;; echo "- [$([ $ci_mark = ok ] && echo x || echo ' ')] **CI** - $(mark $ci_mark) ${CI_DETAIL:-no required checks}" cr_head_note="reviewed-head=${CR_REVIEWED_HEAD}" [ "$CR_HEAD_UNREVIEWABLE" = "yes" ] && cr_head_note="reviewed-head=${CR_REVIEWED_HEAD} (auto-satisfied: HEAD diff is CR-unreviewable, e.g. *.pen)" - [ "$CR_RL_BACKSTOPPED" = "yes" ] && cr_head_note="reviewed-head=${CR_REVIEWED_HEAD} (auto-satisfied: CR rate-limited, current local /eng:cr review backstops)" + # Only claim the reviewed-head dimension was AUTO-SATISFIED when it actually needed + # rescuing. In the failure-status rate-limit shape CR has often already reviewed + # HEAD (reviewed-head=yes) and only the status is rate-limited, so attaching the + # note there would credit the backstop for something CR did on its own. + [ "$CR_RL_BACKSTOPPED" = "yes" ] && [ "$CR_REVIEWED_HEAD" = "no" ] \ + && cr_head_note="reviewed-head=${CR_REVIEWED_HEAD} (auto-satisfied: CR rate-limited, current local /eng:cr review backstops)" cr_verdict_note="verdict=${CR_VERDICT}" [ "$CR_VERDICT" = "unresolved-waived" ] && cr_verdict_note="verdict=${CR_VERDICT} (only OUTDATED CR threads left unresolved; CR status green on HEAD; current threads would still block)" - echo "- [$([ $cr_mark = ok ] && echo x || echo ' ')] **CodeRabbit** - $(mark $cr_mark) ${cr_verdict_note}, status=${CR_STATUS_STATE}, ${cr_head_note}" + # The status cell states WHICH of the two failure escapes applied, so a cleared + # failure is never indistinguishable from a green one in the rendered checklist. + cr_status_note="status=${CR_STATUS_STATE}" + case "$CR_FAILURE_DISPOSITION" in + cleared-rate-limited) cr_status_note="status=failure (auto-satisfied: rate-limited, current local /eng:cr review backstops)" ;; + cleared-override) cr_status_note="status=failure (OPERATOR OVERRIDE --override-cr-failure; current local /eng:cr review backstops)" ;; + esac + echo "- [$([ $cr_mark = ok ] && echo x || echo ' ')] **CodeRabbit** - $(mark $cr_mark) ${cr_verdict_note}, ${cr_status_note}, ${cr_head_note}" echo "- [$([ $rev_mark = ok ] && echo x || echo ' ')] **Local /review** - $(mark $rev_mark) ${REVIEW_STATE}$([ $SKIP_REVIEW = 1 ] && echo ' (skipped)')" echo "- [$([ $qa_mark = ok ] && echo x || echo ' ')] **QA checklist** - $(mark $qa_mark) ${QA_STATE}$([ $SKIP_QA = 1 ] && echo ' (skipped)')" [ "$IS_BOOKKEEPING" = "yes" ] && echo "- ⚡ **Bookkeeping fast-lane** - diff is docs/inventory only; /review + QA auto-waived (CI + CodeRabbit still enforced)" @@ -553,11 +690,15 @@ if [ "$WANT_JSON" -eq 1 ]; then --arg review "$REVIEW_STATE" --arg qa "$QA_STATE" --argjson clear "$CLEAR" \ --arg crhead "$CR_REVIEWED_HEAD" --arg crheadauto "$CR_HEAD_UNREVIEWABLE" \ --arg crratelimited "$CR_RATE_LIMITED" --arg crrlbackstop "$CR_RL_BACKSTOPPED" \ + --arg crfailrl "$CR_FAILURE_RATE_LIMITED" --arg crfailoverride "$CR_FAILURE_OVERRIDDEN" \ + --arg crfaildisp "$CR_FAILURE_DISPOSITION" \ --argjson bk "$([ "$IS_BOOKKEEPING" = "yes" ] && echo true || echo false)" \ --argjson blockers "$(printf '%s\n' "${BLOCKERS[@]:-}" | jq -R . | jq -sc 'map(select(length>0))')" \ '{repo:$repo, pr:$pr, head:$head, base:$base, ci:$ci, coderabbit:$cr, coderabbit_status:$crstatus, coderabbit_reviewed_head:$crhead, coderabbit_head_auto_satisfied:($crheadauto=="yes"), coderabbit_rate_limited:($crratelimited=="yes"), coderabbit_rate_limit_backstopped:($crrlbackstop=="yes"), + coderabbit_failure_rate_limited:($crfailrl=="yes"), coderabbit_failure_overridden:($crfailoverride=="yes"), + coderabbit_failure_disposition:$crfaildisp, review:$review, qa:$qa, bookkeeping_fast_lane:$bk, clear:($clear==1), blockers:$blockers}' fi @@ -581,27 +722,56 @@ case "$TTL" in ''|*[!0-9]*) TTL="$DEFAULT_TTL" ;; esac # status description so the bypass is greppable later, never silent. CR_HEAD_EVIDENCE="reviewed-head=${CR_REVIEWED_HEAD}" [ "$CR_HEAD_UNREVIEWABLE" = "yes" ] && CR_HEAD_EVIDENCE="auto-satisfied: CR-unreviewable-only HEAD" -[ "$CR_RL_BACKSTOPPED" = "yes" ] && CR_HEAD_EVIDENCE="auto-satisfied: CR rate-limited, local review backstops" +[ "$CR_RL_BACKSTOPPED" = "yes" ] && [ "$CR_REVIEWED_HEAD" = "no" ] \ + && CR_HEAD_EVIDENCE="auto-satisfied: CR rate-limited, local review backstops" + +# Same for the CR commit status: when a "failure" was cleared, the evidence records +# WHICH escape did it, so an audit of the stamp never has to guess. +CR_STATUS_EVIDENCE="$CR_STATUS_STATE" +case "$CR_FAILURE_DISPOSITION" in + cleared-rate-limited) CR_STATUS_EVIDENCE="failure auto-satisfied: rate-limited, local /eng:cr review backstops" ;; + cleared-override) CR_STATUS_EVIDENCE="failure OVERRIDDEN by operator --override-cr-failure (local /eng:cr review backstops)" ;; +esac STAMP_JSON=$(jq -nc \ --argjson pr "$PR_NUM" --arg head "$HEAD" --arg base "$BASE" \ --arg iso "$ISO" --argjson epoch "$NOW" --argjson ttl "$TTL" \ --arg ci "$CI_STATE" --arg cr "$CR_VERDICT" --arg review "$REVIEW_STATE" --arg qa "$QA_STATE" \ - --arg crhead "$CR_HEAD_EVIDENCE" \ + --arg crhead "$CR_HEAD_EVIDENCE" --arg crstatus "$CR_STATUS_EVIDENCE" \ + --argjson override "$([ "$CR_FAILURE_OVERRIDDEN" = "yes" ] && echo true || echo false)" \ --argjson bk "$([ "$IS_BOOKKEEPING" = "yes" ] && echo true || echo false)" \ '{pr:$pr, head:$head, base:$base, checked_at:$iso, checked_at_epoch:$epoch, ttl_seconds:$ttl, tool:"land-and-deploy", - evidence:{ci:$ci, coderabbit:$cr, coderabbit_head:$crhead, review:$review, qa:$qa, - bookkeeping_fast_lane:$bk}}') + evidence:{ci:$ci, coderabbit:$cr, coderabbit_head:$crhead, + coderabbit_status:$crstatus, coderabbit_failure_overridden:$override, + review:$review, qa:$qa, bookkeeping_fast_lane:$bk}}') # GitHub commit status - the hard authority the branch ruleset requires. The # description carries the auto-satisfy note when it applied, so the audit trail # is visible on the commit status itself (descriptions cap ~140 chars). +# Built by ACCUMULATING every applicable note, not by a run of last-wins overwrites. +# Overwrites silently dropped information: a docs-only PR whose CR status failed on a +# rate limit posted only the bookkeeping note, so the most security-relevant fact +# (a hard CR failure was auto-satisfied) vanished from the most durable audit +# surface, while the local stamp still recorded it. The two disagreeing is the exact +# failure this file exists to prevent. Order is by importance, and the join is +# truncated to GitHub's ~140-char description cap, so the leading (most important) +# note always survives. +DESC_NOTES=() +[ "$CR_FAILURE_OVERRIDDEN" = "yes" ] && DESC_NOTES+=("OPERATOR OVERRIDE --override-cr-failure on a genuine CR failure, local review backstops") +[ "$CR_FAILURE_DISPOSITION" = "cleared-rate-limited" ] && DESC_NOTES+=("CR status failure = rate limit, local review backstops") +[ "$CR_RL_BACKSTOPPED" = "yes" ] && [ "$CR_REVIEWED_HEAD" = "no" ] && DESC_NOTES+=("CR rate-limited, local review backstops") +[ "$CR_HEAD_UNREVIEWABLE" = "yes" ] && DESC_NOTES+=("CR-head auto-satisfied: unreviewable-only HEAD") +[ "$CR_VERDICT" = "unresolved-waived" ] && DESC_NOTES+=("CR green on HEAD, only OUTDATED CR threads waived") +[ "$IS_BOOKKEEPING" = "yes" ] && DESC_NOTES+=("bookkeeping fast-lane: docs/inventory only, review+QA waived") STATUS_DESC="Cleared by merge-clearance ($ISO)" -[ "$CR_HEAD_UNREVIEWABLE" = "yes" ] && STATUS_DESC="Cleared ($ISO); CR-head auto-satisfied: unreviewable-only HEAD" -[ "$CR_RL_BACKSTOPPED" = "yes" ] && STATUS_DESC="Cleared ($ISO); CR rate-limited, local /eng:cr review backstops" -[ "$CR_VERDICT" = "unresolved-waived" ] && STATUS_DESC="Cleared ($ISO); CR green on HEAD, only OUTDATED CR threads waived" -[ "$IS_BOOKKEEPING" = "yes" ] && STATUS_DESC="Cleared ($ISO) via bookkeeping fast-lane: docs/inventory only, review+QA waived" +if [ "${#DESC_NOTES[@]}" -gt 0 ]; then + # printf reuses the format once per argument; strip the leading separator. + # (IFS + ${arr[*]} would join on only the FIRST character of IFS, not "; ".) + DESC_JOINED=$(printf '; %s' "${DESC_NOTES[@]}"); DESC_JOINED=${DESC_JOINED#'; '} + STATUS_DESC="Cleared ($ISO); ${DESC_JOINED}" + STATUS_DESC="${STATUS_DESC:0:140}" +fi if gh api -X POST "repos/$REPO/statuses/$HEAD" \ -f state=success \ -f context="$CLEARANCE_CONTEXT" \ diff --git a/eng/hooks/scripts/ship-watch-nudge-lib.sh b/eng/hooks/scripts/ship-watch-nudge-lib.sh index 9c2a673..dc61a2d 100755 --- a/eng/hooks/scripts/ship-watch-nudge-lib.sh +++ b/eng/hooks/scripts/ship-watch-nudge-lib.sh @@ -20,9 +20,12 @@ # mc_cr_rate_limited (from merge-clearance-lib.sh) the merge gate uses for a MISSING # CR status, which is exactly the state at a fresh /ship create, so the two agree in # the case the nudge targets. (The merge gate additionally uses the stricter -# mc_cr_rate_limited_latest for a stuck-"pending" status; that state does not exist -# at create time, so the nudge does not need it and the two are not claimed to agree -# universally, only for the missing-status create moment.) +# mc_cr_rate_limited_latest for a stuck-"pending" status, and +# mc_cr_failure_rate_limited for a "failure" status carrying a rate-limit +# description; NEITHER state can exist at create time - a pending status means CR +# already started, and the failure shape means CR completed an incremental pass over +# a commit that does not exist yet - so the nudge needs neither, and the two are not +# claimed to agree universally, only for the missing-status create moment.) # # Requires grep (present everywhere). No jq dependency here; the hook does the # JSON parsing and passes plain strings in. diff --git a/eng/hooks/tests/merge-clearance-lib.bats b/eng/hooks/tests/merge-clearance-lib.bats index 5a9e1bc..143a856 100644 --- a/eng/hooks/tests/merge-clearance-lib.bats +++ b/eng/hooks/tests/merge-clearance-lib.bats @@ -905,3 +905,203 @@ rlmarker="" + +cr_comment() { + # cr_comment -> a one-element CR-authored comments array + jq -nc --arg b "$1" '[{author:"coderabbitai[bot]", body:$b}]' +} + +@test "cr failure rate-limited: failure + 'Review rate limited' description -> yes" { + run mc_cr_failure_rate_limited "failure" "Review rate limited" "[]" + [ "$output" = "yes" ] + [ "$status" -eq 0 ] +} + +@test "cr failure rate-limited: description match is case-insensitive, spaced or hyphenated" { + local d + # A plain substring test on "rate limit" missed the hyphenated spelling, and that + # miss is SILENT: the operator gets told a rate-limited failure is genuine. + for d in "RATE LIMIT EXCEEDED - please wait 12 minutes" \ + "CodeRabbit is Rate Limited for this repository" \ + "Rate-limited" \ + "review rate-limit hit"; do + run mc_cr_failure_rate_limited "failure" "$d" "[]" + [ "$output" = "yes" ] || fail "expected yes for description: $d" + done +} + +@test "cr failure rate-limited: a NEGATED mention of rate limiting does not qualify" { + local d + for d in "not rate limited" \ + "not a rate limit issue" \ + "no rate-limit was hit; parser crashed"; do + run mc_cr_failure_rate_limited "failure" "$d" "[]" + [ "$output" = "no" ] || fail "expected no for description: $d" + [ "$status" -eq 1 ] + done +} + +@test "cr failure rate-limited: failure + marker as CR's latest comment -> yes" { + run mc_cr_failure_rate_limited "failure" "" "$(cr_comment "$MARKER")" + [ "$output" = "yes" ] + [ "$status" -eq 0 ] +} + +@test "cr failure rate-limited: failure + latest marker under an unrelated description -> yes" { + run mc_cr_failure_rate_limited "failure" "Review failed" "$(cr_comment "$MARKER")" + [ "$output" = "yes" ] + [ "$status" -eq 0 ] +} + +@test "cr failure rate-limited: a STALE marker cannot make a later GENUINE failure read as rate-limited" { + # The regression that mattered: CR is rate-limited on an early commit (marker + # posted, never deleted), then recovers, reviews, and its status on a later HEAD + # resolves to a GENUINE failure. Keying on "marker anywhere on the PR" would + # auto-clear that failure with no operator flag AND label the audit trail + # "rate-limited". Requiring the marker to be CR's LATEST comment closes it, the + # same discipline mc_cr_rate_limited_latest applies to the stuck-pending shape. + local history + history=$(jq -nc --arg m "$MARKER" '[ + {author:"coderabbitai[bot]", body:$m}, + {author:"coderabbitai[bot]", body:"Actionable comments posted: 4"}, + {author:"coderabbitai[bot]", body:"**Walkthrough**\n\nThe changes ..."} + ]') + run mc_cr_failure_rate_limited "failure" "Review failed" "$history" + [ "$output" = "no" ] + [ "$status" -eq 1 ] +} + +@test "cr failure rate-limited: GENUINE failure (neither signal) -> no" { + run mc_cr_failure_rate_limited "failure" "Review completed with errors" \ + "$(cr_comment 'Actionable comments posted: 3')" + [ "$output" = "no" ] + [ "$status" -eq 1 ] +} + +@test "cr failure rate-limited: genuine failure with an empty description -> no" { + run mc_cr_failure_rate_limited "failure" "" "[]" + [ "$output" = "no" ] + [ "$status" -eq 1 ] +} + +@test "cr failure rate-limited: non-failure states never qualify" { + # Even carrying both signals: success / pending / missing are handled by the + # other two shapes (or are not a gap at all), so this function must stay quiet. + local state + for state in success pending missing ""; do + run mc_cr_failure_rate_limited "$state" "Review rate limited" "$(cr_comment "$MARKER")" + [ "$output" = "no" ] || fail "expected no for state: ${state:-}" + [ "$status" -eq 1 ] + done +} + +@test "cr failure rate-limited: a rate-limit marker from a NON-CR author does not count" { + run mc_cr_failure_rate_limited "failure" "Review failed" \ + "$(jq -nc --arg b "$MARKER" '[{author:"mujtaba3B", body:$b}]')" + [ "$output" = "no" ] + [ "$status" -eq 1 ] +} + +@test "cr failure rate-limited: unreadable comments fail closed -> no" { + run mc_cr_failure_rate_limited "failure" "Review failed" '{"not":"an array"}' + [ "$output" = "no" ] + [ "$status" -eq 1 ] + run mc_cr_failure_rate_limited "failure" "Review failed" "garbage" + [ "$output" = "no" ] + [ "$status" -eq 1 ] +} + +@test "cr failure rate-limited: comments argument is optional (defaults to [])" { + run mc_cr_failure_rate_limited "failure" "Review rate limited" + [ "$output" = "yes" ] + run mc_cr_failure_rate_limited "failure" "Review failed" + [ "$output" = "no" ] +} + +# ---- mc_cr_failure_disposition (the whole CR-failure decision, as a table) ---- +# This is the gate's most security-relevant decision: what stands between +# --override-cr-failure and a bare merge bypass. Every row below is a case that +# used to be verifiable only by hand. + +disp() { + # disp + mc_cr_failure_disposition "$1" "$2" "$3" "$4" +} + +@test "cr failure disposition: a genuine failure blocks, with or without a current review" { + run disp failure no 0 current + [ "$output" = "block-genuine" ] + [ "$status" -eq 1 ] + run disp failure no 0 missing + [ "$output" = "block-genuine" ] + [ "$status" -eq 1 ] +} + +@test "cr failure disposition: the override clears ONLY with a current local review" { + run disp failure no 1 current + [ "$output" = "cleared-override" ] + [ "$status" -eq 0 ] + # The interlock. Anything other than a current review must refuse, including the + # n/a case (running outside a checkout, where there is no stamp to read at all). + local rs + for rs in stale missing n/a ""; do + run disp failure no 1 "$rs" + [ "$output" = "block-override-needs-review" ] || fail "override cleared with review_state=${rs:-}" + [ "$status" -eq 1 ] + done +} + +@test "cr failure disposition: a rate-limited failure clears ONLY with a current local review" { + run disp failure yes 0 current + [ "$output" = "cleared-rate-limited" ] + [ "$status" -eq 0 ] + local rs + for rs in stale missing n/a ""; do + run disp failure yes 0 "$rs" + [ "$output" = "block-rate-limited-unbackstopped" ] || fail "rate-limit cleared with review_state=${rs:-}" + [ "$status" -eq 1 ] + done +} + +@test "cr failure disposition: rate-limit wins over the flag, so the audit trail stays honest" { + # An operator who passes the flag defensively on a failure the machine can already + # classify must NOT have it recorded as a human override; otherwise a later grep + # for real overrides returns false positives. + run disp failure yes 1 current + [ "$output" = "cleared-rate-limited" ] + [ "$status" -eq 0 ] + run disp failure yes 1 stale + [ "$output" = "block-rate-limited-unbackstopped" ] + [ "$status" -eq 1 ] +} + +@test "cr failure disposition: the flag is inert on any non-failure status" { + local state + for state in success pending missing; do + run disp "$state" no 0 current + [ "$output" = "n/a" ] || fail "expected n/a for state: $state" + [ "$status" -eq 0 ] + # Flag passed where there is no failure to override: reported so the caller can + # tell the operator, but it never clears anything on its own. + run disp "$state" no 1 current + [ "$output" = "override-inert" ] || fail "expected override-inert for state: $state" + [ "$status" -eq 0 ] + done +} + +@test "cr failure disposition: defaults deny (missing/garbage arguments never clear)" { + run mc_cr_failure_disposition failure + [ "$output" = "block-genuine" ] + [ "$status" -eq 1 ] + run disp failure "garbage" "garbage" current + [ "$output" = "block-genuine" ] + [ "$status" -eq 1 ] +} diff --git a/eng/skills/pr-watcher/SKILL.md b/eng/skills/pr-watcher/SKILL.md index 8d03a44..94cbc47 100644 --- a/eng/skills/pr-watcher/SKILL.md +++ b/eng/skills/pr-watcher/SKILL.md @@ -255,7 +255,7 @@ After the sensor returns, branch on `outcome`: - `"continue"` → the slice budget expired before CR settled: run the same sensor command again immediately (already covered by the slice loop above; it is not a failure and does not reach the decisions below). - `"pr_closed"` → print `PR is closed/merged. Watcher exiting.` and end the skill. - `"already_settled"` → CodeRabbit's review on the current HEAD is terminal `success` and there are no unprocessed CR items. (Sensor returns this only for `success`, never for `failure`/`error`.) Print `🐇 CodeRabbit is caught up on HEAD (status: success). Nothing to address. Watcher exiting.` and end the skill. Do NOT loop again; another sense cycle would just reproduce this outcome. -- `"cr_failure"` → CodeRabbit's review on the current HEAD ended in `failure` or `error` with no actionable comments to drain. CR has emitted its final word; no new transition will arrive without a new push. **Check for a rate-limit FIRST, before the genuine-failure exit below.** If CR's comments contain the `rate limited by coderabbit.ai` marker, this is not a real CR error: take the Step 4h rate-limited short-circuit instead of exiting to inspect. If a current `/eng:cr` review backstops this HEAD (`review-skill-head` == HEAD) the PR is clear to land via `/land-and-deploy`; otherwise run `/eng:cr` first, then land. Watching will not help, because CR will not review this HEAD without a new push. **Only if it is NOT a rate-limit** (a genuine CR failure): print `⚠️ CodeRabbit review on HEAD ended in (updated_at: ). No comments were posted; this typically indicates a CR-side problem (internal error, repo config). Watcher exiting; please inspect the PR and re-invoke /eng:pr-watcher after the next push.` and end the skill. Do NOT loop; another sense cycle would reproduce this outcome. +- `"cr_failure"` → CodeRabbit's review on the current HEAD ended in `failure` or `error` with no actionable comments to drain. CR has emitted its final word; no new transition will arrive without a new push. **Check for a rate-limit FIRST, before the genuine-failure exit below.** Two independent signals, either of which means rate limit rather than a real CR error: (a) CR's comments contain the `rate limited by coderabbit.ai` marker, or (b) the CodeRabbit commit status's own DESCRIPTION says rate limited (e.g. `Review rate limited`). Signal (b) is the common one on an incremental pass that burns the limit, and CR often posts NO marker comment in that flavour, so check it explicitly: `gh api repos///commits//statuses -q 'first(.[] | select(.context=="CodeRabbit") | .description)'` and match `rate limit` case-insensitively. The sensor does not carry the description; this is a one-call dispatcher-side check. On either signal, take the Step 4h rate-limited short-circuit instead of exiting to inspect. If a current `/eng:cr` review backstops this HEAD (`review-skill-head` == HEAD) the PR is clear to land via `/land-and-deploy`; otherwise run `/eng:cr` first, then land. Watching will not help, because CR will not review this HEAD without a new push. **Only if it is NOT a rate-limit** (a genuine CR failure): print `⚠️ CodeRabbit review on HEAD ended in (updated_at: ). No comments were posted; this typically indicates a CR-side problem (internal error, repo config). Watcher exiting; please inspect the PR and re-invoke /eng:pr-watcher after the next push.` and end the skill. Do NOT loop; another sense cycle would reproduce this outcome. - `"idle_timeout"` → ask the user (via AskUserQuestion) whether to keep watching or stop. Default recommendation: **stop** (long silence after watcher start almost always means CR is done; the user can re-invoke /eng:pr-watcher when there is new activity). If they choose to keep watching, start another sense cycle. - `"new_cr_feedback"` → proceed to Step 4. - `"error"` → the script's `gh` calls failed repeatedly (rate limit, expired auth, network) or its lib is missing; `error_message` says which. Count it as a sensor failure. @@ -286,7 +286,17 @@ For every item across `new_issue_comments`, `new_reviews`, `new_review_comments` - `out_of_scope` — valid suggestion but outside fix scope, or architectural / cross-cutting. - `needs_user_input` — ambiguous, requires human judgment. -**Rate-limit detection (overrides the classifications above).** If any item's body contains the literal `rate limited by coderabbit.ai` (CodeRabbit's rate-limit notice, posted as an auto-generated `status_ping`-shaped comment), set a `cr_rate_limited` flag for this batch. CodeRabbit did NOT actually review this HEAD, so a green `CodeRabbit` commit status here is not a real review: do not let it read as a clean pass. Handle it in the Step 4h short-circuit rather than looping. This is the same rate-limit marker the merge gate keys on (`merge-clearance.sh` `mc_cr_rate_limited` for a missing CR status; it uses the stricter `mc_cr_rate_limited_latest` only for a stuck-`pending` status), so the watcher and the gate agree on when a rate-limited CR is safe to move past in the common missing/settled-status case. +**Rate-limit detection (overrides the classifications above).** Set a `cr_rate_limited` flag for this batch when EITHER signal is present: (a) an item's body contains the literal `rate limited by coderabbit.ai` (CodeRabbit's rate-limit notice, posted as an auto-generated `status_ping`-shaped comment), or (b) the CodeRabbit commit status on HEAD is `failure` and its description matches `rate limit` (the same one-call check the `cr_failure` branch in Step 3 makes). CodeRabbit did not COMPLETE a review of this HEAD (in the missing and pending shapes it never reviewed it at all; in the failure shape it may have reviewed HEAD and only tripped the limit on a trailing incremental pass), so its status here is not a real review verdict: do not let it read as a clean pass. Handle it in the Step 4h short-circuit rather than looping. This is the same rate-limit marker the merge gate keys on. The gate recognizes THREE rate-limit shapes, and the watcher should reach the same conclusion on each: + +| CR commit status on HEAD | Signal | Gate function | +|---|---|---| +| `missing` (CR never started) | marker comment anywhere on the PR | `mc_cr_rate_limited` | +| `pending`, stuck (started, then hit the limit) | marker is CR's LATEST comment | `mc_cr_rate_limited_latest` | +| `failure` (an incremental pass burned the limit) | status DESCRIPTION says rate limited (non-negated), or the marker is CR's LATEST comment | `mc_cr_failure_rate_limited` | + +Note the marker strictness: only row 1 accepts the marker anywhere on the PR, and it can afford to because a `missing` status means CR never posted anything for this HEAD at all. Rows 2 and 3 require the marker to be CR's LATEST comment, because in both of those CR demonstrably ran: a stale marker from an earlier commit must not make a later, genuine CR verdict read as a rate limit. + +In every shape the gate treats the rate limit as satisfied ONLY when a current local `/eng:cr` review backstops the HEAD, so the watcher's advice ("run `/eng:cr`, then land") is exactly what the gate will require. A GENUINE CR failure (none of these signals) is different: the gate blocks it unless the operator passes `--override-cr-failure`, which also requires the current local review. The watcher never makes that call; it exits and asks the human to inspect. Apply the project's coding principles when filtering. Reject suggestions that introduce single-use abstractions, speculative error handling, or "cleanup" outside the task. @@ -367,7 +377,7 @@ Exit the skill (do NOT start another sense cycle) when ALL the following hold fo - The sensor returned `cr_status_state == "success"` AND `settled_via == "status_transition"`. (CR's terminal pass on the current HEAD finished cleanly. `failure`/`error` is also "done" in CR's sense, but signals a CR-side problem worth keeping the watcher alive for a human to inspect, so do not auto-exit on those.) - All findings in the batch classified as `status_ping`, `nitpick_only`, `already_fixed`, or `false_positive` (no `valid_actionable`, `out_of_scope`, or `needs_user_input` left in flight). `out_of_scope` and `needs_user_input` items both escalate to `escalations.jsonl` rather than being replied to on the PR (see Step 4e), so leaving them unresolved means there is still pending human work; the watcher should keep the loop alive so the user can see them when they return. -**Rate-limited short-circuit (check this FIRST, before the clean-exit line below).** If `cr_rate_limited` was set for this batch (Step 4b), CodeRabbit posted its `rate limited by coderabbit.ai` notice INSTEAD of reviewing, so its green status is not a real review and there is nothing to keep watching for: CR will not review this HEAD without a new push. Do the same thing the merge gate does (`merge-clearance.sh` `CR_RATE_LIMITED` / `CR_RL_BACKSTOPPED`): fall back to the current local `/eng:cr` review. Compare `/review-skill-head` against the PR HEAD: +**Rate-limited short-circuit (check this FIRST, before the clean-exit line below).** If `cr_rate_limited` was set for this batch (Step 4b, via either the marker comment or a rate-limited status description), CodeRabbit hit its limit INSTEAD of finishing the review, so its status on this HEAD is not a real review verdict and there is nothing to keep watching for: CR will not review this HEAD without a new push. Do the same thing the merge gate does (`merge-clearance.sh` `CR_RATE_LIMITED` / `CR_RL_BACKSTOPPED`): fall back to the current local `/eng:cr` review. Compare `/review-skill-head` against the PR HEAD: - Backstopped (`review-skill-head` == HEAD): a current `/eng:cr` review already covers this HEAD. Print `🐇 CodeRabbit is rate-limited (no real review on HEAD ); a current /eng:cr review backstops it. Nothing to watch. Land via /land-and-deploy.` and end the skill. - Not backstopped: print `🐇 CodeRabbit is rate-limited (no real review on HEAD ) and no current /eng:cr review backstops it. Run /eng:cr on this HEAD, then land via /land-and-deploy. Not watching further.` and end the skill.