fix(queue): thread prCreatedAt through the trailing mergeable-state re-check, and guard the rest in CI (#9551) - #9552
Conversation
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-28 09:07:06 UTC
Review summary Nits — 7 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | 81d0fbe | Commit Preview URL Branch Preview URL |
Jul 28 2026, 08:49 AM |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9552 +/- ##
=======================================
Coverage 89.60% 89.61%
=======================================
Files 861 861
Lines 110629 110629
Branches 26334 26335 +1
=======================================
+ Hits 99133 99135 +2
+ Misses 10231 10229 -2
Partials 1265 1265
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Bundle ReportBundle size has no change ✅ |
cebe127 to
6f98aaa
Compare
…e-check, and guard the rest in CI (#9551)
6f98aaa to
81d0fbe
Compare
Summary
Phase 1 of #9499: make the queue's one real oldest-first ordering mechanism actually work, before deciding whether an explicit focus gate (Phase 2) is still needed on top of it.
Closes #9551
The bug
jobClaimSortKeysortsagent-regate-prjobs by the PR's owncreatedAtascending — genuine oldest-first. But a producer that omitsprCreatedAtdoesn't merely lose the ordering: it falls back toLEGACY_AGENT_REGATE_SORT_BASE_MS + prNumber(≈9.5e11), which sorts ahead of every real 2026 PR (≈1.78e12). An omission actively inverts the ordering for that producer's jobs, and five of eight producers had drifted that way at the time of the original audit.Of the five: one genuine omission remained on
main.scheduleTrailingMergeableStateReReview— the #merge-race trailing re-check scheduled when a livemergeable_statecomes back"unknown"— never carriedprCreatedAt. The other four sites the audit flagged already carry it on currentmain, confirmed by direct inspection rather than by re-reading the original issue.prCreatedAtis now threaded through: the function takes an optionalprCreatedAtparam, and its one call site passespr.createdAt.The guard:
scripts/check-regate-sort-key.tsA type-level guard can't express this —
prCreatedAtis legitimately optional onJobMessage, since a producer that genuinely has no PR record must still be able to enqueue. So the check reads producer sites instead: everytype: "agent-regate-pr"object literal must carryprCreatedAt, or be explicitly named in an allowlist with a reason (the one legitimate exception is the maintainer-triggered manual re-gate route, which jumps the queue on purpose).A real false negative found while writing it, worth flagging on its own: an early version scanned a fixed line window past each
type:line. Two producers sitting a few lines apart let the first one'sprCreatedAtsatisfy the scan for the second one's — so deleting a field from one producer went uncaught. The scan is now bounded by the object literal's own closing brace (tracked via{/}depth), so each producer is judged strictly on its own literal. Pinned by a dedicated regression test with two adjacent producers, one clean, one not.Two corrections to the parent issue, both verified directly rather than assumed
mergeTrainModeis not dead code. The parent flagged doubt that it might be, based on asrc/-scoped grep finding no manifest override. The parser lives inpackages/loopover-engine/src/focus-manifest.ts:3160, outside that grep's scope. Verified end to end (parseFocusManifestContent→resolveEffectiveSettings): a repo.loopover.ymlsettings.mergeTrainMode: enforceresolves toenforce. Posted as a correction on #9499 so it isn't re-investigated.The coalesce-key claim for
maybeEnqueueSiblingRegateForMergedPris wrong, not just already-fixed.jobCoalesceKeyresolvesagent-regate-pr:${repo}#${prNumber}centrally, atsend(), for everyagent-regate-prmessage regardless of producer — a key that has existed since#1678, long before this audit series. No producer needs its own coalesce key; the "45 jobs for 15 siblings" scenario the parent describes does not occur. Posted as a correction on #9551.One item was genuinely already fixed (not a false claim):
isRegateRepairExhausted's budget keys onrepo#prNumber, no head SHA — confirmed directly atprocessors.ts:1136, with a comment citing the identical fix already applied to the fresh-rebase counter.Scope
Phase 2 (an explicit focus gate) is not here — the parent issue's own stated trade-off (strict FIFO is a behaviour change, not a tightening) means it needs a decision after Phase 1 lands and is measured, not a change bundled with this mechanical fix. #9499 stays open for it.
Validation
npx tsc --noEmit,regate-sort-key:check,dead-source-files:check,git diff --check— all cleanRegressions: the legacy fallback sorts strictly ahead of every real PR (the magnitude relationship that makes an omission a bug, not a lost optimization); the trailing re-check now carries
prCreatedAtand sorts by it, not the legacy base; the checker flags an omitting producer even when a neighbour's field would otherwise mask it.Invariants: the fixture with no
created_atdegrades to the legacy base exactly as before (the field is threaded, not forced); a failing enqueue never fails the webhook pass; a clean producer with the field passes; the allowlist is exact-file, not a blanket exemption; a file with no regate producer at all yields nothing.One arm is annotated rather than tested: the outer
.catch()at the trailing-recheck call site is unreachable, because the callee already catches its own enqueue failure and never rethrows.