diff --git a/packages/cli/src/commands/__tests__/branch-group.test.ts b/packages/cli/src/commands/__tests__/branch-group.test.ts index ea99db76de..c072b12837 100644 --- a/packages/cli/src/commands/__tests__/branch-group.test.ts +++ b/packages/cli/src/commands/__tests__/branch-group.test.ts @@ -244,9 +244,11 @@ describe("branch-group CLI abandon (agent-native parity, Fix #7)", () => { await runBranchGroupAbandon("BG-1"); expect(closeGroupPullRequestMock).not.toHaveBeenCalled(); + // A group that never had a PR keeps prState "none" — "closed" would falsely + // imply a PR existed and was explicitly closed. expect(store.updateBranchGroup).toHaveBeenCalledWith( "BG-1", - expect.objectContaining({ status: "abandoned", prState: "closed" }), + expect.objectContaining({ status: "abandoned", prState: "none" }), ); }); diff --git a/packages/cli/src/commands/branch-group.ts b/packages/cli/src/commands/branch-group.ts index 208cdbf081..1c86094b8a 100644 --- a/packages/cli/src/commands/branch-group.ts +++ b/packages/cli/src/commands/branch-group.ts @@ -137,7 +137,9 @@ export async function runBranchGroupAbandon(id: string, projectName?: string) { process.exit(1); } - let prState: BranchGroup["prState"] = "closed"; + // A group with a PR abandons to "closed"; a group that never had a PR keeps + // its existing prState — "closed" would falsely imply a PR existed. + let prState: BranchGroup["prState"] = group.prNumber != null ? "closed" : group.prState; let prNumber = group.prNumber; let prUrl = group.prUrl; diff --git a/packages/dashboard/src/__tests__/routes-branch-groups.test.ts b/packages/dashboard/src/__tests__/routes-branch-groups.test.ts index bf444573fb..b6be5df505 100644 --- a/packages/dashboard/src/__tests__/routes-branch-groups.test.ts +++ b/packages/dashboard/src/__tests__/routes-branch-groups.test.ts @@ -330,6 +330,22 @@ describe("branch group abandon (U6, R7)", () => { expect(closeGroupPr).not.toHaveBeenCalled(); expect(updateBranchGroup).not.toHaveBeenCalled(); }); + + it("preserves prState 'none' when abandoning a group that never had a PR", async () => { + // "closed" would falsely imply a PR existed and was explicitly closed. + const noPr = { ...buildOpenGroup(), prState: "none" as const, prNumber: undefined, prUrl: undefined }; + const { store, updateBranchGroup } = buildAbandonStore(noPr); + const closeGroupPr = vi.fn(); + const app = mount(store, closeGroupPr as unknown as ReturnType); + + const res = await REQUEST(app, "POST", "/branch-groups/BG-AB/abandon", JSON.stringify({}), { "content-type": "application/json" }); + expect(res.status).toBe(200); + expect(closeGroupPr).not.toHaveBeenCalled(); + expect(updateBranchGroup).toHaveBeenCalledWith( + "BG-AB", + expect.objectContaining({ status: "abandoned", prState: "none" }), + ); + }); }); describe("branch group reconcile-on-read (Fix #3)", () => { diff --git a/packages/dashboard/src/routes/register-branch-groups-routes.ts b/packages/dashboard/src/routes/register-branch-groups-routes.ts index d64ae5ca20..080d035451 100644 --- a/packages/dashboard/src/routes/register-branch-groups-routes.ts +++ b/packages/dashboard/src/routes/register-branch-groups-routes.ts @@ -176,9 +176,11 @@ export function createBranchGroupsRouter(store: TaskStore, options?: BranchGroup throw badRequest("Branch group is already abandoned, finalized, or merged and cannot be abandoned"); } - // The guard above already rejected `prState === "merged"`, so abandon always - // resolves to "closed" unless the GitHub reconcile below reports otherwise. - let prState: BranchGroup["prState"] = "closed"; + // The guard above already rejected `prState === "merged"`. A group with a PR + // abandons to "closed" (unless the GitHub reconcile below reports otherwise); + // a group that never had a PR keeps its existing prState — "closed" would + // falsely imply a PR existed and was closed when none ever did. + let prState: BranchGroup["prState"] = group.prNumber != null ? "closed" : group.prState; let prNumber = group.prNumber; let prUrl = group.prUrl; diff --git a/packages/engine/src/__tests__/group-pr-sync-on-landing.test.ts b/packages/engine/src/__tests__/group-pr-sync-on-landing.test.ts new file mode 100644 index 0000000000..4e1a521076 --- /dev/null +++ b/packages/engine/src/__tests__/group-pr-sync-on-landing.test.ts @@ -0,0 +1,90 @@ +import { describe, expect, it, vi } from "vitest"; + +import type { BranchGroup, Task } from "@fusion/core"; +import { syncGroupPrOnLanding } from "../merger.js"; +import type { SyncGroupPrFn } from "../group-merge-coordinator.js"; + +/** + * ## Surface Enumeration + * + * Narrow-seam coverage (FN-5048) for the U6 sync-on-landing write guard, + * extracted from the merger's fire-and-forget background block: + * - no persisted open PR → the sync callback is never invoked + * - matching snapshot + out-of-band terminal state → reconciliation persisted + * - stale snapshot (a newer PR stored mid-sync) → the stale write is skipped + * The full landing pipeline (real git, aiMergeTask) is covered by the + * reliability suite `branch-group-pr-sync.test.ts`; this file pins the race + * deterministically without expanding that slow suite. + */ +function makeGroup(partial: Partial): BranchGroup { + return { + id: "BG-1", + sourceType: "planning", + sourceId: "PS-1", + branchName: "fusion/groups/g1", + autoMerge: true, + prState: "open", + prNumber: 13, + prUrl: "https://github.com/o/r/pull/13", + status: "open", + createdAt: Date.now(), + updatedAt: Date.now(), + ...partial, + } as BranchGroup; +} + +function makeStore(initial: BranchGroup) { + let group: BranchGroup = initial; + return { + getBranchGroup: vi.fn(() => group), + listTasksByBranchGroup: vi.fn(async () => [] as Task[]), + updateBranchGroup: vi.fn((_id: string, patch: Partial) => { + group = { ...group, ...patch } as BranchGroup; + return group; + }), + // test hook to simulate a concurrent landing/promotion swapping the PR + _swap(patch: Partial) { + group = { ...group, ...patch } as BranchGroup; + }, + _current() { + return group; + }, + }; +} + +describe("syncGroupPrOnLanding (U6 stale-snapshot write guard)", () => { + it("does not invoke the callback when the group has no persisted open PR", async () => { + const store = makeStore(makeGroup({ prState: "none", prNumber: undefined })); + const syncGroupPr = vi.fn() as unknown as SyncGroupPrFn; + await syncGroupPrOnLanding({ store, groupId: "BG-1", cwd: "/tmp/project", syncGroupPr }); + expect(syncGroupPr).not.toHaveBeenCalled(); + }); + + it("persists out-of-band terminal reconciliation when the snapshot still matches", async () => { + const store = makeStore(makeGroup({})); + const syncGroupPr: SyncGroupPrFn = vi.fn(async ({ group }) => ({ + prNumber: group.prNumber!, + prUrl: group.prUrl!, + prState: "merged" as const, + })); + await syncGroupPrOnLanding({ store, groupId: "BG-1", cwd: "/tmp/project", syncGroupPr }); + expect(store.updateBranchGroup).toHaveBeenCalledTimes(1); + expect(store._current().prState).toBe("merged"); + expect(store._current().prNumber).toBe(13); + }); + + it("skips the stale write when a newer PR was stored between sync and write", async () => { + const store = makeStore(makeGroup({})); + // GitHub reports PR #13 merged out-of-band; but while the sync awaits, a + // newer landing/promotion replaces it with a newer OPEN PR #88. The stale + // "merged" write must be skipped so #88 survives. + const syncGroupPr: SyncGroupPrFn = vi.fn(async ({ group }) => { + store._swap({ prState: "open", prNumber: 88, prUrl: "https://github.com/o/r/pull/88" }); + return { prNumber: group.prNumber!, prUrl: group.prUrl!, prState: "merged" as const }; + }); + await syncGroupPrOnLanding({ store, groupId: "BG-1", cwd: "/tmp/project", syncGroupPr }); + expect(store.updateBranchGroup).not.toHaveBeenCalled(); + expect(store._current().prNumber).toBe(88); + expect(store._current().prState).toBe("open"); + }); +}); diff --git a/packages/engine/src/__tests__/reliability-interactions/branch-group-pr-sync.test.ts b/packages/engine/src/__tests__/reliability-interactions/branch-group-pr-sync.test.ts index 6df38a003b..4adca4527f 100644 --- a/packages/engine/src/__tests__/reliability-interactions/branch-group-pr-sync.test.ts +++ b/packages/engine/src/__tests__/reliability-interactions/branch-group-pr-sync.test.ts @@ -195,44 +195,4 @@ describe("U6: group PR sync on member landing", () => { } }, 45_000); - it.skipIf(!hasGit)("does not clobber a newer PR stored between sync and write (stale snapshot guard)", async () => { - const fixture = await makeReliabilityFixture({ taskId: "FN-U6-SYNC-STALE", settings: { testMode: true, autoMerge: true } as any }); - try { - const { rootDir, store, task } = fixture; - const group = store.createBranchGroup({ - sourceType: "planning", - sourceId: "PS-U6-STALE", - branchName: "fusion/groups/fn-u6-stale", - autoMerge: true, - }); - await store.setTaskBranchGroup(task.id, group.id); - await store.updateTask(task.id, { branchContext: { groupId: group.id, source: "planning", assignmentMode: "shared" } } as any); - // Snapshot synced by this background task: open PR #13. - store.updateBranchGroup(group.id, { prState: "open", prNumber: 13, prUrl: "https://github.com/o/r/pull/13" }); - - // GitHub reports PR #13 merged out-of-band; but while we await, a newer - // landing/promotion replaces it with a newer OPEN PR #88. The stale write - // (which would mark the group merged) must be skipped so #88 survives. - const syncGroupPr: SyncGroupPrFn = vi.fn(async ({ group: g }) => { - store.updateBranchGroup(group.id, { prState: "open", prNumber: 88, prUrl: "https://github.com/o/r/pull/88" }); - return { prNumber: g.prNumber!, prUrl: g.prUrl!, prState: "merged" as const }; - }); - - let syncSettled: Promise = Promise.resolve(); - await stageMergeBranch(store, rootDir, task.id, "fnU6Stale"); - const merge = await aiMergeTask(store, rootDir, task.id, { - syncGroupPr, - onGroupPrSyncSettled: (settled) => { - syncSettled = settled; - }, - }); - expect(merge.merged).toBe(true); - await syncSettled; - // The newer open PR #88 is untouched; the stale "merged" reconciliation was skipped. - expect(store.getBranchGroup(group.id)?.prNumber).toBe(88); - expect(store.getBranchGroup(group.id)?.prState).toBe("open"); - } finally { - await fixture.cleanup(); - } - }, 45_000); }); diff --git a/packages/engine/src/merger.ts b/packages/engine/src/merger.ts index b40d1ec292..2d4b624b2c 100644 --- a/packages/engine/src/merger.ts +++ b/packages/engine/src/merger.ts @@ -7440,6 +7440,54 @@ async function tryEarlyEmptyOwnDiffFinalize(input: { return result; } +/** + * U6 (R6) sync-on-landing seam, extracted for narrow unit testing (FN-5048: the + * stale-snapshot write guard is covered in-memory, not via the slow real-git + * reliability suite). Pushes the group PR body for a group with a persisted + * open PR, then persists out-of-band reconciliation — but only when the group + * still points at the exact PR snapshot that was synced (same prNumber AND + * prState). A newer landing/promotion that swapped in a different PR mid-sync + * must not be clobbered by this stale write. + */ +export async function syncGroupPrOnLanding(input: { + store: Pick; + groupId: string; + cwd: string; + syncGroupPr: import("./group-merge-coordinator.js").SyncGroupPrFn; +}): Promise { + const { store, groupId, cwd, syncGroupPr } = input; + const latestGroup = store.getBranchGroup(groupId); + if (!latestGroup || latestGroup.prNumber == null || latestGroup.prState !== "open") { + return; + } + const members = await store.listTasksByBranchGroup(latestGroup.id); + const reconciled = await syncGroupPr({ + cwd, + group: latestGroup, + members, + }); + // Guard against stale snapshots: a newer landing/promotion may have stored a + // different (e.g. newer open) PR for this group while we were awaiting the + // sync. Re-read and only persist when the snapshot still matches. + const currentGroup = store.getBranchGroup(groupId); + if ( + !currentGroup || + currentGroup.prNumber !== latestGroup.prNumber || + currentGroup.prState !== latestGroup.prState + ) { + return; + } + // Out-of-band reconciliation: if GitHub reports the PR is no longer open + // (closed/merged), persist the corrected prState rather than leaving a stale "open". + if (reconciled.prState !== currentGroup.prState) { + store.updateBranchGroup(currentGroup.id, { + prState: reconciled.prState, + prNumber: reconciled.prNumber, + prUrl: reconciled.prUrl, + }); + } +} + export async function aiMergeTask( store: TaskStore, rootDir: string, @@ -7542,42 +7590,12 @@ export async function aiMergeTask( if (options.syncGroupPr) { const syncGroupPr = options.syncGroupPr; const groupId = groupRouting.branchGroup.id; - const settled = (async () => { - const latestGroup = store.getBranchGroup(groupId); - if (!latestGroup || latestGroup.prNumber == null || latestGroup.prState !== "open") { - return; - } - const members = await store.listTasksByBranchGroup(latestGroup.id); - const reconciled = await syncGroupPr({ - cwd: projectRootDir, - group: latestGroup, - members, - }); - // Guard against stale snapshots: a newer landing/promotion may have - // stored a different (e.g. newer open) PR for this group while we were - // awaiting the sync. Re-read the current group and only persist the - // reconciled state if it still points at the exact PR snapshot we - // synced (same prNumber AND prState); otherwise skip to avoid clobbering - // the newer PR. - const currentGroup = store.getBranchGroup(groupId); - if ( - !currentGroup || - currentGroup.prNumber !== latestGroup.prNumber || - currentGroup.prState !== latestGroup.prState - ) { - return; - } - // Out-of-band reconciliation: if GitHub reports the PR is no longer - // open (closed/merged), persist the corrected prState rather than - // leaving a stale "open". - if (reconciled.prState !== currentGroup.prState) { - store.updateBranchGroup(currentGroup.id, { - prState: reconciled.prState, - prNumber: reconciled.prNumber, - prUrl: reconciled.prUrl, - }); - } - })().catch((err) => { + const settled = syncGroupPrOnLanding({ + store, + groupId, + cwd: projectRootDir, + syncGroupPr, + }).catch((err) => { // Non-fatal: never fail the merge/landing because PR sync failed. try { store.recordRunAuditEvent({