fix(github): sweep rescues never-reviewed PRs (ws-22l)#909
Conversation
…no longer strands (ws-22l) Fourth stall mode (PR #906, 2026-07-06): the PR-opened auto-review dispatch died with a container restart mid-deploy, and nothing re-triggered — the promote/merge machinery needs a prior Quinn review to act, so _evaluateApproveOnGreen returned 'skip' from both the edge path and the sweep, stranding a green PR unreviewed forever. Manual workaround was an @protoquinn mention. The shared decision now returns 'unreviewed' for an open, non-draft PR with terminal CI, no Quinn review at all, and age past a 10-minute grace (younger PRs may have the original opened-dispatch still in flight). Both callers rescue: the CI-completion edge path re-dispatches exactly as it does for needs-review, and the reconciliation sweep dispatches ONE review per head SHA per process (TTL-bounded rescue map — the dispatcher cooldown alone would re-dispatch every 3-min sweep while a slow review runs). The re-dispatch block is extracted to _dispatchReviewFor, shared by both paths. Fixes ws-22l. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👀 Quinn is reviewing — verdict (PASS / WARN / FAIL) + findings to follow. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 6 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: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds an "unreviewed" decision outcome to the GitHub approve-on-green evaluation logic, dispatching a one-time rescue review per PR head SHA when Quinn hasn't reviewed an old, CI-terminal PR. The reconciliation sweep is updated to accept an EventBus, track dispatches, and log rescue counts. Tests are extended accordingly. ChangesApprove-on-Green Rescue Dispatch
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Sweep as ReconciliationSweep
participant Eval as _evaluateApproveOnGreen
participant Dispatch as _dispatchReviewFor
participant AutoReview as _handleAutoReview
participant Bus as EventBus
Sweep->>Eval: evaluate PR CI status and review state
Eval-->>Sweep: "unreviewed" (PR old enough, CI terminal)
Sweep->>Sweep: check reviewRescueDispatched for PR head SHA
Sweep->>Dispatch: dispatch rescue review
Dispatch->>AutoReview: invoke with skipDedup true
AutoReview->>Bus: publish pr_review event
Sweep->>Sweep: increment rescued counter, log summary
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![protoquinn[bot]](https://avatars.githubusercontent.com/in/3282126?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA Audit — PR #909 | fix(github): sweep rescues never-reviewed PRs (ws-22l)
VERDICT: WARN
CI Status
- test: queued
- dashboard: in_progress
⏳ CI is still settling. Per the approve-on-green policy, this COMMENT review will be promoted to APPROVED automatically once every check passes with no blockers — no manual re-review needed.
Diff Review
- Adds rescue sweep for never-reviewed PRs: when the opened-webhook dispatch is lost, the reconcile sweep detects open/terminal/zero-review PRs older than 10 min and re-dispatches
pr_reviewvia the shared_dispatchReviewFormethod. reviewRescueDispatchedmap (per-process, keyedPR@sha) ensures at most one rescue — clean dedup. A restart or new push resets it._dispatchReviewForextraction is a faithful refactor of the existing CI-completion re-dispatch block — no behavior change on theneeds-reviewpath.- Three new tests cover rescue-once semantics, young-PR exclusion (<10 min), and pending-CI exclusion. Test harness updated cleanly (
runSweepWithPluginnow captures dispatched topics viaInMemoryEventBus).
Observations
- LOW (Gap): Clawpatch structural review unavailable (gateway 500) — diff-only review. The diff is also truncated at 200/294 lines; the tail of
_dispatchReviewForand_evaluateApproveOnGreenchanges are not visible, but the three new tests cover the rescue behaviors exhaustively. - LOW:
reviewRescueDispatchedmap has no explicit pruning shown in the visible diff — the comment says "TTL-pruned alongside use." If the pruning code is in the truncated tail, this is fine; if it's missing, the map grows unbounded over a long-running process. Worth a spot-check.
— Quinn, QA Engineer
|
Submitted COMMENT review on #909. |
There was a problem hiding this comment.
CI terminal-green, no blockers on prior review — auto-approving on green (#748).
|
✅ CI went terminal-green with no blockers on the prior review — promoting it to APPROVED per the approve-on-green policy (#748). |
There was a problem hiding this comment.
QA Audit — PR #909 | fix(github): sweep rescues never-reviewed PRs (ws-22l)
VERDICT: PASS (prior APPROVE on 8303116 — head unchanged, reaffirming)
CI Status
- test: queued
- dashboard: queued
⏳ Two checks still settling. This PR was already APPROVED on this exact head SHA in a prior review pass. Per the approve-on-green policy (#748), this APPROVE will promote to a formal merge signal automatically once every check reports terminal-green — no further review needed.
Diff Review
- Extracts
_dispatchReviewForas a shared re-dispatch helper for both the CI-completionneeds-reviewpath and the newunreviewedrescue path (ws-22l). - Adds
reviewRescueDispatchedmap +RESCUE_MIN_AGE_MS(10 min) to deduplicate sweep rescues and avoid double-review of young PRs whose original opened-dispatch may still be in flight. _evaluateApproveOnGreennow returns"unreviewed"for open, non-draft PRs with terminal CI, zero Quinn reviews, and age > 10 min.- 3 new tests: rescue-once semantics across two sweeps, young-PR exclusion, pending-CI exclusion. All 1,183 tests green.
Observations
- No unresolved CodeRabbit threads.
- No blockers — the fix is targeted, well-tested, and follows the established pattern from prior approve-on-green stall-mode patches (#888, #901, #907).
— Quinn, QA Engineer
|
Submitted COMMENT review on #909. |
|
✅ CI went terminal-green with no blockers on the prior review — promoting it to APPROVED per the approve-on-green policy (#748). |
There was a problem hiding this comment.
CI terminal-green, no blockers on prior review — auto-approving on green (#748).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/plugins/github.ts (1)
1166-1172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the
Returns:doc block for the new outcomes.The docstring enumerates only
"approved"/"needs-review"/"skip", but the signature now returns"merged"and"unreviewed"as well. The"needs-review"note ("the reconciliation sweep ignores this and skips") is still accurate, but"unreviewed"is now actively rescued by the sweep and should be documented alongside it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/plugins/github.ts` around lines 1166 - 1172, Update the Returns doc block in the github plugin to match the current return union from the function it documents. The existing summary only lists "approved", "needs-review", and "skip", but the implementation now also returns "merged" and "unreviewed"; revise the Returns section so it documents all outcomes and note that "unreviewed" is rescued by the reconciliation sweep while the existing "needs-review" behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/plugins/github.ts`:
- Around line 1108-1113: The rescue dispatch path in the review decision block
is missing the per-PR/head-SHA dedup guard, so the same CI completion can be
dispatched more than once. Update the `unreviewed` branch in the
`_handleCiCompletion` flow to use the same `reviewRescueDispatched` check as the
`needs-review` rescue path before calling `_dispatchReviewFor`, keyed by the PR
and head SHA, so duplicate sweeps or webhook retries do not re-dispatch the same
review.
---
Nitpick comments:
In `@lib/plugins/github.ts`:
- Around line 1166-1172: Update the Returns doc block in the github plugin to
match the current return union from the function it documents. The existing
summary only lists "approved", "needs-review", and "skip", but the
implementation now also returns "merged" and "unreviewed"; revise the Returns
section so it documents all outcomes and note that "unreviewed" is rescued by
the reconciliation sweep while the existing "needs-review" behavior remains
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 430e1a3b-141c-4b71-8a72-bd95efc859a5
📒 Files selected for processing (2)
lib/plugins/__tests__/github-approve-on-green.test.tslib/plugins/github.ts
There was a problem hiding this comment.
QA Audit — PR #909 | fix(github): sweep rescues never-reviewed PRs (ws-22l)
VERDICT: WARN (pending CI — see below)
CI Status
- dashboard: queued
- test: queued
⏳ CI is still settling. This is a re-review at unchanged head 915d0b9 — the prior review was APPROVED. The approve-on-green policy (PR #748) promotes this review to formal APPROVED automatically once every check is terminal-green. No action needed from the author.
Diff Review
- 2 files, +133/-38: extracts
_dispatchReviewForand addsunrevieweddecision path for lost-opening-dispatch rescue (ws-22l). - 3 new tests cover rescue-once semantics, young-PR exclusion, and pending-CI exclusion — all tight and well-scoped.
- Refactor is clean: shared dispatch block, no behavior change on the existing
needs-reviewpath.
Observations
- LOW: 1 unresolved CodeRabbit thread at
lib/plugins/github.ts:1113re: stability (truncated in fetch — may be a stale false positive on the newreviewRescueDispatchedrescue map; worth closing if resolved).
— Quinn, QA Engineer
|
Submitted COMMENT review on #909. |
There was a problem hiding this comment.
QA Audit — PR #909 | fix(github): sweep rescues never-reviewed PRs (ws-22l)
VERDICT: PASS
CI Status
- dashboard: success
- test: success
- build-and-push: skipped
Diff Review
- 2 files, +133/-38: extracts
_dispatchReviewForand addsunrevieweddecision path for lost-opening-dispatch rescue (ws-22l). - 3 new tests cover rescue-once semantics, young-PR exclusion, and pending-CI exclusion — tight and well-scoped.
- Refactor is clean: shared dispatch block, no behavior change on the existing
needs-reviewpath.
Observations
- LOW: 1 unresolved CodeRabbit thread at
lib/plugins/github.ts:1113re:reviewRescueDispatchedrescue map stability — truncated in fetch, may be stale; worth closing if resolved. - LOW: clawpatch structural review unavailable (gateway 500) — diff-based review is sufficient for this focused change.
— Quinn, QA Engineer
|
Submitted APPROVE review on #909. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
QA Audit — PR #909 | fix(github): sweep rescues never-reviewed PRs (ws-22l)
VERDICT: WARN (pending CI — see below)
CI Status
- dashboard: queued
- test: queued
⏳ CI is still settling. The approve-on-green policy (PR #748) promotes this review to formal APPROVED automatically once every check is terminal-green. No action needed from the author.
Diff Review (incremental, head 915d0b9 → c62e816)
- 2 files, +144/-38: extracts
_dispatchReviewFor, addsunrevieweddecision +reviewRescueDispatchedTTL rescue map to recover lost opening-webhook dispatches (ws-22l). - 3 new tests cover rescue-once semantics, young-PR exclusion, and pending-CI exclusion — now wired via
InMemoryEventBusto assert dispatch directly. - Refactor is clean: shared dispatch block, no behavior change on the existing
needs-reviewpath. - Prior LOW (unresolved CodeRabbit thread) now resolved.
Observations
- LOW: clawpatch structural review unavailable (HTTP 500) — diff-based review complete. The change is contained (2 focused files, single feature addition with tests) and risk surface is low.
- The rescue guard (
_claimRescueTTL map + 10-min grace) is well-designed: one dispatch per head SHA per process avoids double-review, while new pushes or restarts retry naturally.
— Quinn, QA Engineer
|
Submitted COMMENT review on #909. |
There was a problem hiding this comment.
QA Audit — PR #909 | fix(github): sweep rescues never-reviewed PRs (ws-22l)
VERDICT: PASS
CI Status
- test: success
- dashboard: success
- build-and-push: skipped
Diff Review
- 2 files, +144/-38: extracts
_dispatchReviewForshared byneeds-reviewand the newunreviewedrescue path, adds_claimRescuewith per-head-SHA dedup, and wires the sweep to re-dispatch lost reviews for PRs older than 10 min with terminal CI. - 3 new tests: rescue-once semantics across two sweeps, young-PR exclusion, and pending-CI exclusion — all tight and well-scoped.
- The
_reconcileApproveOnGreensignature now accepts anInMemoryEventBus, and tests verify dispatched messages — a solid improvement in test coverage.
Observations
- LOW: clawpatch structural review returned HTTP 500 — reviewed manually via diff instead. No issues found.
- The prior WARN at
915d0b9was solely due to pending CI — now terminal green, and the unresolved CodeRabbit thread is cleared. The incremental diff is clean.
— Quinn, QA Engineer
|
Submitted APPROVE review on #909. |
…ger strand (#911) Fifth stall mode, hit twice today (#909/#910): with strict branch protection, the first merge moves main under every other in-flight PR; GitHub's native auto-merge does not update branches, so an approved+green PR sits BLOCKED(behind) forever. This is the open question ws-2b5 originally asked — the answer is yes, an auto-update step is needed whenever two PRs are in flight at once. The merge-completion path now handles it: when the sweep's merge attempt is refused with 405 and the PR's mergeable_state is 'behind', it PUTs update-branch with expected_head_sha (same push-race guard as the merge call) and drops the watch — the synchronize event runs CI + re-review on the fresh head and the normal promote→merge cycle converges. Any other 405 keeps the existing retry-next-sweep behavior. The sweep's decision ladder is now: approve → merge → rescue-review → update-behind — one chokepoint for all five observed stall modes. Co-authored-by: Automaker <automaker@localhost> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Fourth (and hopefully last) stall mode in the merge pipeline, observed on #906 this morning.
The failure
The PR-opened auto-review dispatch died with the container restart during #905's deploy window (in-process DeepAgent runs aren't durable). Nothing re-triggers a never-reviewed PR:
_evaluateApproveOnGreenreturnedskipwhenquinnHasReviewedwas false — from both the CI-completion edge path and the reconciliation sweep — so a green PR sat unreviewed forever. Manual workaround was an@protoquinnmention comment.The fix
The shared decision returns
unreviewedfor an open, non-draft PR with terminal CI, zero Quinn reviews, and age > 10 min (younger PRs may have the original opened-dispatch legitimately still in flight — rescuing those would double-review). Both callers act:needs-review— most lost dispatches recover within minutes of CI finishing.N unreviewed PR(s) rescue-dispatched.The synthesized re-dispatch block is extracted to
_dispatchReviewFor, shared by both paths — no behavior change on the existing needs-review flow.With this, the sweep now covers all four observed stall modes: missed CI webhook (#888), unregistered repo (#901), wedged auto-merge (#907), and lost review dispatch.
Verification
1,183 tests green (3 new: rescue-once semantics across two sweeps, young-PR exclusion, pending-CI exclusion),
NODE_ENV=production bun testgreen, tsc clean.Fixes ws-22l.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes