From 32d1ff77f59fa8cd58f3fcbb8e1de26812e380e5 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 7 Jul 2026 15:12:50 -0700 Subject: [PATCH] fix(review): scope merge-train FIFO to overlapping siblings, fix dead persistence The merge-train gate blocked a merge behind ANY older open sibling, so a PR stuck in manual review could stall unrelated PRs indefinitely. Scope the block to siblings that share a linked issue or a meaningful changed file (lockfiles/build output excluded) -- the actual conflict-prevention purpose of FIFO ordering, since an unrelated older sibling never blocks regardless of its review state. Also fixes a critical, previously-shipped bug: mergeTrainMode was computed correctly in upsertRepositorySettings but omitted from both the INSERT values and UPDATE set column lists, so it silently never reached the DB on either write path and always read back as the SQL default 'off'. --- src/db/repositories.ts | 3 + src/queue/processors.ts | 2 + src/review/merge-train.ts | 93 ++++++++++++--- src/services/agent-action-executor.ts | 49 ++++++-- src/services/agent-approval-queue.ts | 2 + test/unit/agent-action-executor.test.ts | 45 +++++-- test/unit/agent-approval-queue.test.ts | 33 ++++++ test/unit/merge-train.test.ts | 112 +++++++++++++++--- ...pository-settings-merge-train-mode.test.ts | 54 +++++++++ 9 files changed, 338 insertions(+), 55 deletions(-) create mode 100644 test/unit/repository-settings-merge-train-mode.test.ts diff --git a/src/db/repositories.ts b/src/db/repositories.ts index ad7c33026d..7948766eb8 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -566,6 +566,7 @@ export async function getRepositorySettings(env: Env, fullName: string): Promise reviewEvasionProtection: "close", // #4011: default-ON -- see normalizeReviewEvasionProtection's doc comment reviewEvasionLabel: DEFAULT_REVIEW_EVASION_LABEL, reviewEvasionComment: true, + mergeTrainMode: "off", screenshotTableGate: { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [] }, }; } @@ -842,6 +843,7 @@ export async function upsertRepositorySettings(env: Env, settings: Partial siblingIssues.includes(issue))) return true; + if (!thisPrChangedFiles || !sibling.changedFiles) return false; + const siblingFiles = new Set(sibling.changedFiles); + return thisPrChangedFiles.some((path) => siblingFiles.has(path) && isMeaningfulPath(path)); +} + +export type ShouldWaitForOlderSiblingsInput = { + thisPrNumber: number; + thisPrCreatedAt: string | null | undefined; + /** This PR's own linked issues (always available -- `PullRequestRecord.linkedIssues` is never optional). */ + thisPrLinkedIssues: readonly number[]; + /** This PR's own changed file paths, when the caller has resolved them. Absent degrades overlap detection + * to linked-issue-only for every sibling (never fails closed into "nothing can overlap"). */ + thisPrChangedFiles?: readonly string[] | undefined; + siblings: readonly MergeTrainSibling[]; + nowMs: number; +}; + +/** True when an OVERLAPPING, older, still-viable sibling exists and `thisPrNumber` should wait its turn. A + * sibling never blocks when it is: the same PR, not older (by createdAt, falling back to PR number when + * createdAt is missing on either side -- mirrors the duplicate-winner election's own createdAt-then-number + * precedent), git-conflicted (`mergeableState === "dirty"` -- it isn't "about to merge," it's stuck), past + * the staleness cap, or simply UNRELATED (shares no linked issue and no meaningful changed file with this PR + * -- see the module header for why overlap-scoping, not blanket FIFO, is the actual fix here). Deterministic + * and total: same inputs always produce the same decision. */ +export function shouldWaitForOlderSiblings(input: ShouldWaitForOlderSiblingsInput): MergeTrainDecision { + const { thisPrNumber, thisPrCreatedAt, thisPrLinkedIssues, thisPrChangedFiles, siblings, nowMs } = input; const thisCreatedMs = thisPrCreatedAt ? Date.parse(thisPrCreatedAt) : Number.NaN; const isOlder = (sibling: MergeTrainSibling): boolean => { const siblingCreatedMs = sibling.createdAt ? Date.parse(sibling.createdAt) : Number.NaN; @@ -51,6 +105,7 @@ export function shouldWaitForOlderSiblings( .filter((sibling) => sibling.number !== thisPrNumber) .filter((sibling) => sibling.mergeableState !== "dirty") .filter((sibling) => isOlder(sibling)) + .filter((sibling) => overlaps(thisPrLinkedIssues, thisPrChangedFiles, sibling)) .filter((sibling) => { const siblingCreatedMs = sibling.createdAt ? Date.parse(sibling.createdAt) : Number.NaN; if (!Number.isFinite(siblingCreatedMs)) return true; // unknown age -- fail open toward still blocking diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 533981d4c5..733092628f 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -7,6 +7,7 @@ import { insertNotificationDeliveryIfAbsent, isGlobalAgentFrozen, listOtherOpenPullRequests, + listRepoPullRequestFilePaths, markPullRequestApproved, markPullRequestMergeBlocked, recordAuditEvent, @@ -190,6 +191,16 @@ export type AgentActionExecutionContext = { // gate below compares this against open siblings fetched fresh, since siblings are only ever fetched lazily // when the gate is actually enabled (see step 8b), not threaded through every caller unconditionally. pullRequestCreatedAt?: string | null | undefined; + // This PR's own linked issues (#selfhost-merge-train-overlap), resolved by the CALLER (already has the PR + // record in scope): the merge-train gate only holds a merge behind an OVERLAPPING older sibling (shared + // linked issue or shared meaningful changed file), never a blanket "any older PR" wait -- see + // merge-train.ts's module header for why. Absent/undefined behaves like an empty list (issue-overlap never + // matches; file-overlap can still apply via pullRequestChangedFiles below). + pullRequestLinkedIssues?: readonly number[] | undefined; + // This PR's own changed file paths, when the caller has them resolved (e.g. a webhook path with the + // `pull_request_files` cache already populated). Absent/undefined degrades the merge-train overlap check to + // linked-issue-only for this PR, never to "no overlap possible". + pullRequestChangedFiles?: readonly string[] | undefined; }; export type ModerationContextSettings = { @@ -461,15 +472,39 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE continue; } } - // 8b) merge-train FIFO gate (#selfhost-merge-train): a still-viable OLDER open sibling in this repo holds - // this merge until it merges, closes, or goes stale (see merge-train.ts's staleness cap). Siblings are - // fetched fresh here, lazily, ONLY when the gate is actually enabled for this repo — not threaded through - // every caller unconditionally, since the vast majority of merges never need this check. "audit" mode logs - // the decision but never actually holds anything, so it's safe to enable everywhere to validate the fix - // before switching a repo to "enforce". + // 8b) merge-train FIFO gate (#selfhost-merge-train): a still-viable, OVERLAPPING older open sibling in this + // repo holds this merge until it merges, closes, or goes stale (see merge-train.ts's staleness cap and its + // module header for why overlap-scoping, not blanket FIFO, is the actual fix -- an unrelated older sibling, + // even one stuck in manual review, never blocks). Siblings + their changed-file paths are fetched fresh + // here, lazily, ONLY when the gate is actually enabled for this repo — not threaded through every caller + // unconditionally, since the vast majority of merges never need this check. "audit" mode logs the decision + // but never actually holds anything, so it's safe to enable everywhere to validate the fix before switching + // a repo to "enforce". if (action.actionClass === "merge" && ctx.mergeTrainMode && ctx.mergeTrainMode !== "off") { const siblings = await listOtherOpenPullRequests(env, ctx.repoFullName, ctx.pullNumber); - const decision = shouldWaitForOlderSiblings(ctx.pullNumber, ctx.pullRequestCreatedAt, siblings, Date.now()); + const filePaths = await listRepoPullRequestFilePaths(env, ctx.repoFullName, { + pullNumbers: [ctx.pullNumber, ...siblings.map((sibling) => sibling.number)], + }); + const pathsByPullNumber = new Map(); + for (const row of filePaths) { + const paths = pathsByPullNumber.get(row.pullNumber) ?? []; + paths.push(row.path); + pathsByPullNumber.set(row.pullNumber, paths); + } + const decision = shouldWaitForOlderSiblings({ + thisPrNumber: ctx.pullNumber, + thisPrCreatedAt: ctx.pullRequestCreatedAt, + thisPrLinkedIssues: ctx.pullRequestLinkedIssues ?? [], + thisPrChangedFiles: pathsByPullNumber.get(ctx.pullNumber) ?? ctx.pullRequestChangedFiles, + siblings: siblings.map((sibling) => ({ + number: sibling.number, + createdAt: sibling.createdAt, + mergeableState: sibling.mergeableState, + linkedIssues: sibling.linkedIssues, + changedFiles: pathsByPullNumber.get(sibling.number), + })), + nowMs: Date.now(), + }); if (decision.wait) { incr("gittensory_merge_train_deferred_total", { repo: ctx.repoFullName, mode: ctx.mergeTrainMode }); if (ctx.mergeTrainMode === "enforce") { diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index e3111f6881..a4a6756eaf 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -409,6 +409,8 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de installationPermissions: installation ? installation.permissions : null, mergeTrainMode: settings.mergeTrainMode, pullRequestCreatedAt: pr?.createdAt, + pullRequestLinkedIssues: pr?.linkedIssues, + pullRequestChangedFiles: pr?.changedFiles, // CI-run cancellation on a contributor_cap close (#2462): a contributor_cap close CAN be staged for // approval (close autonomy = auto_with_approval), so the accept-replay path needs this resolved the // same way the live webhook path does (src/queue/processors.ts) for the cancel hook to fire here too. diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index 0b45adf862..43000e8dee 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -72,7 +72,7 @@ import { import type { PlannedAgentAction } from "../../src/settings/agent-actions"; import { STRUCTURED_CLOSE_REASONS_MAX_COUNT } from "../../src/settings/agent-execution"; import { AGENT_LABEL_PENDING_CLOSURE } from "../../src/review/linked-issue-hard-rules"; -import { clearProcessLocalGlobalAgentFrozenCacheForTest, getGlobalContributorBlacklist, isGlobalAgentFrozen, setGlobalAgentFrozen, upsertGlobalModerationConfig, upsertPullRequestFromGitHub } from "../../src/db/repositories"; +import { clearProcessLocalGlobalAgentFrozenCacheForTest, getGlobalContributorBlacklist, isGlobalAgentFrozen, setGlobalAgentFrozen, upsertGlobalModerationConfig, upsertPullRequestFile, upsertPullRequestFromGitHub } from "../../src/db/repositories"; import * as repositoriesModule from "../../src/db/repositories"; import * as sentryModule from "../../src/selfhost/sentry"; import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; @@ -1883,18 +1883,28 @@ describe("executeAgentMaintenanceActions merge-train gate (#selfhost-merge-train expect(mergePullRequest).toHaveBeenCalled(); }); - it("mergeTrainMode: \"enforce\" holds the merge behind a still-open OLDER sibling", async () => { + it("mergeTrainMode: \"enforce\" holds the merge behind a still-open, OVERLAPPING OLDER sibling", async () => { const env = createTestEnv({}); - await upsertPullRequestFromGitHub(env, "owner/repo", { number: 3, title: "Older sibling", state: "open", user: { login: "c" }, head: { sha: "sha3" }, labels: [], body: "", created_at: "2026-07-05T08:00:00.000Z" }); - await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "This PR", state: "open", user: { login: "c" }, head: { sha: "sha7" }, labels: [], body: "", created_at: "2026-07-05T10:00:00.000Z" }); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 3, title: "Older sibling", state: "open", user: { login: "c" }, head: { sha: "sha3" }, labels: [], body: "Fixes #1", created_at: "2026-07-05T08:00:00.000Z" }); + 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" }); - const outcomes = await executeAgentMaintenanceActions(env, ctx({ mergeTrainMode: "enforce", pullRequestCreatedAt: "2026-07-05T10:00:00.000Z" }), [merge]); + const outcomes = await executeAgentMaintenanceActions(env, ctx({ mergeTrainMode: "enforce", pullRequestCreatedAt: "2026-07-05T10:00:00.000Z", pullRequestLinkedIssues: [1] }), [merge]); expect(outcomes[0]).toMatchObject({ actionClass: "merge", outcome: "denied" }); expect(outcomes[0]?.detail).toContain("merge train"); expect(outcomes[0]?.detail).toContain("#3"); expect(mergePullRequest).not.toHaveBeenCalled(); }); + it("does NOT hold an enforce-mode merge behind an older sibling that shares no linked issue or changed file (#selfhost-merge-train-overlap)", async () => { + const env = createTestEnv({}); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 3, title: "Unrelated older sibling", state: "open", user: { login: "c" }, head: { sha: "sha3" }, labels: [], body: "Fixes #99", created_at: "2026-07-05T08:00:00.000Z" }); + 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" }); + + const outcomes = await executeAgentMaintenanceActions(env, ctx({ mergeTrainMode: "enforce", pullRequestCreatedAt: "2026-07-05T10:00:00.000Z", pullRequestLinkedIssues: [1] }), [merge]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(mergePullRequest).toHaveBeenCalled(); + }); + it("mergeTrainMode: \"enforce\" merges normally when no older sibling exists", async () => { const env = createTestEnv({}); await upsertPullRequestFromGitHub(env, "owner/repo", { number: 9, title: "Newer sibling", state: "open", user: { login: "c" }, head: { sha: "sha9" }, labels: [], body: "", created_at: "2026-07-05T11:00:00.000Z" }); @@ -1907,10 +1917,10 @@ describe("executeAgentMaintenanceActions merge-train gate (#selfhost-merge-train it("mergeTrainMode: \"audit\" records the would-hold decision but still executes the merge", async () => { const env = createTestEnv({}); - await upsertPullRequestFromGitHub(env, "owner/repo", { number: 3, title: "Older sibling", state: "open", user: { login: "c" }, head: { sha: "sha3" }, labels: [], body: "", created_at: "2026-07-05T08:00:00.000Z" }); - await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "This PR", state: "open", user: { login: "c" }, head: { sha: "sha7" }, labels: [], body: "", created_at: "2026-07-05T10:00:00.000Z" }); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 3, title: "Older sibling", state: "open", user: { login: "c" }, head: { sha: "sha3" }, labels: [], body: "Fixes #1", created_at: "2026-07-05T08:00:00.000Z" }); + 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" }); - const outcomes = await executeAgentMaintenanceActions(env, ctx({ mergeTrainMode: "audit", pullRequestCreatedAt: "2026-07-05T10:00:00.000Z" }), [merge]); + const outcomes = await executeAgentMaintenanceActions(env, ctx({ mergeTrainMode: "audit", pullRequestCreatedAt: "2026-07-05T10:00:00.000Z", pullRequestLinkedIssues: [1] }), [merge]); // Exactly ONE outcome for the one planned action -- the audit-mode signal must not double it. expect(outcomes).toHaveLength(1); expect(outcomes[0]?.outcome).toBe("completed"); @@ -1923,11 +1933,24 @@ describe("executeAgentMaintenanceActions merge-train gate (#selfhost-merge-train it("a git-conflicted (\"dirty\") older sibling never blocks an enforce-mode merge", async () => { const env = createTestEnv({}); - await upsertPullRequestFromGitHub(env, "owner/repo", { number: 3, title: "Conflicted older sibling", state: "open", user: { login: "c" }, head: { sha: "sha3" }, labels: [], body: "", created_at: "2026-07-05T08:00:00.000Z", mergeable_state: "dirty" }); - await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "This PR", state: "open", user: { login: "c" }, head: { sha: "sha7" }, labels: [], body: "", created_at: "2026-07-05T10:00:00.000Z" }); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 3, title: "Conflicted older sibling", state: "open", user: { login: "c" }, head: { sha: "sha3" }, labels: [], body: "Fixes #1", created_at: "2026-07-05T08:00:00.000Z", mergeable_state: "dirty" }); + 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" }); - const outcomes = await executeAgentMaintenanceActions(env, ctx({ mergeTrainMode: "enforce", pullRequestCreatedAt: "2026-07-05T10:00:00.000Z" }), [merge]); + const outcomes = await executeAgentMaintenanceActions(env, ctx({ mergeTrainMode: "enforce", pullRequestCreatedAt: "2026-07-05T10:00:00.000Z", pullRequestLinkedIssues: [1] }), [merge]); expect(outcomes[0]?.outcome).toBe("completed"); expect(mergePullRequest).toHaveBeenCalled(); }); + + it("holds an enforce-mode merge behind an older sibling that shares no linked issue but DOES share a meaningful changed file", async () => { + const env = createTestEnv({}); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 3, title: "Older sibling, no linked issue", state: "open", user: { login: "c" }, head: { sha: "sha3" }, labels: [], body: "", created_at: "2026-07-05T08:00:00.000Z" }); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "This PR", state: "open", user: { login: "c" }, head: { sha: "sha7" }, labels: [], body: "", created_at: "2026-07-05T10:00:00.000Z" }); + await upsertPullRequestFile(env, { repoFullName: "owner/repo", pullNumber: 3, path: "src/queue/processors.ts", status: "modified", additions: 1, deletions: 1, changes: 2, payload: {} }); + await upsertPullRequestFile(env, { repoFullName: "owner/repo", pullNumber: 7, path: "src/queue/processors.ts", status: "modified", additions: 1, deletions: 1, changes: 2, payload: {} }); + + const outcomes = await executeAgentMaintenanceActions(env, ctx({ mergeTrainMode: "enforce", pullRequestCreatedAt: "2026-07-05T10:00:00.000Z", pullRequestChangedFiles: ["src/queue/processors.ts"] }), [merge]); + expect(outcomes[0]).toMatchObject({ actionClass: "merge", outcome: "denied" }); + expect(outcomes[0]?.detail).toContain("#3"); + expect(mergePullRequest).not.toHaveBeenCalled(); + }); }); diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 1718b4229c..21fb3817a4 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -193,6 +193,39 @@ describe("agent approval queue (#779)", () => { expect(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "squash", sha: "h7" }); }); + it("accept holds a staged merge behind a still-open, OVERLAPPING older sibling under mergeTrainMode: enforce (#selfhost-merge-train-overlap)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" }, mergeTrainMode: "enforce" }); + await seedInstallation(env); + // Relative to Date.now() (this file never pins the system clock) so the sibling is unambiguously OLDER + // than the current PR but still well within the 24h merge-train staleness cap. + const olderCreatedAt = new Date(Date.now() - 60 * 60 * 1000).toISOString(); + const newerCreatedAt = new Date(Date.now() - 30 * 60 * 1000).toISOString(); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 3, title: "Older overlapping sibling", state: "open", user: { login: "contributor" }, head: { sha: "h3" }, labels: [], body: "Fixes #1", created_at: olderCreatedAt }); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "Fixes #1", created_at: newerCreatedAt }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.executionOutcome).toBe("denied"); + expect(mergePullRequest).not.toHaveBeenCalled(); + }); + + it("accept does NOT hold a staged merge behind an older sibling sharing no linked issue or file, even under mergeTrainMode: enforce", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" }, mergeTrainMode: "enforce" }); + await seedInstallation(env); + const olderCreatedAt = new Date(Date.now() - 60 * 60 * 1000).toISOString(); + const newerCreatedAt = new Date(Date.now() - 30 * 60 * 1000).toISOString(); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 3, title: "Older unrelated sibling", state: "open", user: { login: "contributor" }, head: { sha: "h3" }, labels: [], body: "Fixes #99", created_at: olderCreatedAt }); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "Fixes #1", created_at: newerCreatedAt }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("accepted"); + expect(result.executionOutcome).toBe("completed"); + expect(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "squash", sha: "h7" }); + }); + it("REGRESSION (#2422): accept denies a merge staged with NO reviewed-head pin, rather than silently merging whatever commit is currently live", async () => { // Unlike a PINNED merge, where GitHub's `sha` param 409s on mismatch (a real backstop), an UNPINNED merge // falls back to performAction's `mergeSha = action.expectedHeadSha ?? ctx.headSha`, which by construction diff --git a/test/unit/merge-train.test.ts b/test/unit/merge-train.test.ts index 87a15e4555..d7db0719af 100644 --- a/test/unit/merge-train.test.ts +++ b/test/unit/merge-train.test.ts @@ -1,52 +1,74 @@ import { describe, expect, it } from "vitest"; -import { MERGE_TRAIN_MAX_WAIT_MS, shouldWaitForOlderSiblings, type MergeTrainSibling } from "../../src/review/merge-train"; +import { MERGE_TRAIN_MAX_WAIT_MS, shouldWaitForOlderSiblings, type MergeTrainSibling, type ShouldWaitForOlderSiblingsInput } from "../../src/review/merge-train"; const NOW = Date.parse("2026-07-07T12:00:00.000Z"); -const sibling = (number: number, createdAt: string | null | undefined, mergeableState?: string | null): MergeTrainSibling => ({ + +// Every sibling defaults to sharing linked issue #1 with "this PR" (see `decide`'s default +// thisPrLinkedIssues below) so the age/staleness/dirty tests below -- none of which are about +// overlap -- don't each have to opt into it separately. Overlap-specific tests override explicitly. +const sibling = (number: number, createdAt: string | null | undefined, mergeableState?: string | null, linkedIssues: readonly number[] = [1]): MergeTrainSibling => ({ number, createdAt, mergeableState, + linkedIssues, }); +function decide( + thisPrNumber: number, + thisPrCreatedAt: string | null | undefined, + siblings: readonly MergeTrainSibling[], + nowMs: number, + overrides: Partial> = {}, +) { + return shouldWaitForOlderSiblings({ + thisPrNumber, + thisPrCreatedAt, + thisPrLinkedIssues: overrides.thisPrLinkedIssues ?? [1], + thisPrChangedFiles: overrides.thisPrChangedFiles, + siblings, + nowMs, + }); +} + describe("shouldWaitForOlderSiblings (#selfhost-merge-train)", () => { - it("waits for a genuinely older, viable sibling (by createdAt)", () => { + it("waits for a genuinely older, viable, overlapping sibling (by createdAt)", () => { const siblings = [sibling(105, "2026-07-07T10:00:00.000Z")]; - expect(shouldWaitForOlderSiblings(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 }); }); it("does not wait for a NEWER sibling (by createdAt)", () => { const siblings = [sibling(115, "2026-07-07T11:30:00.000Z")]; - expect(shouldWaitForOlderSiblings(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: false }); + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: false }); }); it("does not wait when there are no other open siblings", () => { - expect(shouldWaitForOlderSiblings(110, "2026-07-07T11:00:00.000Z", [], NOW)).toEqual({ wait: false }); + expect(decide(110, "2026-07-07T11:00:00.000Z", [], NOW)).toEqual({ wait: false }); }); it("never counts itself as its own blocking sibling", () => { const siblings = [sibling(110, "2026-07-07T09:00:00.000Z")]; - expect(shouldWaitForOlderSiblings(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: false }); + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: false }); }); it("a git-conflicted older sibling never blocks — it is stuck, not about to merge", () => { const siblings = [sibling(105, "2026-07-07T10:00:00.000Z", "dirty")]; - expect(shouldWaitForOlderSiblings(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: false }); + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: false }); }); it("a non-dirty mergeableState (clean/unknown/unstable) still blocks", () => { const siblings = [sibling(105, "2026-07-07T10:00:00.000Z", "unstable")]; - expect(shouldWaitForOlderSiblings(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 }); }); 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(shouldWaitForOlderSiblings(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 }); }); it("staleness cap: an older sibling past MERGE_TRAIN_MAX_WAIT_MS no longer blocks", () => { const staleCreatedAt = new Date(NOW - MERGE_TRAIN_MAX_WAIT_MS - 1000).toISOString(); const siblings = [sibling(105, staleCreatedAt)]; - expect(shouldWaitForOlderSiblings(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: false }); + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: false }); }); it("staleness cap: an older sibling just under the cap still blocks", () => { @@ -54,38 +76,92 @@ 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(shouldWaitForOlderSiblings(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 }); }); it("missing createdAt on the sibling falls back to PR-number tiebreak (lower number = older)", () => { const siblings = [sibling(105, null)]; - expect(shouldWaitForOlderSiblings(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 }); }); it("missing createdAt on the sibling + higher sibling number ⇒ does not block", () => { const siblings = [sibling(115, undefined)]; - expect(shouldWaitForOlderSiblings(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: false }); + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW)).toEqual({ wait: false }); }); it("missing createdAt on THIS pr but sibling has one still falls back to PR-number tiebreak", () => { const siblings = [sibling(115, "2026-07-07T09:00:00.000Z")]; - expect(shouldWaitForOlderSiblings(110, null, siblings, NOW)).toEqual({ wait: false }); + expect(decide(110, null, siblings, NOW)).toEqual({ wait: false }); }); it("missing createdAt on both sides falls back to PR-number tiebreak", () => { const siblings = [sibling(105, undefined)]; - expect(shouldWaitForOlderSiblings(110, undefined, siblings, NOW)).toEqual({ wait: true, blockingPr: 105 }); + expect(decide(110, undefined, siblings, NOW)).toEqual({ wait: true, blockingPr: 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(shouldWaitForOlderSiblings(110, tie, siblings, NOW)).toEqual({ wait: true, blockingPr: 105 }); + expect(decide(110, tie, siblings, NOW)).toEqual({ wait: true, blockingPr: 105 }); }); it("an exact createdAt tie with a LOWER-numbered current PR does not block", () => { const tie = "2026-07-07T11:55:00.000Z"; // recent, well clear of the staleness boundary tested separately above const siblings = [sibling(115, tie)]; - expect(shouldWaitForOlderSiblings(110, tie, siblings, NOW)).toEqual({ wait: false }); + expect(decide(110, tie, siblings, NOW)).toEqual({ wait: false }); + }); + + describe("overlap scoping (#selfhost-merge-train-overlap)", () => { + it("does NOT wait for an older, unrelated sibling (no shared linked issue, no shared file)", () => { + const siblings = [sibling(105, "2026-07-07T10:00:00.000Z", null, [99])]; + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW, { thisPrLinkedIssues: [1], thisPrChangedFiles: ["src/a.ts"] })).toEqual({ wait: false }); + }); + + 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 }); + }); + + 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 }); + }); + + it("does NOT treat a shared lockfile or generated-output path as meaningful overlap", () => { + const siblings: MergeTrainSibling[] = [{ number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [99], changedFiles: ["package-lock.json", "dist/bundle.js"] }]; + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW, { thisPrLinkedIssues: [1], thisPrChangedFiles: ["package-lock.json", "dist/bundle.js"] })).toEqual({ wait: false }); + }); + + 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 }); + }); + + 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 }); + }); + + it("this PR having no changedFiles resolved does not manufacture a file-based match (issue-only fallback)", () => { + const siblings: MergeTrainSibling[] = [{ number: 105, createdAt: "2026-07-07T10:00:00.000Z", linkedIssues: [99], changedFiles: ["src/a.ts"] }]; + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW, { thisPrLinkedIssues: [1] })).toEqual({ wait: false }); + }); + + it("an unrelated older sibling stuck in review does not block a newer, unrelated, ready PR", () => { + // The scenario question 1 worried about: an older sibling held for manual review (still mergeableState + // "clean"/"unstable", not "dirty") must not wedge an unrelated newer PR just because it's older. + const siblings = [sibling(105, "2026-07-07T10:00:00.000Z", "unstable", [777])]; + expect(decide(110, "2026-07-07T11:00:00.000Z", siblings, NOW, { thisPrLinkedIssues: [1], thisPrChangedFiles: ["docs/readme.md"] })).toEqual({ wait: false }); + }); + + 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 }); + }); + + 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 }); + }); }); }); diff --git a/test/unit/repository-settings-merge-train-mode.test.ts b/test/unit/repository-settings-merge-train-mode.test.ts new file mode 100644 index 0000000000..3907c2f84b --- /dev/null +++ b/test/unit/repository-settings-merge-train-mode.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, it } from "vitest"; +import { getRepositorySettings, upsertRepositorySettings } from "../../src/db/repositories"; +import { createTestEnv } from "../helpers/d1"; + +// #selfhost-merge-train: mergeTrainMode ("off" | "audit" | "enforce") was added to the schema/type/openapi +// layer but the INSERT and UPDATE column lists in upsertRepositorySettings never actually included it -- the +// resolved value was computed correctly but silently never written, so setting it via the settings API/ +// dashboard (or any upsertRepositorySettings caller) was completely inert; the column stayed at its SQL +// DEFAULT 'off' forever, on both a brand-new row (INSERT) and an existing one (UPDATE via onConflictDoUpdate). +// Caught only by an integration-level merge-train test wired through the settings-resolution path, not by +// the pure decision function's own unit tests (merge-train.test.ts), which never touch the DB at all. +describe("repository_settings: mergeTrainMode persistence (#selfhost-merge-train)", () => { + it("getRepositorySettings returns off for a repo with no DB row at all (conservative default)", async () => { + const env = createTestEnv(); + const settings = await getRepositorySettings(env, "acme/brand-new-repo"); + expect(settings.mergeTrainMode).toBe("off"); + }); + + it("REGRESSION: an explicit mergeTrainMode persists on the FIRST upsert (INSERT path)", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/fresh-insert", mergeTrainMode: "enforce" }); + const settings = await getRepositorySettings(env, "acme/fresh-insert"); + expect(settings.mergeTrainMode).toBe("enforce"); + }); + + it("REGRESSION: an explicit mergeTrainMode persists on a SECOND upsert of an already-existing row (UPDATE path)", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/existing-row" }); + const before = await getRepositorySettings(env, "acme/existing-row"); + expect(before.mergeTrainMode).toBe("off"); + + await upsertRepositorySettings(env, { repoFullName: "acme/existing-row", mergeTrainMode: "audit" }); + const after = await getRepositorySettings(env, "acme/existing-row"); + expect(after.mergeTrainMode).toBe("audit"); + }); + + it("a true read-modify-write caller (spread current settings, then re-upsert) carries mergeTrainMode forward explicitly", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/round-trip", mergeTrainMode: "enforce" }); + const settings = await getRepositorySettings(env, "acme/round-trip"); + expect(settings.mergeTrainMode).toBe("enforce"); + await upsertRepositorySettings(env, { ...settings, repoFullName: "acme/round-trip" }); + const after = await getRepositorySettings(env, "acme/round-trip"); + expect(after.mergeTrainMode).toBe("enforce"); + }); + + it("an invalid persisted DB value fails closed to off on read", async () => { + const env = createTestEnv(); + await upsertRepositorySettings(env, { repoFullName: "acme/malformed" }); + await env.DB.prepare("UPDATE repository_settings SET merge_train_mode = ? WHERE repo_full_name = ?").bind("sometimes", "acme/malformed").run(); + const settings = await getRepositorySettings(env, "acme/malformed"); + expect(settings.mergeTrainMode).toBe("off"); + }); +});