From 3c7dd35fe1b05ad477c26d292509a34ac14cc6df Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 30 Jul 2026 13:08:09 -0700 Subject: [PATCH] feat(gate): tell a queued PR its position in the merge train, not just the PR ahead of it The wait comment named the immediate blocker and nothing else: Queued in the merge train behind #4, which touches overlapping work and was opened first. "Behind #4" and "behind #4 and six others" are very different waits, and a queue that will not say which one it is reads as a stall rather than a wait -- which is how it was read in practice. The information was already computed and thrown away: shouldWaitForOlderSiblings builds the full sorted list of viable overlapping older siblings and returned only viable[0]. It now returns the whole list, and the comment reports the position: Queued in the merge train at position 4, behind 3 overlapping PRs opened before this one (#1, #2, #3). The nearest is #1. The single-blocker wording is unchanged -- rendering a one-item queue as "1 PR ahead: #4" is noise. The queue inherits every eviction rule the blocker choice already had, which the tests pin: a draft, a conflicted PR, a manual-review hold and a non-overlapping sibling are all absent from it. A contributor must never be told they are behind PRs that are not actually in front of them. Closes #9952 --- src/review/merge-train.ts | 16 +++++- src/services/agent-action-executor.ts | 12 +++- test/unit/agent-action-executor.test.ts | 19 +++++++ test/unit/merge-train.test.ts | 75 ++++++++++++++++++------- 4 files changed, 99 insertions(+), 23 deletions(-) diff --git a/src/review/merge-train.ts b/src/review/merge-train.ts index c14eea9ba2..b3e16c8ae0 100644 --- a/src/review/merge-train.ts +++ b/src/review/merge-train.ts @@ -70,7 +70,19 @@ export type MergeTrainSibling = { * indefinitely, so this is the escape hatch, not a tight SLA. */ export const MERGE_TRAIN_MAX_WAIT_MS = 24 * 60 * 60 * 1000; -export type MergeTrainDecision = { wait: true; blockingPr: number } | { wait: false }; +export type MergeTrainDecision = + | { + wait: true; + /** The immediate blocker: the OLDEST viable overlapping sibling. */ + blockingPr: number; + /** Every viable overlapping sibling ahead of this PR, oldest first -- `blockingPr` is its first entry. + * Surfaced so the contributor-facing wait comment can say WHERE in the train this PR sits rather than + * only naming the one PR in front of it: "behind #4" and "behind #4, and 6 others" are very different + * waits, and a queue that will not say which one it is reads as a stall. Length is the position minus + * one, so `queueAhead.length + 1` is this PR's own place in line. */ + queueAhead: readonly number[]; + } + | { wait: false }; /** Low-priority path buckets (lockfiles, generated/build output, vendored third-party trees) that overlapping * alone never counts as real conflict risk. Lockfile-NAME matching delegates to the canonical `isLockfile` @@ -151,5 +163,5 @@ export function shouldWaitForOlderSiblings(input: ShouldWaitForOlderSiblingsInpu }) .sort((a, b) => a.number - b.number); const blocker = viable[0]; - return blocker ? { wait: true, blockingPr: blocker.number } : { wait: false }; + return blocker ? { wait: true, blockingPr: blocker.number, queueAhead: viable.map((sibling) => sibling.number) } : { wait: false }; } diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 804251f003..dcd7e75c2a 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -765,8 +765,16 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE ctx.installationId, ctx.repoFullName, ctx.pullNumber, - `Queued in the merge train behind #${decision.blockingPr}, which touches overlapping work and was opened first. ` + - `This PR merges automatically once #${decision.blockingPr} completes (or leaves the train). No action needed. ` + + // #9952: name the POSITION, not just the PR in front. "behind #4" and "behind #4 and six + // others" are very different waits, and a queue that will not say which one it is reads as a + // stall rather than a wait. `queueAhead` is oldest-first, so its length is this PR's place in + // line minus one. Only the immediate blocker is named when it is the only one ahead -- listing + // a one-item queue as "1 PR ahead: #4" is noise. + (decision.queueAhead.length > 1 + ? `Queued in the merge train at position ${decision.queueAhead.length + 1}, behind ${decision.queueAhead.length} overlapping PRs opened before this one (${decision.queueAhead.map((pr) => `#${pr}`).join(", ")}). ` + + `The nearest is #${decision.blockingPr}. This PR merges automatically as they complete (or leave the train). No action needed. ` + : `Queued in the merge train behind #${decision.blockingPr}, which touches overlapping work and was opened first. ` + + `This PR merges automatically once #${decision.blockingPr} completes (or leaves the train). No action needed. `) + `This is an automated maintenance action.`, ).then(() => true).catch(() => false); // Recorded ONLY after a successful post: a failed post leaves no row, so the next pass retries diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index 273bc988bb..ec039db4f4 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -2327,6 +2327,25 @@ describe("executeAgentMaintenanceActions merge-train gate (#selfhost-merge-train expect(mergePullRequest).not.toHaveBeenCalled(); }); + it("#9952: with several PRs ahead the comment names the POSITION, not just the nearest blocker", async () => { + // "Behind #3" and "behind #3 and two others" are very different waits. Naming only the nearest made a + // queue read as a stall, because nothing told the contributor how long the line actually was. + const env = createTestEnv({}); + for (const [number, at] of [[3, "2026-07-05T08:00:00.000Z"], [4, "2026-07-05T08:30:00.000Z"], [5, "2026-07-05T09:00:00.000Z"]] as const) { + await upsertPullRequestFromGitHub(env, "owner/repo", { number, title: `Older overlapping sibling ${number}`, state: "open", user: { login: "c" }, head: { sha: `sha${number}` }, labels: [], body: "Fixes #1", created_at: at }); + } + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "This PR", state: "open", user: { login: "c" }, head: { sha: "sha7" }, labels: [], body: "Fixes #1", created_at: "2026-07-05T10:00:00.000Z" }); + + await executeAgentMaintenanceActions(env, ctx({ mergeTrainMode: "enforce", pullRequestCreatedAt: "2026-07-05T10:00:00.000Z", pullRequestLinkedIssues: [1] }), [merge]); + + const [, , , , body] = vi.mocked(createIssueComment).mock.calls[0]!; + expect(body).toContain("position 4"); + expect(body).toContain("behind 3 overlapping PRs"); + expect(body).toContain("#3, #4, #5"); + // The nearest is still called out by name -- position alone does not tell you what to watch. + expect(body).toContain("The nearest is #3"); + }); + it("REGRESSION (#merge-train-honest-comment): an enforce-mode train denial tells the contributor, once", async () => { // Observed live on JSONbored/loopover#9837: the published surface said the PR was MERGING (the planner // legitimately concluded wouldMerge before the executor's train check), then the denial was recorded diff --git a/test/unit/merge-train.test.ts b/test/unit/merge-train.test.ts index 9f65e25b17..b2e9243b93 100644 --- a/test/unit/merge-train.test.ts +++ b/test/unit/merge-train.test.ts @@ -33,7 +33,7 @@ function decide( describe("shouldWaitForOlderSiblings (#selfhost-merge-train)", () => { it("waits for a genuinely older, viable, overlapping sibling (by createdAt)", () => { const siblings = [sibling(105, "2026-07-07T10:00:00.000Z")]; - expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: true, blockingPr: 105 }); + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: true, blockingPr: 105, queueAhead: [105] }); }); it("does not wait for a NEWER sibling (by createdAt)", () => { @@ -57,12 +57,14 @@ describe("shouldWaitForOlderSiblings (#selfhost-merge-train)", () => { it("a non-dirty mergeableState (clean/unknown/unstable) still blocks", () => { const siblings = [sibling(105, "2026-07-07T10:00:00.000Z", "unstable")]; - expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: true, blockingPr: 105 }); + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: true, blockingPr: 105, queueAhead: [105] }); }); it("the OLDEST of several viable older siblings is the blocker", () => { const siblings = [sibling(107, "2026-07-07T10:30:00.000Z"), sibling(105, "2026-07-07T10:00:00.000Z"), sibling(108, "2026-07-07T10:45:00.000Z")]; - expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: true, blockingPr: 105 }); + // #9952: all three are viable and older, so the queue carries all three (oldest first) while blockingPr + // stays the nearest -- this PR is fourth in line, which is what the wait comment now reports. + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: true, blockingPr: 105, queueAhead: [105, 107, 108] }); }); it("staleness cap: an older sibling past MERGE_TRAIN_MAX_WAIT_MS no longer blocks", () => { @@ -76,12 +78,12 @@ describe("shouldWaitForOlderSiblings (#selfhost-merge-train)", () => { // is just under the cap is unambiguously older than this PR too, decoupling "is it stale" from "is it older". const freshCreatedAt = new Date(NOW - MERGE_TRAIN_MAX_WAIT_MS + 1000).toISOString(); const siblings = [sibling(105, freshCreatedAt)]; - expect(decide(110, new Date(NOW).toISOString(), siblings, NOW)).toEqual({ wait: true, blockingPr: 105 }); + expect(decide(110, new Date(NOW).toISOString(), siblings, NOW)).toEqual({ wait: true, blockingPr: 105, queueAhead: [105] }); }); it("missing createdAt on the sibling falls back to PR-number tiebreak (lower number = older)", () => { const siblings = [sibling(105, null)]; - expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: true, blockingPr: 105 }); + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: true, blockingPr: 105, queueAhead: [105] }); }); it("missing createdAt on the sibling + higher sibling number ⇒ does not block", () => { @@ -96,13 +98,13 @@ describe("shouldWaitForOlderSiblings (#selfhost-merge-train)", () => { it("missing createdAt on both sides falls back to PR-number tiebreak", () => { const siblings = [sibling(105, undefined)]; - expect(decide(110, undefined, siblings, NOW)).toEqual({ wait: true, blockingPr: 105 }); + expect(decide(110, undefined, siblings, NOW)).toEqual({ wait: true, blockingPr: 105, queueAhead: [105] }); }); it("an exact createdAt tie falls back to PR-number tiebreak", () => { const tie = "2026-07-07T11:55:00.000Z"; // recent, well clear of the staleness boundary tested separately above const siblings = [sibling(105, tie)]; - expect(decide(110, tie, siblings, NOW)).toEqual({ wait: true, blockingPr: 105 }); + expect(decide(110, tie, siblings, NOW)).toEqual({ wait: true, blockingPr: 105, queueAhead: [105] }); }); it("an exact createdAt tie with a LOWER-numbered current PR does not block", () => { @@ -119,12 +121,12 @@ describe("shouldWaitForOlderSiblings (#selfhost-merge-train)", () => { it("waits for an older sibling sharing a linked issue, even with no changed-file data on either side", () => { const siblings = [sibling(105, "2026-07-07T10:00:00.000Z", null, [42])]; - expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW, { thisPrLinkedIssues: [42] })).toEqual({ wait: true, blockingPr: 105 }); + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW, { thisPrLinkedIssues: [42] })).toEqual({ wait: true, blockingPr: 105, queueAhead: [105] }); }); it("waits for an older sibling sharing a meaningful changed file, even with no linked-issue overlap", () => { const siblings: MergeTrainSibling[] = [{ number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [99], changedFiles: ["src/queue/processors.ts"] }]; - expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW, { thisPrLinkedIssues: [1], thisPrChangedFiles: ["src/queue/processors.ts"] })).toEqual({ wait: true, blockingPr: 105 }); + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW, { thisPrLinkedIssues: [1], thisPrChangedFiles: ["src/queue/processors.ts"] })).toEqual({ wait: true, blockingPr: 105, queueAhead: [105] }); }); it("does NOT treat a shared lockfile or generated-output path as meaningful overlap", () => { @@ -146,12 +148,12 @@ describe("shouldWaitForOlderSiblings (#selfhost-merge-train)", () => { it("a sibling with no linkedIssues field at all (undefined) can still match via a shared changed file", () => { const siblings: MergeTrainSibling[] = [{ number: 105, createdAt: "2026-07-07T10:00:00.000Z", changedFiles: ["src/a.ts"] }]; - expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW, { thisPrLinkedIssues: [1], thisPrChangedFiles: ["src/a.ts"] })).toEqual({ wait: true, blockingPr: 105 }); + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW, { thisPrLinkedIssues: [1], thisPrChangedFiles: ["src/a.ts"] })).toEqual({ wait: true, blockingPr: 105, queueAhead: [105] }); }); it("a sibling with unresolved changedFiles can still match via a shared linked issue", () => { const siblings: MergeTrainSibling[] = [{ number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [7] }]; - expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW, { thisPrLinkedIssues: [7], thisPrChangedFiles: ["src/a.ts"] })).toEqual({ wait: true, blockingPr: 105 }); + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW, { thisPrLinkedIssues: [7], thisPrChangedFiles: ["src/a.ts"] })).toEqual({ wait: true, blockingPr: 105, queueAhead: [105] }); }); it("this PR having no changedFiles resolved does not manufacture a file-based match (issue-only fallback)", () => { @@ -168,12 +170,47 @@ describe("shouldWaitForOlderSiblings (#selfhost-merge-train)", () => { it("an OVERLAPPING older sibling stuck in review still blocks (bounded by the 24h staleness cap)", () => { const siblings = [sibling(105, "2026-07-07T10:00:00.000Z", "unstable", [1])]; - expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: true, blockingPr: 105 }); + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: true, blockingPr: 105, queueAhead: [105] }); }); it("with several older siblings, only the oldest OVERLAPPING one is the blocker (an unrelated older sibling is skipped over)", () => { const siblings = [sibling(104, "2026-07-07T09:30:00.000Z", null, [999]), sibling(105, "2026-07-07T10:00:00.000Z", null, [1]), sibling(107, "2026-07-07T10:30:00.000Z", null, [1])]; - expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: true, blockingPr: 105 }); + // #9952: the queue is overlap-scoped too -- #104 shares no issue, so it is absent from queueAhead and + // never inflates the reported position. A contributor must not be told they are behind unrelated work. + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: true, blockingPr: 105, queueAhead: [105, 107] }); + }); + }); + + describe("queue position (#9952)", () => { + // The wait comment used to name only the PR in front. "Behind #4" and "behind #4 and six others" are + // very different waits, and a queue that will not say which one it is reads as a stall. + it("reports the whole queue oldest-first, so position is queueAhead.length + 1", () => { + const siblings = [sibling(105, "2026-07-07T10:00:00.000Z"), sibling(106, "2026-07-07T10:10:00.000Z"), sibling(107, "2026-07-07T10:20:00.000Z")]; + const decision = decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW); + expect(decision).toEqual({ wait: true, blockingPr: 105, queueAhead: [105, 106, 107] }); + // The nearest blocker is always the head of the queue -- the comment quotes both and they must agree. + expect(decision.wait && decision.queueAhead[0]).toBe(decision.wait && decision.blockingPr); + }); + + it("a single blocker yields a one-item queue -- the caller keeps the simpler wording for it", () => { + expect(decide(110, "2026-07-07T11:00:00.000Z", [sibling(105, "2026-07-07T10:00:00.000Z")], NOW)).toEqual({ + wait: true, + blockingPr: 105, + queueAhead: [105], + }); + }); + + it("INVARIANT: evicted siblings never inflate the position", () => { + // A draft, a conflicted PR and a manual-review hold are all skipped as blockers, so none of them may + // appear in the queue either -- otherwise the contributor is told they are behind PRs that are not + // actually in front of them. + const siblings: MergeTrainSibling[] = [ + { number: 101, createdAt: "2026-07-07T09:00:00.000Z", linkedIssues: [1], isDraft: true }, + { number: 102, createdAt: "2026-07-07T09:15:00.000Z", linkedIssues: [1], mergeableState: "dirty" }, + { number: 103, createdAt: "2026-07-07T09:30:00.000Z", linkedIssues: [1], heldForManualReview: true }, + { number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [1] }, + ]; + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: true, blockingPr: 105, queueAhead: [105] }); }); }); @@ -194,9 +231,9 @@ describe("shouldWaitForOlderSiblings (#selfhost-merge-train)", () => { it("isDraft: false (or absent/undefined) leaves the OVERLAPPING older sibling blocking exactly as before", () => { const explicit = [{ number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [1], isDraft: false }]; - expect(decide(110, "2026-07-07T11:00:00.000Z", explicit, NOW)).toEqual({ wait: true, blockingPr: 105 }); + expect(decide(110, "2026-07-07T11:00:00.000Z", explicit, NOW)).toEqual({ wait: true, blockingPr: 105, queueAhead: [105] }); const absent: MergeTrainSibling[] = [{ number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [1] }]; - expect(decide(110, "2026-07-07T11:00:00.000Z", absent, NOW)).toEqual({ wait: true, blockingPr: 105 }); + expect(decide(110, "2026-07-07T11:00:00.000Z", absent, NOW)).toEqual({ wait: true, blockingPr: 105, queueAhead: [105] }); }); it("a draft sibling is skipped over in favor of the next-oldest still-viable overlapping sibling", () => { @@ -206,7 +243,7 @@ describe("shouldWaitForOlderSiblings (#selfhost-merge-train)", () => { { number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [1], isDraft: true }, { number: 107, createdAt: "2026-07-07T10:30:00.000Z", linkedIssues: [1] }, ]; - expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: true, blockingPr: 107 }); + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: true, blockingPr: 107, queueAhead: [107] }); }); it("a draft that is ALSO manual-review-held is evicted once, not twice -- the two rules compose", () => { @@ -223,9 +260,9 @@ describe("shouldWaitForOlderSiblings (#selfhost-merge-train)", () => { it("heldForManualReview: false (or absent/undefined) leaves the OVERLAPPING older sibling blocking exactly as before", () => { const held = [{ number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [1], heldForManualReview: false }]; - expect(decide(110, "2026-07-07T11:00:00.000Z", held, NOW)).toEqual({ wait: true, blockingPr: 105 }); + expect(decide(110, "2026-07-07T11:00:00.000Z", held, NOW)).toEqual({ wait: true, blockingPr: 105, queueAhead: [105] }); const absent: MergeTrainSibling[] = [{ number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [1] }]; - expect(decide(110, "2026-07-07T11:00:00.000Z", absent, NOW)).toEqual({ wait: true, blockingPr: 105 }); + expect(decide(110, "2026-07-07T11:00:00.000Z", absent, NOW)).toEqual({ wait: true, blockingPr: 105, queueAhead: [105] }); }); it("a manual-review-held sibling is skipped over in favor of the next-oldest still-viable overlapping sibling", () => { @@ -233,7 +270,7 @@ describe("shouldWaitForOlderSiblings (#selfhost-merge-train)", () => { { number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [1], heldForManualReview: true }, { number: 107, createdAt: "2026-07-07T10:30:00.000Z", linkedIssues: [1] }, ]; - expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: true, blockingPr: 107 }); + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: true, blockingPr: 107, queueAhead: [107] }); }); it("a manual-review-held sibling that is also unrelated/newer/dirty is unaffected -- the flag only matters when it would otherwise block", () => {