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

- syncGroupPrCallback forwards owner/repo to updatePr (multi-project daemons
  could 404 or edit an unrelated same-numbered PR via process-cwd fallback)
- merger background reconcile re-reads the group before persisting and skips
  the write when the PR snapshot changed (stale-write race vs newer open PR)
- branchContext.groupId trimmed on metadata emit/parse round-trip
- triageSlice non-shared invariant assertions (no groupId, no group row)

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

packages/cli/src/commands/task-lifecycle.ts

@@ -39,7 +39,7 @@ interface GitHubOperations {
   }>;
   mergePr(params: { number: number; method?: "merge" | "squash" | "rebase" }): Promise<PrInfo>;
   getPrStatus(owner: string, repo: string, number: number): Promise<PrInfo>;
-  updatePr(params: { number: number; title?: string; body?: string }): Promise<PrInfo>;
+  updatePr(params: { owner?: string; repo?: string; number: number; title?: string; body?: string }): Promise<PrInfo>;
   closePr(params: { number: number }): Promise<PrInfo>;
 }
 
@@ -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),

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:<id> 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 () => {

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();

packages/core/src/store.ts

@@ -202,8 +202,8 @@ function parseTaskBranchContextFromSourceMetadata(sourceMetadata: Record<string,
   // groupId is optional: only shared-mode members carry one. A non-shared
   // member persists source/assignmentMode without a groupId, so a missing or
   // empty groupId must NOT discard the whole context.
-  const groupId = typeof candidate.groupId === "string" && candidate.groupId.trim()
-    ? candidate.groupId
+  const groupId = typeof candidate.groupId === "string"
+    ? candidate.groupId.trim() || undefined
     : undefined;
   if (candidate.source !== "planning" && candidate.source !== "mission" && candidate.source !== "new-task") return undefined;
   if (candidate.assignmentMode !== "shared" && candidate.assignmentMode !== "per-task-derived") return undefined;
@@ -226,7 +226,9 @@ function withTaskBranchContextInSourceMetadata(
   return {
     ...(sourceMetadata ?? {}),
     [TASK_BRANCH_CONTEXT_METADATA_KEY]: {
-      ...(branchContext.groupId ? { groupId: branchContext.groupId } : {}),
+      ...(branchContext.groupId?.trim()
+        ? { groupId: branchContext.groupId.trim() }
+        : {}),
       source: branchContext.source,
       assignmentMode: branchContext.assignmentMode,
       ...(branchContext.inheritedBaseBranch ? { inheritedBaseBranch: branchContext.inheritedBaseBranch } : {}),

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

@@ -194,4 +194,45 @@ describe("U6: group PR sync on member landing", () => {
       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<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);
 });

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,