diff --git a/packages/cli/src/commands/__tests__/task-lifecycle.test.ts b/packages/cli/src/commands/__tests__/task-lifecycle.test.ts index 1941c8ec85..6e253b7a58 100644 --- a/packages/cli/src/commands/__tests__/task-lifecycle.test.ts +++ b/packages/cli/src/commands/__tests__/task-lifecycle.test.ts @@ -1366,6 +1366,11 @@ describe("syncGroupPrCallback (U6)", () => { const result = await sync({ cwd: "/tmp/project", group: group as never, members }); expect(result).toEqual({ prNumber: 42, prUrl: "https://github.com/owner/repo/pull/42", prState: "open" }); expect(github.updatePr).toHaveBeenCalledTimes(1); + // T4: owner/repo must be forwarded so multi-project daemons target the + // resolved per-project repo, not process.cwd(). + expect(github.updatePr).toHaveBeenCalledWith( + expect.objectContaining({ owner: "owner", repo: "repo", number: 42 }), + ); const body = (github.updatePr.mock.calls[0][0] as { body: string }).body; expect(body).toContain("Completion: 0/2 landed"); expect(body).toContain("FN-A: Alpha"); diff --git a/packages/cli/src/commands/task-lifecycle.ts b/packages/cli/src/commands/task-lifecycle.ts index dc89f87f4d..3011f868e1 100644 --- a/packages/cli/src/commands/task-lifecycle.ts +++ b/packages/cli/src/commands/task-lifecycle.ts @@ -39,7 +39,7 @@ interface GitHubOperations { }>; mergePr(params: { number: number; method?: "merge" | "squash" | "rebase" }): Promise; getPrStatus(owner: string, repo: string, number: number): Promise; - updatePr(params: { number: number; title?: string; body?: string }): Promise; + updatePr(params: { owner?: string; repo?: string; number: number; title?: string; body?: string }): Promise; closePr(params: { number: number }): Promise; } @@ -287,6 +287,8 @@ export function syncGroupPrCallback( return { prNumber: current.number, prUrl: current.url, prState: currentState }; } const updated = await github.updatePr({ + owner: repo.owner, + repo: repo.repo, number: group.prNumber, title: buildGroupPullRequestTitle(group, members), body: buildGroupPrSyncBody(group, members), diff --git a/packages/core/src/__tests__/mission-store.test.ts b/packages/core/src/__tests__/mission-store.test.ts index 654561f072..6bafd91275 100644 --- a/packages/core/src/__tests__/mission-store.test.ts +++ b/packages/core/src/__tests__/mission-store.test.ts @@ -2469,6 +2469,10 @@ describe("MissionStore", () => { expect(triaged[0].id).toBe(f1.id); expect(task?.branchContext?.assignmentMode).toBe("per-task-derived"); + // Non-shared invariant: a per-task-derived member must NOT carry a groupId + // and must NOT create a synthetic mission: branch group. + expect(task?.branchContext?.groupId).toBeUndefined(); + expect(ts.getBranchGroupBySource("mission", mission.id)).toBeNull(); }); it("triageSlice respects explicit branch options over mission strategy defaults", async () => { diff --git a/packages/core/src/__tests__/store-persistence.test.ts b/packages/core/src/__tests__/store-persistence.test.ts index 690b4412ca..bad4b3b4bd 100644 --- a/packages/core/src/__tests__/store-persistence.test.ts +++ b/packages/core/src/__tests__/store-persistence.test.ts @@ -315,6 +315,27 @@ describe("TaskStore", () => { }); }); + it("canonicalizes (trims) a padded groupId when persisting branch context", async () => { + const task = await harness.store().createTask({ + description: "Padded groupId canonicalization", + branchContext: { + groupId: " BG-123 ", + source: "planning", + assignmentMode: "shared", + }, + }); + + // The persisted branch-context metadata must carry the trimmed groupId so + // it matches exact group-id comparisons later (a padded " BG-123 " would + // look valid here but fail equality checks downstream). The reloaded task + // re-parses from that metadata, so its groupId is canonical too. + const detail = await harness.store().getTask(task.id); + expect(detail.branchContext?.groupId).toBe("BG-123"); + expect(detail.sourceMetadata).toMatchObject({ + fusionBranchContext: { groupId: "BG-123" }, + }); + }); + it("round-trips branch fields through listTasks and reload", async () => { harness.store().close(); await harness.reopenDiskBackedStore(); diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index f2081ecfcd..3f7cbc63fd 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -202,8 +202,8 @@ function parseTaskBranchContextFromSourceMetadata(sourceMetadata: Record { await fixture.cleanup(); } }, 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 0717545feb..b40d1ec292 100644 --- a/packages/engine/src/merger.ts +++ b/packages/engine/src/merger.ts @@ -7553,11 +7553,25 @@ export async function aiMergeTask( 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 !== latestGroup.prState) { - store.updateBranchGroup(latestGroup.id, { + if (reconciled.prState !== currentGroup.prState) { + store.updateBranchGroup(currentGroup.id, { prState: reconciled.prState, prNumber: reconciled.prNumber, prUrl: reconciled.prUrl,