diff --git a/.changeset/fn-pr-commands.md b/.changeset/fn-pr-commands.md index f656968891..22aa9594a9 100644 --- a/.changeset/fn-pr-commands.md +++ b/.changeset/fn-pr-commands.md @@ -1,5 +1,5 @@ --- -"@runfusion/fusion": minor +"@runfusion/fusion": major --- Add the unified `fn pr` command namespace for CLI parity with the dashboard's diff --git a/packages/cli/src/__tests__/bin.test.ts b/packages/cli/src/__tests__/bin.test.ts index 1acc9d5ee8..0547525ce6 100644 --- a/packages/cli/src/__tests__/bin.test.ts +++ b/packages/cli/src/__tests__/bin.test.ts @@ -805,10 +805,10 @@ describe("bin command routing and fallbacks", () => { }); it("routes daemon command with all flags", async () => { - await runBin(["daemon", "--port", "4040", "--host", "127.0.0.1", "--token", "fn_abc123", "--paused", "--token-only"]); + await runBin(["daemon", "--port", "5055", "--host", "127.0.0.1", "--token", "fn_abc123", "--paused", "--token-only"]); expect(commandMocks.runDaemon).toHaveBeenCalledWith({ - port: 4040, + port: 5055, paused: true, interactive: false, host: "127.0.0.1", diff --git a/packages/cli/src/__tests__/pr-command.test.ts b/packages/cli/src/__tests__/pr-command.test.ts index f8f7b84782..ab2e8c1e98 100644 --- a/packages/cli/src/__tests__/pr-command.test.ts +++ b/packages/cli/src/__tests__/pr-command.test.ts @@ -16,15 +16,31 @@ vi.mock("@fusion/engine", () => ({ releaseHeldTaskByEvent: (...args: unknown[]) => releaseHeldTaskByEvent(...args), })); -// @fusion/dashboard is only touched by runPrCreate (not exercised here); stub it -// so importing the module never pulls the heavy dashboard graph. +// @fusion/dashboard is touched by runPrCreate; stub it so importing the module +// never pulls the heavy dashboard graph. `createPr` is controllable so the create +// path can be asserted to write the unified PR entity. +const createPr = vi.fn(); vi.mock("@fusion/dashboard", () => ({ - GitHubClient: class {}, + GitHubClient: class { + createPr(...args: unknown[]) { + return createPr(...args); + } + }, generatePrMetadata: vi.fn(), })); +// gh-cli helpers used by runPrCreate (repo resolution + auth gating). +vi.mock("@fusion/core/gh-cli", () => ({ + classifyGhError: vi.fn(() => ({ message: "err" })), + getGhErrorMessage: vi.fn(() => "err"), + getCurrentRepo: vi.fn(() => ({ owner: "owner", repo: "repo" })), + isGhAuthenticated: vi.fn(() => true), + isGhAvailable: vi.fn(() => true), +})); + const { resolveProject } = await import("../project-context.js"); const { + runPrCreate, runPrList, runPrShow, runPrApprove, @@ -86,6 +102,54 @@ describe("fn pr commands", () => { vi.restoreAllMocks(); }); + // ── create → legacy prInfo + unified PR entity ────────────────────────────── + + it("runPrCreate writes the unified PR entity (not just legacy prInfo)", async () => { + const prInfo = { + url: "https://github.com/owner/repo/pull/7", + number: 7, + status: "open", + title: "T", + headBranch: "fusion/fn-001", + baseBranch: "main", + commentCount: 0, + }; + delete process.env.GITHUB_REPOSITORY; + createPr.mockResolvedValue(prInfo); + const getTask = vi.fn().mockResolvedValue({ + id: "FN-001", + title: "Task one", + description: "do a thing", + column: "in-review", + prInfo: undefined, + }); + const updatePrInfo = vi.fn(); + const ensurePrEntityForSource = vi.fn().mockReturnValue(makeEntity({ id: "PR-NEW", state: "creating" })); + const updatePrEntity = vi.fn().mockReturnValue(makeEntity({ id: "PR-NEW" })); + const logEntry = vi.fn(); + mockStore({ getTask, updatePrInfo, ensurePrEntityForSource, updatePrEntity, logEntry }); + + await runPrCreate("FN-001", { ai: false }); + + // Legacy field is still written (additive, migration-safe). + expect(updatePrInfo).toHaveBeenCalledWith("FN-001", prInfo); + // Unified entity is created via the same store path the pr-create node uses. + expect(ensurePrEntityForSource).toHaveBeenCalledWith({ + sourceType: "task", + sourceId: "FN-001", + repo: "owner/repo", + headBranch: "fusion/fn-001", + baseBranch: "main", + state: "creating", + }); + // …then flipped to open with the persisted PR number/url. + expect(updatePrEntity).toHaveBeenCalledWith("PR-NEW", { + state: "open", + prNumber: 7, + prUrl: "https://github.com/owner/repo/pull/7", + }); + }); + // ── read commands ────────────────────────────────────────────────────────── it("runPrList reads active entities from the store", async () => { diff --git a/packages/cli/src/commands/pr.ts b/packages/cli/src/commands/pr.ts index 2b27bf0012..f7c8aefa1f 100644 --- a/packages/cli/src/commands/pr.ts +++ b/packages/cli/src/commands/pr.ts @@ -2,7 +2,7 @@ import { TaskStore, isPrEntityActive, isPrEntityActionable, - isPrEntityAutoMergeReady, + autoMergeGateReason, type PrEntity, type PrThreadState, } from "@fusion/core"; @@ -199,8 +199,27 @@ export async function runPrCreate(id: string, options: PrCreateOptions = {}, pro reviewers: options.reviewers, }); - // Store PR info + // 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(); @@ -235,16 +254,6 @@ function requireEntity(store: TaskStore, id: string): PrEntity { return entity; } -function autoMergeReason(entity: PrEntity): string { - if (!entity.autoMerge) return "Auto-merge off"; - if (entity.mergeable === "conflicting") return "Blocked: conflict"; - if (entity.reviewDecision !== "APPROVED") return "Waiting for approval"; - if (entity.checksRollup !== "success") return "Waiting for checks"; - if (entity.mergeable !== "clean") return "Waiting for checks"; - if (isPrEntityAutoMergeReady(entity)) return "Ready to merge"; - return "Waiting for checks"; -} - export async function runPrList(projectName?: string) { const { store } = await getPrContext(projectName); const entities = store.listActivePrEntities(); @@ -284,7 +293,7 @@ export async function runPrShow(id: string, projectName?: string) { 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"} (${autoMergeReason(entity)})`); + 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(); @@ -363,5 +372,5 @@ export async function runPrAutomerge(id: string, enabled: boolean | undefined, p 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} (${autoMergeReason(updated)})\n`); + console.log(`\n ✓ Auto-merge ${updated.autoMerge ? "enabled" : "disabled"} for ${id} (${autoMergeGateReason(updated)})\n`); } diff --git a/packages/cli/src/commands/task-lifecycle.ts b/packages/cli/src/commands/task-lifecycle.ts index 19a4a862ec..38224079ec 100644 --- a/packages/cli/src/commands/task-lifecycle.ts +++ b/packages/cli/src/commands/task-lifecycle.ts @@ -340,7 +340,7 @@ export function syncGroupPrCallback( * without the stale-head guard, which the reconcile still corroborates. */ async function resolveBranchHeadOid(cwd: string, branch: string): Promise { try { - const { stdout } = await execAsync(`git rev-parse "${branch}"`, { cwd, timeout: 30_000 }); + const { stdout } = await execFileAsync("git", ["rev-parse", branch], { cwd, timeout: 30_000 }); const oid = stdout.trim(); return oid.length > 0 ? oid : undefined; } catch { diff --git a/packages/core/src/__tests__/pr-entity.test.ts b/packages/core/src/__tests__/pr-entity.test.ts index cdf42054c2..11d2c4b512 100644 --- a/packages/core/src/__tests__/pr-entity.test.ts +++ b/packages/core/src/__tests__/pr-entity.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from "vitest"; import { + autoMergeGateReason, isPrBacked, isPrEntityActionable, isPrEntityActive, @@ -71,4 +72,19 @@ describe("PR entity predicates", () => { expect(isPrEntityAutoMergeReady({ ...base, mergeable: "unknown" })).toBe(false); expect(isPrEntityAutoMergeReady({ ...base, unverified: true })).toBe(false); }); + + it("autoMergeGateReason is the single R13-shared status string for both surfaces", () => { + const ready = entity({ + autoMerge: true, + reviewDecision: "APPROVED", + checksRollup: "success", + mergeable: "clean", + }); + expect(autoMergeGateReason(ready)).toBe("Ready to merge"); + expect(autoMergeGateReason({ ...ready, autoMerge: false })).toBe("Auto-merge off"); + expect(autoMergeGateReason({ ...ready, mergeable: "conflicting" })).toBe("Blocked: conflict"); + expect(autoMergeGateReason({ ...ready, reviewDecision: "CHANGES_REQUESTED" })).toBe("Waiting for approval"); + expect(autoMergeGateReason({ ...ready, checksRollup: "pending" })).toBe("Waiting for checks"); + expect(autoMergeGateReason({ ...ready, mergeable: "unknown" })).toBe("Waiting for checks"); + }); }); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 6afdbe9a37..a928b694db 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -611,6 +611,7 @@ export { isPrBacked, isPrEntityActionable, isPrEntityAutoMergeReady, + autoMergeGateReason, } from "./pr-entity.js"; export { findVitestProcessIds, diff --git a/packages/core/src/pr-entity.ts b/packages/core/src/pr-entity.ts index 6f2eb1a191..39f071a1da 100644 --- a/packages/core/src/pr-entity.ts +++ b/packages/core/src/pr-entity.ts @@ -63,3 +63,21 @@ export function isPrEntityAutoMergeReady( if (entity.mergeable !== "clean") return false; return true; } + +/** + * The live auto-merge gate reason shown next to the toggle (R11). Mirrors the + * auto-merge-ready predicate ordering so every surface (the dashboard route and + * the `fn pr` CLI) reports the same status and never disagrees with what the gate + * will actually do. Shared in @fusion/core (R13) so the two surfaces cannot drift. + */ +export function autoMergeGateReason( + entity: Pick, +): string { + if (!entity.autoMerge) return "Auto-merge off"; + if (entity.mergeable === "conflicting") return "Blocked: conflict"; + if (entity.reviewDecision !== "APPROVED") return "Waiting for approval"; + if (entity.checksRollup !== "success") return "Waiting for checks"; + if (entity.mergeable !== "clean") return "Waiting for checks"; + if (isPrEntityAutoMergeReady(entity)) return "Ready to merge"; + return "Waiting for checks"; +} diff --git a/packages/dashboard/app/__tests__/lazy-loaded-views-docs.test.ts b/packages/dashboard/app/__tests__/lazy-loaded-views-docs.test.ts index 9699c5d481..bff329f1df 100644 --- a/packages/dashboard/app/__tests__/lazy-loaded-views-docs.test.ts +++ b/packages/dashboard/app/__tests__/lazy-loaded-views-docs.test.ts @@ -76,7 +76,7 @@ function extractAppLazyViews(appSource: string): Set { } describe("AGENTS lazy-loaded views inventory", () => { - it("documents the App-level lazy views accurately and keeps the curated 19-view list in sync", () => { + it("documents the App-level lazy views accurately and keeps the curated 20-view list in sync", () => { const agentsDoc = readFileSync(resolve(__dirname, "../../../../AGENTS.md"), "utf-8"); const appSource = readFileSync(resolve(__dirname, "../App.tsx"), "utf-8"); diff --git a/packages/dashboard/src/routes/register-pull-requests-routes.ts b/packages/dashboard/src/routes/register-pull-requests-routes.ts index 8b8ecede1e..294168c3a8 100644 --- a/packages/dashboard/src/routes/register-pull-requests-routes.ts +++ b/packages/dashboard/src/routes/register-pull-requests-routes.ts @@ -4,6 +4,7 @@ import { isPrEntityActive, isPrEntityActionable, isPrEntityAutoMergeReady, + autoMergeGateReason, } from "@fusion/core"; import { badRequest, notFound, ApiError } from "../api-error.js"; @@ -41,20 +42,10 @@ function parseProjectId(req: Request): string | undefined { return typeof value === "string" && value.trim() ? value.trim() : undefined; } -/** - * The live auto-merge gate reason shown next to the toggle (R11). Mirrors the - * engine's auto-merge-ready predicate ordering so the UI never disagrees with - * what the gate will actually do. - */ -export function autoMergeGateReason(entity: PrEntity): string { - if (!entity.autoMerge) return "Auto-merge off"; - if (entity.mergeable === "conflicting") return "Blocked: conflict"; - if (entity.reviewDecision !== "APPROVED") return "Waiting for approval"; - if (entity.checksRollup !== "success") return "Waiting for checks"; - if (entity.mergeable !== "clean") return "Waiting for checks"; - if (isPrEntityAutoMergeReady(entity)) return "Ready to merge"; - return "Waiting for checks"; -} +// `autoMergeGateReason` is the single R13-shared definition in @fusion/core +// (consumed here and by the `fn pr` CLI); re-exported so existing dashboard +// importers keep working. +export { autoMergeGateReason }; /** Whether the entity is in a hard conflict (Merge must be disabled, R11). */ export function isPrConflicting(entity: PrEntity): boolean { diff --git a/packages/engine/src/__tests__/pr-nodes.test.ts b/packages/engine/src/__tests__/pr-nodes.test.ts index 07fb0bc426..d5b6d1cc59 100644 --- a/packages/engine/src/__tests__/pr-nodes.test.ts +++ b/packages/engine/src/__tests__/pr-nodes.test.ts @@ -185,14 +185,46 @@ describe("PR node handlers (U3)", () => { expect(store.getPrEntity(created.id)?.responseRounds).toBe(1); }); - it("pr-respond delegates to the injected respond callback", async () => { - store.ensurePrEntityForSource({ ...SOURCE, state: "open", prNumber: 9 }); - const respond = vi.fn(async () => ({ value: "fixed" as const, contextPatch: { k: "v" } })); + it("pr-respond delegates to the injected respond callback with the POST-increment entity", async () => { + const created = store.ensurePrEntityForSource({ ...SOURCE, state: "open", prNumber: 9 }); + store.updatePrEntity(created.id, { responseRounds: 3 }); + let forwardedRounds: number | undefined; + const respond: PrNodeDeps["respond"] = async (input) => { + forwardedRounds = input.entity.responseRounds; + return { value: "fixed" as const, contextPatch: { k: "v" } }; + }; const handlers = createPrNodeHandlers(deps({ respond })); const result = await handlers["pr-respond"]({ id: "r", kind: "pr-respond" } as WorkflowIrNode, ctx()); expect(result).toEqual({ outcome: "success", value: "fixed", contextPatch: { k: "v" } }); - expect(respond).toHaveBeenCalledTimes(1); + // The handler must forward the entity returned by updatePrEntity (post-increment), + // not the stale pre-increment copy — otherwise the R8 cap check fires one round late. + expect(forwardedRounds).toBe(4); + }); + + it("pr-merge / pr-respond resolve a branch-group entity via branchContext.groupId, not task id", async () => { + // Branch-group PR entities are keyed by the GROUP id (sourceId = branch_groups.id). + // A shared-mode task carries that id on branchContext.groupId, NOT task.id. + const groupId = "BG-1"; + store.ensurePrEntityForSource({ + sourceType: "branch-group", + sourceId: groupId, + repo: "owner/repo", + headBranch: "fusion/bg-1", + state: "open", + prNumber: 11, + }); + const groupCtx = { + task: { id: "T-shared", branchContext: { groupId } } as unknown as TaskDetail, + settings: undefined, + context: {}, + } as WorkflowNodeExecutionContext; + + const mergePr = vi.fn(async () => ({ status: "merged-requested" }) as PrMergeCallResult); + const handlers = createPrNodeHandlers(deps({ mergePr })); + const merge = await handlers["pr-merge"]({ id: "m", kind: "pr-merge" } as WorkflowIrNode, groupCtx); + expect(merge).toEqual({ outcome: "success", value: "merged-requested" }); + expect(mergePr).toHaveBeenCalledTimes(1); }); it("unwired pr-* deps fail closed (value:pr-nodes-unwired)", async () => { diff --git a/packages/engine/src/pr-nodes.ts b/packages/engine/src/pr-nodes.ts index d1a9c8f0e0..a9c3e04da7 100644 --- a/packages/engine/src/pr-nodes.ts +++ b/packages/engine/src/pr-nodes.ts @@ -48,6 +48,22 @@ export interface PrNodeStore extends PrResponseRunStore { updatePrEntity(id: string, patch: PrEntityUpdate): PrEntity; } +/** + * Resolve the single live PR entity backing a task: prefer the task-keyed entity, + * then fall back to the branch-group entity. Branch-group PR entities are keyed by + * the branch GROUP id (sourceId = branch_groups.id, per migration 113), which the + * task carries on `branchContext.groupId` — NOT the task id. Falling back on + * `task.id` can never match a branch-group entity, so a shared-mode task would + * spuriously resolve to no-entity. + */ +function resolveActivePrEntity(store: PrNodeStore, task: TaskDetail): PrEntity | null { + const taskEntity = store.getActivePrEntityBySource("task", task.id); + if (taskEntity) return taskEntity; + const groupId = task.branchContext?.groupId; + if (!groupId) return null; + return store.getActivePrEntityBySource("branch-group", groupId); +} + /** Identity of the PR an entity is created for, resolved from the task + node. */ export interface PrSourceDescriptor extends PrEntityCreateInput {} @@ -339,8 +355,7 @@ export function createPrNodeHandlers(deps: PrNodeDeps): Record< // merge request emits value:"merged-requested". const prMerge: WorkflowNodeHandler = async (node, ctx) => { const store = deps.getStore(); - const entity = store.getActivePrEntityBySource("task", ctx.task.id) - ?? store.getActivePrEntityBySource("branch-group", ctx.task.id); + const entity = resolveActivePrEntity(store, ctx.task); if (!entity) { audit("pr-merge-no-entity", `pr-merge node '${node.id}' found no live PR entity for task ${ctx.task.id}`); @@ -386,8 +401,7 @@ export function createPrNodeHandlers(deps: PrNodeDeps): Record< // responseRounds (the R8 iteration-cap counter, survives restart). const prRespond: WorkflowNodeHandler = async (node, ctx) => { const store = deps.getStore(); - const entity = store.getActivePrEntityBySource("task", ctx.task.id) - ?? store.getActivePrEntityBySource("branch-group", ctx.task.id); + const entity = resolveActivePrEntity(store, ctx.task); if (!entity) { audit("pr-respond-no-entity", `pr-respond node '${node.id}' found no live PR entity for task ${ctx.task.id}`); @@ -400,8 +414,11 @@ export function createPrNodeHandlers(deps: PrNodeDeps): Record< return { outcome: "success", value: "not-actionable" }; } - // Bump the rework-cycle counter (R8 cap backing; persisted). - store.updatePrEntity(entity.id, { responseRounds: entity.responseRounds + 1 }); + // Bump the rework-cycle counter (R8 cap backing; persisted). Forward the + // POST-update entity so runPrResponseRun's cap check (`responseRounds > cap`) + // sees this round's count — passing the stale pre-increment entity fires the + // cap one round too late. + const updatedEntity = store.updatePrEntity(entity.id, { responseRounds: entity.responseRounds + 1 }); if (!deps.respond) { // U3 default: inert but routable. U5 wires the real review-response run. @@ -410,7 +427,7 @@ export function createPrNodeHandlers(deps: PrNodeDeps): Record< let result: PrRespondCallResult; try { - result = await deps.respond({ task: ctx.task, node, entity, context: ctx.context }); + result = await deps.respond({ task: ctx.task, node, entity: updatedEntity, context: ctx.context }); } catch (err) { const reason = classifyError(err); audit("pr-respond-error", `pr-respond node '${node.id}' response run failed: ${reason}`); @@ -456,8 +473,7 @@ export function createAutoMergeGateHandler(deps: Pick { const store = deps.getStore(); - const entity = store.getActivePrEntityBySource("task", ctx.task.id) - ?? store.getActivePrEntityBySource("branch-group", ctx.task.id); + const entity = resolveActivePrEntity(store, ctx.task); if (!entity) { // No live entity → cannot auto-merge; park for manual handling (never block).