diff --git a/.changeset/fn-7738-cli-cmd-lock-retry.md b/.changeset/fn-7738-cli-cmd-lock-retry.md new file mode 100644 index 0000000000..5b2d2b250e --- /dev/null +++ b/.changeset/fn-7738-cli-cmd-lock-retry.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: `fn branch-group`/`fn pr` now retry a locked board database and exit promptly instead of hanging or leaking. +category: fix +dev: Applies the FN-7731 CLI retryOnLock + closeProjectStore pattern to packages/cli/src/commands/branch-group.ts and pr.ts (agent/node audited and left unchanged); honors FUSION_CLI_LOCK_RETRY_MS; closes both cached and uncached CWD-fallback stores on every exit path. diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 167e76f086..73fc270e4f 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -598,6 +598,17 @@ interactive GitHub import, `fn task logs --follow`) keep their interactive prompt loop or tail session un-retried by design but still close the resolved store on every exit path, including on `Ctrl+C`. +The same retry-on-lock and deterministic-teardown behavior extends to +`fn branch-group *` (`list`/`show`/`abandon`/`promote`) and `fn pr *` +(`create`/`list`/`show`/`approve`/`respond`/`retry`/`merge`/`close`/ +`automerge`/`automerge-cleanup`) (FN-7738). Discrete board reads/writes +retry through a momentary `database is locked` (subject to the same +`FUSION_CLI_LOCK_RETRY_MS` deadline override); external GitHub API calls and +workflow-release side effects are not retried, to avoid re-issuing an +already-completed side effect. The resolved `TaskStore` — whether resolved +from the registered/default project or the uncached CWD-fallback project — +is always closed on exit so the CLI process exits promptly. + ### Execution and status ```bash diff --git a/packages/cli/src/commands/__tests__/branch-group-lock-retry.test.ts b/packages/cli/src/commands/__tests__/branch-group-lock-retry.test.ts new file mode 100644 index 0000000000..4972c86569 --- /dev/null +++ b/packages/cli/src/commands/__tests__/branch-group-lock-retry.test.ts @@ -0,0 +1,221 @@ +/** + * FNXC:CliBoardMutation 2026-07-09-00:00: + * Regression coverage for FN-7738 — `fn branch-group *` must retry through a + * momentarily-locked SQLite board database instead of surfacing a raw + * `database is locked` error or hanging, and must always close the resolved + * `TaskStore` (cached AND the uncached CWD-fallback branch) so the CLI + * process exits promptly. Mirrors the FN-7731 `task-lock-retry.test.ts` + * pattern: mocked-store lock exhaustion/not-found/teardown coverage (fast, + * fake-timer based, no real waits per FN-5048). + */ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@fusion/dashboard", () => ({ + GitHubClient: vi.fn(function GitHubClient() {}), + closeGroupPullRequest: vi.fn(), +})); +vi.mock("@fusion/engine", () => ({ + promoteBranchGroup: vi.fn(), + resolveIntegrationBranch: vi.fn(async () => "main"), +})); +vi.mock("../task-lifecycle.js", () => ({ + createGroupPrCallback: vi.fn(() => async () => ({ prNumber: 1, prUrl: "x", prState: "open" as const })), +})); + +const BASE_GROUP = { + id: "BG-1", + sourceType: "planning", + sourceId: "PS-1", + branchName: "feature/shared", + status: "open" as const, + prState: "none" as const, + autoMerge: false, +}; + +function makeStore(overrides: Record = {}) { + return { + getBranchGroup: vi.fn(() => BASE_GROUP), + listBranchGroups: vi.fn(() => [BASE_GROUP]), + listTasks: vi.fn(async () => []), + listTasksByBranchGroup: vi.fn(async () => []), + updateBranchGroup: vi.fn((_id: string, patch: Record) => ({ ...BASE_GROUP, ...patch })), + getSettings: vi.fn(async () => ({ + autoMerge: false, + globalPause: false, + enginePaused: false, + mergeStrategy: "merge", + baseBranch: "main", + })), + recordRunAuditEvent: vi.fn(), + close: vi.fn().mockResolvedValue(undefined), + ...overrides, + }; +} + +async function loadWithMockedStore(store: Record, opts?: { cached?: boolean }) { + const cached = opts?.cached ?? true; + const closeProjectStore = vi.fn(async (context: { store: { close?: () => Promise } }) => { + await context.store.close?.().catch(() => {}); + }); + const context = { + projectId: cached ? "proj_test" : process.cwd(), + projectPath: cached ? "/proj" : process.cwd(), + projectName: "proj", + isRegistered: cached, + store, + }; + const resolveProject = cached + ? vi.fn().mockResolvedValue(context) + : vi.fn().mockRejectedValue(new Error("no registered project")); + const asLocalProjectContext = vi.fn(() => context); + vi.doMock("../../project-context.js", () => ({ resolveProject, closeProjectStore, asLocalProjectContext })); + const mod = await import("../branch-group.js"); + return { mod, closeProjectStore, resolveProject }; +} + +describe("fn branch-group * — lock retry, leak/close, and not-found teardown (FN-7738)", () => { + beforeEach(() => { + vi.resetModules(); + }); + + afterEach(() => { + vi.doUnmock("../../project-context.js"); + vi.restoreAllMocks(); + delete process.env.FUSION_CLI_LOCK_RETRY_MS; + }); + + it("runBranchGroupShow: succeeds on first attempt (no lock contention) and closes the store once", async () => { + const store = makeStore(); + const { mod, closeProjectStore } = await loadWithMockedStore(store); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + + await mod.runBranchGroupShow("BG-1"); + + expect(store.getBranchGroup).toHaveBeenCalledTimes(1); + expect(closeProjectStore).toHaveBeenCalledTimes(1); + logSpy.mockRestore(); + }); + + it("runBranchGroupShow: retries through a transient lock error and succeeds once it clears, then closes the store", async () => { + vi.useFakeTimers(); + try { + process.env.FUSION_CLI_LOCK_RETRY_MS = "5000"; + const lockError = new Error("database is locked"); + const getBranchGroup = vi.fn().mockImplementationOnce(() => { + throw lockError; + }).mockImplementation(() => BASE_GROUP); + const store = makeStore({ getBranchGroup }); + const { mod, closeProjectStore } = await loadWithMockedStore(store); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + + const promise = mod.runBranchGroupShow("BG-1"); + for (let i = 0; i < 10 && getBranchGroup.mock.calls.length < 2; i++) { + await vi.advanceTimersByTimeAsync(1_000); + } + await promise; + + expect(getBranchGroup.mock.calls.length).toBeGreaterThan(1); + expect(closeProjectStore).toHaveBeenCalled(); + logSpy.mockRestore(); + } finally { + vi.useRealTimers(); + } + }); + + it("runBranchGroupAbandon: bounded exhaustion across many fast lock retries fails clearly and closes the store", async () => { + vi.useFakeTimers(); + try { + process.env.FUSION_CLI_LOCK_RETRY_MS = "500"; + const updateBranchGroup = vi.fn().mockImplementation(() => { + throw new Error("SQLITE_BUSY: database is locked"); + }); + const store = makeStore({ updateBranchGroup }); + const { mod, closeProjectStore } = await loadWithMockedStore(store); + + const exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`process.exit(${code})`); + }) as never); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + + const promise = mod.runBranchGroupAbandon("BG-1"); + const assertion = expect(promise).rejects.toThrow(/process\.exit\(1\)/); + for (let i = 0; i < 10 && updateBranchGroup.mock.calls.length < 2; i++) { + await vi.advanceTimersByTimeAsync(1_000); + } + await vi.advanceTimersByTimeAsync(1_000); + await assertion; + + expect(updateBranchGroup.mock.calls.length).toBeGreaterThan(1); + const printed = errorSpy.mock.calls.flat().join("\n"); + expect(printed).toMatch(/locked|FUSION_CLI_LOCK_RETRY_MS/i); + expect(closeProjectStore).toHaveBeenCalled(); + + exitSpy.mockRestore(); + errorSpy.mockRestore(); + } finally { + vi.useRealTimers(); + } + }); + + it("runBranchGroupShow: a not-found error does not retry-loop and closes the store before exiting", async () => { + process.env.FUSION_CLI_LOCK_RETRY_MS = "5000"; + const getBranchGroup = vi.fn().mockReturnValue(null); + const store = makeStore({ getBranchGroup }); + const { mod, closeProjectStore } = await loadWithMockedStore(store); + + const exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`process.exit(${code})`); + }) as never); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + + await expect(mod.runBranchGroupShow("BG-404")).rejects.toThrow(/process\.exit\(1\)/); + + expect(getBranchGroup).toHaveBeenCalledTimes(1); + expect(closeProjectStore).toHaveBeenCalled(); + expect(errorSpy.mock.calls.flat().join("\n")).toContain("BG-404"); + + exitSpy.mockRestore(); + errorSpy.mockRestore(); + }); + + it("runBranchGroupAbandon: a terminal-guard input (already-abandoned) exits cleanly and closes the store, without calling update", async () => { + const store = makeStore({ getBranchGroup: vi.fn(() => ({ ...BASE_GROUP, status: "abandoned" })) }); + const { mod, closeProjectStore } = await loadWithMockedStore(store); + + const exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`process.exit(${code})`); + }) as never); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + + await expect(mod.runBranchGroupAbandon("BG-1")).rejects.toThrow(/process\.exit\(1\)/); + + expect(store.updateBranchGroup).not.toHaveBeenCalled(); + expect(closeProjectStore).toHaveBeenCalled(); + + exitSpy.mockRestore(); + errorSpy.mockRestore(); + }); + + it("runBranchGroupShow (uncached CWD-fallback): resolves via asLocalProjectContext and still closes the store", async () => { + const store = makeStore(); + const { mod, closeProjectStore } = await loadWithMockedStore(store, { cached: false }); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + + await mod.runBranchGroupShow("BG-1"); + + expect(closeProjectStore).toHaveBeenCalledTimes(1); + logSpy.mockRestore(); + }); + + it("runBranchGroupList: the happy path adds no retry latency and closes the store once", async () => { + const store = makeStore(); + const { mod, closeProjectStore } = await loadWithMockedStore(store); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + + await mod.runBranchGroupList(); + + expect(store.listBranchGroups).toHaveBeenCalledTimes(1); + expect(closeProjectStore).toHaveBeenCalledTimes(1); + logSpy.mockRestore(); + }); +}); diff --git a/packages/cli/src/commands/__tests__/branch-group.test.ts b/packages/cli/src/commands/__tests__/branch-group.test.ts index c072b12837..ac2fa9eeec 100644 --- a/packages/cli/src/commands/__tests__/branch-group.test.ts +++ b/packages/cli/src/commands/__tests__/branch-group.test.ts @@ -4,6 +4,16 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; vi.mock("../../project-context.js", () => ({ resolveProject: vi.fn(), + closeProjectStore: vi.fn(async (context: { store?: { close?: () => Promise } }) => { + await context.store?.close?.().catch(() => {}); + }), + asLocalProjectContext: (store: unknown) => ({ + projectId: process.cwd(), + projectPath: process.cwd(), + projectName: "current-project", + isRegistered: false, + store, + }), })); const promoteBranchGroupMock = vi.fn(); diff --git a/packages/cli/src/commands/__tests__/pr-lock-retry.test.ts b/packages/cli/src/commands/__tests__/pr-lock-retry.test.ts new file mode 100644 index 0000000000..dd886a4942 --- /dev/null +++ b/packages/cli/src/commands/__tests__/pr-lock-retry.test.ts @@ -0,0 +1,226 @@ +/** + * FNXC:CliBoardMutation 2026-07-09-00:00: + * Regression coverage for FN-7738 — `fn pr *` must retry through a + * momentarily-locked SQLite board database instead of surfacing a raw + * `database is locked` error or hanging, and must always close the resolved + * `TaskStore` (cached AND the uncached CWD-fallback branch) so the CLI + * process exits promptly. Mirrors the FN-7731 `task-lock-retry.test.ts` + * pattern: mocked-store lock exhaustion/not-found/teardown coverage (fast, + * fake-timer based, no real waits per FN-5048). + */ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@fusion/core/gh-cli", () => ({ + classifyGhError: vi.fn((err: unknown) => ({ message: String(err) })), + getGhErrorMessage: vi.fn((err: unknown) => String(err)), + getCurrentRepo: vi.fn(() => ({ owner: "acme", repo: "widgets" })), + isGhAuthenticated: vi.fn(() => true), + isGhAvailable: vi.fn(() => true), +})); +vi.mock("@fusion/engine", () => ({ + releaseHeldTaskByEvent: vi.fn(async () => ({ released: true, toColumn: "done" })), +})); +vi.mock("@fusion/dashboard", () => ({ + generatePrMetadata: vi.fn(), + GitHubClient: vi.fn(function GitHubClient() { + return { createPr: vi.fn() }; + }), +})); + +const BASE_ENTITY = { + id: "PR-1", + sourceType: "task", + sourceId: "FN-1", + repo: "acme/widgets", + headBranch: "fusion/fn-1", + baseBranch: "main", + state: "open" as const, + prNumber: 5, + prUrl: "https://example/pr/5", + mergeable: "mergeable" as const, + reviewDecision: null, + checksRollup: null, + autoMerge: false, + responseRounds: 0, +}; + +function makeStore(overrides: Record = {}) { + return { + getPrEntity: vi.fn(() => BASE_ENTITY), + listActivePrEntities: vi.fn(() => [BASE_ENTITY]), + listPrThreadStates: vi.fn(() => []), + updatePrEntity: vi.fn((id: string, patch: Record) => ({ ...BASE_ENTITY, ...patch })), + reconcileLegacyAutoMergeStamps: vi.fn(async () => []), + close: vi.fn().mockResolvedValue(undefined), + ...overrides, + }; +} + +async function loadWithMockedStore(store: Record, opts?: { cached?: boolean }) { + const cached = opts?.cached ?? true; + const closeProjectStore = vi.fn(async (context: { store: { close?: () => Promise } }) => { + await context.store.close?.().catch(() => {}); + }); + const context = { + projectId: cached ? "proj_test" : process.cwd(), + projectPath: cached ? "/proj" : process.cwd(), + projectName: "proj", + isRegistered: cached, + store, + }; + const resolveProject = cached + ? vi.fn().mockResolvedValue(context) + : vi.fn().mockRejectedValue(new Error("no registered project")); + const asLocalProjectContext = vi.fn(() => context); + vi.doMock("../../project-context.js", () => ({ resolveProject, closeProjectStore, asLocalProjectContext })); + const mod = await import("../pr.js"); + return { mod, closeProjectStore, resolveProject }; +} + +describe("fn pr * — lock retry, leak/close, and not-found teardown (FN-7738)", () => { + beforeEach(() => { + vi.resetModules(); + }); + + afterEach(() => { + vi.doUnmock("../../project-context.js"); + vi.restoreAllMocks(); + delete process.env.FUSION_CLI_LOCK_RETRY_MS; + }); + + it("runPrList: succeeds on first attempt (no lock contention) and closes the store once", async () => { + const store = makeStore(); + const { mod, closeProjectStore } = await loadWithMockedStore(store); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + + await mod.runPrList(); + + expect(store.listActivePrEntities).toHaveBeenCalledTimes(1); + expect(closeProjectStore).toHaveBeenCalledTimes(1); + logSpy.mockRestore(); + }); + + it("runPrList: retries through a transient lock error and succeeds once it clears, then closes the store", async () => { + vi.useFakeTimers(); + try { + process.env.FUSION_CLI_LOCK_RETRY_MS = "5000"; + const lockError = new Error("database is locked"); + const listActivePrEntities = vi.fn().mockImplementationOnce(() => { + throw lockError; + }).mockImplementation(() => [BASE_ENTITY]); + const store = makeStore({ listActivePrEntities }); + const { mod, closeProjectStore } = await loadWithMockedStore(store); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + + const promise = mod.runPrList(); + for (let i = 0; i < 10 && listActivePrEntities.mock.calls.length < 2; i++) { + await vi.advanceTimersByTimeAsync(1_000); + } + await promise; + + expect(listActivePrEntities.mock.calls.length).toBeGreaterThan(1); + expect(closeProjectStore).toHaveBeenCalled(); + logSpy.mockRestore(); + } finally { + vi.useRealTimers(); + } + }); + + it("runPrAutomerge: bounded exhaustion across many fast lock retries fails clearly and closes the store", async () => { + vi.useFakeTimers(); + try { + process.env.FUSION_CLI_LOCK_RETRY_MS = "500"; + const updatePrEntity = vi.fn().mockImplementation(() => { + throw new Error("SQLITE_BUSY: database is locked"); + }); + const store = makeStore({ updatePrEntity }); + const { mod, closeProjectStore } = await loadWithMockedStore(store); + + const exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`process.exit(${code})`); + }) as never); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + + const promise = mod.runPrAutomerge("PR-1", true); + const assertion = expect(promise).rejects.toThrow(/process\.exit\(1\)/); + for (let i = 0; i < 10 && updatePrEntity.mock.calls.length < 2; i++) { + await vi.advanceTimersByTimeAsync(1_000); + } + await vi.advanceTimersByTimeAsync(1_000); + await assertion; + + expect(updatePrEntity.mock.calls.length).toBeGreaterThan(1); + const printed = errorSpy.mock.calls.flat().join("\n"); + expect(printed).toMatch(/locked|FUSION_CLI_LOCK_RETRY_MS/i); + expect(closeProjectStore).toHaveBeenCalled(); + + exitSpy.mockRestore(); + errorSpy.mockRestore(); + } finally { + vi.useRealTimers(); + } + }); + + it("runPrShow: a not-found PR entity does not retry-loop and closes the store before exiting", async () => { + process.env.FUSION_CLI_LOCK_RETRY_MS = "5000"; + const getPrEntity = vi.fn().mockReturnValue(null); + const store = makeStore({ getPrEntity }); + const { mod, closeProjectStore } = await loadWithMockedStore(store); + + const exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`process.exit(${code})`); + }) as never); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + + await expect(mod.runPrShow("PR-404")).rejects.toThrow(/process\.exit\(1\)/); + + expect(getPrEntity).toHaveBeenCalledTimes(1); + expect(closeProjectStore).toHaveBeenCalled(); + expect(errorSpy.mock.calls.flat().join("\n")).toContain("PR-404"); + + exitSpy.mockRestore(); + errorSpy.mockRestore(); + }); + + it("runPrMerge: a terminal PR entity (already merged) exits cleanly and closes the store, without releasing", async () => { + const { releaseHeldTaskByEvent } = await import("@fusion/engine"); + const store = makeStore({ getPrEntity: vi.fn(() => ({ ...BASE_ENTITY, state: "merged" })) }); + const { mod, closeProjectStore } = await loadWithMockedStore(store); + + const exitSpy = vi.spyOn(process, "exit").mockImplementation(((code?: number) => { + throw new Error(`process.exit(${code})`); + }) as never); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + + await expect(mod.runPrMerge("PR-1")).rejects.toThrow(/process\.exit\(1\)/); + + expect(releaseHeldTaskByEvent).not.toHaveBeenCalled(); + expect(closeProjectStore).toHaveBeenCalled(); + + exitSpy.mockRestore(); + errorSpy.mockRestore(); + }); + + it("runPrList (uncached CWD-fallback): resolves via asLocalProjectContext and still closes the store", async () => { + const store = makeStore(); + const { mod, closeProjectStore } = await loadWithMockedStore(store, { cached: false }); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + + await mod.runPrList(); + + expect(closeProjectStore).toHaveBeenCalledTimes(1); + logSpy.mockRestore(); + }); + + it("runPrAutomergeCleanup: the happy path adds no retry latency and closes the store once", async () => { + const store = makeStore(); + const { mod, closeProjectStore } = await loadWithMockedStore(store); + const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + + await mod.runPrAutomergeCleanup({}); + + expect(store.reconcileLegacyAutoMergeStamps).toHaveBeenCalledTimes(1); + expect(closeProjectStore).toHaveBeenCalledTimes(1); + logSpy.mockRestore(); + }); +}); diff --git a/packages/cli/src/commands/branch-group.ts b/packages/cli/src/commands/branch-group.ts index 1c86094b8a..77d7940e95 100644 --- a/packages/cli/src/commands/branch-group.ts +++ b/packages/cli/src/commands/branch-group.ts @@ -1,8 +1,33 @@ import { TaskStore, isBranchGroupComplete, isBranchGroupMemberLanded, filterTasksByBranchGroup, type BranchGroup, type Settings, type Task } from "@fusion/core"; import { promoteBranchGroup, resolveIntegrationBranch } from "@fusion/engine"; import { GitHubClient, closeGroupPullRequest } from "@fusion/dashboard"; -import { resolveProject } from "../project-context.js"; +import { resolveProject, closeProjectStore, asLocalProjectContext, type ProjectContext } from "../project-context.js"; import { createGroupPrCallback } from "./task-lifecycle.js"; +import { retryOnLock, LockRetryExhaustedError } from "../lock-retry.js"; + +/** + * FNXC:CliBoardMutation 2026-07-09-00:00: + * FN-7738 audit finding: `getBranchGroupContext` resolves a `TaskStore` + * (cached via `resolveProject`, OR an UNCACHED `new TaskStore(process.cwd())` + * CWD-fallback) and, before this change, NO `runBranchGroup*` handler ever + * closed it — a leaked SQLite/WAL handle can keep the CLI process's event + * loop alive after the command's real work is done. None of the board + * mutations here (`updateBranchGroup`, `recordRunAuditEvent` via + * `promoteBranchGroup`) retried through a momentary `database is locked` + * either, so a promote/abandon racing an active engine/agent writer failed + * outright. This mirrors the class FN-7731 fixed for `fn task show`/`move` + * and FN-7704 fixed for `fn agent stop`/`start`; the fix below reuses the + * SAME `retryOnLock`/`closeProjectStore` helpers (no forked second + * implementation) scoped to the discrete store read/write calls — NOT the + * external GitHub API calls or the `promoteBranchGroup` coordinator call + * itself, since retrying those on a later lock error would risk re-issuing + * an already-completed side effect (e.g. a second PR close). The resolved + * store is closed on every exit path, including guard/not-found + * `process.exit()` calls (closed explicitly BEFORE the exit, since a + * pending `finally` does not run after `process.exit()` — see project + * memory) and including the uncached CWD-fallback branch via + * `asLocalProjectContext`. + */ /** * Agent-native parity (R10): expose the same branch-group surfacing/controls a @@ -21,16 +46,11 @@ import { createGroupPrCallback } from "./task-lifecycle.js"; * directly rather than calling the dashboard HTTP API. */ -interface BranchGroupCommandContext { - store: TaskStore; - projectPath: string; -} - -async function getBranchGroupContext(projectName?: string): Promise { +async function getBranchGroupContext(projectName?: string): Promise { try { const context = await resolveProject(projectName); if (context) { - return { store: context.store, projectPath: context.projectPath }; + return context; } } catch { // fall through to a local store rooted at cwd @@ -40,7 +60,22 @@ async function getBranchGroupContext(projectName?: string): Promise { + const message = error instanceof Error ? error.message : String(error); + console.error(`\n \u2717 ${message}\n`); + if (context) { + await closeProjectStore(context); + } + return process.exit(1); } /** @@ -71,158 +106,244 @@ async function serializeCompletion(store: TaskStore, group: BranchGroup, allTask } export async function runBranchGroupList(projectName?: string) { - const { store } = await getBranchGroupContext(projectName); - const groups = store.listBranchGroups(); + try { + await retryOnLock( + async () => { + const context = await getBranchGroupContext(projectName); + try { + const { store } = context; + const groups = store.listBranchGroups(); - if (groups.length === 0) { - console.log("\n No branch groups yet.\n"); - return; + if (groups.length === 0) { + console.log("\n No branch groups yet.\n"); + return; + } + + // Fix #8/#9 parity with the dashboard list route: fetch tasks ONCE and filter + // per group in memory rather than one full scan per group (the old N+1). + const allTasks = await store.listTasks({ includeArchived: false, slim: true }); + + console.log(); + for (const group of groups) { + const completion = await serializeCompletion(store, group, allTasks); + const prState = group.prState === "none" ? "no PR" : `PR ${group.prState}`; + const gate = completion.complete ? "complete" : `${completion.landed}/${completion.total}`; + console.log(` ${group.id} ${group.branchName} [${group.status}] (${gate}) ${prState}`); + } + console.log(); + } finally { + await closeProjectStore(context); + } + }, + { id: "branch-groups", action: "list branch groups" }, + ); + } catch (error) { + if (error instanceof LockRetryExhaustedError) { + await failBranchGroupCommand(error); + } + throw error; } - - // Fix #8/#9 parity with the dashboard list route: fetch tasks ONCE and filter - // per group in memory rather than one full scan per group (the old N+1). - const allTasks = await store.listTasks({ includeArchived: false, slim: true }); - - console.log(); - for (const group of groups) { - const completion = await serializeCompletion(store, group, allTasks); - const prState = group.prState === "none" ? "no PR" : `PR ${group.prState}`; - const gate = completion.complete ? "complete" : `${completion.landed}/${completion.total}`; - console.log(` ${group.id} ${group.branchName} [${group.status}] (${gate}) ${prState}`); - } - console.log(); } export async function runBranchGroupShow(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); - } + try { + await retryOnLock( + async () => { + const context = await getBranchGroupContext(projectName); + try { + const { store } = context; + const group = store.getBranchGroup(id); + if (!group) { + console.error(`\n \u2717 Branch group ${id} not found\n`); + await closeProjectStore(context); + process.exit(1); + } - const completion = await serializeCompletion(store, group); + const completion = await serializeCompletion(store, group); - console.log(); - console.log(` Branch group ${group.id}`); - console.log(` Branch: ${group.branchName}`); - console.log(` Source: ${group.sourceType}/${group.sourceId}`); - console.log(` Status: ${group.status}`); - console.log(` PR state: ${group.prState}${group.prNumber != null ? ` (#${group.prNumber})` : ""}`); - if (group.prUrl) { - console.log(` PR URL: ${group.prUrl}`); + console.log(); + console.log(` Branch group ${group.id}`); + console.log(` Branch: ${group.branchName}`); + console.log(` Source: ${group.sourceType}/${group.sourceId}`); + console.log(` Status: ${group.status}`); + console.log(` PR state: ${group.prState}${group.prNumber != null ? ` (#${group.prNumber})` : ""}`); + if (group.prUrl) { + console.log(` PR URL: ${group.prUrl}`); + } + console.log(` Progress: ${completion.landed} of ${completion.total} members finished${completion.complete ? " (complete)" : ""}`); + console.log(); + console.log(" Members:"); + for (const member of completion.members) { + const mark = member.landed ? "\u2713" : "\u25cb"; + console.log(` ${mark} ${member.taskId} ${member.title} [${member.column}]`); + } + console.log(); + } finally { + await closeProjectStore(context); + } + }, + { id, action: "show branch group" }, + ); + } catch (error) { + if (error instanceof LockRetryExhaustedError) { + await failBranchGroupCommand(error); + } + throw error; } - console.log(` Progress: ${completion.landed} of ${completion.total} members finished${completion.complete ? " (complete)" : ""}`); - console.log(); - console.log(" Members:"); - for (const member of completion.members) { - const mark = member.landed ? "✓" : "○"; - console.log(` ${mark} ${member.taskId} ${member.title} [${member.column}]`); - } - 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); - } + let context: ProjectContext | undefined; + try { + context = await retryOnLock(() => getBranchGroupContext(projectName), { id, action: "resolve project" }); + const { store } = context; - // 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); - } + const group = await retryOnLock(async () => store.getBranchGroup(id), { id, action: "read branch group" }); + if (!group) { + console.error(`\n \u2717 Branch group ${id} not found\n`); + await closeProjectStore(context); + process.exit(1); + } - // A group with a PR abandons to "closed"; a group that never had a PR keeps - // its existing prState — "closed" would falsely imply a PR existed. - let prState: BranchGroup["prState"] = group.prNumber != null ? "closed" : group.prState; - let prNumber = group.prNumber; - let prUrl = group.prUrl; + // 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 \u2717 Branch group ${id} is already ${group.status === "abandoned" ? "abandoned" : "finalized/merged"} and cannot be abandoned\n`); + await closeProjectStore(context); + process.exit(1); + } - // 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)}`); + // A group with a PR abandons to "closed"; a group that never had a PR keeps + // its existing prState — "closed" would falsely imply a PR existed. + let prState: BranchGroup["prState"] = group.prNumber != null ? "closed" : group.prState; + 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. + // NOT retried through retryOnLock — this is a network call to GitHub, not a + // board interaction, and it is already best-effort/non-blocking on failure. + 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 = await retryOnLock( + async () => + store.updateBranchGroup(id, { + status: "abandoned", + prState, + prNumber: prNumber ?? null, + prUrl: prUrl ?? null, + }), + { id, action: "abandon branch group" }, + ); + + console.log(`\n \u2713 Branch group ${updated.id} abandoned (status: ${updated.status}, prState: ${updated.prState})\n`); + } catch (error) { + if (error instanceof LockRetryExhaustedError) { + await failBranchGroupCommand(error, context); + } + throw error; + } finally { + if (context) { + await closeProjectStore(context); } } - - 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); - if (!group) { - console.error(`\n ✗ Branch group ${id} not found\n`); - process.exit(1); - } - - // Completion gate — mirror the dashboard `POST /:id/promote` gate (R8) so the - // CLI rejects an incomplete group with the same message a dashboard user sees. - const members = await store.listTasksByBranchGroup(group.id); - if (!isBranchGroupComplete(members, group)) { - console.error("\n ✗ Branch group completion gate not satisfied\n"); - process.exit(1); - } - - const settings = (await store.getSettings()) as Settings; - const resolvedIntegrationBranch = await resolveIntegrationBranch(projectPath, settings); - const githubClient = new GitHubClient(process.env.GITHUB_TOKEN); - - console.log(`\n Promoting branch group ${group.id}…\n`); - + let context: ProjectContext | undefined; try { - const result = await promoteBranchGroup({ - store, - rootDir: projectPath, - groupId: group.id, - settings: { - autoMerge: settings.autoMerge, - globalPause: settings.globalPause, - enginePaused: settings.enginePaused, - mergeStrategy: settings.mergeStrategy, - integrationBranch: resolvedIntegrationBranch, - baseBranch: settings.baseBranch, - }, - createGroupPr: createGroupPrCallback(githubClient), - recordAudit: (event) => { - store.recordRunAuditEvent({ - agentId: "cli:branch-group-promote", - runId: `cli-promote-${group.id}`, - domain: event.domain as Parameters[0]["domain"], - mutationType: event.mutationType as Parameters[0]["mutationType"], - target: event.target, - metadata: event.metadata, - }); - }, - }); + context = await retryOnLock(() => getBranchGroupContext(projectName), { id, action: "resolve project" }); + const { store, projectPath } = context; - if (result.prUrl) { - console.log(` ✓ Group ${result.groupId} — PR ${result.prState}: ${result.prUrl}`); - } else { - console.log(` ✓ Group ${result.groupId} — ${result.reason} (status: ${result.status}, prState: ${result.prState})`); + const group = await retryOnLock(async () => store.getBranchGroup(id), { id, action: "read branch group" }); + if (!group) { + console.error(`\n \u2717 Branch group ${id} not found\n`); + await closeProjectStore(context); + process.exit(1); + } + + // Completion gate — mirror the dashboard `POST /:id/promote` gate (R8) so the + // CLI rejects an incomplete group with the same message a dashboard user sees. + const members = await retryOnLock(async () => store.listTasksByBranchGroup(group.id), { id, action: "read branch group members" }); + if (!isBranchGroupComplete(members, group)) { + console.error("\n \u2717 Branch group completion gate not satisfied\n"); + await closeProjectStore(context); + process.exit(1); + } + + const settings = (await retryOnLock(async () => store.getSettings(), { id, action: "read settings" })) as Settings; + const resolvedIntegrationBranch = await resolveIntegrationBranch(projectPath, settings); + const githubClient = new GitHubClient(process.env.GITHUB_TOKEN); + + console.log(`\n Promoting branch group ${group.id}…\n`); + + // NOT wrapped in retryOnLock as a whole: `promoteBranchGroup` performs its own + // git/GitHub side effects (branch merge, PR creation) via `createGroupPr` and + // `recordAudit`, so blanket-retrying the coordinator call on a later lock error + // would risk re-issuing an already-completed side effect (e.g. a second PR). + // The `recordAudit` callback below wraps ONLY the discrete store write. + try { + const result = await promoteBranchGroup({ + store, + rootDir: projectPath, + groupId: group.id, + settings: { + autoMerge: settings.autoMerge, + globalPause: settings.globalPause, + enginePaused: settings.enginePaused, + mergeStrategy: settings.mergeStrategy, + integrationBranch: resolvedIntegrationBranch, + baseBranch: settings.baseBranch, + }, + createGroupPr: createGroupPrCallback(githubClient), + recordAudit: async (event) => { + await retryOnLock( + async () => + store.recordRunAuditEvent({ + agentId: "cli:branch-group-promote", + runId: `cli-promote-${group.id}`, + domain: event.domain as Parameters[0]["domain"], + mutationType: event.mutationType as Parameters[0]["mutationType"], + target: event.target, + metadata: event.metadata, + }), + { id, action: "record branch group promote audit event" }, + ); + }, + }); + + if (result.prUrl) { + console.log(` \u2713 Group ${result.groupId} — PR ${result.prState}: ${result.prUrl}`); + } else { + console.log(` \u2713 Group ${result.groupId} — ${result.reason} (status: ${result.status}, prState: ${result.prState})`); + } + console.log(); + } catch (err) { + if (err instanceof LockRetryExhaustedError) { + throw err; + } + console.error(`\n \u2717 ${err instanceof Error ? err.message : String(err)}\n`); + await closeProjectStore(context); + process.exit(1); + } + } catch (error) { + if (error instanceof LockRetryExhaustedError) { + await failBranchGroupCommand(error, context); + } + throw error; + } finally { + if (context) { + await closeProjectStore(context); } - console.log(); - } catch (err) { - console.error(`\n ✗ ${err instanceof Error ? err.message : String(err)}\n`); - process.exit(1); } } diff --git a/packages/cli/src/commands/pr.ts b/packages/cli/src/commands/pr.ts index e15be8cf37..b0c286559f 100644 --- a/packages/cli/src/commands/pr.ts +++ b/packages/cli/src/commands/pr.ts @@ -9,7 +9,29 @@ import { import { classifyGhError, getGhErrorMessage, getCurrentRepo, isGhAuthenticated, isGhAvailable } from "@fusion/core/gh-cli"; import { releaseHeldTaskByEvent } from "@fusion/engine"; import * as dashboard from "@fusion/dashboard"; -import { resolveProject } from "../project-context.js"; +import { resolveProject, closeProjectStore, asLocalProjectContext, type ProjectContext } from "../project-context.js"; +import { retryOnLock, LockRetryExhaustedError } from "../lock-retry.js"; + +/** + * FNXC:CliBoardMutation 2026-07-09-00:00: + * FN-7738 audit finding: `getPrContext` resolves a `TaskStore` (cached via + * `resolveProject`, OR an UNCACHED `new TaskStore(process.cwd())` + * CWD-fallback) and, before this change, NO `runPr*` handler ever closed it + * — the same leaked-handle class FN-7731 fixed for `fn task show`/`move`. + * None of the board mutations here (`updatePrInfo`, `ensurePrEntityForSource`, + * `updatePrEntity`, `releaseHeldTaskByEvent`, `reconcileLegacyAutoMergeStamps`) + * retried through a momentary `database is locked` either. The fix below + * reuses the SAME `retryOnLock`/`closeProjectStore` helpers (no forked + * second implementation), scoped to the discrete store read/write calls — + * NOT the GitHub API calls (`client.createPr`, `dashboard.generatePrMetadata`) + * or `releaseHeldTaskByEvent` (which itself owns further workflow/GitHub side + * effects) — to avoid re-issuing an already-completed external side effect on + * a later lock-triggered retry. The resolved store is closed on every exit + * path, including guard/not-found `process.exit()` calls (closed explicitly + * before the exit, since a pending `finally` does not run after + * `process.exit()` — see project memory) and including the uncached + * CWD-fallback branch via `asLocalProjectContext`. + */ /** * Agent-native parity (R13, U8): expose the SAME unified-PR-entity surface and @@ -39,16 +61,11 @@ import { resolveProject } from "../project-context.js"; * calling the dashboard HTTP API. */ -interface PrCommandContext { - store: TaskStore; - projectPath: string; -} - -async function getPrContext(projectName?: string): Promise { +async function getPrContext(projectName?: string): Promise { try { const context = await resolveProject(projectName); if (context) { - return { store: context.store, projectPath: context.projectPath }; + return context; } } catch { // fall through to a local store rooted at cwd @@ -58,7 +75,23 @@ async function getPrContext(projectName?: string): Promise { } const store = new TaskStore(process.cwd()); await store.init(); - return { store, projectPath: process.cwd() }; + return asLocalProjectContext(store); +} + +/** + * FNXC:CliBoardMutation 2026-07-09-00:00: + * Translate a `LockRetryExhaustedError` (or any other error) from a PR board + * interaction into the CLI's standard "print + exit(1)" failure shape, + * matching `task.ts`'s `failBoardCommand`. When `context` is still open, + * close it BEFORE exiting. + */ +async function failPrCommand(error: unknown, context?: ProjectContext): Promise { + const message = error instanceof Error ? error.message : String(error); + console.error(`\n \u2717 ${message}\n`); + if (context) { + await closeProjectStore(context); + } + return process.exit(1); } function formatGhErrorForCli(err: unknown): string { @@ -86,190 +119,245 @@ export interface PrCreateOptions { } export async function runPrCreate(id: string, options: PrCreateOptions = {}, projectName?: string) { - const { store, projectPath } = await getPrContext(projectName); - - // Fetch task and validate it exists - let task; + let context: ProjectContext | undefined; try { - task = await store.getTask(id); - } catch (err) { - if (typeof err === "object" && err !== null && (err as Record).code === "ENOENT") { - console.error(`Error: Task ${id} not found`); - process.exit(1); - } - throw err; - } - if (!task) { - console.error(`Error: Task ${id} not found`); - process.exit(1); - } + context = await retryOnLock(() => getPrContext(projectName), { id, action: "resolve project" }); + const { store, projectPath } = context; - // Validate task is in 'in-review' column - if (task.column !== "in-review") { - console.error(`Error: Task must be in 'in-review' column to create a PR (current: ${task.column})`); - process.exit(1); - } - - // Check if task already has PR info - if (task.prInfo) { - console.error(`Error: Task already has PR #${task.prInfo.number}: ${task.prInfo.url}`); - process.exit(1); - } - - // Determine owner/repo from GITHUB_REPOSITORY env or git remote - let owner: string; - let repo: string; - - const envRepo = process.env.GITHUB_REPOSITORY; - if (envRepo) { - const [o, r] = envRepo.split("/"); - if (!o || !r) { - console.error("Error: GITHUB_REPOSITORY format is invalid (expected: owner/repo)"); - process.exit(1); - } - owner = o; - repo = r; - } else { - const gitRepo = getCurrentRepo(projectPath); - if (!gitRepo) { - console.error("Error: Could not determine GitHub repository. Set GITHUB_REPOSITORY env var or configure git remote."); - process.exit(1); - } - owner = gitRepo.owner; - repo = gitRepo.repo; - } - - // Validate GitHub auth - if (!isGhAvailable() || !isGhAuthenticated()) { - console.error("Error: GitHub CLI (gh) is not available or not authenticated. Run 'gh auth login'."); - process.exit(1); - } - - // Build branch name using the established project convention - const branchName = `fusion/${id.toLowerCase()}`; - - // Build deterministic fallback PR title - const fallbackTitle = options.title - ? options.title - : task.title - ? task.title - : (() => { - const desc = task.description.trim(); - let derived = desc.charAt(0).toUpperCase() + desc.slice(1, 50); - if (desc.length > 50) { - derived += "…"; - } - return derived; - })(); - - let resolvedTitle = fallbackTitle; - let resolvedBody = options.body; - - const shouldUseAi = options.ai !== false && !(options.title && options.body); - if (shouldUseAi) { + // Fetch task and validate it exists + let task; try { - const settings = ("getSettings" in store - ? await store.getSettings() - : {}) as Parameters[0]["settings"]; - const generated = await dashboard.generatePrMetadata({ task, repoRoot: projectPath, settings }); - if (!options.title) { - resolvedTitle = generated.title; - } - if (!options.body) { - resolvedBody = generated.body; - } - console.log(" → Using AI-generated title/body (use --no-ai to skip)"); + task = await retryOnLock(async () => store.getTask(id), { id, action: "read task" }); } catch (err) { - process.stderr.write(`AI metadata generation failed; using fallback PR metadata. ${getGhErrorMessage(err)}\n`); + if (err instanceof LockRetryExhaustedError) { + throw err; + } + if (typeof err === "object" && err !== null && (err as Record).code === "ENOENT") { + console.error(`Error: Task ${id} not found`); + await closeProjectStore(context); + process.exit(1); + } + throw err; } - } - - // Create PR via GitHubClient - const client = new dashboard.GitHubClient(); - - try { - const prInfo = await client.createPr({ - owner, - repo, - title: resolvedTitle, - body: resolvedBody, - head: branchName, - base: options.base, - draft: options.draft, - reviewers: options.reviewers, - }); - - // Store PR info (legacy field, still read by some surfaces during migration). - await store.updatePrInfo(task.id, prInfo); - - // Also write the unified PR entity via the SAME store path the pr-create - // workflow node uses (mirrors pr-nodes.ts: ensure → flip to open with the - // persisted PR number/url). Without this the PR would be invisible to - // `fn pr list/show`, the reconciler, and the workflow nodes (R13 parity). - const entity = store.ensurePrEntityForSource({ - sourceType: "task", - sourceId: task.id, - repo: `${owner}/${repo}`, - headBranch: branchName, - baseBranch: prInfo.baseBranch, - state: "creating", - }); - store.updatePrEntity(entity.id, { - state: "open", - prNumber: prInfo.number, - prUrl: prInfo.url, - }); - - await store.logEntry(task.id, "Created PR", `PR #${prInfo.number}: ${prInfo.url}`); - - console.log(); - console.log(` ✓ Created PR for ${task.id}`); - console.log(` PR #${prInfo.number}: ${prInfo.url}`); - console.log(` Branch: ${branchName} → ${prInfo.baseBranch}`); - console.log(); - } catch (err) { - const msg = err instanceof Error ? err.message : String(err); - if (msg.includes("already exists")) { - console.error(`Error: A pull request already exists for ${owner}/${repo}:${branchName}`); + if (!task) { + console.error(`Error: Task ${id} not found`); + await closeProjectStore(context); process.exit(1); - } else if (msg.includes("No commits between")) { - console.error(`Error: No commits between ${options.base || "default base"} and ${branchName}. Push changes before creating PR.`); + } + + // Validate task is in 'in-review' column + if (task.column !== "in-review") { + console.error(`Error: Task must be in 'in-review' column to create a PR (current: ${task.column})`); + await closeProjectStore(context); process.exit(1); + } + + // Check if task already has PR info + if (task.prInfo) { + console.error(`Error: Task already has PR #${task.prInfo.number}: ${task.prInfo.url}`); + await closeProjectStore(context); + process.exit(1); + } + + // Determine owner/repo from GITHUB_REPOSITORY env or git remote + let owner: string; + let repo: string; + + const envRepo = process.env.GITHUB_REPOSITORY; + if (envRepo) { + const [o, r] = envRepo.split("/"); + if (!o || !r) { + console.error("Error: GITHUB_REPOSITORY format is invalid (expected: owner/repo)"); + await closeProjectStore(context); + process.exit(1); + } + owner = o; + repo = r; } else { - process.stderr.write(formatGhErrorForCli(err)); + const gitRepo = getCurrentRepo(projectPath); + if (!gitRepo) { + console.error("Error: Could not determine GitHub repository. Set GITHUB_REPOSITORY env var or configure git remote."); + await closeProjectStore(context); + process.exit(1); + } + owner = gitRepo.owner; + repo = gitRepo.repo; + } + + // Validate GitHub auth + if (!isGhAvailable() || !isGhAuthenticated()) { + console.error("Error: GitHub CLI (gh) is not available or not authenticated. Run 'gh auth login'."); + await closeProjectStore(context); process.exit(1); } + + // Build branch name using the established project convention + const branchName = `fusion/${id.toLowerCase()}`; + + // Build deterministic fallback PR title + const fallbackTitle = options.title + ? options.title + : task.title + ? task.title + : (() => { + const desc = task.description.trim(); + let derived = desc.charAt(0).toUpperCase() + desc.slice(1, 50); + if (desc.length > 50) { + derived += "…"; + } + return derived; + })(); + + let resolvedTitle = fallbackTitle; + let resolvedBody = options.body; + + const shouldUseAi = options.ai !== false && !(options.title && options.body); + if (shouldUseAi) { + try { + const settings = ("getSettings" in store + ? await retryOnLock(async () => store.getSettings(), { id, action: "read settings" }) + : {}) as Parameters[0]["settings"]; + const generated = await dashboard.generatePrMetadata({ task, repoRoot: projectPath, settings }); + if (!options.title) { + resolvedTitle = generated.title; + } + if (!options.body) { + resolvedBody = generated.body; + } + console.log(" → Using AI-generated title/body (use --no-ai to skip)"); + } catch (err) { + process.stderr.write(`AI metadata generation failed; using fallback PR metadata. ${getGhErrorMessage(err)}\n`); + } + } + + // Create PR via GitHubClient + const client = new dashboard.GitHubClient(); + + try { + // NOT retried as a whole — `client.createPr` is a GitHub network call with a + // real, non-idempotent side effect (opening a PR); only the discrete store + // writes afterward are wrapped in retryOnLock. + const prInfo = await client.createPr({ + owner, + repo, + title: resolvedTitle, + body: resolvedBody, + head: branchName, + base: options.base, + draft: options.draft, + reviewers: options.reviewers, + }); + + await retryOnLock( + async () => { + // Store PR info (legacy field, still read by some surfaces during migration). + await store.updatePrInfo(task.id, prInfo); + + // Also write the unified PR entity via the SAME store path the pr-create + // workflow node uses (mirrors pr-nodes.ts: ensure → flip to open with the + // persisted PR number/url). Without this the PR would be invisible to + // `fn pr list/show`, the reconciler, and the workflow nodes (R13 parity). + const entity = store.ensurePrEntityForSource({ + sourceType: "task", + sourceId: task.id, + repo: `${owner}/${repo}`, + headBranch: branchName, + baseBranch: prInfo.baseBranch, + state: "creating", + }); + store.updatePrEntity(entity.id, { + state: "open", + prNumber: prInfo.number, + prUrl: prInfo.url, + }); + + await store.logEntry(task.id, "Created PR", `PR #${prInfo.number}: ${prInfo.url}`); + }, + { id, action: "record created PR" }, + ); + + console.log(); + console.log(` ✓ Created PR for ${task.id}`); + console.log(` PR #${prInfo.number}: ${prInfo.url}`); + console.log(` Branch: ${branchName} → ${prInfo.baseBranch}`); + console.log(); + } catch (err) { + if (err instanceof LockRetryExhaustedError) { + throw err; + } + const msg = err instanceof Error ? err.message : String(err); + if (msg.includes("already exists")) { + console.error(`Error: A pull request already exists for ${owner}/${repo}:${branchName}`); + } else if (msg.includes("No commits between")) { + console.error(`Error: No commits between ${options.base || "default base"} and ${branchName}. Push changes before creating PR.`); + } else { + process.stderr.write(formatGhErrorForCli(err)); + } + await closeProjectStore(context); + process.exit(1); + } + } catch (error) { + if (error instanceof LockRetryExhaustedError) { + await failPrCommand(error, context); + } + throw error; + } finally { + if (context) { + await closeProjectStore(context); + } } } // ── Entity read commands (parity with GET /api/pull-requests[/:id]) ─────────── -/** Resolve a PR entity by its id (or 404-style exit). */ -function requireEntity(store: TaskStore, id: string): PrEntity { - const entity = store.getPrEntity(id); +/** + * Resolve a PR entity by its id (or 404-style exit). Wraps the discrete read + * in `retryOnLock` and, on not-found, closes `context`'s store BEFORE calling + * `process.exit(1)` (a pending `finally` does not run after `process.exit()`). + */ +async function requireEntity(context: ProjectContext, id: string): Promise { + const entity = await retryOnLock(async () => context.store.getPrEntity(id), { id, action: "read PR entity" }); if (!entity) { - console.error(`\n ✗ PR entity ${id} not found\n`); + console.error(`\n \u2717 PR entity ${id} not found\n`); + await closeProjectStore(context); process.exit(1); } return entity; } export async function runPrList(projectName?: string) { - const { store } = await getPrContext(projectName); - const entities = store.listActivePrEntities(); + try { + await retryOnLock( + async () => { + const context = await getPrContext(projectName); + try { + const { store } = context; + const entities = store.listActivePrEntities(); - if (entities.length === 0) { - console.log("\n No active pull requests.\n"); - return; - } + if (entities.length === 0) { + console.log("\n No active pull requests.\n"); + return; + } - console.log(); - for (const entity of entities) { - const num = entity.prNumber != null ? `#${entity.prNumber}` : "(no #)"; - const am = entity.autoMerge ? " auto-merge" : ""; - console.log(` ${entity.id} ${num} ${entity.repo} [${entity.state}]${am}`); + console.log(); + for (const entity of entities) { + const num = entity.prNumber != null ? `#${entity.prNumber}` : "(no #)"; + const am = entity.autoMerge ? " auto-merge" : ""; + console.log(` ${entity.id} ${num} ${entity.repo} [${entity.state}]${am}`); + } + console.log(); + } finally { + await closeProjectStore(context); + } + }, + { id: "pull-requests", action: "list PR entities" }, + ); + } catch (error) { + if (error instanceof LockRetryExhaustedError) { + await failPrCommand(error); + } + throw error; } - console.log(); } export async function runPrShow(id: string, projectName?: string) { @@ -277,26 +365,38 @@ export async function runPrShow(id: string, projectName?: string) { console.error("Usage: fn pr show "); process.exit(1); } - const { store } = await getPrContext(projectName); - const entity = requireEntity(store, id); - const threads: PrThreadState[] = store.listPrThreadStates(entity.id); - const pending = threads.filter((t) => t.outcome === "pending").length; - const disagreed = threads.filter((t) => t.outcome === "disagreed").length; + let context: ProjectContext | undefined; + try { + context = await retryOnLock(() => getPrContext(projectName), { id, action: "resolve project" }); + const entity = await requireEntity(context, id); + const threads: PrThreadState[] = await retryOnLock(async () => context!.store.listPrThreadStates(entity.id), { id, action: "read PR thread states" }); + const pending = threads.filter((t) => t.outcome === "pending").length; + const disagreed = threads.filter((t) => t.outcome === "disagreed").length; - console.log(); - console.log(` PR entity ${entity.id}`); - console.log(` Source: ${entity.sourceType}/${entity.sourceId}`); - console.log(` Repo: ${entity.repo}`); - console.log(` Branch: ${entity.headBranch}${entity.baseBranch ? ` → ${entity.baseBranch}` : ""}`); - console.log(` State: ${entity.state}${entity.prNumber != null ? ` (#${entity.prNumber})` : ""}`); - if (entity.prUrl) console.log(` URL: ${entity.prUrl}`); - console.log(` Mergeable: ${entity.mergeable ?? "unknown"}`); - console.log(` Review: ${entity.reviewDecision ?? "none"}`); - console.log(` Checks: ${entity.checksRollup ?? "none"}`); - console.log(` Auto-merge: ${entity.autoMerge ? "on" : "off"} (${autoMergeGateReason(entity)})`); - console.log(` Active: ${isPrEntityActive(entity) ? "yes" : "no"}; actionable: ${isPrEntityActionable(entity) ? "yes" : "no"}`); - console.log(` Rounds: ${entity.responseRounds}; threads: ${threads.length} (${pending} pending, ${disagreed} disagreed)`); - console.log(); + console.log(); + console.log(` PR entity ${entity.id}`); + console.log(` Source: ${entity.sourceType}/${entity.sourceId}`); + console.log(` Repo: ${entity.repo}`); + console.log(` Branch: ${entity.headBranch}${entity.baseBranch ? ` → ${entity.baseBranch}` : ""}`); + console.log(` State: ${entity.state}${entity.prNumber != null ? ` (#${entity.prNumber})` : ""}`); + if (entity.prUrl) console.log(` URL: ${entity.prUrl}`); + console.log(` Mergeable: ${entity.mergeable ?? "unknown"}`); + console.log(` Review: ${entity.reviewDecision ?? "none"}`); + console.log(` Checks: ${entity.checksRollup ?? "none"}`); + console.log(` Auto-merge: ${entity.autoMerge ? "on" : "off"} (${autoMergeGateReason(entity)})`); + console.log(` Active: ${isPrEntityActive(entity) ? "yes" : "no"}; actionable: ${isPrEntityActionable(entity) ? "yes" : "no"}`); + console.log(` Rounds: ${entity.responseRounds}; threads: ${threads.length} (${pending} pending, ${disagreed} disagreed)`); + console.log(); + } catch (error) { + if (error instanceof LockRetryExhaustedError) { + await failPrCommand(error, context); + } + throw error; + } finally { + if (context) { + await closeProjectStore(context); + } + } } // ── User-controlled actions (parity with POST /api/pull-requests/:id/*) ─────── @@ -317,24 +417,44 @@ async function runReleaseAction( console.error(`Usage: fn pr ${label} `); process.exit(1); } - const { store } = await getPrContext(projectName); - const entity = requireEntity(store, id); + let context: ProjectContext | undefined; + try { + context = await retryOnLock(() => getPrContext(projectName), { id, action: "resolve project" }); + const { store } = context; + const entity = await requireEntity(context, id); - if (!isPrEntityActive(entity)) { - console.error(`\n ✗ PR ${id} is already terminal (merged/closed/failed)\n`); - process.exit(1); - } - if (opts.rejectConflict && entity.mergeable === "conflicting") { - console.error(`\n ✗ Resolve conflicts on GitHub before merging\n`); - process.exit(1); - } + if (!isPrEntityActive(entity)) { + console.error(`\n \u2717 PR ${id} is already terminal (merged/closed/failed)\n`); + await closeProjectStore(context); + process.exit(1); + } + if (opts.rejectConflict && entity.mergeable === "conflicting") { + console.error(`\n \u2717 Resolve conflicts on GitHub before merging\n`); + await closeProjectStore(context); + process.exit(1); + } - const result = await releaseHeldTaskByEvent(store, entity.sourceId, eventTag); - if (!result.released) { - console.error(`\n ✗ ${label} did not release ${id}: ${result.rejection ?? "unknown"}\n`); - process.exit(1); + // NOT wrapped in retryOnLock: `releaseHeldTaskByEvent` fires the workflow's + // user-controlled release edge, owning further engine/GitHub side effects — + // retrying it on a later lock error would risk re-firing an already-applied + // release. + const result = await releaseHeldTaskByEvent(store, entity.sourceId, eventTag); + if (!result.released) { + console.error(`\n \u2717 ${label} did not release ${id}: ${result.rejection ?? "unknown"}\n`); + await closeProjectStore(context); + process.exit(1); + } + console.log(`\n \u2713 ${label} fired for ${id}${result.toColumn ? ` → ${result.toColumn}` : ""}\n`); + } catch (error) { + if (error instanceof LockRetryExhaustedError) { + await failPrCommand(error, context); + } + throw error; + } finally { + if (context) { + await closeProjectStore(context); + } } - console.log(`\n ✓ ${label} fired for ${id}${result.toColumn ? ` → ${result.toColumn}` : ""}\n`); } export async function runPrApprove(id: string, projectName?: string) { @@ -362,17 +482,31 @@ export async function runPrAutomerge(id: string, enabled: boolean | undefined, p console.error("Usage: fn pr automerge [on|off]"); process.exit(1); } - const { store } = await getPrContext(projectName); - const entity = requireEntity(store, id); + let context: ProjectContext | undefined; + try { + context = await retryOnLock(() => getPrContext(projectName), { id, action: "resolve project" }); + const { store } = context; + const entity = await requireEntity(context, id); - if (!isPrEntityActive(entity)) { - console.error(`\n ✗ PR ${id} is already terminal (merged/closed/failed)\n`); - process.exit(1); + if (!isPrEntityActive(entity)) { + console.error(`\n \u2717 PR ${id} is already terminal (merged/closed/failed)\n`); + await closeProjectStore(context); + process.exit(1); + } + + const next = typeof enabled === "boolean" ? enabled : !entity.autoMerge; + const updated = await retryOnLock(async () => store.updatePrEntity(id, { autoMerge: next }), { id, action: "update PR auto-merge" }); + console.log(`\n \u2713 Auto-merge ${updated.autoMerge ? "enabled" : "disabled"} for ${id} (${autoMergeGateReason(updated)})\n`); + } catch (error) { + if (error instanceof LockRetryExhaustedError) { + await failPrCommand(error, context); + } + throw error; + } finally { + if (context) { + await closeProjectStore(context); + } } - - const next = typeof enabled === "boolean" ? enabled : !entity.autoMerge; - const updated = store.updatePrEntity(id, { autoMerge: next }); - console.log(`\n ✓ Auto-merge ${updated.autoMerge ? "enabled" : "disabled"} for ${id} (${autoMergeGateReason(updated)})\n`); } export interface PrAutomergeCleanupOptions { @@ -381,11 +515,34 @@ export interface PrAutomergeCleanupOptions { } export async function runPrAutomergeCleanup(options: PrAutomergeCleanupOptions = {}, projectName?: string) { - const { store } = await getPrContext(projectName); - const results = options.apply - ? await store.reconcileLegacyAutoMergeStamps({ apply: true }) - : await store.reconcileLegacyAutoMergeStamps(); + let context: ProjectContext | undefined; + try { + context = await retryOnLock(() => getPrContext(projectName), { id: "pr-automerge-cleanup", action: "resolve project" }); + const { store } = context; + const results = await retryOnLock( + async () => + options.apply + ? store.reconcileLegacyAutoMergeStamps({ apply: true }) + : store.reconcileLegacyAutoMergeStamps(), + { id: "pr-automerge-cleanup", action: "reconcile legacy auto-merge stamps" }, + ); + printAutomergeCleanupResults(results, options); + } catch (error) { + if (error instanceof LockRetryExhaustedError) { + await failPrCommand(error, context); + } + throw error; + } finally { + if (context) { + await closeProjectStore(context); + } + } +} +function printAutomergeCleanupResults( + results: Awaited>, + options: PrAutomergeCleanupOptions, +): void { if (options.json) { console.log(JSON.stringify({ mode: options.apply ? "apply" : "dry-run", diff --git a/packages/cli/src/project-context.ts b/packages/cli/src/project-context.ts index f32b887595..0e25a59143 100644 --- a/packages/cli/src/project-context.ts +++ b/packages/cli/src/project-context.ts @@ -356,3 +356,29 @@ export async function resolveProjectPathOnly( await closeProjectStore(context); return context.projectPath; } + +/** + * FNXC:CliBoardMutation 2026-07-09-00:00: + * Wrap an already-constructed, UNCACHED local `TaskStore` (the CWD-fallback + * branch several board command files build directly via + * `new TaskStore(process.cwd())` when `resolveProject` throws — e.g. + * `getBranchGroupContext`/`getPrContext` in `packages/cli/src/commands/ + * branch-group.ts`/`pr.ts`, FN-7738) as a well-formed `ProjectContext` so + * `closeProjectStore` can close+evict it the same way it handles a cached + * context, even though `storeCache` holds no matching entry for it (eviction + * is then a harmless no-op; the `.close()` call is what matters). Mirrors + * `packages/cli/src/commands/task.ts`'s private `asLocalProjectContext` + * helper (kept private there per FN-7734/FN-7738 scope boundaries — this + * export exists so `branch-group.ts`/`pr.ts` do not need to fork a second + * copy). + */ +export function asLocalProjectContext(store: TaskStore): ProjectContext { + const cwd = process.cwd(); + return { + projectId: cwd, + projectPath: cwd, + projectName: basename(cwd) || "current-project", + isRegistered: false, + store, + }; +}