pr-review: rate-limited reviews are never auto-retried (no equivalent of dev-lead-retry) #711

Activity

1. don-petry commented on Jun 14, 2026

   

   don-petry

   on Jun 14, 2026 – with Claude

   CollaboratorAuthor

   More actions

   Analysis & review (code-verified)

   I traced the full path — scripts/lib/advisory-review-gate.sh, scripts/review-one-pr.sh, scripts/sweep-stuck-reviews.sh, scripts/lib/ci-status.sh, and the dev-lead-retry pattern (scripts/dev-lead-retry.sh). A few of the issue's stated mechanics don't match what the code actually does today, which changes the recommended fix. Findings below, then a refined recommendation.

   What the code actually does

   1. The advisory gate does not detect rate-limiting at all.
   get_advisory_bot_states (scripts/lib/advisory-review-gate.sh:59-85) only records whether each monitored bot submitted a review/comment. The bots it watches are gemini-code-assist, copilot-pull-request-reviewer, sonarqubecloud, chatgpt-codex-connector (:32-37). A RATE_LIMITED state exists in format_bot_status (:110) but nothing ever sets it — it's dead code. A bot that is out of quota / "review limit reached" simply never submits, so the gate sees it as absent, not rate-limited. Notably CodeRabbit is not in the monitored set at all, so its limits don't directly gate approval.

   2. The withheld run writes no marker. When the gate returns 1 (waiting), review-one-pr.sh:160-170 logs a JSON line and exit 100 — it does not gh pr comment. The idempotency marker <!-- pr-review-agent v1 sha=<sha> --> is only written by post-pr-review.sh on an actual approval/escalation. The ci-pending path writes a different marker (<!-- pr-review-agent ci-pending-ack -->, review-one-pr.sh:121) that the idempotency check ignores.

   ➡️ This contradicts the issue's hypothesis #1. A rate-limited withhold leaves no "already reviewed at head" marker, so the sweep's reviewed_at_head predicate (sweep-stuck-reviews.sh, checks <!-- pr-review-agent v1 sha=<head>) does not skip the PR.

   3. Cancelled sibling checks already don't suppress the sweep. compute_ci_status (scripts/lib/ci-status.sh:60-64, 84) explicitly treats CANCELLED as non-blocking (issue #608) — exactly the concurrency-cancelled dev-lead / dispatch case. Only a genuine FAILURE/ACTION_REQUIRED/TIMED_OUT resolves to failing and makes the sweep skip.

   ➡️ This partially contradicts hypothesis #2: cancelled siblings don't block; only a real failed sibling does.

   4. The gate already self-heals on re-trigger — but only on re-trigger. It is a non-blocking instant check with two timeout fallbacks (:222-231): head-commit age > 20 min, or quiescence > 10 min, both → return 0 (approve). For any PR re-dispatched more than ~20 min after its head push, the gate would proceed. So if #683 had been re-dispatched even once, it would have approved.

   Most likely real root cause

   Because (2)+(4) hold, the sweep should have recovered #683 — unless its selection predicate skipped it every pass. The remaining suppressors are:

   • A genuinely FAILURE/non-cancelled sibling check keeping ci_status != passing (most likely), or
   • reviewDecision != REVIEW_REQUIRED (e.g. a bot left CHANGES_REQUESTED), or
   • The head-age fallback never firing because the GraphQL committer-date lookup (_get_head_committer_date, :93-99) returned empty and something kept resetting quiescence.

   These are testable against #683's actual statusCheckRollup / reviewDecision history. The headline is: the problem is in the sweep's CI-green predicate (and the absence of any explicit rate-limit signal), not in idempotency-marker semantics.

   Recommendation — lean Option 2, more strongly than the issue frames it

   Two foundational pieces are required regardless of option, and neither exists yet:

   1. Make the gate actually detect rate-limit. Parse the monitored bots' submitted review/comment bodies for the known strings ("usage limits", "review limit reached", quota/limit language), populate the already-stubbed RATE_LIMITED state, and have review-one-pr.sh write a machine-detectable marker on withhold: <!-- pr-review-agent status=rate-limited sha=<head> reset=<ISO> -->. This is the linchpin of every acceptance criterion.

   2. Recover via the proven dev-lead-retry pattern. dev-lead-retry.sh already scans status=rate-limited markers, reads reset=<ISO>, gates on is_reset_in_future, guards against re-fire with terminal markers, and re-dispatches — exactly this shape. Mirror it as a pr-review-retry (or extend dev-lead-retry.sh to also scan the pr-review marker) and re-dispatch with force_review=true so the gate doesn't simply re-withhold.

   I lean Option 2 more than the issue does, because the failure on #683 implicates the sweep's compute_ci_status predicate — the one thing a marker-keyed retry can bypass — and because the retry needs force_review=true, a behavior the generic sweep deliberately does not apply to ordinary PRs (sweep-stuck-reviews.sh dispatches with pr_url only). Bolting rate-limit-special-casing onto the sweep (Option 1) means teaching it to (a) read a new marker, (b) ignore its own CI-green gate for that case, and (c) pass force_review — at which point you've effectively rebuilt dev-lead-retry inside the sweep. Reusing the battle-tested retry is cleaner and keeps the two concerns (stuck-event recovery vs. rate-limit recovery) separate.

   Net: Option 2, with the gate-side detection + marker as the prerequisite. The bats coverage in the acceptance criteria can follow tests/dev-lead/e2e/scenarios/07-rate-limit-retry.sh (stub engine + stub gh, assert marker written on all-rate-limited, assert retry re-dispatches once reset has passed and is suppressed before).

   Code-verified analysis by Claude Code.

   ---

   Generated by Claude Code
2. don-petry

   added

   dev-leadFor dev-lead agent pickupFor dev-lead agent pickup

   on Jun 14, 2026
3. don-petry commented on Jun 14, 2026

   

   don-petry

   on Jun 14, 2026

   CollaboratorAuthor

   More actions

   Dev-Lead Implementation Plan

   Issue: #711 — pr-review: rate-limited reviews are never auto-retried (no equivalent of dev-lead-retry)

   Scope

   Make the pr-review withhold-due-to-advisory-bot-rate-limit path leave a machine-detectable status=rate-limited marker, and teach the existing pr-review-sweep to auto-retry such PRs once the limit window clears — closing the gap with the least surface area (Option 1, no new workflow).

   Root-cause confirmation

   • review-one-pr.sh advisory-gate withhold (gate rc=1) exits 100 leaving no marker → the run is invisible to any retry mechanism.
   • The existing sweep skips a PR when (a) an idempotency marker <!-- pr-review-agent v1 sha=<head> --> is present at head, or (b) compute_ci_status is not passing. (Note: lib/ci-status.sh already treats CANCELLED as non-blocking per pr-review CI gate should gate on REQUIRED checks only — non-required CANCELLED *and* FAILED checks falsely block reviews #608, so concurrency-cancelled siblings are handled; genuinely-failing siblings remain.)

   Design

   • New marker (distinct prefix so it never collides with the idempotency marker): <!-- pr-review-agent rate-limited v1 sha=<HEAD> status=rate-limited reset=<ISO> -->.
   • Detection: when the advisory gate withholds, scan the latest submission of each monitored advisory bot (+ coderabbitai) for rate-limit phrasings; if matched, post the marker (deduped) with a backoff reset (parsed if available, else now+RATE_LIMIT_RETRY_BACKOFF_MIN, default 60m).
   • Sweep retry branch: a PR with a rate-limited marker at head, whose reset has passed and which has no newer idempotency marker at head, is re-dispatched via the normal trigger even if the CI gate isn't passing (so cancelled/failed siblings can't suppress it). review-one-pr.sh remains the authoritative gate; after the limit clears, its head-age timeout fallback lets donpetry-bot approve. Non-force dispatch — never bypasses the advisory gate, so no premature approval.

   Implementation Checklist

   • scripts/lib/advisory-review-gate.sh — add detect_advisory_rate_limit (pure, over reviews+comments JSON) + maybe_post_rate_limited_marker (fetch, detect, dedup-post)
   • scripts/review-one-pr.sh — in the gate rc=1 / non-FORCE_REVIEW branch, post the rate-limited marker before exiting 100
   • scripts/sweep-stuck-reviews.sh — add the rate-limited retry branch (marker + reset + newer-idempotency-marker checks)

   Tests to Write

   • tests/test_sweep_stuck_reviews.bats — rate-limited marker + green CI → re-dispatch; reset-in-future → no dispatch; newer idempotency marker → no dispatch; failing/cancelled sibling does not suppress the rate-limited retry
   • tests/dev-lead/unit/test_advisory_review_gate.bats — detect_advisory_rate_limit true/false cases + dedup behavior of maybe_post_rate_limited_marker

   Test Command

   bats tests/test_sweep_stuck_reviews.bats tests/dev-lead/unit/test_advisory_review_gate.bats (full suite: the bats list in lint.yml + bats tests/dev-lead/unit/), then bash .dev-lead/scripts/dev-lead-lint.sh
4. don-petry commented on Jun 14, 2026

   

   don-petry

   on Jun 14, 2026

   CollaboratorAuthor

   More actions

   Dev-Lead Implementation Report — issue #711

   Status: complete (Option 1 — make pr-review-sweep rate-limit-aware). Changes are local only; no commit/push/PR per the engagement rules.

   What changed

   The withhold-due-to-advisory-bot-rate-limit path (introduced by #657/#658) correctly keeps donpetry-bot from auto-approving while Codex/CodeRabbit are out of quota — but it left no marker, so once CI settled green no event re-fired and the PR stranded at REVIEW_REQUIRED. Two coordinated pieces close the gap:

   1. scripts/lib/advisory-review-gate.sh — two new pure-ish helpers:

      • detect_advisory_rate_limit <reviews/comments json>: returns 0 when a known advisory/review bot's latest submission body matches a focused rate-limit phrase pattern (so a newer real review supersedes an older notice, and vice versa). Detection set is the gate's ADVISORY_BOTS plus coderabbitai (CodeRabbit posts an explicit out-of-quota comment but isn't gated on). Non-bot authors are ignored.
      • maybe_post_rate_limited_marker <pr_url> <head_sha> <reset_iso> <comments_json>: posts a deduplicated marker comment. The marker prefix (rate-limited before v1) deliberately never matches the idempotency regex <!-- pr-review-agent v1 sha=….

   2. scripts/review-one-pr.sh — in the non-FORCE_REVIEW withhold branch (gate rc=1), before exit 100, an isolated subshell sources the gate and, if the withhold is rate-limit-driven, stamps:

      <!-- pr-review-agent rate-limited v1 sha=<HEAD> status=rate-limited reset=<ISO> -->
      

      reset = now + RATE_LIMIT_RETRY_BACKOFF (default 3600s). Non-rate-limited withholds are untouched.

   3. scripts/sweep-stuck-reviews.sh — a rate-limit retry branch placed before the CI gate so unrelated cancelled/failed sibling checks can't suppress the retry. For a marker at the current head: if already reviewed at head → skip (resolved); if reset elapsed (or unparseable → fail-open) → re-dispatch via the normal trigger (never force_review) so the advisory gate stays armed; if reset is still in the future → defer. Dispatch logic refactored into a shared dispatch_review helper (honours DRY_RUN/MAX_DISPATCH).

   Acceptance criteria

   • AC1 rate-limited run leaves a machine-detectable status=rate-limited marker — ✅ (maybe_post_rate_limited_marker, wired into the withhold path)
   • AC2 auto re-review after reset, no manual force_review — ✅ (sweep re-dispatches normal trigger; gate's head-age timeout then lets approval proceed once the window passes)
   • AC3 sibling cancelled/failed checks don't suppress the retry — ✅ (retry branch runs before the CI gate; tests cover FAILING + CANCELLED)
   • AC4 default behaviour for non-rate-limited PRs unchanged — ✅ (detect returns 1; existing stuck-green path intact; all prior tests still pass)
   • AC5 bats coverage — ✅ (see below)

   Tests (TDD — written red first, then green)

   • tests/test_sweep_stuck_reviews.bats: +7 tests (elapsed-reset dispatch & no force_review; FAILING sibling AC3; CANCELLED sibling; future-reset deferral; reviewed-at-head resolution; stale-head no-op; DRY_RUN). 21/21 pass.
   • tests/dev-lead/unit/test_advisory_review_gate.bats: +11 tests (detect_advisory_rate_limit true/false incl. latest-wins both directions, non-bot author, coderabbit, empty input; maybe_post_rate_limited_marker post + dedup). 50/50 pass.

   Verification

   • bats tests/test_sweep_stuck_reviews.bats → 21/21
   • bats tests/dev-lead/unit/ → 485/485
   • full lint.yml bats list (18 files) → 293/293
   • bash .dev-lead/scripts/dev-lead-lint.sh → all checks passed (shellcheck on 51 files + 4 agent profiles)
   • shellcheck on the three changed scripts → only pre-existing SC1091/SC2016 info findings; nothing new

   Notes / trade-offs

   • A false positive (a gated bot's latest message coincidentally matches a rate-limit phrase while another bot is still withholding) only causes a delayed re-review — the re-dispatched run re-evaluates the gate and never approves prematurely.
   • No new test files were added, so no lint.yml test-list change is required (both edited files are already covered).
5. don-petry

   added a commit that references this issue on Jun 14, 2026

   feat: implement issue #711 — pr-review: rate-limited reviews are neve…

   ...

   59e9aa3
6. don-petry

   closed this as completedin #720on Jun 14, 2026
7. don-petry

   added 3 commits that reference this issue on Jul 14, 2026

   fix(dev-lead): label opened PRs auto-rebase:ready to break the #711 r…

   ...

   458e0b3

   fix(agents): label standards-sync + readme-refresh PRs auto-rebase:re…

   ...

   44914f4

   fix(agents): auto-rebase:ready labels + standards-sync summary dedup (#…

   ...

   46e8ad9