🐛 fix(pr-review-queue): remediate merged-#35 CodeRabbit findings#36
Open
AojdevStudio wants to merge 1 commit into
Open
🐛 fix(pr-review-queue): remediate merged-#35 CodeRabbit findings#36AojdevStudio wants to merge 1 commit into
AojdevStudio wants to merge 1 commit into
Conversation
Post-merge follow-up to #35. Fixes the one dead-on-arrival blocker plus the run.mjs stability cluster CodeRabbit flagged, with regression tests. - protocols.md: document the required --authorized flag on claim.mjs (the election command failed on every literal run without it) and correct the exit-status semantics so a genuine failure is no longer misread as a benign head-moved race. - review-gate.mjs: map STARTUP_FAILURE conclusion to fail instead of throwing and aborting the whole gate evaluation. - run.mjs: per-PR failure isolation in pollOnce with a systemic-failure guard so maxErrors still fires; check child exit status in runScript before JSON.parse; atomic state write + corrupt-file recovery on load; unref the backoff timer so graceful stop cannot hang up to the 5-minute cap. - .gitignore: ignore local .worktrees/ (mirrors .claude/worktrees). Deferred to a follow-up issue: the two protocol-race findings (gate-only re-check claim step, in-progress claim renewal).
WalkthroughThe changes refine review-queue election handling, classify startup failures, isolate polling errors, harden observation persistence and runtime behavior, add regression tests, and ignore local ChangesReview queue behavior
Local worktree ignore
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
skills/pr-review-queue/scripts/run.test.mjs (1)
90-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStrengthen the isolation test to cover state retention.
Since the intent of per-PR isolation is to keep the prior observation and retry the PR next cycle, consider prepopulating the
observationsmap for the failing PR in this test and asserting that it is retained in the outputobservations. This will prevent regressions related to state retention.🛠️ Proposed test enhancement
test("pollOnce isolates a single failing PR and still processes the rest of the queue", async () => { + const priorObservations = new Map([ + [35, { pr: 35, head: "old", updatedAt: "old", gateFingerprint: "old", claimState: null }] + ]); const { actionable, observations, errors } = await pollOnce({ fetchQueue: async () => [35, 36], fetchPrState: async (pr) => { if (pr === 35) throw new Error("PR 35 was closed mid-cycle"); return { head: "abc123", election: election(), gateEvidence: evidence() }; }, - observations: new Map(), + observations: priorObservations, now: "2026-07-20T00:00:00Z", }); assert.deepEqual( actionable, [{ pr: 36, head: "abc123", action: "full-review" }], "the healthy PR must still be flagged", ); assert.equal(errors.length, 1); assert.equal(errors[0].pr, 35); - assert.ok(!observations.has(35), "a PR that failed to poll must not get a fabricated observation"); + assert.equal(observations.get(35)?.head, "old", "the failed PR must retain its previous observation"); });🤖 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 `@skills/pr-review-queue/scripts/run.test.mjs` around lines 90 - 108, Strengthen the pollOnce isolation test by prepopulating the observations map with an existing observation for PR 35 before calling pollOnce, then assert that the returned observations retain that exact entry despite fetchPrState failing. Keep the existing assertions that PR 36 remains actionable and PR 35 is reported as an error.
🤖 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 `@skills/pr-review-queue/protocols.md`:
- Around line 41-51: The re-election command that assigns RECHECK_JSON must pass
the required authorization set. Update the claim.mjs invocation in the “re-read
and re-elect” section to include --authorized "$AUTHORIZED_WORKERS", preserving
the existing repository, PR, and expected-head arguments.
In `@skills/pr-review-queue/scripts/run.mjs`:
- Around line 36-63: Update the polling function’s nextObservations
initialization to use an empty Map rather than cloning observations, so PRs no
longer in queue are discarded. In the fetchPrState error path, preserve the
prior observation for the failed PR when one exists, while continuing to update
successful PRs as before.
- Around line 235-237: Update the sleepFn timer in main to remove .unref() so
the daemon remains alive through the backoff sleep and next poll cycle. At the
end of main, explicitly exit with status 1 when status.lastError is set,
otherwise exit with status 0.
---
Nitpick comments:
In `@skills/pr-review-queue/scripts/run.test.mjs`:
- Around line 90-108: Strengthen the pollOnce isolation test by prepopulating
the observations map with an existing observation for PR 35 before calling
pollOnce, then assert that the returned observations retain that exact entry
despite fetchPrState failing. Keep the existing assertions that PR 36 remains
actionable and PR 35 is reported as an error.
🪄 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
Run ID: 1fa39bf9-7229-4a41-ba13-1b3d4e4f4941
📒 Files selected for processing (6)
.gitignoreskills/pr-review-queue/protocols.mdskills/pr-review-queue/scripts/review-gate.mjsskills/pr-review-queue/scripts/review-gate.test.mjsskills/pr-review-queue/scripts/run.mjsskills/pr-review-queue/scripts/run.test.mjs
Comment on lines
+41
to
+51
| `$AUTHORIZED_WORKERS` is the comma-separated set of fleet-worker logins | ||
| allowed to hold a claim (it must include this worker's own login). It comes | ||
| from the dispatcher's assignment; `claim.mjs` folds any claim or abandon | ||
| event from a login outside this set as if it never existed, and **requires** | ||
| the flag. Omitting `--authorized` makes every election exit non-zero. | ||
|
|
||
| ```bash | ||
| HEAD_SHA="$(gh pr view "$PR" -R "$REPO" --json headRefOid --jq .headRefOid)" | ||
| set +e | ||
| ELECTION_JSON="$(bun <skill-directory>/scripts/claim.mjs --repo "$REPO" --pr "$PR" --expected-head "$HEAD_SHA")" | ||
| ELECTION_JSON="$(bun <skill-directory>/scripts/claim.mjs --repo "$REPO" --pr "$PR" \ | ||
| --expected-head "$HEAD_SHA" --authorized "$AUTHORIZED_WORKERS")" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Missing --authorized flag in the re-election command.
The new requirement for the --authorized flag correctly applies to the initial claim.mjs election. However, the re-election command under the "Then re-read and re-elect to confirm you actually won:" section (around line 101) still omits this flag. Since claim.mjs throws an error if --authorized is missing, the recheck will always fail and exit non-zero.
Please update the RECHECK_JSON command to include --authorized "$AUTHORIZED_WORKERS".
🤖 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 `@skills/pr-review-queue/protocols.md` around lines 41 - 51, The re-election
command that assigns RECHECK_JSON must pass the required authorization set.
Update the claim.mjs invocation in the “re-read and re-elect” section to include
--authorized "$AUTHORIZED_WORKERS", preserving the existing repository, PR, and
expected-head arguments.
Comment on lines
+36
to
63
| const errors = []; | ||
| const nextObservations = new Map(observations); | ||
| for (const pr of queue) { | ||
| const { head, election, gateEvidence: evidence } = await fetchPrState(pr); | ||
| let state; | ||
| try { | ||
| state = await fetchPrState(pr); | ||
| } catch (error) { | ||
| // Per-PR isolation: one bad PR (closed mid-cycle, permission hiccup, | ||
| // transient gh error) must not abort polling for every other PR. Keep | ||
| // the prior observation and retry this PR next cycle. | ||
| errors.push({ pr, message: error.message }); | ||
| continue; | ||
| } | ||
| const { head, election, gateEvidence: evidence } = state; | ||
| const previous = observations.get(pr) ?? initialObservation(pr); | ||
| const next = updateObservation(previous, { head, gateEvidence: evidence, election, now }); | ||
| const gatesChanged = observationChanged(previous, next); | ||
| nextObservations.set(pr, next); | ||
| const action = nextAction(election, gatesChanged); | ||
| if (action !== "skip") actionable.push({ pr, head, action }); | ||
| } | ||
| return { actionable, observations: nextObservations }; | ||
| // Systemic-failure guard: if every PR in a non-empty queue failed, surface it | ||
| // as a real cycle error so runLoop's maxErrors abort still fires. | ||
| if (queue.length > 0 && errors.length === queue.length) { | ||
| throw new Error(`all ${queue.length} queued PRs failed to poll: ${errors[0].message}`); | ||
| } | ||
| return { actionable, observations: nextObservations, errors }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Memory leak: nextObservations accumulates stale PRs indefinitely.
By initializing nextObservations = new Map(observations) and only updating PRs that are currently in the queue, PRs that fall out of the queue (e.g., merged or closed PRs) are never removed. This will cause the .pr-review-queue-state.json state file and the worker's memory to grow endlessly over time.
To fix this, start with an empty map and only retain previous observations for PRs that throw an error during fetchPrState.
♻️ Proposed fix
- const nextObservations = new Map(observations);
+ const nextObservations = new Map();
for (const pr of queue) {
let state;
try {
state = await fetchPrState(pr);
} catch (error) {
// Per-PR isolation: one bad PR (closed mid-cycle, permission hiccup,
// transient gh error) must not abort polling for every other PR. Keep
// the prior observation and retry this PR next cycle.
errors.push({ pr, message: error.message });
+ if (observations.has(pr)) {
+ nextObservations.set(pr, observations.get(pr));
+ }
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const errors = []; | |
| const nextObservations = new Map(observations); | |
| for (const pr of queue) { | |
| const { head, election, gateEvidence: evidence } = await fetchPrState(pr); | |
| let state; | |
| try { | |
| state = await fetchPrState(pr); | |
| } catch (error) { | |
| // Per-PR isolation: one bad PR (closed mid-cycle, permission hiccup, | |
| // transient gh error) must not abort polling for every other PR. Keep | |
| // the prior observation and retry this PR next cycle. | |
| errors.push({ pr, message: error.message }); | |
| continue; | |
| } | |
| const { head, election, gateEvidence: evidence } = state; | |
| const previous = observations.get(pr) ?? initialObservation(pr); | |
| const next = updateObservation(previous, { head, gateEvidence: evidence, election, now }); | |
| const gatesChanged = observationChanged(previous, next); | |
| nextObservations.set(pr, next); | |
| const action = nextAction(election, gatesChanged); | |
| if (action !== "skip") actionable.push({ pr, head, action }); | |
| } | |
| return { actionable, observations: nextObservations }; | |
| // Systemic-failure guard: if every PR in a non-empty queue failed, surface it | |
| // as a real cycle error so runLoop's maxErrors abort still fires. | |
| if (queue.length > 0 && errors.length === queue.length) { | |
| throw new Error(`all ${queue.length} queued PRs failed to poll: ${errors[0].message}`); | |
| } | |
| return { actionable, observations: nextObservations, errors }; | |
| } | |
| const errors = []; | |
| const nextObservations = new Map(); | |
| for (const pr of queue) { | |
| let state; | |
| try { | |
| state = await fetchPrState(pr); | |
| } catch (error) { | |
| // Per-PR isolation: one bad PR (closed mid-cycle, permission hiccup, | |
| // transient gh error) must not abort polling for every other PR. Keep | |
| // the prior observation and retry this PR next cycle. | |
| errors.push({ pr, message: error.message }); | |
| if (observations.has(pr)) { | |
| nextObservations.set(pr, observations.get(pr)); | |
| } | |
| continue; | |
| } | |
| const { head, election, gateEvidence: evidence } = state; | |
| const previous = observations.get(pr) ?? initialObservation(pr); | |
| const next = updateObservation(previous, { head, gateEvidence: evidence, election, now }); | |
| const gatesChanged = observationChanged(previous, next); | |
| nextObservations.set(pr, next); | |
| const action = nextAction(election, gatesChanged); | |
| if (action !== "skip") actionable.push({ pr, head, action }); | |
| } | |
| // Systemic-failure guard: if every PR in a non-empty queue failed, surface it | |
| // as a real cycle error so runLoop's maxErrors abort still fires. | |
| if (queue.length > 0 && errors.length === queue.length) { | |
| throw new Error(`all ${queue.length} queued PRs failed to poll: ${errors[0].message}`); | |
| } | |
| return { actionable, observations: nextObservations, errors }; | |
| } |
🤖 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 `@skills/pr-review-queue/scripts/run.mjs` around lines 36 - 63, Update the
polling function’s nextObservations initialization to use an empty Map rather
than cloning observations, so PRs no longer in queue are discarded. In the
fetchPrState error path, preserve the prior observation for the failed PR when
one exists, while continuing to update successful PRs as before.
Comment on lines
+235
to
+237
| // unref so a pending backoff timer never keeps the process alive after a | ||
| // graceful stop resolves the sleep-unless-stopped promise early. | ||
| sleepFn: (ms) => new Promise((resolve) => setTimeout(resolve, ms).unref()), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Critical: .unref() will cause the daemon to exit immediately during sleep.
Awaiting a promise does not keep the Node.js/Bun event loop alive. Because gh is spawned synchronously and I/O completes before the sleep begins, there are typically no other active handles during the sleep phase. Calling .unref() on the only active timer will cause the event loop to empty out, and the process will exit immediately without waiting for the sleep to finish or the next poll cycle to start.
To ensure the process exits promptly after a graceful stop without breaking the sleep, remove .unref() and instead explicitly call process.exit(status.lastError ? 1 : 0) at the end of main.
🐛 Proposed fix
- // unref so a pending backoff timer never keeps the process alive after a
- // graceful stop resolves the sleep-unless-stopped promise early.
- sleepFn: (ms) => new Promise((resolve) => setTimeout(resolve, ms).unref()),
+ sleepFn: (ms) => new Promise((resolve) => setTimeout(resolve, ms)),And then at the end of main (line 242):
- process.exitCode = status.lastError ? 1 : 0;
+ process.exit(status.lastError ? 1 : 0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // unref so a pending backoff timer never keeps the process alive after a | |
| // graceful stop resolves the sleep-unless-stopped promise early. | |
| sleepFn: (ms) => new Promise((resolve) => setTimeout(resolve, ms).unref()), | |
| sleepFn: (ms) => new Promise((resolve) => setTimeout(resolve, ms)), |
🤖 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 `@skills/pr-review-queue/scripts/run.mjs` around lines 235 - 237, Update the
sleepFn timer in main to remove .unref() so the daemon remains alive through the
backoff sleep and next poll cycle. At the end of main, explicitly exit with
status 1 when status.lastError is set, otherwise exit with status 0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Post-merge follow-up to #35. Fixes the one dead-on-arrival blocker plus the
run.mjsstability cluster CodeRabbit flagged on the merged PR, each with a regression test.Blocker (was dead on arrival)
protocols.md— the §1 election command omitted the--authorizedflag thatclaim.mjshard-requires, so every literal run exited non-zero. The doc then told the worker to read a non-zero exit as a benign "head moved" race, so it would silently skip every PR forever. Now documents--authorized "$AUTHORIZED_WORKERS"and splits the exit-status semantics: a head-mismatch payload is a race (retry once), a payload-less failure is genuine (fail loud, stop).Stability cluster (
run.mjs)pollOnce— one bad PR (closed mid-cycle, permission hiccup) no longer aborts the whole cycle; a systemic-failure guard still throws when every PR in a non-empty queue fails, sorunLoop'smaxErrorsabort keeps working.runScriptnow checks the child exit status beforeJSON.parse, so a non-zero sibling script no longer gets parsed as if it succeeded..tmp+ rename) and corrupt-file recovery on load, so a kill mid-write can't brick every restart.unref()the backoff timer so a graceful stop can't keep the process alive up to the 5-minute cap.Other
review-gate.mjs— map theSTARTUP_FAILUREcheck conclusion tofailinstead of throwingunknown check-run conclusionand aborting the entire gate evaluation..gitignore— ignore local.worktrees/(mirrors the already-ignored.claude/worktrees); it was breaking local biome with a nested-root-config error.Tests
Added regression tests:
pollOnceisolation + systemic-failure guard, corrupt-state-file recovery, andSTARTUP_FAILUREclassification. Full suite green (145 pass),bun run check+bun run pack:drypass.Deferred to a follow-up issue
The two protocol-race findings CodeRabbit rated "heavy lift": the gate-only re-check path has no claim step, and there's no in-progress claim renewal. Both are correctness-hardening on concurrency, not blockers, and want their own design pass.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores