diff --git a/src/settings/agent-sweep.ts b/src/settings/agent-sweep.ts index 5392088707..e4f1195677 100644 --- a/src/settings/agent-sweep.ts +++ b/src/settings/agent-sweep.ts @@ -152,12 +152,23 @@ export function selectRegateCandidates(input: { }; const hasRepairPriority = (pr: PullRequestRecord): boolean => priorityPullNumbers.has(pr.number); + // One-shot review, fail-closed (#never-endless-reregate, incident 2026-07-09): a PR the sweep has ALREADY + // regated even once is permanently ineligible for future sweep candidacy -- full stop, no re-check-for-drift + // window, no periodic revisit. This deliberately drops the "catch silent drift" behavior the sweep used to + // provide (a moved base, a merged sibling duplicate, a changed focus-manifest could previously go unnoticed + // until the next real push) -- a PR gets exactly one automatic review at a given head SHA. Re-review is + // opt-in only, through two channels neither of which is this sweep: (1) a genuinely new push stamps a new + // headSha and is handled entirely by the real-time webhook path, never this sort (see doc comment above); + // (2) an explicit maintainer-triggered re-review (the PR panel's re-run checkbox, role-gated, never + // identity-hardlocked) also runs through the webhook path, not the sweep. `hasRepairPriority` remains a + // narrow bypass: it means THIS PR's prior review never actually landed (a crashed/incomplete publish), so + // retrying it delivers the one review it was owed, not a second one. + const candidates = eligible.filter( + (pr) => !hasBeenRegated(pr) || hasRepairPriority(pr), + ); const oldestFirstInitialDrain = orderMode === "oldest-first" && - eligible.some((pr) => !hasBeenRegated(pr) && !hasRepairPriority(pr)); - const candidates = oldestFirstInitialDrain - ? eligible.filter((pr) => !hasBeenRegated(pr) || hasRepairPriority(pr)) - : eligible; + candidates.some((pr) => !hasBeenRegated(pr) && !hasRepairPriority(pr)); const orderKey = orderMode === "oldest-first" && oldestFirstInitialDrain ? creationOrder diff --git a/test/unit/agent-sweep.test.ts b/test/unit/agent-sweep.test.ts index 974e91102a..8506994ab0 100644 --- a/test/unit/agent-sweep.test.ts +++ b/test/unit/agent-sweep.test.ts @@ -57,7 +57,13 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { expect(picked.map((p) => p.number)).toEqual([]); // stalest by re-gate, but a webhook just touched it → skip }); - it("live case: when updatedAt and lastRegatedAt move together, the PR is eligible once outside the window (not double-excluded)", () => { + it("a never-regated PR is eligible once its updatedAt is outside the freshness window (not blocked by the guard alone)", () => { + const pulls = [pr({ number: 1, updatedAt: minutesAgo(120) })]; + const picked = selectRegateCandidates({ pulls, now: NOW }); + expect(picked.map((p) => p.number)).toEqual([1]); // old updatedAt, never regated → eligible + }); + + it("one-shot review (#never-endless-reregate): a PR already regated even once is excluded regardless of how old both timestamps are", () => { const pulls = [ pr({ number: 1, @@ -66,7 +72,7 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { }), ]; const picked = selectRegateCandidates({ pulls, now: NOW }); - expect(picked.map((p) => p.number)).toEqual([1]); // both old → freshness allows it, re-gate orders it + expect(picked.map((p) => p.number)).toEqual([]); // already regated once → never re-selected by the sweep }); it("keeps every open non-draft PR when `now` is unparseable (no freshness cutoff possible)", () => { @@ -85,9 +91,10 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { }); describe("convergence sort key (lastRegatedAt, NOT GitHub updatedAt)", () => { - it("INVARIANT arm (i): orders by lastRegatedAt ascending when present — the staler RE-GATE sorts first", () => { - // #1 was re-gated recently but created long ago; #2 was re-gated long ago but created recently. The re-gate - // marker — not createdAt — drives the order, so #2 (stalest re-gate) comes first. + it("INVARIANT arm (i): orders by lastRegatedAt ascending when present — the staler RE-GATE sorts first (among repair-eligible candidates; a plain already-regated PR is otherwise excluded, see #never-endless-reregate)", () => { + // Both PRs already have a regate stamp, so both need repairPriority to remain eligible at all post-#never- + // endless-reregate. #1 was re-gated recently but created long ago; #2 was re-gated long ago but created + // recently. The re-gate marker — not createdAt — drives the order, so #2 (stalest re-gate) comes first. const pulls = [ pr({ number: 1, @@ -100,7 +107,11 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { createdAt: minutesAgo(1), }), ]; - const picked = selectRegateCandidates({ pulls, now: NOW }); + const picked = selectRegateCandidates({ + pulls, + now: NOW, + priorityPullNumbers: new Set([1, 2]), + }); expect(picked.map((p) => p.number)).toEqual([2, 1]); }); @@ -119,39 +130,56 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { expect(picked.map((p) => p.number)).toEqual([4, 7, 9]); // all epoch → deterministic number order }); - it("a never-regated PR (lastRegatedAt absent) outranks a just-regated one — the property that makes the sweep converge", () => { + it("one-shot review (#never-endless-reregate): a just-regated PR is excluded outright, never merely outranked — only the never-regated PR remains", () => { const pulls = [ pr({ number: 1, lastRegatedAt: minutesAgo(1), createdAt: minutesAgo(1000), - }), // just re-gated → freshest - pr({ number: 2, createdAt: minutesAgo(50) }), // never re-gated → its createdAt (50m) is staler than #1's re-gate (1m) + }), // already regated once → permanently ineligible for the sweep, regardless of staleness + pr({ number: 2, createdAt: minutesAgo(50) }), // never regated → the only real candidate ]; const picked = selectRegateCandidates({ pulls, now: NOW }); - expect(picked.map((p) => p.number)).toEqual([2, 1]); + expect(picked.map((p) => p.number)).toEqual([2]); }); - it("bounds the batch to max (rate-aware) after ordering by re-gate staleness", () => { + it("bounds the batch to max (rate-aware) after ordering by re-gate staleness among repair-eligible candidates", () => { const pulls = [ pr({ number: 1, lastRegatedAt: minutesAgo(120) }), pr({ number: 2, lastRegatedAt: minutesAgo(600) }), pr({ number: 3, lastRegatedAt: minutesAgo(300) }), ]; - const picked = selectRegateCandidates({ pulls, now: NOW, max: 2 }); + const picked = selectRegateCandidates({ + pulls, + now: NOW, + max: 2, + priorityPullNumbers: new Set([1, 2, 3]), + }); expect(picked.map((p) => p.number)).toEqual([2, 3]); // stalest re-gate (600m), then 300m; 120m dropped by cap }); - it("#selfhost-fifo-ordering: a repair-flagged PR does NOT jump ahead of staler ordinary PRs — same orderKey for everyone", () => { - // #2 has a missing public surface (surfaceRepairPriorityPullNumbers would flag it) but is also the LEAST - // stale of the three by lastRegatedAt. An earlier revision sorted repair candidates first regardless of - // staleness — this pinned #2 ahead of #1/#3 and was observed live as PRs dispatching out of their - // creation/staleness order ("spraying") whenever a repo had a mixed repair/ordinary backlog. Repair status - // must only affect eligibility (see the freshness-bypass + oldest-first-pool tests below), never order. + it("one-shot review (#never-endless-reregate): bounds the batch to max among never-regated candidates (the ordinary, non-repair case)", () => { + const pulls = [ + pr({ number: 1, createdAt: minutesAgo(120) }), + pr({ number: 2, createdAt: minutesAgo(600) }), + pr({ number: 3, createdAt: minutesAgo(300) }), + ]; + const picked = selectRegateCandidates({ pulls, now: NOW, max: 2 }); + expect(picked.map((p) => p.number)).toEqual([2, 3]); // none ever regated → falls back to createdAt; oldest-created first, 120m dropped by cap + }); + + it("#selfhost-fifo-ordering: a repair-flagged PR does NOT jump ahead of staler ordinary (never-regated) PRs — same orderKey for everyone", () => { + // #1 and #3 have never been regated (the ordinary, post-#never-endless-reregate candidate shape). #2 has a + // missing public surface (surfaceRepairPriorityPullNumbers would flag it) and already has its own regate + // stamp from 10m ago — repair priority keeps it eligible despite that stamp, but its OWN regateProgress + // (10m, very fresh) correctly sorts it LAST, not ahead of the staler ordinary backlog. An earlier revision + // sorted repair candidates first regardless of staleness — this pinned repair work ahead of the ordinary + // backlog and was observed live as PRs dispatching out of their creation/staleness order ("spraying") + // whenever a repo had a mixed repair/ordinary backlog. Repair status must only affect eligibility, never order. const pulls = [ - pr({ number: 1, lastRegatedAt: minutesAgo(900) }), + pr({ number: 1, createdAt: minutesAgo(900) }), pr({ number: 2, lastRegatedAt: minutesAgo(10) }), - pr({ number: 3, lastRegatedAt: minutesAgo(800) }), + pr({ number: 3, createdAt: minutesAgo(800) }), ]; const picked = selectRegateCandidates({ pulls, @@ -159,7 +187,7 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { max: 2, priorityPullNumbers: new Set([2]), }); - expect(picked.map((p) => p.number)).toEqual([1, 3]); // stalest-by-regate first, same as with no priority set at all + expect(picked.map((p) => p.number)).toEqual([1, 3]); // oldest ordinary candidates first; repair-flagged #2 (freshly stamped) dropped by the cap }); it("REGRESSION (repair priority): priority repairs can bypass webhook freshness when the current Gate check is missing", () => { @@ -171,8 +199,10 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { }), pr({ number: 2, - updatedAt: minutesAgo(120), - lastRegatedAt: minutesAgo(800), + updatedAt: minutesAgo(120), // outside the freshness window on its own + createdAt: minutesAgo(50), // more recent than #1's 900m regate stamp, so it still sorts after #1 + // never regated (the ordinary, post-#never-endless-reregate candidate shape) — #1's own regate + // stamp above only stays eligible because of the repair-priority bypass being tested here. }), ]; const picked = selectRegateCandidates({ @@ -256,9 +286,10 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { expect(picked.map((p) => p.number)).toEqual([1, 2]); // #1 (oldest-created) first, unlike staleness (which has no history here either, so both modes agree in this case) }); - it("orders by re-gate staleness after the oldest-first initial drain is complete", () => { - // #1 was re-gated most recently (10m ago) despite being the OLDEST-created PR by far; #2 was re-gated - // longer ago (100m) despite being the NEWEST-created. Once every eligible PR has a lastRegatedAt stamp, + it("orders by re-gate staleness among repair-eligible candidates once every one of them has a stamp (an ordinary already-regated PR stays excluded, see #never-endless-reregate)", () => { + // Both PRs already have a regate stamp, so both need repairPriority to remain eligible at all. #1 was + // re-gated most recently (10m ago) despite being the OLDEST-created PR by far; #2 was re-gated longer ago + // (100m) despite being the NEWEST-created. Once every eligible (repair) PR has a lastRegatedAt stamp, // oldest-first uses re-gate staleness so ongoing sweeps keep converging instead of pinning old PRs. const pulls = [ pr({ @@ -276,14 +307,16 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { pulls, now: NOW, orderMode: "oldest-first", + priorityPullNumbers: new Set([1, 2]), }); expect(picked.map((p) => p.number)).toEqual([2, 1]); }); - it("does not starve the sweep when EVERY eligible PR ties on the same lastRegatedAt (a fully-covered small backlog)", () => { + it("does not starve the sweep when EVERY repair-eligible PR ties on the same lastRegatedAt (a fully-covered small backlog)", () => { // Both PRs were dispatched together in the exact same prior sweep (identical lastRegatedAt stamp — see - // markPullRequestsRegated, which stamps every candidate in one UPDATE). Since the initial drain is complete, - // oldest-first falls back to the full staleness pool rather than returning nothing. + // markPullRequestsRegated, which stamps every candidate in one UPDATE) and need repairPriority to remain + // eligible post-#never-endless-reregate. Since the initial drain is complete, oldest-first falls back to + // the full (repair) pool rather than returning nothing. const pulls = [ pr({ number: 1, @@ -300,6 +333,7 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { pulls, now: NOW, orderMode: "oldest-first", + priorityPullNumbers: new Set([1, 2]), }); expect(picked.map((p) => p.number)).toEqual([1, 2]); // both tie → guard proceeds with the full pool, oldest first }); @@ -436,7 +470,7 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { expect(covered.size).toBe(open); }); - it("falls back to re-gate staleness once every eligible PR has been swept once", () => { + it("falls back to re-gate staleness among repair-eligible candidates once every one of them has been swept once", () => { const pulls = [ pr({ number: 1, @@ -458,9 +492,23 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { pulls, now: NOW, orderMode: "oldest-first", + priorityPullNumbers: new Set([1, 2, 3]), }); expect(picked.map((p) => p.number)).toEqual([2, 3, 1]); }); + + it("one-shot review (#never-endless-reregate): an ordinary (non-repair) already-regated PR is excluded outright under oldest-first too", () => { + const pulls = [ + pr({ number: 1, createdAt: minutesAgo(1000), lastRegatedAt: minutesAgo(5) }), + pr({ number: 2, createdAt: minutesAgo(900) }), // never regated → the only real candidate + ]; + const picked = selectRegateCandidates({ + pulls, + now: NOW, + orderMode: "oldest-first", + }); + expect(picked.map((p) => p.number)).toEqual([2]); + }); }); }); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 141c848abb..db088ce57d 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -7311,15 +7311,13 @@ describe("queue processors", () => { } } await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 5, title: "Draft without a head", state: "open", draft: true, user: { login: "c" }, labels: [], body: "" } as never); + // Only PR2 gets a regate stamp (post-#never-endless-reregate, an ordinary already-regated PR is permanently + // excluded from the sweep -- see agent-sweep.test.ts -- so PR1/3/4 must stay never-regated to remain eligible + // ordinary candidates at all). PR2 is missing its current Gate check (surfaceRepairPriorityPullNumbers would + // flag it as a repair candidate), so its repair-priority bypass keeps it eligible DESPITE already having a + // stamp -- this is exactly the scenario the repair-priority bypass exists for. await env.DB.prepare( - `update pull_requests - set last_regated_at = case number - when 1 then '2026-05-27T01:00:00.000Z' - when 2 then '2026-05-28T01:50:00.000Z' - when 3 then '2026-05-27T02:00:00.000Z' - when 4 then '2026-05-27T03:00:00.000Z' - end - where repo_full_name = ?`, + `update pull_requests set last_regated_at = '2026-05-28T01:50:00.000Z' where repo_full_name = ? and number = 2`, ) .bind("owner/agent-repo") .run(); @@ -7328,14 +7326,14 @@ describe("queue processors", () => { await processJob(env, { type: "agent-regate-sweep", requestedBy: "test", repoFullName: "owner/agent-repo" }); const fanned = sent.filter((job) => job.type === "agent-regate-pr"); - // PR2 is missing its current Gate check (surfaceRepairPriorityPullNumbers would flag it as a repair - // candidate) but is also the LEAST stale by lastRegatedAt (10 min ago vs. 23-25h for the others). An earlier - // revision sorted repair candidates first regardless of staleness, jumping PR2 to the front of this batch -- - // that let a PR needing repair cut ahead of older PRs that merely went stale, observed live as PRs - // dispatching out of order ("spraying") whenever a repo had a mixed repair/ordinary backlog. Repair status - // now only affects ELIGIBILITY (staying in the pool, bypassing the freshness guard), never final order, so - // PR2 takes its rightful (last, since it's the freshest-regated) place and is dropped by the max:3 cap this - // round -- same as it would be with no repair flag at all. + // PR2 is missing its current Gate check and already has a regate stamp from 10 min ago (very fresh by + // lastRegatedAt), while PR1/3/4 have never been regated at all (the ordinary, post-#never-endless-reregate + // candidate shape). An earlier revision sorted repair candidates first regardless of staleness, jumping PR2 + // to the front of this batch -- that let a PR needing repair cut ahead of older PRs that merely went stale, + // observed live as PRs dispatching out of order ("spraying") whenever a repo had a mixed repair/ordinary + // backlog. Repair status only affects ELIGIBILITY (staying in the pool despite already having a stamp), + // never final order, so PR2 takes its rightful (last, since it's the freshest-regated) place and is dropped + // by the max:3 cap this round -- same as it would be with no repair flag at all. expect(fanned.map((job) => (job as Extract).prNumber)).toEqual([1, 3, 4]); });