From bde7bdf766bd903f5f07bacb03051bdc83edb24a Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 3 Jun 2026 11:23:14 -0700 Subject: [PATCH] =?UTF-8?q?fix(FN-branch-group):=20review=20fixes=20?= =?UTF-8?q?=E2=80=94=20fast-path=20mergeTargetSource=20+=20open-PR=20reuse?= =?UTF-8?q?=20only?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code review (Tier 2) found two P1s: (1) the early no-op fast-path persisted mergeConfirmed/mergeTargetBranch without mergeTargetSource, so a shared-group member landing via it could never satisfy the strict completion predicate — promotion permanently blocked; thread mergeTarget.source through like the standard landing sites. (2) createGroupPrCallback's findPrForBranch used state:'all' and could reuse a closed/merged PR from a prior group, persisting a terminal prState onto a fresh promotion; create path now matches open PRs only. --- .../commands/__tests__/task-lifecycle.test.ts | 74 ++++++++++++++++ packages/cli/src/commands/task-lifecycle.ts | 2 +- .../merger-finalize-unproven.real-git.test.ts | 86 ++++++++++++++++++- packages/engine/src/merger.ts | 6 +- 4 files changed, 163 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/commands/__tests__/task-lifecycle.test.ts b/packages/cli/src/commands/__tests__/task-lifecycle.test.ts index 685a1449f2..ec2f4643cb 100644 --- a/packages/cli/src/commands/__tests__/task-lifecycle.test.ts +++ b/packages/cli/src/commands/__tests__/task-lifecycle.test.ts @@ -34,6 +34,7 @@ vi.mock("@fusion/core", async () => { import { activeSessionRegistry } from "@fusion/engine"; import { cleanupMergedTaskArtifacts, + createGroupPrCallback, processPullRequestMergeTask, getTaskBranchName, syncGroupPrCallback, @@ -1373,3 +1374,76 @@ describe("syncGroupPrCallback (U6)", () => { }); }); +describe("createGroupPrCallback", () => { + beforeEach(() => { + execMock.mockReset(); + execMock.mockImplementation(() => ""); + }); + + const group = { + id: "BG-1", + sourceType: "planning" as const, + sourceId: "P-1", + branchName: "fusion/groups/p-1", + autoMerge: false, + prState: "none" as const, + status: "open" as const, + createdAt: Date.now(), + updatedAt: Date.now(), + }; + const members = [{ id: "FN-A", title: "Alpha", description: "a", column: "in-review" } as never]; + + it("queries only OPEN PRs for the head branch (does not reuse terminal PRs)", async () => { + const github = { + findPrForBranch: vi.fn(async () => null), + createPr: vi.fn(async () => ({ + number: 99, + url: "https://github.com/owner/repo/pull/99", + status: "open" as const, + })), + }; + + const callback = createGroupPrCallback(github as never); + await callback({ + cwd: "/repo", + group: group as never, + members, + headBranch: group.branchName, + baseBranch: "main", + }); + + expect(github.findPrForBranch).toHaveBeenCalledWith({ head: group.branchName, state: "open" }); + }); + + it("does not reuse a closed PR from a prior group — creates a fresh one", async () => { + // With state:"open", findPrForBranch returns null for a head whose only PR + // is closed/merged, so the create path runs instead of resurrecting the + // terminal PR (which would poison the newly promoted group's prState). + const github = { + findPrForBranch: vi.fn(async () => null), + createPr: vi.fn(async () => ({ + number: 123, + url: "https://github.com/owner/repo/pull/123", + status: "open" as const, + })), + }; + + const callback = createGroupPrCallback(github as never); + const result = await callback({ + cwd: "/repo", + group: group as never, + members, + headBranch: group.branchName, + baseBranch: "main", + }); + + expect(github.findPrForBranch).toHaveBeenCalledWith({ head: group.branchName, state: "open" }); + expect(github.createPr).toHaveBeenCalledTimes(1); + expect(result).toEqual({ + prNumber: 123, + prUrl: "https://github.com/owner/repo/pull/123", + prState: "open", + }); + }); +}); + diff --git a/packages/cli/src/commands/task-lifecycle.ts b/packages/cli/src/commands/task-lifecycle.ts index 4f9fccf619..3e01ebfc69 100644 --- a/packages/cli/src/commands/task-lifecycle.ts +++ b/packages/cli/src/commands/task-lifecycle.ts @@ -203,7 +203,7 @@ export function createGroupPrCallback( github: Pick, ): CreateGroupPrFn { return async ({ cwd, group, members, headBranch, baseBranch }) => { - const existing = await github.findPrForBranch({ head: headBranch, state: "all" }); + const existing = await github.findPrForBranch({ head: headBranch, state: "open" }); if (existing) { return { prNumber: existing.number, prUrl: existing.url, prState: toBranchGroupPrState(existing) }; } diff --git a/packages/engine/src/__tests__/merger-finalize-unproven.real-git.test.ts b/packages/engine/src/__tests__/merger-finalize-unproven.real-git.test.ts index 6924df6ffc..2589cbc5fd 100644 --- a/packages/engine/src/__tests__/merger-finalize-unproven.real-git.test.ts +++ b/packages/engine/src/__tests__/merger-finalize-unproven.real-git.test.ts @@ -3,8 +3,8 @@ import { execSync, spawnSync } from "node:child_process"; import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import type { Settings, Task, TaskStore } from "@fusion/core"; -import { DEFAULT_SETTINGS } from "@fusion/core"; +import type { BranchGroup, Settings, Task, TaskStore } from "@fusion/core"; +import { DEFAULT_SETTINGS, isBranchGroupMemberLanded } from "@fusion/core"; vi.mock("../pi.js", () => ({ createFnAgent: vi.fn(async () => ({ session: { prompt: vi.fn(async () => undefined), dispose: vi.fn() } })), @@ -28,7 +28,11 @@ function git(repo: string, command: string): string { return execSync(command, { cwd: repo, encoding: "utf-8", stdio: ["pipe", "pipe", "pipe"] }).trim(); } -function createStore(task: Task, settings: Partial = {}): TaskStore { +function createStore( + task: Task, + settings: Partial = {}, + branchGroup?: BranchGroup, +): TaskStore { let currentTask = { ...task }; const mergedSettings: Settings = { ...DEFAULT_SETTINGS, @@ -69,6 +73,9 @@ function createStore(task: Task, settings: Partial = {}): TaskStore { getVerificationCacheHit: vi.fn(() => null), recordVerificationCachePass: vi.fn(() => undefined), upsertTaskCommitAssociation: vi.fn(async () => undefined), + getBranchGroup: vi.fn(() => branchGroup ?? null), + recordBranchGroupMemberLanded: vi.fn(async () => undefined), + recordRunAuditEvent: vi.fn(async () => undefined), } as unknown as TaskStore; } @@ -251,4 +258,77 @@ describeIfGit("aiMergeTask finalize no-op unproven reproduction (real git)", () expect((store.moveTask as ReturnType).mock.calls.some(([, column]) => column === "done")).toBe(false); expect((store.moveTask as ReturnType).mock.calls.some(([, column]) => column === "todo")).toBe(true); }, 20_000); + + // FN-5345/FN-5377 + branch-group completion regression: a shared-group member + // landing via the early empty-own-diff fast-path MUST stamp + // mergeTargetSource === "branch-group-integration" on the persisted + // mergeDetails (mirroring the standard landing paths), otherwise + // isBranchGroupMemberLanded can never match and group promotion is + // permanently blocked. + it("stamps mergeTargetSource on early no-op fast-path so a shared-group member counts as landed", async () => { + const repo = mkdtempSync(join(tmpdir(), "fusion-merger-group-noop-")); + repos.push(repo); + git(repo, "git init -b main"); + git(repo, 'git config user.email "test@example.com"'); + git(repo, 'git config user.name "Test User"'); + git(repo, "git commit --allow-empty -m 'init'"); + const baseSha = git(repo, "git rev-parse HEAD"); + + // Shared group integration branch (NOT a fusion/fn-* sibling) that the + // member's own commits net to zero against → early fast-path territory. + const groupBranch = "group/shared-integration"; + git(repo, `git checkout -b ${groupBranch}`); + git(repo, "git checkout main"); + + const memberBranch = "fusion/fn-grp-member"; + git(repo, `git checkout -b ${memberBranch} ${groupBranch}`); + // 1 own commit with zero net tree change vs the group merge-base. + git(repo, "git commit --allow-empty -m 'test(FN-GRP): handoff'"); + expect(git(repo, "git rev-parse HEAD")).not.toBe(baseSha); // aheadCount >= 1 + git(repo, "git checkout main"); + + const group: BranchGroup = { + id: "grp-1", + sourceType: "planning" as BranchGroup["sourceType"], + sourceId: "src-1", + branchName: groupBranch, + autoMerge: true, + prState: "none" as BranchGroup["prState"], + status: "open" as BranchGroup["status"], + createdAt: Date.now(), + updatedAt: Date.now(), + }; + + const task = { + id: "FN-GRP", + title: "FN-GRP", + description: "FN-GRP", + column: "in-review", + branch: memberBranch, + branchContext: { assignmentMode: "shared", groupId: group.id } as Task["branchContext"], + dependencies: [], + steps: [], + currentStep: 0, + log: [], + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + prompt: "# FN-GRP", + } as unknown as Task; + + const store = createStore(task, {}, group); + const result = await aiMergeTask(store, repo, "FN-GRP"); + + // Early no-op fast-path fired and finalized as a branch-group landing. + expect(result.noOp).toBe(true); + expect(result.merged).toBe(true); + expect(result.mergeTargetBranch).toBe(groupBranch); + expect(result.mergeTargetSource).toBe("branch-group-integration"); + + // Persisted mergeDetails carry the source so the completion predicate matches. + const persisted = await store.getTask("FN-GRP"); + expect(persisted.mergeDetails?.mergeConfirmed).toBe(true); + expect(persisted.mergeDetails?.mergeTargetBranch).toBe(groupBranch); + expect(persisted.mergeDetails?.mergeTargetSource).toBe("branch-group-integration"); + expect(isBranchGroupMemberLanded(persisted, group)).toBe(true); + }, 20_000); }); diff --git a/packages/engine/src/merger.ts b/packages/engine/src/merger.ts index c4af61163e..c5e9613d24 100644 --- a/packages/engine/src/merger.ts +++ b/packages/engine/src/merger.ts @@ -7223,9 +7223,10 @@ async function tryEarlyEmptyOwnDiffFinalize(input: { log: { warn: (m: string) => void; log: (m: string) => void }; projectRootDir: string; mergeTargetBranch: string; + mergeTargetSource: MergeDetails["mergeTargetSource"]; completeTask: (result: MergeResult) => Promise; }): Promise { - const { task, taskId, store, audit, log, projectRootDir, mergeTargetBranch } = input; + const { task, taskId, store, audit, log, projectRootDir, mergeTargetBranch, mergeTargetSource } = input; const branch = resolveTaskWorkingBranch(task); // 1. Branch exists? @@ -7287,6 +7288,7 @@ async function tryEarlyEmptyOwnDiffFinalize(input: { mergedAt, prNumber: task.prInfo?.number, mergeTargetBranch, + mergeTargetSource, }; await store.updateTask(taskId, { mergeDetails, modifiedFiles: [] }); await store.logEntry( @@ -7426,6 +7428,7 @@ async function tryEarlyEmptyOwnDiffFinalize(input: { noOpReason, mergedAt, mergeTargetBranch, + mergeTargetSource, }; await input.completeTask(result); return result; @@ -7687,6 +7690,7 @@ export async function aiMergeTask( log: mergerLog, projectRootDir, mergeTargetBranch: mergeTarget.branch, + mergeTargetSource: mergeTarget.source, completeTask: (result) => completeTask(store, taskId, result), }); if (earlyResult) return earlyResult;