Factory PR babysit: fair queue and merged/closed terminal status - #5015
Conversation
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 5 potential issues 🔴
Review Details
Code Review Summary
PR #5015 adds persisted fair-queue ordering for Factory PR babysit work, reconciles merged/closed GitHub terminal states, and updates audit/inbox presentation and localization. The overall direction is sensible: queue ordering is isolated, terminal state is represented in metadata, and the change includes focused tests and additive migration support. This is standard risk because it changes business logic, polling/reconciliation, persistence, and user-facing triage behavior.
Key Findings
- 🔴 HIGH — The fair-queue helper narrows full triage rows to
BabysitQueueRow, causing confirmed TypeScript errors in the response mapping and preventing the Factory build from typechecking. - 🟡 MEDIUM — Fair rotation is applied only after a bounded newest-row scan, so older eligible PRs can still starve.
- 🟡 MEDIUM — Reopened merged/closed PRs retain terminal metadata and remain filtered from babysit work.
- 🟡 MEDIUM — GraphQL completeness is overwritten by a count heuristic, falsely truncating exactly-full but complete comment pages.
- 🟡 MEDIUM — Missing tier-3 check timestamps use an ordering key inconsistent with the persisted cursor, so legacy rows can repeat and starve later rows.
- 🟡 MEDIUM — Existing resolved/replied threads can suppress a new post-ping bot error because the check does not establish that the activity occurred after the current ping.
The focused tests reported by reviewers passed, but the typecheck failure is a release blocker. Browser verification was attempted, but executor infrastructure had no browser-control tool available.
🧪 Browser testing: Will run after this review (PR touches UI code) — initial attempt was unavailable because browser-control tooling was missing.
| } | ||
| const page = eligible.slice(0, effectiveLimit); | ||
| const fairSorted = usesBabysitFairQueue | ||
| ? sortBabysitQueueRows(eligible, babysitQueueCursor) |
There was a problem hiding this comment.
🟡 Include older eligible rows in fair queue rotation
The fair sort and persisted cursor run only after the normal scan has collected a bounded newest window (up to 10 pages, typically 300 rows for the babysit limit). Older eligible PRs never become candidates, so a continuously updated newer backlog can still starve them despite the round-robin cursor. Apply the fair ordering while paginating or use a scan/cursor strategy that reaches the full eligible queue.
Additional Info
Reported independently by 3 of 4 review agents.
| @@ -764,15 +907,21 @@ export default defineAction({ | |||
| nextAuthor: pullRequest.userLogin, | |||
| existingBabysitState, | |||
| babysitReopened: reopenParked, | |||
There was a problem hiding this comment.
🟡 Clear terminal babysit metadata when a PR is reopened
When GitHub reports a previously merged or closed/draft PR as open and non-draft, this path restores pr_observed but can retain prBabysitState: "merged" or the other terminal state. The triage list then filters the row using the terminal metadata guard, leaving the reopened PR invisible to babysit. Clear terminal state and terminal-only metadata on the provider transition back to open.
Additional Info
Reported independently by 2 of 4 review agents.
There was a problem hiding this comment.
Fixed in 7ae1d44: buildPullRequestPollMetadataJson now clears terminal babysit metadata via reopenedFromTerminalBabysitMetadataPatch() when GitHub reports an open non-draft PR that still carries merged/closed-or-draft state.
There was a problem hiding this comment.
Still addressed in 7ae1d44 via reopenedFromTerminalBabysitMetadataPatch() in buildPullRequestPollMetadataJson; resolving this thread.
There was a problem hiding this comment.
Still addressed in 7ae1d44 via reopenedFromTerminalBabysitMetadataPatch(); resolving this thread.
|
Here's a visual recap of what changed: Open the full interactive recap |
7ed53ca to
7ae1d44
Compare
Required review items addressed (7ae1d44)
Deferred optional items
These are real edge cases but lower frequency than the required fixes; happy to tackle in a follow-up if you want them in this PR. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Incremental required fixes (db31971)
Prior required items remain in 7ae1d44. Still deferred (optional)
|
There was a problem hiding this comment.
Builder reviewed your changes and found 4 potential issues 🟡
Review Details
Incremental Code Review Summary
The latest commits improve the earlier implementation: the fair sorter now preserves the full triage-row type, GraphQL pagination completeness is retained, terminal rows are excluded from open-evidence rechecks, and terminal/draft reconciliation paths are covered by additional tests. I resolved the prior comments for terminal-row evidence skipping and draft terminalization after verifying those fixes. The previously open bounded-scan and reopened-terminal-metadata comments remain applicable and were not reposted.
New Findings
- 🟡 MEDIUM — Historical resolved/outdated review threads are still treated as evidence that the latest babysit ping was addressed, so a new Builder failure after the ping can bypass the
stucksafety path. - 🟡 MEDIUM — Poll-driven merged/closed/draft reconciliation updates the triage row but does not emit terminal audit evidence, so the new AuditView terminal copy is unavailable for the normal polling path.
- 🟡 MEDIUM — GraphQL responses containing both partial data and top-level errors are treated as complete review evidence.
- 🟡 MEDIUM — Tier-1 and tier-2 queue rows do not participate in cursor rotation, allowing a queue larger than the work limit to repeatedly return the same rows and starve later work.
These are standard-risk business-logic and observability concerns. Focused Factory tests reported by reviewers passed, although one run still encountered the existing Vitest teardown hang. Browser verification was attempted again, but browser-control tooling was unavailable to executors; the Factory route was separately confirmed reachable on a gateway configured with the Factory app.
🧪 Browser testing: Will run after this review (PR touches UI code) — this attempt was blocked by unavailable browser-control tooling.
| eq(triageItems.updatedAt, current.updatedAt), | ||
| ), | ||
| ); | ||
| updated += 1; |
There was a problem hiding this comment.
🟡 Record terminal reconciliation in the Factory audit
The poll-driven merged/closed/draft reconciliation updates the triage row but does not append a corresponding terminal audit event. Because these rows are excluded from the normal open-provider/newly-observed audit path, the AuditView cannot surface the new GitHub terminal disposition for transitions discovered by polling. Persist a per-item terminal audit event or extend the audit projection to consume the terminal reconciliation evidence.
Additional Info
Reported by 1 of 4 incremental review agents; included because it leaves the PR's primary audit-copy behavior unobservable on the normal polling path.
| if (leftTier === 3) { | ||
| return compareBabysitRoundRobinRows(left, right, cursor); | ||
| } | ||
| return compareBabysitQueueRows(left, right, nowMs); |
There was a problem hiding this comment.
🟡 Rotate tier-1 and tier-2 queue rows fairly
The fair sorter applies cursor rotation only to tiers 0 and 3; tier-1 and tier-2 rows fall back to stable updated-time ordering, and the cursor writer returns no cursor when a page contains only those tiers. If either tier exceeds the work limit, each run returns the same first slice and later eligible PRs starve. Apply cursor rotation and persist a cursor for every tier that can exceed the page limit.
Additional Info
Reported by 1 of 4 incremental review agents; included as a concrete starvation path distinct from the still-open bounded-scan comment.
Fair-queue nested factory automations, terminalize merged/closed PRs, and align audit/inbox dispositions for builder-active threads.
Preserve full triage row types through fair-queue sorting, return GraphQL comment completeness from evidence reads, and clear terminal babysit metadata when a merged or closed pull request reopens on GitHub.
Route draft and closed pull request rechecks through closedPullRequestKind before fetching open evidence, and skip terminal numbers in the post-poll recheck loop so one closed row cannot abort the transaction.
Only count Builder thread activity at or after the latest Factory ping when vetoing bot-error stuck recommendations, reject GraphQL partial-error payloads so evidence falls back to REST, and document intentional catch fallbacks for the no-silent-coercion guard.
db31971 to
4923278
Compare
Required review items addressed
Rebased onto current Optional items deferred
These are starvation/observability hardening beyond the required safety paths above. |
There was a problem hiding this comment.
Builder reviewed your changes and found 2 potential issues 🟡
Review Details
Incremental Code Review Summary
The latest commits address two prior concerns: GraphQL top-level partial errors now fall back to REST, and thread disposition evidence is time-bounded to post-ping activity. I resolved those two prior review threads. The remaining open queue and terminal-reconciliation comments were not reposted.
New Findings
- 🟡 MEDIUM — The persisted queue cursor is shared between tier 0 and tier 3. When one page contains both, the cursor is written using tier 0's
updatedAtbut later applied to tier 3'sprBabysitLastCheckedAt, so tier-3 rotation can repeatedly restart and starve backlog rows. - 🟡 MEDIUM — Newly observed draft PRs bypass the existing terminal reconciliation path because only existing recheck rows populate
closedRecheckUpdates. A first-seen draft can be inserted aspr_observed, enter review candidates, and consume a babysit slot until a later poll.
Reviewers reported focused suites passing (up to 84 tests), with one environment unable to run because pnpm was unavailable. Browser verification was attempted with a standalone Factory server and seeded data; backend action results were confirmed, but all visual cases remained unverifiable because browser-control tools were unavailable to executor sessions.
🧪 Browser testing: Will run after this review (PR touches UI code) — app reachability and seeded action data were confirmed, but visual execution remains blocked by missing browser tooling.
| return { | ||
| lastCheckedAt: | ||
| lastCheckedAtKey(lastTier3.metadataJson) || lastTier3.updatedAt, | ||
| lastId: lastTier3.id, |
There was a problem hiding this comment.
🟡 Keep queue cursors independent across tiers
nextBabysitQueueCursor returns the tier-0 row's updatedAt whenever a page contains any tier-0 work, even if the same page also contains tier-3 rows. The next sort applies that cursor to tier 3's prBabysitLastCheckedAt, mixing incompatible ordering keys and potentially restarting the tier-3 rotation so the same backlog row repeats while others starve. Persist the cursor tier or maintain independent cursors and advance the tier that supplied the page boundary.
Additional Info
Reported by 1 of 4 incremental review agents; this is distinct from the existing open bounded-scan and tier-1/2 fairness comments.
| const terminalKind = closedPullRequestKind(summary); | ||
| if (terminalKind) { | ||
| closedRecheckUpdates.set(number, { row, summary }); | ||
| return; |
There was a problem hiding this comment.
🟡 Terminalize newly observed draft pull requests
The terminal reconciliation map is populated only for existing recheck rows. A first-seen draft from the open-pull-request listing follows the normal insert path, where it is stored as pr_observed with only draft: true metadata. It can therefore enter needsReview candidates and consume a babysit slot until a later poll. Apply the draft terminal patch/status in the main ingest path as well, with coverage for a draft PR that has no existing triage row.
Additional Info
Reported by 1 of 4 incremental review agents; this is a new first-seen-draft path, distinct from the previously resolved existing-row draft reconciliation issue.
2 participants
Summary
Test plan
cd templates/factory && npx vitest --run server/triage/babysit-queue.spec.ts server/triage/babysit-pr-terminal.spec.ts actions/list-triage-items.spec.ts