From e54417c9874606bc9b2597aa433749476380b422 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 3 Jun 2026 12:43:25 -0700 Subject: [PATCH] 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. --- packages/cli/src/bin.ts | 17 +- .../commands/__tests__/branch-group.test.ts | 88 +++++++- .../commands/__tests__/task-lifecycle.test.ts | 17 +- packages/cli/src/commands/branch-group.ts | 45 ++++- packages/cli/src/commands/task-lifecycle.ts | 7 +- .../src/__tests__/branch-assignment.test.ts | 66 ++++++ .../src/__tests__/branch-group-store.test.ts | 14 ++ packages/core/src/branch-assignment.ts | 62 ++++++ packages/core/src/index.ts | 3 + packages/core/src/store.ts | 26 +-- .../__tests__/github-close-group-pr.test.ts | 28 ++- .../integrated-routers-group-pr-token.test.ts | 87 ++++++++ .../__tests__/routes-branch-groups.test.ts | 189 +++++++++++++++++- packages/dashboard/src/github.ts | 25 +++ packages/dashboard/src/index.ts | 2 +- .../routes/register-branch-groups-routes.ts | 60 +++++- .../src/routes/register-integrated-routers.ts | 20 +- 17 files changed, 717 insertions(+), 39 deletions(-) create mode 100644 packages/dashboard/src/__tests__/integrated-routers-group-pr-token.test.ts diff --git a/packages/cli/src/bin.ts b/packages/cli/src/bin.ts index f4c6edd21b..4ade1a74a9 100644 --- a/packages/cli/src/bin.ts +++ b/packages/cli/src/bin.ts @@ -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 Show a branch group's members and completion gate fn branch-group promote Promote a complete group (opens/links the single managed PR) + fn branch-group abandon + Abandon a group (best-effort closes the managed PR) fn agent stop Stop a running agent (pause execution) fn agent start Start a stopped agent (resume execution) fn agent import [--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 "); + process.exit(1); + } + await runBranchGroupAbandon(id, projectName); + break; + } default: console.error(`Unknown subcommand: branch-group ${subcommand || ""}`); - console.log("Try: fn branch-group list | show | promote "); + console.log("Try: fn branch-group list | show | promote | abandon "); process.exit(1); } break; diff --git a/packages/cli/src/commands/__tests__/branch-group.test.ts b/packages/cli/src/commands/__tests__/branch-group.test.ts index 580a5fcb20..a9404d0ac5 100644 --- a/packages/cli/src/commands/__tests__/branch-group.test.ts +++ b/packages/cli/src/commands/__tests__/branch-group.test.ts @@ -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, members: unknown[]) { getBranchGroup: vi.fn(() => group), listBranchGroups: vi.fn(() => [group]), listTasksByBranchGroup: vi.fn(async () => members), + updateBranchGroup: vi.fn((_id: string, patch: Record) => ({ ...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; + let logSpy: ReturnType; + let errSpy: ReturnType; + + 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) { + 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(); + }); +}); diff --git a/packages/cli/src/commands/__tests__/task-lifecycle.test.ts b/packages/cli/src/commands/__tests__/task-lifecycle.test.ts index ec2f4643cb..7f6c82b4ff 100644 --- a/packages/cli/src/commands/__tests__/task-lifecycle.test.ts +++ b/packages/cli/src/commands/__tests__/task-lifecycle.test.ts @@ -4,6 +4,10 @@ import { EventEmitter } from "node:events"; // Mock child_process so we can intercept the `git push -u origin ` // 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 = { 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 () => { diff --git a/packages/cli/src/commands/branch-group.ts b/packages/cli/src/commands/branch-group.ts index dc940a8eb6..fe0f3a1312 100644 --- a/packages/cli/src/commands/branch-group.ts +++ b/packages/cli/src/commands/branch-group.ts @@ -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); diff --git a/packages/cli/src/commands/task-lifecycle.ts b/packages/cli/src/commands/task-lifecycle.ts index 3e01ebfc69..d218a80842 100644 --- a/packages/cli/src/commands/task-lifecycle.ts +++ b/packages/cli/src/commands/task-lifecycle.ts @@ -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 { + 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"); diff --git a/packages/core/src/__tests__/branch-group-store.test.ts b/packages/core/src/__tests__/branch-group-store.test.ts index 601fc7045b..e216fe6652 100644 --- a/packages/core/src/__tests__/branch-group-store.test.ts +++ b/packages/core/src/__tests__/branch-group-store.test.ts @@ -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(); diff --git a/packages/core/src/branch-assignment.ts b/packages/core/src/branch-assignment.ts index c72d0dda5b..78323b5b61 100644 --- a/packages/core/src/branch-assignment.ts +++ b/packages/core/src/branch-assignment.ts @@ -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 + * `:` 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() diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 10717c2115..8d0aa646c9 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -6,6 +6,9 @@ export { sanitizeBranchSegment, derivePerTaskBranchName, deriveAutoTaskBranchName, + isValidBranchGroupBranchName, + validateBranchGroupBranchName, + filterTasksByBranchGroup, } from "./branch-assignment.js"; export type { EntryPointAssignmentMode, diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 7bc69d99f0..00e58a708c 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -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 { } 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 { async listTasksByBranchGroup(groupId: string): Promise { 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:` / - // `mission:`) 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( diff --git a/packages/dashboard/src/__tests__/github-close-group-pr.test.ts b/packages/dashboard/src/__tests__/github-close-group-pr.test.ts index 5920708646..3fdc241980 100644 --- a/packages/dashboard/src/__tests__/github-close-group-pr.test.ts +++ b/packages/dashboard/src/__tests__/github-close-group-pr.test.ts @@ -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"); + }); +}); diff --git a/packages/dashboard/src/__tests__/integrated-routers-group-pr-token.test.ts b/packages/dashboard/src/__tests__/integrated-routers-group-pr-token.test.ts new file mode 100644 index 0000000000..92e29293a2 --- /dev/null +++ b/packages/dashboard/src/__tests__/integrated-routers-group-pr-token.test.ts @@ -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 = []; + +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("@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) => { + 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"); + }); +}); diff --git a/packages/dashboard/src/__tests__/routes-branch-groups.test.ts b/packages/dashboard/src/__tests__/routes-branch-groups.test.ts index b35a196092..92497246d2 100644 --- a/packages/dashboard/src/__tests__/routes-branch-groups.test.ts +++ b/packages/dashboard/src/__tests__/routes-branch-groups.test.ts @@ -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); 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); + + 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) => { + current = { ...current, ...patch }; + return current; + }), + } as unknown as TaskStore; + return { store, getCurrent: () => current }; + } + + function mount(store: TaskStore, reconcileGroupPr?: ReturnType) { + 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); + + 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 }); }); }); diff --git a/packages/dashboard/src/github.ts b/packages/dashboard/src/github.ts index f18af7865d..ca5d5feb12 100644 --- a/packages/dashboard/src/github.ts +++ b/packages/dashboard/src/github.ts @@ -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, + group: Pick, +): Promise { + 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 diff --git a/packages/dashboard/src/index.ts b/packages/dashboard/src/index.ts index 91098aad06..8ec1c3e229 100644 --- a/packages/dashboard/src/index.ts +++ b/packages/dashboard/src/index.ts @@ -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 { diff --git a/packages/dashboard/src/routes/register-branch-groups-routes.ts b/packages/dashboard/src/routes/register-branch-groups-routes.ts index 49fddd8149..900ba4c2a5 100644 --- a/packages/dashboard/src/routes/register-branch-groups-routes.ts +++ b/packages/dashboard/src/routes/register-branch-groups-routes.ts @@ -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; } 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; diff --git a/packages/dashboard/src/routes/register-integrated-routers.ts b/packages/dashboard/src/routes/register-integrated-routers.ts index 08c6d6c9a4..e14b8eef83 100644 --- a/packages/dashboard/src/routes/register-integrated-routers.ts +++ b/packages/dashboard/src/routes/register-integrated-routers.ts @@ -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; + }, })); }