fix(FN-branch-group): review fixes — fast-path mergeTargetSource + open-PR reuse only
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.
This commit is contained in:
@@ -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",
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -203,7 +203,7 @@ export function createGroupPrCallback(
|
||||
github: Pick<GitHubOperations, "findPrForBranch" | "createPr">,
|
||||
): 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) };
|
||||
}
|
||||
|
||||
@@ -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<Settings> = {}): TaskStore {
|
||||
function createStore(
|
||||
task: Task,
|
||||
settings: Partial<Settings> = {},
|
||||
branchGroup?: BranchGroup,
|
||||
): TaskStore {
|
||||
let currentTask = { ...task };
|
||||
const mergedSettings: Settings = {
|
||||
...DEFAULT_SETTINGS,
|
||||
@@ -69,6 +73,9 @@ function createStore(task: Task, settings: Partial<Settings> = {}): 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<typeof vi.fn>).mock.calls.some(([, column]) => column === "done")).toBe(false);
|
||||
expect((store.moveTask as ReturnType<typeof vi.fn>).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);
|
||||
});
|
||||
|
||||
@@ -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<void>;
|
||||
}): Promise<MergeResult | null> {
|
||||
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;
|
||||
|
||||
Reference in New Issue
Block a user