From 88b4b0d5b32125e460a63220c744199a8b01144c Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 3 Jun 2026 09:23:44 -0700 Subject: [PATCH] fix(FN-branch-group): unify landed/completion predicate in core (U2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Route and coordinator disagreed on landed/complete: the route required mergeConfirmed + matching mergeTargetBranch, the coordinator accepted bare column===done/in-review and never checked the branch. Extract canonical isBranchGroupMemberLanded/isBranchGroupComplete in @fusion/core (stricter route semantics win — load-bearing for merge-target safety) and consume from both sides. Tightens promotion gating to fire only when all members are merge-confirmed onto the group branch. --- .../__tests__/branch-group-completion.test.ts | 87 +++++++++++++++++++ packages/core/src/branch-group-completion.ts | 35 ++++++++ packages/core/src/index.ts | 4 + .../__tests__/routes-branch-groups.test.ts | 26 ++++++ .../routes/register-branch-groups-routes.ts | 16 ++-- .../__tests__/group-merge-coordinator.test.ts | 83 ++++++++++++++---- .../shared-branch-group-lifecycle.test.ts | 2 +- .../engine/src/group-merge-coordinator.ts | 19 ++-- 8 files changed, 239 insertions(+), 33 deletions(-) create mode 100644 packages/core/src/__tests__/branch-group-completion.test.ts create mode 100644 packages/core/src/branch-group-completion.ts diff --git a/packages/core/src/__tests__/branch-group-completion.test.ts b/packages/core/src/__tests__/branch-group-completion.test.ts new file mode 100644 index 0000000000..43634f6e5f --- /dev/null +++ b/packages/core/src/__tests__/branch-group-completion.test.ts @@ -0,0 +1,87 @@ +import { describe, expect, it } from "vitest"; + +import { isBranchGroupComplete, isBranchGroupMemberLanded } from "../branch-group-completion.js"; +import type { BranchGroup, Task } from "../types.js"; + +const GROUP_BRANCH = "fusion/groups/planning-x"; + +const group = { branchName: GROUP_BRANCH } as Pick; + +function landedMember(): Pick { + return { + mergeDetails: { + mergeConfirmed: true, + mergeTargetSource: "branch-group-integration", + mergeTargetBranch: GROUP_BRANCH, + }, + }; +} + +describe("isBranchGroupMemberLanded", () => { + it("returns true when merge is confirmed onto the group branch via integration", () => { + expect(isBranchGroupMemberLanded(landedMember(), group)).toBe(true); + }); + + it("returns false when mergeTargetBranch does not match the group branch", () => { + expect( + isBranchGroupMemberLanded( + { + mergeDetails: { + mergeConfirmed: true, + mergeTargetSource: "branch-group-integration", + mergeTargetBranch: "fusion/fn-sibling", + }, + }, + group, + ), + ).toBe(false); + }); + + it("returns false when the merge is not confirmed", () => { + expect( + isBranchGroupMemberLanded( + { + mergeDetails: { + mergeConfirmed: false, + mergeTargetSource: "branch-group-integration", + mergeTargetBranch: GROUP_BRANCH, + }, + }, + group, + ), + ).toBe(false); + }); + + it("returns false when the merge target source is not branch-group-integration", () => { + expect( + isBranchGroupMemberLanded( + { + mergeDetails: { + mergeConfirmed: true, + mergeTargetSource: "project-default", + mergeTargetBranch: GROUP_BRANCH, + }, + }, + group, + ), + ).toBe(false); + }); + + it("returns false when there are no merge details", () => { + expect(isBranchGroupMemberLanded({}, group)).toBe(false); + }); +}); + +describe("isBranchGroupComplete", () => { + it("returns true when every member is landed", () => { + expect(isBranchGroupComplete([landedMember(), landedMember()], group)).toBe(true); + }); + + it("returns false when one member is not landed", () => { + expect(isBranchGroupComplete([landedMember(), {}], group)).toBe(false); + }); + + it("returns false for an empty membership", () => { + expect(isBranchGroupComplete([], group)).toBe(false); + }); +}); diff --git a/packages/core/src/branch-group-completion.ts b/packages/core/src/branch-group-completion.ts new file mode 100644 index 0000000000..024466f076 --- /dev/null +++ b/packages/core/src/branch-group-completion.ts @@ -0,0 +1,35 @@ +import type { BranchGroup, Task } from "./types.js"; + +/** + * Canonical "member landed" predicate, shared by the dashboard branch-groups + * route and the engine group-merge coordinator so the two gates can never + * diverge (the historical divergence: the route required `mergeConfirmed` + + * matching `mergeTargetBranch`, while the coordinator accepted bare + * `column === "done"` or `in-review` + integration source and never checked + * the target branch). + * + * The stricter route semantics win: a member is landed iff it was actually + * merge-confirmed onto THIS group's branch via the branch-group-integration + * path. This is load-bearing for merge-target safety — a member marked done + * against a sibling `fusion/fn-*` branch or a mismatched branch MUST NOT count + * as landed (root cause of the 2026-05-23 lost-work incident). + */ +export function isBranchGroupMemberLanded( + task: Pick, + group: Pick, +): boolean { + return task.mergeDetails?.mergeConfirmed === true + && task.mergeDetails?.mergeTargetSource === "branch-group-integration" + && task.mergeDetails?.mergeTargetBranch === group.branchName; +} + +/** + * Canonical "group complete" predicate. A group is complete iff it has at + * least one member and every member is landed by {@link isBranchGroupMemberLanded}. + */ +export function isBranchGroupComplete( + members: Pick[], + group: Pick, +): boolean { + return members.length > 0 && members.every((member) => isBranchGroupMemberLanded(member, group)); +} diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index b448ee560e..10717c2115 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -325,6 +325,10 @@ export { type MergeTargetResolution, type MergeTargetResolverOptions, } from "./task-merge.js"; +export { + isBranchGroupMemberLanded, + isBranchGroupComplete, +} from "./branch-group-completion.js"; export { countRecentIdenticalStallEntries, getInReviewStallReason, diff --git a/packages/dashboard/src/__tests__/routes-branch-groups.test.ts b/packages/dashboard/src/__tests__/routes-branch-groups.test.ts index 727ca2afa8..2ae1436a8b 100644 --- a/packages/dashboard/src/__tests__/routes-branch-groups.test.ts +++ b/packages/dashboard/src/__tests__/routes-branch-groups.test.ts @@ -3,6 +3,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import express from "express"; import type { BranchGroup, Task, TaskStore } from "@fusion/core"; +import { evaluateBranchGroupCompletion } from "@fusion/engine"; import { createApiRoutes } from "../routes.js"; import { request as REQUEST } from "../test-request.js"; @@ -109,6 +110,31 @@ describe("branch group routes", () => { expect(res.status).toBe(400); }); + it("route serialization and coordinator agree on landed/complete for the same fixture", async () => { + // Same fixture exercised through BOTH paths must yield identical results. + const completeTasks = [buildTask("FN-1", group.id, true), buildTask("FN-2", group.id, true)]; + const mixedTasks = [buildTask("FN-1", group.id, true), buildTask("FN-2", group.id, false)]; + + // Coordinator path. + const completeCoord = evaluateBranchGroupCompletion({ members: completeTasks, group }); + const mixedCoord = evaluateBranchGroupCompletion({ members: mixedTasks, group }); + expect(completeCoord.complete).toBe(true); + expect(mixedCoord.complete).toBe(false); + + // Route serialization path. + const completeApp = buildApp(createStore(group, completeTasks)); + const completeRes = await REQUEST(completeApp, "GET", "/api/branch-groups/BG-1"); + expect(completeRes.body.group.completion.complete).toBe(true); + + const mixedApp = buildApp(createStore(group, mixedTasks)); + const mixedRes = await REQUEST(mixedApp, "GET", "/api/branch-groups/BG-1"); + expect(mixedRes.body.group.completion.complete).toBe(false); + + // No divergence between the two gates. + expect(completeRes.body.group.completion.complete).toBe(completeCoord.complete); + expect(mixedRes.body.group.completion.complete).toBe(mixedCoord.complete); + }); + it("creates group on assign when groupId absent", async () => { const store = createStore(group, tasks); const app = buildApp(store); diff --git a/packages/dashboard/src/routes/register-branch-groups-routes.ts b/packages/dashboard/src/routes/register-branch-groups-routes.ts index 04d1ad8269..2029cd7553 100644 --- a/packages/dashboard/src/routes/register-branch-groups-routes.ts +++ b/packages/dashboard/src/routes/register-branch-groups-routes.ts @@ -1,5 +1,6 @@ import { Router, type Request } from "express"; -import type { BranchGroup, Task, TaskStore } from "@fusion/core"; +import type { BranchGroup, TaskStore } from "@fusion/core"; +import { isBranchGroupComplete, isBranchGroupMemberLanded } from "@fusion/core"; import { badRequest, notFound } from "../api-error.js"; export interface BranchGroupsRouterOptions { @@ -11,19 +12,13 @@ function parseProjectId(req: Request): string | undefined { return typeof value === "string" && value.trim() ? value.trim() : undefined; } -function isMemberLanded(task: Task, group: BranchGroup): boolean { - return task.mergeDetails?.mergeConfirmed === true - && task.mergeDetails?.mergeTargetSource === "branch-group-integration" - && task.mergeDetails?.mergeTargetBranch === group.branchName; -} - async function serializeGroup(store: TaskStore, group: BranchGroup) { const members = await store.listTasksByBranchGroup(group.id); const memberRows = members.map((task) => ({ taskId: task.id, title: task.title ?? task.description, column: task.column, - landed: isMemberLanded(task, group), + landed: isBranchGroupMemberLanded(task, group), })); const landedCount = memberRows.filter((member) => member.landed).length; return { @@ -32,7 +27,7 @@ async function serializeGroup(store: TaskStore, group: BranchGroup) { completion: { landed: landedCount, total: memberRows.length, - complete: memberRows.length > 0 && landedCount === memberRows.length, + complete: isBranchGroupComplete(members, group), }, }; } @@ -100,8 +95,7 @@ export function createBranchGroupsRouter(store: TaskStore, options?: BranchGroup if (!group) throw notFound("Branch group not found"); const members = await store.listTasksByBranchGroup(group.id); - const landed = members.filter((member) => isMemberLanded(member, group)).length; - if (members.length === 0 || landed !== members.length) { + if (!isBranchGroupComplete(members, group)) { throw badRequest("Branch group completion gate not satisfied"); } diff --git a/packages/engine/src/__tests__/group-merge-coordinator.test.ts b/packages/engine/src/__tests__/group-merge-coordinator.test.ts index 5ceded3223..4096dd6320 100644 --- a/packages/engine/src/__tests__/group-merge-coordinator.test.ts +++ b/packages/engine/src/__tests__/group-merge-coordinator.test.ts @@ -30,12 +30,22 @@ afterEach(async () => { }); describe("evaluateBranchGroupCompletion", () => { - it("returns complete when all members are landed", () => { + const branchName = "fusion/groups/planning-x"; + const group = { branchName } as const; + const landed = (id: string) => ({ + id, + column: "done" as const, + mergeDetails: { + mergeConfirmed: true, + mergeTargetSource: "branch-group-integration", + mergeTargetBranch: branchName, + } as any, + }); + + it("returns complete when all members are landed onto the group branch", () => { const result = evaluateBranchGroupCompletion({ - members: [ - { id: "FN-A", column: "done" as const }, - { id: "FN-B", column: "in-review" as const, mergeDetails: { mergeTargetSource: "branch-group-integration" } as any }, - ] as any, + members: [landed("FN-A"), landed("FN-B")] as any, + group, }); expect(result).toEqual({ @@ -49,9 +59,10 @@ describe("evaluateBranchGroupCompletion", () => { it("returns pending ids when one member is not landed", () => { const result = evaluateBranchGroupCompletion({ members: [ - { id: "FN-A", column: "done" as const }, - { id: "FN-B", column: "todo" as const }, + landed("FN-A"), + { id: "FN-B", column: "todo" as const } as any, ] as any, + group, }); expect(result.complete).toBe(false); @@ -60,7 +71,7 @@ describe("evaluateBranchGroupCompletion", () => { }); it("treats empty groups as incomplete", () => { - const result = evaluateBranchGroupCompletion({ members: [] }); + const result = evaluateBranchGroupCompletion({ members: [], group }); expect(result).toEqual({ complete: false, totalMembers: 0, @@ -69,16 +80,46 @@ describe("evaluateBranchGroupCompletion", () => { }); }); - it("counts mixed done + landed in-review members as complete", () => { + it("does NOT count a member confirmed onto a mismatched branch", () => { const result = evaluateBranchGroupCompletion({ members: [ - { id: "FN-A", column: "done" as const }, - { id: "FN-B", column: "in-review" as const, mergeDetails: { mergeTargetSource: "branch-group-integration" } as any }, + landed("FN-A"), + { + id: "FN-B", + column: "done" as const, + mergeDetails: { + mergeConfirmed: true, + mergeTargetSource: "branch-group-integration", + mergeTargetBranch: "fusion/fn-sibling", + } as any, + } as any, ] as any, + group, }); - expect(result.complete).toBe(true); - expect(result.pendingMemberIds).toEqual([]); + expect(result.complete).toBe(false); + expect(result.landedMemberIds).toEqual(["FN-A"]); + expect(result.pendingMemberIds).toEqual(["FN-B"]); + }); + + it("does NOT count a member whose merge is not confirmed", () => { + const result = evaluateBranchGroupCompletion({ + members: [ + { + id: "FN-A", + column: "in-review" as const, + mergeDetails: { + mergeConfirmed: false, + mergeTargetSource: "branch-group-integration", + mergeTargetBranch: branchName, + } as any, + } as any, + ] as any, + group, + }); + + expect(result.complete).toBe(false); + expect(result.pendingMemberIds).toEqual(["FN-A"]); }); }); @@ -191,6 +232,16 @@ describe("promoteBranchGroup", () => { }; } + const landedMember = (id: string, branchName: string) => ({ + id, + column: "done" as const, + mergeDetails: { + mergeConfirmed: true, + mergeTargetSource: "branch-group-integration", + mergeTargetBranch: branchName, + }, + }); + it("returns incomplete without merging when members are pending", async () => { const rootDir = makeRepo(); const group = makeGroup(); @@ -222,7 +273,7 @@ describe("promoteBranchGroup", () => { recordAudit: async (event) => { audits.push(event as Record); }, store: { getBranchGroup: () => group, - listTasksByBranchGroup: async () => [{ id: "FN-A", column: "done" }], + listTasksByBranchGroup: async () => [landedMember("FN-A", group.branchName)], updateBranchGroup: () => { throw new Error("should not update"); }, @@ -251,7 +302,7 @@ describe("promoteBranchGroup", () => { recordAudit: async (event) => { audits.push(event as Record); }, store: { getBranchGroup: () => group, - listTasksByBranchGroup: async () => [{ id: "FN-A", column: "done" }], + listTasksByBranchGroup: async () => [landedMember("FN-A", group.branchName)], updateBranchGroup: (_id: string, patch: Partial) => { group = { ...group, ...patch }; return group; @@ -272,7 +323,7 @@ describe("promoteBranchGroup", () => { recordAudit: async (event) => { audits.push(event as Record); }, store: { getBranchGroup: () => group, - listTasksByBranchGroup: async () => [{ id: "FN-A", column: "done" }], + listTasksByBranchGroup: async () => [landedMember("FN-A", group.branchName)], updateBranchGroup: (_id: string, patch: Partial) => { group = { ...group, ...patch }; return group; diff --git a/packages/engine/src/__tests__/reliability-interactions/shared-branch-group-lifecycle.test.ts b/packages/engine/src/__tests__/reliability-interactions/shared-branch-group-lifecycle.test.ts index c35665a669..697f590464 100644 --- a/packages/engine/src/__tests__/reliability-interactions/shared-branch-group-lifecycle.test.ts +++ b/packages/engine/src/__tests__/reliability-interactions/shared-branch-group-lifecycle.test.ts @@ -215,7 +215,7 @@ describe("FN-5820 reliability interactions: shared branch group lifecycle", () = updateBranchGroup: (...args: any[]) => (store as any).updateBranchGroup(...args), listTasksByBranchGroup: async () => { const members = [await store.getTask(task.id), await store.getTask(second.id)].filter(Boolean) as any[]; - expect(evaluateBranchGroupCompletion({ members: members as any }).complete).toBe(true); + expect(evaluateBranchGroupCompletion({ members: members as any, group }).complete).toBe(true); return members as any; }, } as any, diff --git a/packages/engine/src/group-merge-coordinator.ts b/packages/engine/src/group-merge-coordinator.ts index 61d1a7b64f..204c0e511e 100644 --- a/packages/engine/src/group-merge-coordinator.ts +++ b/packages/engine/src/group-merge-coordinator.ts @@ -2,7 +2,7 @@ import { exec } from "node:child_process"; import { promisify } from "node:util"; import type { BranchGroup, BranchGroupPrState, MergeTargetResolution, Settings, Task, TaskStore } from "@fusion/core"; -import { resolveEffectiveGroupAutoMerge, resolveTaskMergeTarget } from "@fusion/core"; +import { isBranchGroupMemberLanded, resolveEffectiveGroupAutoMerge, resolveTaskMergeTarget } from "@fusion/core"; import { resolveIntegrationBranch } from "./integration-branch.js"; const execAsync = promisify(exec); @@ -41,16 +41,25 @@ export interface BranchGroupPromotionDecision { | "eligible"; } +/** + * Evaluates branch-group completion using the canonical `@fusion/core` + * `isBranchGroupMemberLanded` predicate so the engine gate can never diverge + * from the dashboard route gate. A member is landed iff it was merge-confirmed + * onto THIS group's branch via the branch-group-integration path; the group is + * complete iff it has at least one member and every member is landed. + * + * `group` (its `branchName`) is required: landing is branch-anchored, so a + * member done against a sibling/mismatched branch must NOT count as landed. + */ export function evaluateBranchGroupCompletion(input: { members: Pick[]; + group: Pick; }): BranchGroupCompletionStatus { const landedMemberIds: string[] = []; const pendingMemberIds: string[] = []; for (const member of input.members) { - const landed = member.column === "done" - || (member.column === "in-review" && member.mergeDetails?.mergeTargetSource === "branch-group-integration"); - if (landed) { + if (isBranchGroupMemberLanded(member, input.group)) { landedMemberIds.push(member.id); } else { pendingMemberIds.push(member.id); @@ -159,7 +168,7 @@ export async function promoteBranchGroup(input: { } const members = await input.store.listTasksByBranchGroup(group.id); - const completion = evaluateBranchGroupCompletion({ members }); + const completion = evaluateBranchGroupCompletion({ members, group }); if (!completion.complete) { return { groupId: group.id,