root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(FN-branch-group): address third-round PR review feedback (#1357)

Browse Source 
- abandon (route + CLI) preserves prState 'none' for groups that never had a
  PR instead of falsely persisting 'closed'; regression tests both sides
- stale-snapshot write guard extracted to syncGroupPrOnLanding and covered by
  a fast in-memory unit test (FN-5048); the slow real-git duplicate removed
This commit is contained in:
gsxdsm
2026-06-03 17:22:11 -07:00
parent d1cd8b6ed8
commit 64d02b5108
7 changed files with 171 additions and 81 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
packages/cli/src/commands/__tests__/branch-group.test.ts
View File

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

4
packages/cli/src/commands/branch-group.ts
View File

@@ -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;

16
packages/dashboard/src/__tests__/routes-branch-groups.test.ts
View File

@@ -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<typeof vi.fn>);
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)", () => {

8
packages/dashboard/src/routes/register-branch-groups-routes.ts
View File

@@ -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;

90
packages/engine/src/__tests__/group-pr-sync-on-landing.test.ts Normal file
View File

@@ -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>): 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<BranchGroup>) => {
group = { ...group, ...patch } as BranchGroup;
return group;
}),
// test hook to simulate a concurrent landing/promotion swapping the PR
_swap(patch: Partial<BranchGroup>) {
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");
});
});

40
packages/engine/src/__tests__/reliability-interactions/branch-group-pr-sync.test.ts
View File

@@ -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<void> = 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);
});

90
packages/engine/src/merger.ts
View File

@@ -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<TaskStore, "getBranchGroup" | "listTasksByBranchGroup" | "updateBranchGroup">;
groupId: string;
cwd: string;
syncGroupPr: import("./group-merge-coordinator.js").SyncGroupPrFn;
}): Promise<void> {
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({