diff --git a/.changeset/cli-pr-merge-renamed-review-lane.md b/.changeset/cli-pr-merge-renamed-review-lane.md new file mode 100644 index 0000000000..e489b2607a --- /dev/null +++ b/.changeset/cli-pr-merge-renamed-review-lane.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Fix PR merges silently never running on boards with a renamed review lane. +category: fix +dev: `processPullRequestMergeTask` now resolves the task's own merge-orchestration lane via `resolveMergeOrchestrationColumn` and passes it to `getTaskMergeBlocker` as `reviewColumns`, instead of letting the blocker fall back to the literal `in-review` and return "skipped". Affects the `daemon`, `serve` and `dashboard` PR-merge drains. Default and v1-upgraded boards are unchanged. diff --git a/packages/cli/src/__tests__/pr-merge-review-lane.test.ts b/packages/cli/src/__tests__/pr-merge-review-lane.test.ts new file mode 100644 index 0000000000..75362054fa --- /dev/null +++ b/packages/cli/src/__tests__/pr-merge-review-lane.test.ts @@ -0,0 +1,107 @@ +/* +FNXC:WorkflowResolvedColumns 2026-07-30-23:55: +THE INVARIANT: a PR merge is decided against THIS task's merge lane, not the literal `in-review`. + +`processPullRequestMergeTask` called its injected `getTaskMergeBlocker` with the task alone, so the +blocker's `options.reviewColumns` was undefined and its identity check fell back to +`task.column === "in-review"`. On a renamed board it answered `task is in 'checking', must be in +'in-review'` and this function returned "skipped" — silently, forever. Nothing logs and nothing fails; +the PR just never merges. `daemon.ts`, `serve.ts` and `dashboard.ts` all drain PR merges through here. + +There was no test for this function at all, which is why it went unnoticed. + +Both cases below fail when the product change is reverted: the first sees the blocker called with no +options, the second gets "skipped" back for a card sitting in its board's real merge lane. +*/ +import { describe, expect, it, vi } from "vitest"; +import { getTaskMergeBlocker } from "@fusion/core"; +import { processPullRequestMergeTask } from "../commands/task-lifecycle.js"; + +const renamedIr = { + id: "wf-renamed", + version: "v2", + columns: [ + { id: "backlog", name: "Backlog", traits: [{ trait: "intake" }] }, + { id: "building", name: "Building", traits: [{ trait: "wip" }] }, + { id: "checking", name: "Checking", traits: [{ trait: "merge" }, { trait: "merge-blocker" }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + ], +}; + +function makeTask(column: string) { + return { + id: "FN-1", + column, + description: "task", + paused: false, + status: null, + error: null, + steps: [], + workflowStepResults: [], + dependencies: [], + currentStep: 0, + log: [], + }; +} + +function createStore(column: string, workflowId: string | undefined = "wf-renamed") { + return { + getTask: vi.fn(async () => makeTask(column)), + getTaskWorkflowSelectionAsync: vi.fn(async () => (workflowId ? { workflowId } : undefined)), + getTaskWorkflowSelection: vi.fn(() => (workflowId ? { workflowId } : undefined)), + getWorkflowDefinition: vi.fn(async () => ({ id: "wf-renamed", ir: renamedIr })), + }; +} + +/* The merge path needs a real repo it will not get here; the blocker decision happens first, which is + the whole point, so the throw afterwards is caught and never asserted on. */ +async function runAndSettle(store: unknown, blocker: unknown): Promise { + try { + return (await processPullRequestMergeTask( + store as never, + "/nonexistent-cwd", + "FN-1", + {} as never, + blocker as never, + )) as string; + } catch (error) { + return `threw:${(error as Error).message}`; + } +} + +describe("PR merge decides against the task's own merge lane", () => { + it("hands the blocker the resolved merge lane, not the literal", async () => { + const blocker = vi.fn(() => undefined); + + await runAndSettle(createStore("checking"), blocker); + + expect(blocker).toHaveBeenCalledWith( + expect.objectContaining({ column: "checking" }), + { reviewColumns: new Set(["checking"]) }, + ); + }); + + it("does not skip a card sitting in the board's real merge lane", async () => { + // The REAL core blocker — the one daemon/serve/dashboard actually inject. + const result = await runAndSettle(createStore("checking"), getTaskMergeBlocker); + + expect(result).not.toBe("skipped"); + }); + + it("still skips a card that is not in any merge lane", async () => { + const result = await runAndSettle(createStore("building"), getTaskMergeBlocker); + + expect(result).toBe("skipped"); + }); + + it("falls back to the legacy lane when the workflow resolves no merge column", async () => { + // A v1-upgraded IR resolves every role empty; `in-review` must still merge there. + const blocker = vi.fn(() => undefined); + const store = createStore("in-review", undefined); + store.getWorkflowDefinition = vi.fn(async () => ({ id: "v1", ir: { id: "v1", version: "v2", columns: [] } })) as never; + + await runAndSettle(store, blocker); + + expect(blocker).toHaveBeenCalledWith(expect.objectContaining({ column: "in-review" }), { reviewColumns: undefined }); + }); +}); diff --git a/packages/cli/src/commands/task-lifecycle.ts b/packages/cli/src/commands/task-lifecycle.ts index 0836ed9201..e7970fe227 100644 --- a/packages/cli/src/commands/task-lifecycle.ts +++ b/packages/cli/src/commands/task-lifecycle.ts @@ -35,7 +35,7 @@ import { WorkspaceTaskMergeError, } from "@fusion/core"; import type { Settings, TaskDetail, PrInfo, MergeResult, BranchGroup, BranchGroupPrState, Task } from "@fusion/core"; -import { resolveWorkflowIrForTask, resolveCompleteColumn } from "@fusion/core"; +import { resolveWorkflowIrForTask, resolveCompleteColumn, resolveMergeOrchestrationColumn } from "@fusion/core"; /* FNXC:WorkflowResolvedColumns 2026-07-30-20:10 (census-invisible moveTask destinations): @@ -778,7 +778,10 @@ export type ProcessPullRequestResult = "waiting" | "merged" | "skipped"; * Type for the task merge blocker function from @fusion/core. * Accepts a task object and returns a reason string if blocked, or undefined if not blocked. */ -type TaskMergeBlockerFn = (task: TaskDetail) => string | undefined; +type TaskMergeBlockerFn = ( + task: TaskDetail, + options?: { reviewColumns?: ReadonlySet }, +) => string | undefined; /** * Process a single task through the PR merge workflow. @@ -809,7 +812,33 @@ export async function processPullRequestMergeTask( pool?: WorktreePool, ): Promise { const task = await store.getTask(taskId); - if (getTaskMergeBlocker(task)) { + /* + FNXC:WorkflowResolvedColumns 2026-07-30-23:55: + Hand the merge blocker THIS task's merge lane, or PR merges never run on a renamed board. + + `getTaskMergeBlocker` was called with the task alone, so its `options.reviewColumns` was undefined + and its identity check fell back to `task.column === "in-review"`. On a board whose merge lane is + named anything else it returned `task is in 'checking', must be in 'in-review'` — truthy — and this + function returned "skipped". Silently, forever: nothing logs, nothing fails, the PR simply never + merges. The same class as #2963/#2964, on a third entry point (`daemon.ts`, `serve.ts` and + `dashboard.ts` all drain PR merges through here). + + NARROW resolution, not `resolveReviewColumns`. That helper is the BROAD set and its own note says a + caller that admits on it and then MOVES the card will act on cards the engine does not consider in + review — and this function merges and moves to the complete lane. `resolveMergeOrchestrationColumn` + is the single lane the engine acts on, matching how `moves.ts` wires the same call. + + `resolveWorkflowIrForTask` substitutes the default IR rather than throwing, so on a default board + this resolves `in-review` and behaviour is byte-identical; an unresolvable lane passes no option at + all and keeps the documented legacy literal. + */ + const mergeLane = resolveMergeOrchestrationColumn(await resolveWorkflowIrForTask(store, taskId)); + /* The option is always PASSED and conditionally VALUED, rather than the whole argument being + conditional: `getTaskMergeBlocker` treats an undefined `reviewColumns` exactly as it treats + absent options, so the two are identical at runtime — but only this shape is visible to + `scripts/lib/lane-wiring-census.mjs`, which matches an object-literal argument and cannot see a + ternary. Wiring the gate cannot check is how this defect survived in the first place. */ + if (getTaskMergeBlocker(task, { reviewColumns: mergeLane ? new Set([mergeLane]) : undefined })) { return "skipped"; } diff --git a/scripts/lib/lane-wiring-baseline.json b/scripts/lib/lane-wiring-baseline.json index 95652dffe6..cd9048f3fb 100644 --- a/scripts/lib/lane-wiring-baseline.json +++ b/scripts/lib/lane-wiring-baseline.json @@ -5,7 +5,6 @@ "packages/engine/src/auto-merge-finalization.ts": 1, "packages/engine/src/project-engine.ts": 1, "packages/engine/src/runtimes/in-process-runtime.ts": 1, - "packages/engine/src/self-healing.ts": 2, - "packages/cli/src/commands/task-lifecycle.ts": 1 + "packages/engine/src/self-healing.ts": 2 } }