Skip to content

Commit 3be31d3

Browse files
authored
fix(queue): thread prCreatedAt through the trailing mergeable-state re-check, and guard the rest in CI (#9551) (#9552)
1 parent f845b85 commit 3be31d3

7 files changed

Lines changed: 409 additions & 3 deletions

File tree

.github/workflows/ci.yml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,12 @@ jobs:
370370
- name: Dead source-file check
371371
if: ${{ github.event_name == 'push' || needs.changes.outputs.backend == 'true' }}
372372
run: npm run dead-source-files:check
373+
# #9499: an agent-regate-pr producer that omits prCreatedAt does not merely lose the oldest-first
374+
# ordering, it INVERTS it -- the legacy sort fallback places such a job ahead of every real PR. Five of
375+
# eight producers had drifted that way before this check existed.
376+
- name: Re-gate sort-key check
377+
if: ${{ github.event_name == 'push' || needs.changes.outputs.backend == 'true' }}
378+
run: npm run regate-sort-key:check
373379
# Mechanical drift tripwire for the hand-duplicated src/{review,settings,signals} <-> loopover-engine
374380
# twin files, plus a version-skew check on the installed @loopover/engine (#4260). Same
375381
# local-only-until-now gap as the drift checks above. Also gated on `miner`: the reverse-direction

package.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@
7272
"validate:no-hand-written-js": "node --experimental-strip-types scripts/validate-no-hand-written-js.ts",
7373
"import-specifiers:check": "node --experimental-strip-types scripts/check-import-specifiers.ts",
7474
"dead-source-files:check": "node --experimental-strip-types scripts/check-dead-source-files.ts",
75+
"regate-sort-key:check": "node --experimental-strip-types scripts/check-regate-sort-key.ts",
7576
"replay-runner-manifest": "tsx scripts/replay-runner-image-manifest.ts",
7677
"replay-runner-manifest:write": "tsx scripts/replay-runner-image-manifest.ts --write",
7778
"replay-runner-manifest:check": "tsx scripts/replay-runner-image-manifest.ts --check",
@@ -116,7 +117,7 @@
116117
"test:smoke:browser:install": "playwright install chromium",
117118
"test:smoke:browser": "node --experimental-strip-types scripts/smoke-ui-browser.ts",
118119
"pretest:ci": "npm run check-node-version",
119-
"test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run ui:openapi:settings-parity && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run dead-source-files:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run command-reference:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build",
120+
"test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run ui:openapi:settings-parity && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run dead-source-files:check && npm run regate-sort-key:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run command-reference:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build",
120121
"test:release": "npm run test:ci && npm run changelog:check",
121122
"test:release:mcp": "npm run test:ci",
122123
"test:watch": "vitest",

scripts/check-regate-sort-key.ts

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
#!/usr/bin/env node
2+
// #9499: every `agent-regate-pr` producer must carry `prCreatedAt`.
3+
//
4+
// `jobClaimSortKey` (src/selfhost/queue-common.ts) sorts regate jobs by the PR's own `createdAt` ascending —
5+
// the ONE real oldest-first ordering mechanism the queue has. A producer that omits `prCreatedAt` falls back
6+
// to `LEGACY_AGENT_REGATE_SORT_BASE_MS + prNumber` (~9.5e11), which sorts AHEAD of every genuinely older 2026
7+
// PR (~1.78e12). So an omission does not degrade the ordering — it INVERTS it, silently, for that producer's
8+
// jobs, and five of eight producers had done exactly that.
9+
//
10+
// A type-level guard cannot express this: `prCreatedAt` is legitimately optional on `JobMessage` (a producer
11+
// that truly has no PR record must still be able to enqueue), so making it required would break the
12+
// deliberate exceptions rather than catch the accidental ones. This check reads the producer sites instead
13+
// and requires each to either pass the field or be explicitly allowlisted with a reason — the same
14+
// "an exception must be stated, not inferred from absence" shape as check-dead-source-files.ts's entry points.
15+
import { readFileSync, readdirSync } from "node:fs";
16+
import { fileURLToPath } from "node:url";
17+
18+
const SCAN_ROOTS = ["src"] as const;
19+
const SOURCE_PATTERN = /(?<!\.d)\.ts$/;
20+
const EXCLUDED_SEGMENT = /(?:^|\/)(?:node_modules|dist|dist-test)(?:\/|$)/;
21+
22+
/** Hard ceiling on how far a producer's object literal may be scanned, purely so a malformed/unbalanced file
23+
* cannot make this walk the rest of the module. The real bound is the literal's own closing brace — see
24+
* {@link producerObjectText}. */
25+
const PRODUCER_SCAN_CEILING_LINES = 60;
26+
27+
/**
28+
* Producers that deliberately omit `prCreatedAt`, each with the reason. Keyed `file:marker`, where the marker
29+
* is a distinctive substring of the producer's own `deliveryId` so the entry survives line-number churn.
30+
*/
31+
const ALLOWED_OMISSIONS: ReadonlyMap<string, string> = new Map([
32+
[
33+
"src/api/routes.ts:manual-regate:",
34+
"The maintainer-triggered manual re-gate route enqueues at priority 99 to jump the queue ON PURPOSE — an operator asking for one PR now is exactly the case oldest-first should not apply to. It also has no PR record in hand (the body carries only repoFullName + prNumber).",
35+
],
36+
]);
37+
38+
export type RegateSortKeyViolation = { file: string; line: number; snippet: string };
39+
40+
function defaultListSourceFiles(root: string): string[] {
41+
try {
42+
return readdirSync(root, { recursive: true })
43+
.map(String)
44+
.filter((entry) => SOURCE_PATTERN.test(entry) && !EXCLUDED_SEGMENT.test(entry))
45+
.map((entry) => `${root}/${entry}`);
46+
} catch {
47+
return [];
48+
}
49+
}
50+
51+
/**
52+
* Pure over its inputs: finds every `type: "agent-regate-pr"` producer whose enqueued object does not carry
53+
* `prCreatedAt` within the following {@link PRODUCER_WINDOW_LINES} lines, minus the allowlisted exceptions.
54+
* `listSourceFiles`/`readFile` are injectable so tests can simulate a fresh offender without touching the tree.
55+
*/
56+
export function findRegateSortKeyViolations(
57+
options: {
58+
roots?: readonly string[];
59+
listSourceFiles?: (root: string) => string[];
60+
readFile?: (file: string) => string;
61+
allowedOmissions?: ReadonlyMap<string, string>;
62+
} = {},
63+
): RegateSortKeyViolation[] {
64+
const {
65+
roots = SCAN_ROOTS,
66+
listSourceFiles = defaultListSourceFiles,
67+
readFile = (file: string) => readFileSync(file, "utf8"),
68+
allowedOmissions = ALLOWED_OMISSIONS,
69+
} = options;
70+
71+
const violations: RegateSortKeyViolation[] = [];
72+
for (const root of roots) {
73+
for (const file of listSourceFiles(root)) {
74+
const lines = readFile(file).split("\n");
75+
for (const [index, line] of lines.entries()) {
76+
if (!line.includes('type: "agent-regate-pr"')) continue;
77+
const window = producerObjectText(lines, index);
78+
if (window.includes("prCreatedAt")) continue;
79+
const allowed = [...allowedOmissions.keys()].some((key) => {
80+
const [allowedFile, marker] = splitAllowKey(key);
81+
return allowedFile === file && marker !== "" && window.includes(marker);
82+
});
83+
if (allowed) continue;
84+
violations.push({ file, line: index + 1, snippet: line.trim() });
85+
}
86+
}
87+
}
88+
return violations.sort((a, b) => (a.file === b.file ? a.line - b.line : a.file.localeCompare(b.file)));
89+
}
90+
91+
/**
92+
* The text of the object literal that OWNS the `type: "agent-regate-pr"` line at `startIndex`, bounded by that
93+
* literal's own closing brace rather than a fixed line count.
94+
*
95+
* A fixed window is subtly wrong here and produced a real false negative while this check was being written:
96+
* two producers sitting within a few lines of each other let the FIRST one's `prCreatedAt` satisfy the scan
97+
* for the SECOND one's, so removing a field from one of them was not caught. Tracking brace depth means each
98+
* producer is judged on its own literal and nothing else.
99+
*/
100+
function producerObjectText(lines: readonly string[], startIndex: number): string {
101+
const collected: string[] = [];
102+
let depth = 0;
103+
for (let i = startIndex; i < Math.min(lines.length, startIndex + PRODUCER_SCAN_CEILING_LINES); i += 1) {
104+
const line = lines[i] ?? "";
105+
collected.push(line);
106+
for (const char of line) {
107+
if (char === "{") depth += 1;
108+
else if (char === "}") depth -= 1;
109+
}
110+
// Depth goes negative at the `}` that closes the literal this `type:` line sits inside — that line is the
111+
// last one belonging to this producer.
112+
if (depth < 0) break;
113+
}
114+
return collected.join("\n");
115+
}
116+
117+
/** Split `path/to/file.ts:marker-text` on the LAST colon that precedes the marker — a marker may itself
118+
* contain colons (`manual-regate:`), so a naive split on the first or last colon gets it wrong. */
119+
function splitAllowKey(key: string): [string, string] {
120+
const boundary = key.indexOf(".ts:");
121+
if (boundary === -1) return [key, ""];
122+
return [key.slice(0, boundary + ".ts".length), key.slice(boundary + ".ts:".length)];
123+
}
124+
125+
function main(): void {
126+
const violations = findRegateSortKeyViolations();
127+
if (violations.length === 0) {
128+
process.stdout.write("agent-regate-pr sort keys: OK\n");
129+
return;
130+
}
131+
process.stderr.write(`Found ${violations.length} agent-regate-pr producer(s) missing prCreatedAt (#9499):\n`);
132+
for (const violation of violations) {
133+
process.stderr.write(` ${violation.file}:${violation.line}${violation.snippet}\n`);
134+
}
135+
process.stderr.write(
136+
"\nAn omitted prCreatedAt does not merely lose the ordering — it INVERTS it: jobClaimSortKey falls back to\n" +
137+
"LEGACY_AGENT_REGATE_SORT_BASE_MS + prNumber (~9.5e11), which sorts ahead of every real 2026 PR (~1.78e12).\n" +
138+
"Pass the PR's createdAt, or — if the producer genuinely must jump the queue — add it to ALLOWED_OMISSIONS\n" +
139+
"in scripts/check-regate-sort-key.ts with the reason.\n",
140+
);
141+
process.exit(1);
142+
}
143+
144+
if (process.argv[1] === fileURLToPath(import.meta.url)) main();

src/queue/processors.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3911,7 +3911,10 @@ async function runAgentMaintenancePlanAndExecute(
39113911
// reproduce, so they're deliberately NOT retried here. Best-effort: an enqueue failure here must never
39123912
// fail this webhook pass, matching every other trailing-schedule call site in this file.
39133913
if ((liveMergeState ?? pr.mergeableState) === "unknown") {
3914-
await scheduleTrailingMergeableStateReReview(env, deliveryId, installationId, repoFullName, pr.number).catch(() => undefined);
3914+
/* v8 ignore next -- scheduleTrailingMergeableStateReReview already catches internally (its own
3915+
enqueue-failure branch logs and returns without rethrowing), so this outer .catch is defensive only:
3916+
unreachable without a throw inside its OWN try, which nothing here can induce. */
3917+
await scheduleTrailingMergeableStateReReview(env, deliveryId, installationId, repoFullName, pr.number, pr.createdAt).catch(() => undefined);
39153918
}
39163919
}
39173920
if (holdoutOnPlan.length === 0) {
@@ -5098,6 +5101,10 @@ async function scheduleTrailingMergeableStateReReview(
50985101
installationId: number,
50995102
repoFullName: string,
51005103
prNumber: number,
5104+
/** #9499: the PR's own createdAt, so this trailing re-review keeps its place in the oldest-first ordering.
5105+
* Optional only because a caller without the record in hand degrades to the legacy sort base exactly as
5106+
* before -- every present caller has it. */
5107+
prCreatedAt?: string | null | undefined,
51015108
): Promise<void> {
51025109
const key = `merge-state-unknown-trailing:${repoFullName.toLowerCase()}#${prNumber}`;
51035110
// Same check-then-claim-only-after-send shape as scheduleTrailingIssueLinkedReReview above (#2371 follow-up
@@ -5106,7 +5113,7 @@ async function scheduleTrailingMergeableStateReReview(
51065113
if (await getTransientKey(env, key)) return;
51075114
try {
51085115
await env.JOBS.send(
5109-
{ type: "agent-regate-pr", deliveryId, repoFullName, prNumber, installationId },
5116+
{ type: "agent-regate-pr", deliveryId, repoFullName, prNumber, installationId, ...(prCreatedAt ? { prCreatedAt } : {}) },
51105117
{ delaySeconds: MERGE_STATE_UNKNOWN_TRAILING_RECHECK_DELAY_SECONDS },
51115118
);
51125119
} catch (error) {
Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
import { describe, expect, it } from "vitest";
2+
import { findRegateSortKeyViolations } from "../../scripts/check-regate-sort-key";
3+
4+
/** Simulates a tree without touching the real one. Mirrors check-import-specifiers-script.test.ts's helper. */
5+
function fakeFiles(byFile: Record<string, string>) {
6+
const files = Object.keys(byFile);
7+
return {
8+
listSourceFiles: (root: string) => files.filter((f) => f.startsWith(`${root}/`)),
9+
readFile: (file: string) => byFile[file] ?? "",
10+
};
11+
}
12+
13+
// #9499: jobClaimSortKey sorts agent-regate-pr jobs by the PR's own createdAt ascending — the one real
14+
// oldest-first mechanism the queue has. A producer omitting prCreatedAt falls back to
15+
// LEGACY_AGENT_REGATE_SORT_BASE_MS + prNumber (~9.5e11), which sorts AHEAD of every genuinely older 2026 PR
16+
// (~1.78e12). So an omission does not degrade the ordering, it INVERTS it — silently, for that producer only.
17+
describe("check-regate-sort-key script", () => {
18+
it("REGRESSION: flags a producer that omits prCreatedAt", () => {
19+
const violations = findRegateSortKeyViolations({
20+
roots: ["src"],
21+
allowedOmissions: new Map(),
22+
...fakeFiles({
23+
"src/queue/thing.ts": `await env.JOBS.send({\n type: "agent-regate-pr",\n deliveryId,\n repoFullName,\n prNumber,\n});\n`,
24+
}),
25+
});
26+
expect(violations).toEqual([{ file: "src/queue/thing.ts", line: 2, snippet: 'type: "agent-regate-pr",' }]);
27+
});
28+
29+
it("INVARIANT: a producer that passes prCreatedAt is clean, in either spread or plain form", () => {
30+
const violations = findRegateSortKeyViolations({
31+
roots: ["src"],
32+
allowedOmissions: new Map(),
33+
...fakeFiles({
34+
"src/a.ts": `send({\n type: "agent-regate-pr",\n prNumber,\n ...(pr.createdAt ? { prCreatedAt: pr.createdAt } : {}),\n});\n`,
35+
"src/b.ts": `send({\n type: "agent-regate-pr",\n prNumber,\n prCreatedAt,\n});\n`,
36+
}),
37+
});
38+
expect(violations).toEqual([]);
39+
});
40+
41+
// The false negative this check actually had while being written: a fixed-line window let one producer's
42+
// field satisfy the scan for a NEIGHBOURING producer's omission, so removing a field was not caught. The
43+
// scan is bounded by the literal's own closing brace precisely so each producer is judged on its own.
44+
it("REGRESSION: a NEIGHBOURING producer's prCreatedAt does not mask this one's omission", () => {
45+
const source = [
46+
"const good = {",
47+
' type: "agent-regate-pr",',
48+
" prNumber,",
49+
" ...(pr.createdAt ? { prCreatedAt: pr.createdAt } : {}),",
50+
"};",
51+
"const bad = {",
52+
' type: "agent-regate-pr",',
53+
" prNumber,",
54+
"};",
55+
"",
56+
].join("\n");
57+
const violations = findRegateSortKeyViolations({
58+
roots: ["src"],
59+
allowedOmissions: new Map(),
60+
...fakeFiles({ "src/two.ts": source }),
61+
});
62+
expect(violations).toEqual([{ file: "src/two.ts", line: 7, snippet: 'type: "agent-regate-pr",' }]);
63+
});
64+
65+
it("INVARIANT: the reverse order also holds — a good producer AFTER a bad one does not rescue it", () => {
66+
const source = [
67+
"const bad = {",
68+
' type: "agent-regate-pr",',
69+
" prNumber,",
70+
"};",
71+
"const good = {",
72+
' type: "agent-regate-pr",',
73+
" prNumber,",
74+
" prCreatedAt,",
75+
"};",
76+
"",
77+
].join("\n");
78+
const violations = findRegateSortKeyViolations({
79+
roots: ["src"],
80+
allowedOmissions: new Map(),
81+
...fakeFiles({ "src/two.ts": source }),
82+
});
83+
expect(violations).toEqual([{ file: "src/two.ts", line: 2, snippet: 'type: "agent-regate-pr",' }]);
84+
});
85+
86+
it("INVARIANT: an allowlisted deliberate omission is not flagged, and the allowlist is matched by MARKER not line number", () => {
87+
// Keyed on a distinctive deliveryId substring so the entry survives ordinary line churn — an
88+
// allowlist keyed on position would silently stop applying (or start applying to the wrong producer)
89+
// the first time someone edited the file above it.
90+
const violations = findRegateSortKeyViolations({
91+
roots: ["src"],
92+
allowedOmissions: new Map([["src/api/routes.ts:manual-regate:", "operator-triggered, jumps the queue on purpose"]]),
93+
...fakeFiles({
94+
"src/api/routes.ts": `const message = {\n type: "agent-regate-pr",\n deliveryId: \`manual-regate:\${id}\`,\n prNumber,\n};\n`,
95+
}),
96+
});
97+
expect(violations).toEqual([]);
98+
});
99+
100+
it("INVARIANT: the allowlist is scoped to its own FILE — the same marker elsewhere is still flagged", () => {
101+
const violations = findRegateSortKeyViolations({
102+
roots: ["src"],
103+
allowedOmissions: new Map([["src/api/routes.ts:manual-regate:", "reason"]]),
104+
...fakeFiles({
105+
"src/queue/copycat.ts": `const message = {\n type: "agent-regate-pr",\n deliveryId: \`manual-regate:\${id}\`,\n prNumber,\n};\n`,
106+
}),
107+
});
108+
expect(violations).toHaveLength(1);
109+
expect(violations[0]?.file).toBe("src/queue/copycat.ts");
110+
});
111+
112+
it("INVARIANT: a file with no regate producer at all yields nothing", () => {
113+
const violations = findRegateSortKeyViolations({
114+
roots: ["src"],
115+
allowedOmissions: new Map(),
116+
...fakeFiles({ "src/unrelated.ts": `send({ type: "recapture-preview", prNumber });\n` }),
117+
});
118+
expect(violations).toEqual([]);
119+
});
120+
121+
it("reports violations sorted by file then line, so failure output is stable", () => {
122+
const bad = `send({\n type: "agent-regate-pr",\n prNumber,\n});\n`;
123+
const violations = findRegateSortKeyViolations({
124+
roots: ["src"],
125+
allowedOmissions: new Map(),
126+
...fakeFiles({ "src/z.ts": bad, "src/a.ts": bad }),
127+
});
128+
expect(violations.map((violation) => violation.file)).toEqual(["src/a.ts", "src/z.ts"]);
129+
});
130+
131+
it("the REAL repo tree is clean — this check runs in CI and must stay green", () => {
132+
expect(findRegateSortKeyViolations()).toEqual([]);
133+
});
134+
});

0 commit comments

Comments
 (0)