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"); + }); +});