Skip to content

fix(queue): thread prCreatedAt through the trailing mergeable-state re-check, and guard the rest in CI (#9551) - #9552

Merged
JSONbored merged 1 commit into
mainfrom
fix/regate-ordering
Jul 28, 2026
Merged

fix(queue): thread prCreatedAt through the trailing mergeable-state re-check, and guard the rest in CI (#9551)#9552
JSONbored merged 1 commit into
mainfrom
fix/regate-ordering

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

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

jobClaimSortKey sorts agent-regate-pr jobs by the PR's own createdAt ascending — genuine oldest-first. But a producer that omits prCreatedAt doesn't merely lose the ordering: it falls back to LEGACY_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 live mergeable_state comes back "unknown" — never carried prCreatedAt. The other four sites the audit flagged already carry it on current main, confirmed by direct inspection rather than by re-reading the original issue.

prCreatedAt is now threaded through: the function takes an optional prCreatedAt param, and its one call site passes pr.createdAt.

The guard: scripts/check-regate-sort-key.ts

A type-level guard can't express this — prCreatedAt is legitimately optional on JobMessage, since a producer that genuinely has no PR record must still be able to enqueue. So the check reads producer sites instead: every type: "agent-regate-pr" object literal must carry prCreatedAt, 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's prCreatedAt satisfy 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

mergeTrainMode is not dead code. The parent flagged doubt that it might be, based on a src/-scoped grep finding no manifest override. The parser lives in packages/loopover-engine/src/focus-manifest.ts:3160, outside that grep's scope. Verified end to end (parseFocusManifestContentresolveEffectiveSettings): a repo .loopover.yml settings.mergeTrainMode: enforce resolves to enforce. Posted as a correction on #9499 so it isn't re-investigated.

The coalesce-key claim for maybeEnqueueSiblingRegateForMergedPr is wrong, not just already-fixed. jobCoalesceKey resolves agent-regate-pr:${repo}#${prNumber} centrally, at send(), for every agent-regate-pr message 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 on repo#prNumber, no head SHA — confirmed directly at processors.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 clean
  • 616 passed across the checker, queue-common, lifecycle-guards, and queue-2/3 suites
  • Patch coverage against this diff: 0 uncovered changed lines

Regressions: 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 prCreatedAt and 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_at degrades 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.

@loopover-orb

loopover-orb Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-28 09:07:06 UTC

7 files · 1 AI reviewer · no blockers · CI green · clean

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review: This PR changes guardrail-protected path(s): .github/workflows/ci.yml (matched .github/workflows/**).

Review summary
This PR threads a `prCreatedAt` param through `scheduleTrailingMergeableStateReReview` so the trailing merge-race re-check keeps its place in `jobClaimSortKey`'s oldest-first ordering, and adds a CI script that scans producer sites for missing `prCreatedAt` (with a brace-depth-bounded scan to avoid the false-negative the author found while writing it). The change is narrow, well-tested (including a regression test proving the legacy fallback inverts rather than degrades ordering), and directly closes issue #9551. The fail-open `.catch(() => undefined)` at the sole call site is preserved, matching the file's existing convention for this call.

Nits — 7 non-blocking
  • scripts/check-regate-sort-key.ts:79 nests to depth 5 in `producerObjectText`'s loop; could flatten the char-loop into a small helper for readability.
  • The `PRODUCER_SCAN_CEILING_LINES = 60` ceiling in scripts/check-regate-sort-key.ts is a silent cutoff — a producer object literal spanning more than 60 lines would be scanned incompletely and could produce a false negative with no warning; consider logging when the ceiling is hit without finding a closing brace.
  • scripts/check-regate-sort-key.ts's stale doc comment above `findRegateSortKeyViolations` references `{@​link PRODUCER_WINDOW_LINES}`, a constant that doesn't exist (it's `PRODUCER_SCAN_CEILING_LINES`).
  • In scripts/check-regate-sort-key.ts, rename or fix the doc comment's `{@​link PRODUCER_WINDOW_LINES}` reference to `PRODUCER_SCAN_CEILING_LINES` to avoid confusing future readers.
  • Consider emitting a distinct warning line when a producer's object literal exceeds `PRODUCER_SCAN_CEILING_LINES` without closing, so a legitimately large future producer doesn't silently escape the check.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ⚠️ Gate result — Not blocking (Advisory; not blocking this PR.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #9551
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ❌ 8/20 High review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 14 registered-repo PR(s), 13 merged, 317 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 14 PR(s), 317 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Linked issue satisfaction

Addressed
The PR threads prCreatedAt through scheduleTrailingMergeableStateReReview (the one genuine omission identified), adds scripts/check-regate-sort-key.ts with brace-depth scanning and a file-scoped allowlist as specified, wires it into CI and test:ci, and includes tests covering the fallback-inversion magnitude, the fixed-window false negative, and allowlist exactness — matching every deliverable and

Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, Ruby, Go, MDX, Shell, Solidity, JavaScript
  • Official Gittensor activity: 14 PR(s), 317 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Then work through the remaining 2 steps in the Signals table above.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask 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.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

Decision record
  • action: hold · clause: guardrail_hold
  • config: 38baf1c9edc564b20b87438206f5f5080eee317e0d6744262ac2b342abab3930 · pack: oss-anti-slop · ci: passed
  • record: c43c2f463f3c9061dfa1aa3fa11efb310787cbd7efef21c7724213bff04b5884 (schema v5, head 81d0fbe)

🟩 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.

  • Re-run LoopOver review

@JSONbored JSONbored self-assigned this Jul 28, 2026
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 28, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

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

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.61%. Comparing base (9713f26) to head (81d0fbe).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

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           
Flag Coverage Δ
backend 95.31% <100.00%> (+<0.01%) ⬆️
control-plane 99.86% <ø> (ø)
rees 89.62% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/queue/processors.ts 94.89% <100.00%> (+0.05%) ⬆️

@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Bundle Report

Bundle size has no change ✅

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 28, 2026
@JSONbored
JSONbored force-pushed the fix/regate-ordering branch from cebe127 to 6f98aaa Compare July 28, 2026 08:28
@JSONbored
JSONbored force-pushed the fix/regate-ordering branch from 6f98aaa to 81d0fbe Compare July 28, 2026 08:46
@JSONbored
JSONbored merged commit 3be31d3 into main Jul 28, 2026
9 checks passed
@JSONbored
JSONbored deleted the fix/regate-ordering branch July 28, 2026 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

queue: make jobClaimSortKey's oldest-first ordering actually work (Phase 1 of #9499) — one omission fixed, a CI guard for the rest

1 participant