fix(FN-branch-group): security, parity, reconcile-on-read, N+1 review residuals

Review residuals #5/#7/#8/#11/#12 + #3 wiring: forward the configured GitHub
token to the abandon route's client; guard abandon against finalized/merged
groups; reconcile an open group PR's state from GitHub on single-group reads
(merged out-of-band now flips prState); add fn branch-group abandon for
agent-native parity; block branchName shell injection (execFile argv push +
core-side branch-name validation at group creation); and collapse the
branch-groups list N+1 to a single task fetch via a shared
filterTasksByBranchGroup helper.
This commit is contained in:
gsxdsm
2026-06-03 12:43:25 -07:00
parent d9272abd0f
commit e54417c987
17 changed files with 717 additions and 39 deletions

View File

@@ -124,7 +124,7 @@ async function loadCommandHandlers() {
const { runSettingsExport } = await import("./commands/settings-export.js");
const { runSettingsImport } = await import("./commands/settings-import.js");
const { runGitStatus, runGitFetch, runGitPull, runGitPush } = await import("./commands/git.js");
const { runBranchGroupList, runBranchGroupShow, runBranchGroupPromote } = await import("./commands/branch-group.js");
const { runBranchGroupList, runBranchGroupShow, runBranchGroupPromote, runBranchGroupAbandon } = await import("./commands/branch-group.js");
const { runBackupCreate, runBackupList, runBackupRestore, runBackupCleanup } = await import("./commands/backup.js");
const { runMemoryBackupCreate, runMemoryBackupList, runMemoryBackupRestore } = await import("./commands/memory-backup.js");
const { runMissionCreate, runMissionList, runMissionShow, runMissionDelete, runMissionActivateSlice, runMissionLinkGoal, runMissionUnlinkGoal, runMissionGoals } = await import("./commands/mission.js");
@@ -188,6 +188,7 @@ async function loadCommandHandlers() {
runBranchGroupList,
runBranchGroupShow,
runBranchGroupPromote,
runBranchGroupAbandon,
runBackupCreate,
runBackupList,
runBackupRestore,
@@ -373,6 +374,8 @@ PR:
fn branch-group show <id> Show a branch group's members and completion gate
fn branch-group promote <id>
Promote a complete group (opens/links the single managed PR)
fn branch-group abandon <id>
Abandon a group (best-effort closes the managed PR)
fn agent stop <id> Stop a running agent (pause execution)
fn agent start <id> Start a stopped agent (resume execution)
fn agent import <path> [--dry-run] [--skip-existing]
@@ -634,6 +637,7 @@ async function main() {
runBranchGroupList,
runBranchGroupShow,
runBranchGroupPromote,
runBranchGroupAbandon,
runBackupCreate,
runBackupList,
runBackupRestore,
@@ -1591,9 +1595,18 @@ async function main() {
await runBranchGroupPromote(id, projectName);
break;
}
case "abandon": {
const id = args[2];
if (!id) {
console.error("Usage: fn branch-group abandon <group-id>");
process.exit(1);
}
await runBranchGroupAbandon(id, projectName);
break;
}
default:
console.error(`Unknown subcommand: branch-group ${subcommand || ""}`);
console.log("Try: fn branch-group list | show <id> | promote <id>");
console.log("Try: fn branch-group list | show <id> | promote <id> | abandon <id>");
process.exit(1);
}
break;

View File

@@ -14,8 +14,10 @@ vi.mock("@fusion/engine", () => ({
// The canonical completion predicate lives in @fusion/core; keep its real
// behavior so the CLI gate matches the dashboard route gate (parity).
const closeGroupPullRequestMock = vi.fn(async () => ({ prNumber: 55, prUrl: "https://example/pr/55", prState: "closed" as const }));
vi.mock("@fusion/dashboard", () => ({
GitHubClient: vi.fn(function GitHubClient() {}),
closeGroupPullRequest: (...args: unknown[]) => closeGroupPullRequestMock(...args),
}));
const createGroupPrCallbackMock = vi.fn(() => async () => ({ prNumber: 1, prUrl: "x", prState: "open" as const }));
@@ -24,7 +26,7 @@ vi.mock("../task-lifecycle.js", () => ({
}));
import { resolveProject } from "../../project-context.js";
import { runBranchGroupPromote, runBranchGroupList } from "../branch-group.js";
import { runBranchGroupPromote, runBranchGroupList, runBranchGroupAbandon } from "../branch-group.js";
const LANDED_TASK = {
id: "FN-1",
@@ -51,6 +53,7 @@ function makeStore(group: Record<string, unknown>, members: unknown[]) {
getBranchGroup: vi.fn(() => group),
listBranchGroups: vi.fn(() => [group]),
listTasksByBranchGroup: vi.fn(async () => members),
updateBranchGroup: vi.fn((_id: string, patch: Record<string, unknown>) => ({ ...group, ...patch })),
getSettings: vi.fn(async () => ({
autoMerge: false,
globalPause: false,
@@ -174,3 +177,86 @@ describe("branch-group CLI promote (agent-native parity)", () => {
expect(out).toContain("PR open");
});
});
describe("branch-group CLI abandon (agent-native parity, Fix #7)", () => {
let exitSpy: ReturnType<typeof vi.spyOn>;
let logSpy: ReturnType<typeof vi.spyOn>;
let errSpy: ReturnType<typeof vi.spyOn>;
beforeEach(() => {
closeGroupPullRequestMock.mockClear();
exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => {
throw new Error(`process.exit(${code})`);
}) as never);
logSpy = vi.spyOn(console, "log").mockImplementation(() => {});
errSpy = vi.spyOn(console, "error").mockImplementation(() => {});
});
afterEach(() => {
exitSpy.mockRestore();
logSpy.mockRestore();
errSpy.mockRestore();
vi.mocked(resolveProject).mockReset();
});
function mountStore(group: Record<string, unknown>) {
const store = makeStore(group, []);
vi.mocked(resolveProject).mockResolvedValue({
projectId: "p", projectPath: "/tmp/p", projectName: "p", isRegistered: true, store: store as never,
});
return store;
}
it("closes the managed PR and marks the group abandoned/closed", async () => {
const store = mountStore({ ...BASE_GROUP, prState: "open", prNumber: 55, prUrl: "https://example/pr/55" });
await runBranchGroupAbandon("BG-1");
expect(closeGroupPullRequestMock).toHaveBeenCalledTimes(1);
expect(store.updateBranchGroup).toHaveBeenCalledWith(
"BG-1",
expect.objectContaining({ status: "abandoned", prState: "closed" }),
);
expect(logSpy.mock.calls.flat().join("\n")).toContain("abandoned");
});
it("abandons without touching GitHub when there is no open PR", async () => {
const store = mountStore({ ...BASE_GROUP, prState: "none", prNumber: undefined });
await runBranchGroupAbandon("BG-1");
expect(closeGroupPullRequestMock).not.toHaveBeenCalled();
expect(store.updateBranchGroup).toHaveBeenCalledWith(
"BG-1",
expect.objectContaining({ status: "abandoned", prState: "closed" }),
);
});
it("still marks abandoned when the PR close fails (best-effort)", async () => {
const store = mountStore({ ...BASE_GROUP, prState: "open", prNumber: 55 });
closeGroupPullRequestMock.mockRejectedValueOnce(new Error("github down"));
await runBranchGroupAbandon("BG-1");
expect(store.updateBranchGroup).toHaveBeenCalledWith(
"BG-1",
expect.objectContaining({ status: "abandoned", prState: "closed" }),
);
});
it("rejects abandon of an already-merged group (terminal-state guard)", async () => {
const store = mountStore({ ...BASE_GROUP, prState: "merged", status: "open" });
await expect(runBranchGroupAbandon("BG-1")).rejects.toThrow(/process.exit/);
expect(closeGroupPullRequestMock).not.toHaveBeenCalled();
expect(store.updateBranchGroup).not.toHaveBeenCalled();
expect(errSpy.mock.calls.flat().join("\n")).toMatch(/finalized\/merged/);
});
it("rejects abandon of an already-abandoned group", async () => {
const store = mountStore({ ...BASE_GROUP, status: "abandoned", prState: "closed" });
await expect(runBranchGroupAbandon("BG-1")).rejects.toThrow(/process.exit/);
expect(store.updateBranchGroup).not.toHaveBeenCalled();
});
});

View File

@@ -4,6 +4,10 @@ import { EventEmitter } from "node:events";
// Mock child_process so we can intercept the `git push -u origin <branch>`
// call that processPullRequestMergeTask issues before createPr.
const execMock = vi.hoisted(() => vi.fn());
// Records raw (file, args[]) tuples for execFile so tests can assert a no-shell
// invocation (Fix #11) — i.e. the branch is a discrete argv entry, not shell-
// interpolated.
const execFileCalls = vi.hoisted(() => [] as Array<{ file: string; args: string[] }>);
vi.mock("node:child_process", () => ({
exec: (cmd: string, opts: unknown, cb: (err: Error | null, stdout: string, stderr: string) => void) => {
try {
@@ -15,6 +19,7 @@ vi.mock("node:child_process", () => ({
},
execFile: (file: string, args: string[] | undefined, opts: unknown, cb: (err: Error | null, stdout: string, stderr: string) => void) => {
try {
execFileCalls.push({ file, args: args ?? [] });
const result = execMock(`${file} ${(args ?? []).join(" ")}`.trim(), opts);
cb(null, typeof result === "string" ? result : "", "");
} catch (err) {
@@ -114,6 +119,7 @@ function makeStatefulStore(task: MockTask, settings: Record<string, unknown> = {
describe("processPullRequestMergeTask", () => {
beforeEach(() => {
execMock.mockReset();
execFileCalls.length = 0;
});
it("pushes the per-task branch to origin before creating a new PR", async () => {
@@ -169,12 +175,21 @@ describe("processPullRequestMergeTask", () => {
expect(github.findPrForBranch).toHaveBeenCalled();
// The git push must happen after findPrForBranch and before createPr.
const pushIdx = callOrder.findIndex((c) => c === `exec:git push -u origin "${branch}"`);
// No-shell invocation (Fix #11): the branch is now a discrete execFile arg, so
// there are no surrounding quotes in the recorded command string.
const pushIdx = callOrder.findIndex((c) => c === `exec:git push -u origin ${branch}`);
const findIdx = callOrder.indexOf("findPrForBranch");
const createIdx = callOrder.indexOf("createPr");
expect(pushIdx).toBeGreaterThan(-1);
expect(pushIdx).toBeGreaterThan(findIdx);
expect(pushIdx).toBeLessThan(createIdx);
// The push goes through execFile with the branch as a separate argv entry —
// never interpolated into a shell command — so a crafted branch name can't
// execute a subshell.
const pushCall = execFileCalls.find((c) => c.file === "git" && c.args[0] === "push");
expect(pushCall).toBeDefined();
expect(pushCall!.args).toEqual(["push", "-u", "origin", branch]);
});
it("creates shared-group PR from integration branch into default branch", async () => {

View File

@@ -1,6 +1,6 @@
import { TaskStore, isBranchGroupComplete, isBranchGroupMemberLanded, type BranchGroup, type Settings } from "@fusion/core";
import { promoteBranchGroup, resolveIntegrationBranch } from "@fusion/engine";
import { GitHubClient } from "@fusion/dashboard";
import { GitHubClient, closeGroupPullRequest } from "@fusion/dashboard";
import { resolveProject } from "../project-context.js";
import { createGroupPrCallback } from "./task-lifecycle.js";
@@ -108,6 +108,49 @@ export async function runBranchGroupShow(id: string, projectName?: string) {
console.log();
}
export async function runBranchGroupAbandon(id: string, projectName?: string) {
const { store } = await getBranchGroupContext(projectName);
const group = store.getBranchGroup(id);
if (!group) {
console.error(`\n ✗ Branch group ${id} not found\n`);
process.exit(1);
}
// Terminal-state guard — same semantics as the dashboard abandon route (Fix #2):
// a finalized/merged or already-abandoned group cannot be abandoned.
if (group.status === "abandoned" || group.status === "finalized" || group.prState === "merged") {
console.error(`\n ✗ Branch group ${id} is already ${group.status === "abandoned" ? "abandoned" : "finalized/merged"} and cannot be abandoned\n`);
process.exit(1);
}
let prState: BranchGroup["prState"] = "closed";
let prNumber = group.prNumber;
let prUrl = group.prUrl;
// Best-effort close of the single managed GitHub PR (R7). If it fails, still
// mark the row abandoned/closed and leave the PR for out-of-band reconciliation.
if (group.prState === "open" && group.prNumber != null) {
try {
const github = new GitHubClient(process.env.GITHUB_TOKEN);
const reconciled = await closeGroupPullRequest(github, group);
prState = reconciled.prState;
prNumber = reconciled.prNumber;
prUrl = reconciled.prUrl;
} catch (err) {
console.error(` ! Could not close GitHub PR (left for out-of-band reconciliation): ${err instanceof Error ? err.message : String(err)}`);
}
}
const updated = store.updateBranchGroup(id, {
status: "abandoned",
prState,
prNumber: prNumber ?? null,
prUrl: prUrl ?? null,
});
console.log(`\n ✓ Branch group ${updated.id} abandoned (status: ${updated.status}, prState: ${updated.prState})\n`);
}
export async function runBranchGroupPromote(id: string, projectName?: string) {
const { store, projectPath } = await getBranchGroupContext(projectName);
const group = store.getBranchGroup(id);

View File

@@ -13,9 +13,10 @@
* - Full PR lifecycle orchestration (create → status check → merge)
*/
import { exec } from "node:child_process";
import { exec, execFile } from "node:child_process";
import { promisify } from "node:util";
const execAsync = promisify(exec);
const execFileAsync = promisify(execFile);
import type { TaskStore } from "@fusion/core";
import { resolveTaskMergeTarget, getCurrentRepo, isBranchGroupMemberLanded } from "@fusion/core";
import type { Settings, TaskDetail, PrInfo, MergeResult, BranchGroup, BranchGroupPrState, Task } from "@fusion/core";
@@ -107,7 +108,9 @@ async function pushTaskBranchToOrigin(cwd: string, branch: string): Promise<void
}
try {
await execAsync(`git push -u origin "${branch}"`, {
// No-shell invocation (Fix #11): pass the branch as a discrete argv entry so a
// crafted branch name (e.g. `$(...)`) cannot be interpreted by a shell.
await execFileAsync("git", ["push", "-u", "origin", branch], {
cwd,
timeout: 60_000,
});

View File

@@ -4,8 +4,74 @@ import {
derivePerTaskBranchName,
resolveEntryPointBranchAssignment,
sanitizeBranchSegment,
isValidBranchGroupBranchName,
validateBranchGroupBranchName,
filterTasksByBranchGroup,
} from "../branch-assignment.js";
describe("isValidBranchGroupBranchName (Fix #11)", () => {
it("accepts legitimate branch names", () => {
for (const name of ["feature/auth-shared", "fusion/fn-123", "main", "release/v1.2.3", "fn/shared", "a"]) {
expect(isValidBranchGroupBranchName(name)).toBe(true);
}
});
it("rejects injection-shaped and unsafe names", () => {
for (const name of [
"$(touch /tmp/x)",
"`whoami`",
"feature; rm -rf /",
"a|b",
"a&b",
"branch with spaces",
"-leading-dash",
'has"quote',
"has'quote",
"back\\slash",
"a..b",
"a~b",
"a^b",
"a:b",
"trailing/",
"/leading",
"",
" ",
"tail.lock",
]) {
expect(isValidBranchGroupBranchName(name)).toBe(false);
}
});
it("validateBranchGroupBranchName throws on invalid and returns valid", () => {
expect(validateBranchGroupBranchName("feature/ok")).toBe("feature/ok");
expect(() => validateBranchGroupBranchName("$(touch /tmp/x)")).toThrow(/Invalid branch group branch name/);
});
});
describe("filterTasksByBranchGroup (Fix #8/#9)", () => {
const tasks = [
{ id: "T1", branchContext: { groupId: "BG-1" } },
{ id: "T2", branchContext: { groupId: "planning:PS-1" } },
{ id: "T3", branchContext: { groupId: "BG-2" } },
{ id: "T4", branchContext: undefined },
];
it("matches the real BG id", () => {
const group = { id: "BG-2", sourceType: "planning", sourceId: "PS-2" };
expect(filterTasksByBranchGroup(tasks, group, "BG-2").map((t) => t.id)).toEqual(["T3"]);
});
it("also matches the legacy synthetic groupId for planning/mission groups", () => {
const group = { id: "BG-1", sourceType: "planning", sourceId: "PS-1" };
expect(filterTasksByBranchGroup(tasks, group, "BG-1").map((t) => t.id).sort()).toEqual(["T1", "T2"]);
});
it("does not apply the legacy fallback for non-planning/mission sources", () => {
const group = { id: "BG-1", sourceType: "task", sourceId: "PS-1" };
expect(filterTasksByBranchGroup(tasks, group, "BG-1").map((t) => t.id)).toEqual(["T1"]);
});
});
describe("branch-assignment", () => {
it("sanitizes branch segments", () => {
expect(sanitizeBranchSegment(" FN-123 add parser!!! ")).toBe("fn-123-add-parser");

View File

@@ -84,6 +84,20 @@ describe("TaskStore branch groups", () => {
).toThrow();
});
it("rejects injection-shaped branch names at createBranchGroup (Fix #11)", () => {
for (const bad of ["$(touch /tmp/x)", "`cmd`", "feature; rm -rf /", "has space", "a|b"]) {
expect(() =>
store.createBranchGroup({ sourceType: "planning", sourceId: `bad-${bad}`, branchName: bad }),
).toThrow(/Invalid branch group branch name/);
}
// ensureBranchGroupForSource shares the createBranchGroup path → also rejected.
expect(() =>
store.ensureBranchGroupForSource("planning", "PS-inj", { branchName: "$(evil)", autoMerge: false }),
).toThrow(/Invalid branch group branch name/);
// Legitimate names still pass.
expect(store.createBranchGroup({ sourceType: "planning", sourceId: "PS-good", branchName: "feature/auth-shared" }).branchName).toBe("feature/auth-shared");
});
it("finds open branch groups by branch name and ignores closed groups", () => {
expect(store.getBranchGroupByBranchName("fn/missing")).toBeNull();

View File

@@ -11,6 +11,68 @@ export interface EntryPointBranchAssignment {
mergeTargetBranch?: string;
}
/**
* Conservative git-ref-safe validation for a branch-group branch name, enforced
* at the persistence boundary (Fix #11). Branch names flow into shell-adjacent
* git invocations across the coordinator/merger; rejecting injection-shaped names
* at group creation blocks the shell-injection path at the source for every
* downstream sink. Legitimate names (slashes, dots, dashes — e.g. `feature/auth`,
* `fusion/fn-123`) must still pass; only names that could break out of an arg
* (whitespace, `$`, backtick, `;`, `|`, `&`, quotes, parens/braces/brackets,
* angle brackets, leading dash, refspec specials) are rejected.
*/
export function isValidBranchGroupBranchName(name: string): boolean {
if (typeof name !== "string") return false;
const trimmed = name.trim();
if (trimmed.length === 0) return false;
if (trimmed !== name) return false; // surrounding whitespace
if (name.length > 255) return false;
if (name.startsWith("-")) return false;
if (/\s/.test(name)) return false;
// Shell / refspec metacharacters that could escape a single git arg.
if (/[$`;|&<>(){}\[\]"'\\!*?~^:]/.test(name)) return false;
if (name.includes("..")) return false;
if (name.includes("@{")) return false;
if (name.startsWith("/") || name.endsWith("/") || name.endsWith(".") || name.endsWith(".lock")) return false;
const reserved = ["HEAD", "FETCH_HEAD", "ORIG_HEAD", "MERGE_HEAD", "CHERRY_PICK_HEAD"];
if (reserved.includes(name)) return false;
return true;
}
/** Throwing wrapper used at the store persistence boundary. */
export function validateBranchGroupBranchName(name: string): string {
if (!isValidBranchGroupBranchName(name)) {
throw new Error(`Invalid branch group branch name: ${JSON.stringify(name)}`);
}
return name;
}
/**
* Pure membership filter shared by `TaskStore.listTasksByBranchGroup` and the
* dashboard list route (Fix #8/#9) so the legacy synthetic-groupId fallback
* semantics can't drift between the two call sites. Groups created before the
* membership-identity fix stamped `branchContext.groupId` with a synthetic
* `<sourceType>:<sourceId>` string instead of the real `BG-` id; this matches
* both forms. Caller is responsible for sorting.
*/
export function filterTasksByBranchGroup<
T extends { branchContext?: { groupId?: string } | null },
>(
tasks: T[],
group: { id: string; sourceType?: string; sourceId?: string } | null | undefined,
groupId: string,
): T[] {
const legacyGroupId =
group && (group.sourceType === "planning" || group.sourceType === "mission")
? `${group.sourceType}:${group.sourceId}`
: undefined;
return tasks.filter(
(task) =>
task.branchContext?.groupId === groupId ||
(legacyGroupId !== undefined && task.branchContext?.groupId === legacyGroupId),
);
}
export function sanitizeBranchSegment(input: string): string {
return input
.trim()

View File

@@ -6,6 +6,9 @@ export {
sanitizeBranchSegment,
derivePerTaskBranchName,
deriveAutoTaskBranchName,
isValidBranchGroupBranchName,
validateBranchGroupBranchName,
filterTasksByBranchGroup,
} from "./branch-assignment.js";
export type {
EntryPointAssignmentMode,

View File

@@ -9,6 +9,7 @@ import { VALID_TRANSITIONS, DEFAULT_SETTINGS, isGlobalOnlySettingsKey, WORKFLOW_
import { DEFAULT_PROJECT_SETTINGS } from "./settings-schema.js";
import { resolveWorktrunkSettings, validateWorktrunkSettings } from "./worktrunk-settings.js";
import { normalizeTaskPriority } from "./task-priority.js";
import { validateBranchGroupBranchName, filterTasksByBranchGroup } from "./branch-assignment.js";
import { canAgentTakeImplementationTaskForExplicitRouting } from "./agent-role-policy.js";
import { GlobalSettingsStore } from "./global-settings.js";
import { Database, SCHEMA_VERSION, toJson, toJsonNullable, fromJson } from "./db.js";
@@ -4336,6 +4337,9 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
}
createBranchGroup(input: BranchGroupCreateInput): BranchGroup {
// Fix #11: reject injection-shaped branch names at the persistence boundary
// so they can never reach a downstream git/shell sink (coordinator, merger).
validateBranchGroupBranchName(input.branchName);
const now = Date.now();
const id = this.generateBranchGroupId();
this.db.prepare(`
@@ -4474,23 +4478,13 @@ export class TaskStore extends EventEmitter<TaskStoreEvents> {
async listTasksByBranchGroup(groupId: string): Promise<Task[]> {
const tasks = await this.listTasks({ includeArchived: false, slim: true });
// LEGACY SHIM (removable): groups created before the membership-identity fix
// stamped branchContext.groupId with a synthetic string (`planning:<sourceId>` /
// `mission:<sourceId>`) instead of the real `BG-` id. Derive that synthetic form
// from the group's source so those old rows still enumerate. New rows match on the
// real id directly; this fallback can be deleted once no legacy groups remain.
// Membership filter (incl. legacy synthetic-groupId fallback) is shared with
// the dashboard list route via `filterTasksByBranchGroup` so semantics can't
// drift between the two call sites (Fix #8/#9).
const group = this.getBranchGroup(groupId);
const legacyGroupId =
group && (group.sourceType === "planning" || group.sourceType === "mission")
? `${group.sourceType}:${group.sourceId}`
: undefined;
return tasks
.filter(
(task) =>
task.branchContext?.groupId === groupId ||
(legacyGroupId !== undefined && task.branchContext?.groupId === legacyGroupId),
)
.sort((a, b) => a.createdAt.localeCompare(b.createdAt));
return filterTasksByBranchGroup(tasks, group, groupId).sort((a, b) =>
a.createdAt.localeCompare(b.createdAt),
);
}
recordBranchGroupMemberLanded(

View File

@@ -16,7 +16,7 @@ vi.mock("@fusion/core", async () => {
});
import { runGh, runGhJsonAsync, isGhAvailable, isGhAuthenticated } from "@fusion/core";
import { GitHubClient, closeGroupPullRequest } from "../github.js";
import { GitHubClient, closeGroupPullRequest, reconcileGroupPullRequest } from "../github.js";
const mockRunGh = vi.mocked(runGh);
const mockRunGhJsonAsync = vi.mocked(runGhJsonAsync);
@@ -72,3 +72,29 @@ describe("closeGroupPullRequest", () => {
expect(mockRunGh.mock.calls.find((c) => c[0]?.[1] === "close")).toBeUndefined();
});
});
describe("reconcileGroupPullRequest (Fix #3)", () => {
beforeEach(() => {
vi.clearAllMocks();
mockIsGhAvailable.mockReturnValue(true);
mockIsGhAuthenticated.mockReturnValue(true);
});
it("maps a merged GitHub PR to prState=merged without mutating it", async () => {
mockRunGhJsonAsync.mockResolvedValue({ ...ghPrViewOpen, state: "MERGED" } as any);
const client = new GitHubClient({ forceMode: undefined as never });
const result = await reconcileGroupPullRequest(client, { id: group.id, prNumber: group.prNumber });
expect(result.prState).toBe("merged");
// Pure read — never edits or closes.
expect(mockRunGh.mock.calls.find((c) => c[0]?.[1] === "close" || c[0]?.[1] === "edit")).toBeUndefined();
});
it("returns prState=open for a still-open PR", async () => {
mockRunGhJsonAsync.mockResolvedValue(ghPrViewOpen as any);
const client = new GitHubClient({ forceMode: undefined as never });
const result = await reconcileGroupPullRequest(client, { id: group.id, prNumber: group.prNumber });
expect(result.prState).toBe("open");
});
});

View File

@@ -0,0 +1,87 @@
// @vitest-environment node
import { describe, expect, it, vi, beforeEach } from "vitest";
import express from "express";
import type { BranchGroup, Task, TaskStore } from "@fusion/core";
import { request as REQUEST } from "../test-request.js";
// Capture how GitHubClient is constructed so we can assert the configured token
// is forwarded (Fix #1) into the abandon/reconcile close path.
const ctorCalls: Array<unknown> = [];
vi.mock("../github.js", () => {
class GitHubClient {
constructor(tokenOrOptions?: unknown) {
ctorCalls.push(tokenOrOptions);
}
}
return {
GitHubClient,
closeGroupPullRequest: vi.fn(async (_client: unknown, group: { prNumber: number; prUrl?: string }) => ({
prNumber: group.prNumber,
prUrl: group.prUrl ?? "https://example/pr",
prState: "closed" as const,
})),
reconcileGroupPullRequest: vi.fn(async () => ({ prNumber: 0, prUrl: "", prState: "open" as const })),
};
});
// reconcileBranchGroupPr is real-ish but harmless here; stub to avoid GitHub.
vi.mock("@fusion/engine", async () => {
const actual = await vi.importActual<typeof import("@fusion/engine")>("@fusion/engine");
return { ...actual, reconcileBranchGroupPr: vi.fn(async () => ({ reconciled: false, prState: "open", prNumber: null, prUrl: null })) };
});
import { registerIntegratedRouters } from "../routes/register-integrated-routers.js";
function buildGroup(): BranchGroup {
return {
id: "BG-TOK",
sourceType: "planning",
sourceId: "PS-TOK",
branchName: "feature/tok",
autoMerge: false,
prState: "open",
prNumber: 99,
prUrl: "https://example/pr/99",
status: "open",
createdAt: Date.now(),
updatedAt: Date.now(),
};
}
function buildStore(group: BranchGroup): TaskStore {
let current = { ...group };
return {
getRootDir: vi.fn(() => "/tmp/project"),
getBranchGroup: vi.fn(() => current),
listBranchGroups: vi.fn(() => [current]),
listTasks: vi.fn(async () => [] as Task[]),
listTasksByBranchGroup: vi.fn(async () => [] as Task[]),
updateBranchGroup: vi.fn((_id: string, patch: Partial<BranchGroup>) => {
current = { ...current, ...patch };
return current;
}),
} as unknown as TaskStore;
}
describe("integrated branch-groups router — GitHub token wiring (Fix #1)", () => {
beforeEach(() => {
ctorCalls.length = 0;
});
it("forwards options.githubToken into GitHubClient for the abandon close path", async () => {
const store = buildStore(buildGroup());
const router = express.Router();
registerIntegratedRouters({ router, store, options: { githubToken: "ghp_test_secret" } as any });
const app = express();
app.use(express.json());
app.use("/api", router);
const res = await REQUEST(app, "POST", "/api/branch-groups/BG-TOK/abandon", JSON.stringify({}), { "content-type": "application/json" });
expect(res.status).toBe(200);
// The closeGroupPr callback constructed a GitHubClient with the configured token.
expect(ctorCalls).toContain("ghp_test_secret");
});
});

View File

@@ -6,8 +6,22 @@ import type { BranchGroup, Task, TaskStore } from "@fusion/core";
import { evaluateBranchGroupCompletion, ProjectEngine } from "@fusion/engine";
import { createApiRoutes } from "../routes.js";
import { createBranchGroupsRouter } from "../routes/register-branch-groups-routes.js";
import { ApiError, sendErrorResponse } from "../api-error.js";
import { request as REQUEST } from "../test-request.js";
// Standalone routers (mounted without createApiRoutes) need the same error
// middleware createApiRoutes provides, so thrown ApiErrors become HTTP responses
// instead of hanging the request.
function attachErrorHandler(app: express.Express) {
app.use((err: unknown, _req: express.Request, res: express.Response, _next: express.NextFunction) => {
if (err instanceof ApiError) {
sendErrorResponse(res, err.statusCode, err.message, { details: err.details });
return;
}
sendErrorResponse(res, 500, err instanceof Error ? err.message : "Internal server error");
});
}
function buildTask(id: string, groupId: string, landed: boolean): Task {
return {
id,
@@ -30,6 +44,7 @@ function createStore(group: BranchGroup, tasks: Task[]): TaskStore {
getRootDir: vi.fn(() => "/tmp/project"),
listBranchGroups: vi.fn(() => [group]),
getBranchGroup: vi.fn((id: string) => (id === group.id ? group : null)),
listTasks: vi.fn(async () => tasks),
listTasksByBranchGroup: vi.fn(async () => tasks),
setTaskBranchGroup: vi.fn(async () => {}),
ensureBranchGroupForSource: vi.fn(() => group),
@@ -237,6 +252,7 @@ describe("branch group abandon (U6, R7)", () => {
const app = express();
app.use(express.json());
app.use("/branch-groups", createBranchGroupsRouter(store, { closeGroupPr }));
attachErrorHandler(app);
return app;
}
@@ -276,16 +292,179 @@ describe("branch group abandon (U6, R7)", () => {
expect(res.body.group.status).toBe("abandoned");
});
it("preserves prState=merged on abandon if the group was already merged", async () => {
it("rejects abandon of an already-merged group with 400 (Fix #2)", async () => {
const merged = { ...buildOpenGroup(), prState: "merged" as const };
const { store } = buildAbandonStore(merged);
const { store, updateBranchGroup } = buildAbandonStore(merged);
const closeGroupPr = vi.fn();
const app = mount(store, closeGroupPr as unknown as ReturnType<typeof vi.fn>);
const res = await REQUEST(app, "POST", "/branch-groups/BG-AB/abandon", JSON.stringify({}), { "content-type": "application/json" });
expect(res.status).toBe(200);
// Already merged → do not close; keep merged terminal state.
// Terminal state — must not flip to abandoned/closed.
expect(res.status).toBe(400);
expect(closeGroupPr).not.toHaveBeenCalled();
expect(res.body.group.prState).toBe("merged");
expect(updateBranchGroup).not.toHaveBeenCalled();
});
it("rejects abandon of a finalized group with 400 (Fix #2)", async () => {
const finalized = { ...buildOpenGroup(), status: "finalized" as const };
const { store, updateBranchGroup } = buildAbandonStore(finalized);
const closeGroupPr = vi.fn();
const app = mount(store, closeGroupPr as unknown as ReturnType<typeof vi.fn>);
const res = await REQUEST(app, "POST", "/branch-groups/BG-AB/abandon", JSON.stringify({}), { "content-type": "application/json" });
expect(res.status).toBe(400);
expect(closeGroupPr).not.toHaveBeenCalled();
expect(updateBranchGroup).not.toHaveBeenCalled();
});
});
describe("branch group reconcile-on-read (Fix #3)", () => {
function buildOpenGroup(): BranchGroup {
return {
id: "BG-RC",
sourceType: "planning",
sourceId: "PS-RC",
branchName: "feature/shared-rc",
autoMerge: false,
prState: "open",
prNumber: 77,
prUrl: "https://example/pr/77",
status: "open",
createdAt: Date.now(),
updatedAt: Date.now(),
};
}
function buildStore(initial: BranchGroup) {
let current = { ...initial };
const store = {
getRootDir: vi.fn(() => "/tmp/project"),
getBranchGroup: vi.fn(() => current),
listTasksByBranchGroup: vi.fn(async () => [] as Task[]),
updateBranchGroup: vi.fn((_id: string, patch: Partial<BranchGroup>) => {
current = { ...current, ...patch };
return current;
}),
} as unknown as TaskStore;
return { store, getCurrent: () => current };
}
function mount(store: TaskStore, reconcileGroupPr?: ReturnType<typeof vi.fn>) {
const app = express();
app.use(express.json());
app.use("/branch-groups", createBranchGroupsRouter(store, { reconcileGroupPr }));
attachErrorHandler(app);
return app;
}
it("flips prState to merged and persists when the injected reconcile reports merged", async () => {
const { store, getCurrent } = buildStore(buildOpenGroup());
const reconcileGroupPr = vi.fn(async ({ group }: { group: BranchGroup }) => {
// Mirror the wired callback: persist via the store, then return fresh row.
store.updateBranchGroup(group.id, { prState: "merged", prNumber: 77, prUrl: group.prUrl ?? null });
return getCurrent();
});
const app = mount(store, reconcileGroupPr);
const res = await REQUEST(app, "GET", "/branch-groups/BG-RC");
expect(res.status).toBe(200);
expect(reconcileGroupPr).toHaveBeenCalledTimes(1);
expect(res.body.group.prState).toBe("merged");
expect(getCurrent().prState).toBe("merged");
});
it("returns 200 with stale state when the reconcile callback throws", async () => {
const { store } = buildStore(buildOpenGroup());
const reconcileGroupPr = vi.fn(async () => { throw new Error("github down"); });
const app = mount(store, reconcileGroupPr);
const res = await REQUEST(app, "GET", "/branch-groups/BG-RC");
expect(res.status).toBe(200);
expect(reconcileGroupPr).toHaveBeenCalledTimes(1);
expect(res.body.group.prState).toBe("open");
});
it("does not reconcile when the group has no open PR", async () => {
const noPr = { ...buildOpenGroup(), prState: "none" as const, prNumber: undefined };
const { store } = buildStore(noPr);
const reconcileGroupPr = vi.fn();
const app = mount(store, reconcileGroupPr as unknown as ReturnType<typeof vi.fn>);
const res = await REQUEST(app, "GET", "/branch-groups/BG-RC");
expect(res.status).toBe(200);
expect(reconcileGroupPr).not.toHaveBeenCalled();
});
});
describe("branch group list N+1 elimination (Fix #6)", () => {
function buildGroups(): BranchGroup[] {
const base = {
sourceType: "planning" as const,
autoMerge: false,
prState: "open" as const,
status: "open" as const,
createdAt: Date.now(),
updatedAt: Date.now(),
};
return [
{ ...base, id: "BG-A", sourceId: "PS-A", branchName: "feature/a" },
{ ...base, id: "BG-B", sourceId: "PS-B", branchName: "feature/b" },
{ ...base, id: "BG-C", sourceId: "PS-C", branchName: "feature/c" },
];
}
// Landed requires mergeTargetBranch === the group's branchName, so build tasks
// with a branch that matches their group.
function memberTask(id: string, groupId: string, branchName: string, landed: boolean): Task {
return {
id,
description: id,
column: landed ? "done" : "in-progress",
dependencies: [],
steps: [],
currentStep: 1,
createdAt: new Date().toISOString(),
updatedAt: new Date().toISOString(),
branchContext: { groupId, source: "planning", assignmentMode: "shared" },
mergeDetails: landed
? { mergeConfirmed: true, mergeTargetSource: "branch-group-integration", mergeTargetBranch: branchName }
: undefined,
} as Task;
}
it("issues exactly ONE listTasks call regardless of group count, with identical results", async () => {
const groups = buildGroups();
const tasks: Task[] = [
memberTask("FN-A1", "BG-A", "feature/a", true),
memberTask("FN-A2", "BG-A", "feature/a", false),
memberTask("FN-B1", "BG-B", "feature/b", true),
];
const listTasks = vi.fn(async () => tasks);
// listTasksByBranchGroup must NOT be used by the list route anymore.
const listTasksByBranchGroup = vi.fn(async (groupId: string) =>
tasks.filter((t) => t.branchContext?.groupId === groupId),
);
const store = {
getRootDir: vi.fn(() => "/tmp/project"),
listBranchGroups: vi.fn(() => groups),
getBranchGroup: vi.fn((id: string) => groups.find((g) => g.id === id) ?? null),
listTasks,
listTasksByBranchGroup,
} as unknown as TaskStore;
const app = express();
app.use(express.json());
app.use("/branch-groups", createBranchGroupsRouter(store));
attachErrorHandler(app);
const res = await REQUEST(app, "GET", "/branch-groups");
expect(res.status).toBe(200);
expect(listTasks).toHaveBeenCalledTimes(1);
expect(listTasksByBranchGroup).not.toHaveBeenCalled();
const byId = Object.fromEntries(res.body.groups.map((g: { id: string }) => [g.id, g]));
expect(byId["BG-A"].completion).toEqual({ landed: 1, total: 2, complete: false });
expect(byId["BG-B"].completion).toEqual({ landed: 1, total: 1, complete: true });
expect(byId["BG-C"].completion).toEqual({ landed: 0, total: 0, complete: false });
});
});

View File

@@ -3847,6 +3847,31 @@ export interface CreateGroupPrResult {
prState: BranchGroupPrState;
}
/**
* Read-only reconciliation of the single managed group PR against GitHub (Fix
* #3). Reads the current PR status and maps it to the persisted `prState`. Used
* by the dashboard's single-group read path (`GET /branch-groups/:id`) to flip
* `prState` → merged/closed when the PR was merged/closed out-of-band. Does not
* mutate the PR; if GitHub still reports it open, returns the open state so the
* caller writes nothing.
*/
export async function reconcileGroupPullRequest(
github: Pick<GitHubClient, "getPrStatus">,
group: Pick<BranchGroup, "id" | "prNumber">,
): Promise<CreateGroupPrResult> {
const prNumber = group.prNumber;
if (prNumber == null) {
throw new Error(`reconcileGroupPullRequest: group ${group.id} has no persisted prNumber`);
}
const { owner, repo } = getCurrentRepoOrThrow();
const current = await github.getPrStatus(owner, repo, prNumber);
return {
prNumber: current.number,
prUrl: current.url,
prState: prInfoToBranchGroupPrState(current),
};
}
/**
* Close the single managed group PR (U6, R7) — best-effort terminal
* reconciliation when a branch group is abandoned. If the PR is already

View File

@@ -11,7 +11,7 @@ export {
type RuntimeLogSink,
} from "./runtime-logger.js";
export { createSkillsAdapter, getProjectSettingsPath, type SkillsAdapter, type DiscoveredSkill, type CatalogEntry, type CatalogFetchResult, type ToggleSkillResult, type UpstreamError, type UpstreamErrorCode, type SkillContent, type SkillFileEntry } from "./skills-adapter.js";
export { GitHubClient, isPrMergeReady, closeGroupPullRequest, type GitHubClientOptions, type PrMergeStatus, type PrCheckStatus, type ReviewDecision, type MergePrParams, type UpdatePrParams, type ClosePrParams, type FindPrParams, type CreateIssueParams, type CreatedIssue, type CreateGroupPrResult } from "./github.js";
export { GitHubClient, isPrMergeReady, closeGroupPullRequest, reconcileGroupPullRequest, type GitHubClientOptions, type PrMergeStatus, type PrCheckStatus, type ReviewDecision, type MergePrParams, type UpdatePrParams, type ClosePrParams, type FindPrParams, type CreateIssueParams, type CreatedIssue, type CreateGroupPrResult } from "./github.js";
export { generatePrMetadata, type GeneratedPrMetadata } from "./pr-metadata-generator.js";
export { maybeCreateTrackingIssue, type MaybeCreateTrackingIssueDeps } from "./github-tracking.js";
export {

View File

@@ -1,6 +1,6 @@
import { Router, type Request } from "express";
import type { BranchGroup, TaskStore } from "@fusion/core";
import { isBranchGroupComplete, isBranchGroupMemberLanded } from "@fusion/core";
import type { BranchGroup, Task, TaskStore } from "@fusion/core";
import { isBranchGroupComplete, isBranchGroupMemberLanded, filterTasksByBranchGroup } from "@fusion/core";
import { badRequest, notFound } from "../api-error.js";
export interface BranchGroupsRouterOptions {
@@ -16,6 +16,18 @@ export interface BranchGroupsRouterOptions {
group: BranchGroup;
projectId?: string;
}) => Promise<{ prNumber: number; prUrl: string; prState: BranchGroup["prState"] } | null>;
/**
* Out-of-band PR reconciliation on single-group read (Fix #3): when a group has
* an open managed PR, this is invoked best-effort before serialization so a PR
* merged/closed directly on GitHub flips `prState` accordingly. Wired over the
* engine's `reconcileBranchGroupPr` + a GitHub-backed `SyncGroupPrFn`. Omitted
* (or throwing) leaves the persisted state untouched. Only the single-group
* GET path calls this — the list stays cheap.
*/
reconcileGroupPr?: (input: {
group: BranchGroup;
projectId?: string;
}) => Promise<BranchGroup>;
}
function parseProjectId(req: Request): string | undefined {
@@ -23,8 +35,18 @@ function parseProjectId(req: Request): string | undefined {
return typeof value === "string" && value.trim() ? value.trim() : undefined;
}
async function serializeGroup(store: TaskStore, group: BranchGroup) {
const members = await store.listTasksByBranchGroup(group.id);
/**
* Serialize a single group. Pass `allTasks` to filter membership in memory from a
* single up-front `listTasks` call (list route, Fix #8/#9 — avoids the N+1 scan);
* omit it to fall back to a per-group `listTasksByBranchGroup` scan (single-group
* read / abandon, where one scan is fine).
*/
async function serializeGroup(store: TaskStore, group: BranchGroup, allTasks?: Task[]) {
const members = allTasks
? filterTasksByBranchGroup(allTasks, group, group.id).sort((a, b) =>
a.createdAt.localeCompare(b.createdAt),
)
: await store.listTasksByBranchGroup(group.id);
const memberRows = members.map((task) => ({
taskId: task.id,
title: task.title ?? task.description,
@@ -54,15 +76,31 @@ export function createBranchGroupsRouter(store: TaskStore, options?: BranchGroup
}
const groups = store.listBranchGroups(status ? { status: status as BranchGroup["status"] } : undefined);
const data = await Promise.all(groups.map((group) => serializeGroup(store, group)));
// Fix #8/#9: fetch tasks ONCE and filter per group in memory rather than one
// full scan per group (the old N+1). Membership semantics (incl. legacy
// synthetic-groupId fallback) come from the shared `filterTasksByBranchGroup`.
const allTasks = await store.listTasks({ includeArchived: false, slim: true });
const data = await Promise.all(groups.map((group) => serializeGroup(store, group, allTasks)));
res.json({ groups: data });
});
router.get("/:id", async (req, res) => {
const id = String(req.params.id ?? "").trim();
if (!id) throw badRequest("id is required");
const group = store.getBranchGroup(id);
let group = store.getBranchGroup(id);
if (!group) throw notFound("Branch group not found");
// Fix #3: reconcile an out-of-band merged/closed PR before serializing so the
// response reflects the real GitHub state. Best-effort — a reconcile failure
// must not break the read; we serialize the (possibly stale) persisted state.
if (group.prNumber != null && group.prState === "open" && options?.reconcileGroupPr) {
try {
group = await options.reconcileGroupPr({ group, projectId: parseProjectId(req) });
} catch {
group = store.getBranchGroup(id) ?? group;
}
}
res.json({ group: await serializeGroup(store, group) });
});
@@ -130,7 +168,15 @@ export function createBranchGroupsRouter(store: TaskStore, options?: BranchGroup
const group = store.getBranchGroup(id);
if (!group) throw notFound("Branch group not found");
let prState: BranchGroup["prState"] = group.prState === "merged" ? "merged" : "closed";
// Fix #2: a finalized or already-merged group is terminal and must not be
// flipped to abandoned/closed (mirrors the promote route's gate style).
if (group.status === "finalized" || group.prState === "merged") {
throw badRequest("Branch group is already finalized or merged and cannot be abandoned");
}
// The guard above already rejected `prState === "merged"`, so abandon always
// resolves to "closed" unless the GitHub reconcile below reports otherwise.
let prState: BranchGroup["prState"] = "closed";
let prNumber = group.prNumber;
let prUrl = group.prUrl;

View File

@@ -13,7 +13,8 @@ import { createDevServerRouter } from "../dev-server-routes.js";
import type { AiSessionStore } from "../ai-session-store.js";
import { createStashRecoveryRouter } from "./register-stash-recovery-routes.js";
import { createBranchGroupsRouter } from "./register-branch-groups-routes.js";
import { GitHubClient, closeGroupPullRequest } from "../github.js";
import { GitHubClient, closeGroupPullRequest, reconcileGroupPullRequest } from "../github.js";
import { reconcileBranchGroupPr } from "@fusion/engine";
interface IntegratedRoutersOptions {
router: Router;
@@ -64,10 +65,25 @@ export function registerIntegratedRouters({
if (group.prNumber == null) {
return null;
}
const client = new GitHubClient();
// Fix #1: forward the configured token so token-only environments (no gh
// CLI) can still close the PR.
const client = new GitHubClient(options?.githubToken);
const result = await closeGroupPullRequest(client, group);
return { prNumber: result.prNumber, prUrl: result.prUrl, prState: result.prState };
},
reconcileGroupPr: async ({ group }) => {
// Fix #3: flip prState when the managed PR was merged/closed out-of-band.
// Build a read-only SyncGroupPrFn over the GitHub client (mirrors the CLI's
// syncGroupPrCallback shape) and delegate persistence to the engine's
// reconcileBranchGroupPr primitive.
const client = new GitHubClient(options?.githubToken);
await reconcileBranchGroupPr({
store,
group,
syncGroupPr: async ({ group: g }) => reconcileGroupPullRequest(client, g),
});
return store.getBranchGroup(group.id) ?? group;
},
}));
}