From be79fe0db6324a4df6d4eca44f84a0623c480a01 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 22:46:19 -0700 Subject: [PATCH] =?UTF-8?q?fix(cli):=20PR=20merges=20silently=20never=20ra?= =?UTF-8?q?n=20on=20a=20renamed=20board=20=E2=80=94=20the=20blocker=20was?= =?UTF-8?q?=20asked=20about=20`in-review`=20(#2976)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## PR merges silently never ran on a renamed board `processPullRequestMergeTask` called its injected blocker with the task alone: ```ts if (getTaskMergeBlocker(task)) return "skipped"; ``` So `options.reviewColumns` was undefined and the blocker's identity check fell back to `task.column === "in-review"`. On a board whose merge lane is named anything else it returns: ``` task is in 'checking', must be in 'in-review' ``` …which is truthy, so this function returns `"skipped"`. **Silently and permanently** — nothing logs, nothing fails, the PR simply never merges. `daemon.ts`, `serve.ts` and `dashboard.ts` all drain PR merges through here, making this a third instance of the #2963/#2964 class ("merge entry points unwired — merging was impossible on a renamed board"). Found via the baseline #2966 shipped: `packages/cli/src/commands/task-lifecycle.ts` was a known-unwired call site in it. ## Narrow resolution, deliberately `resolveReviewColumns` is the **broad** set, and its own FNXC note warns that a caller which admits on it *and then moves the card* will act on cards the engine does not consider in review. This function merges and moves to the complete lane — a state-changing admission — so it uses `resolveMergeOrchestrationColumn`, the single lane the engine acts on. That matches how `moves.ts` wires the same call. Degradation is unchanged in both directions: `resolveWorkflowIrForTask` substitutes the default IR rather than throwing, so a default board resolves `in-review` and behaves identically; a v1-upgraded IR resolves every role empty and keeps the documented legacy literal (covered by a test). ## One shape choice worth flagging The option is always **passed** and conditionally **valued**: ```ts getTaskMergeBlocker(task, { reviewColumns: mergeLane ? new Set([mergeLane]) : undefined }) ``` rather than making the whole argument conditional. These are identical at runtime — the blocker treats an undefined `reviewColumns` exactly as it treats absent options — but **only this shape is visible to `lane-wiring-census.mjs`**, which matches an object-literal argument and cannot see a ternary. I wrote the ternary first, and the gate still reported the site as unwired; wiring a gate cannot check is how this defect survived in the first place. The gate then confirmed the fix and asked for the baseline in the same commit: ``` [check-lane-wiring] unwired call sites decreased: packages/cli/src/commands/task-lifecycle.ts: 1 -> 0 ``` Baseline re-recorded 9 → 8 in this commit, so the allowance cannot be regrown into. ## Revert proof **There was no test for this function at all** — that is why it went unnoticed. Restoring only `task-lifecycle.ts`: ``` AssertionError: expected "vi.fn()" to be called with arguments: [ ObjectContaining{…}, …(1) ] AssertionError: expected 'skipped' not to be 'skipped' AssertionError: expected "vi.fn()" to be called with arguments: [ ObjectContaining{…}, undefined ] Tests 3 failed | 1 passed (4) ``` The one case that passes both ways is "still skips a card that is not in any merge lane" — it guards against over-admission rather than proving the fix, and I am not claiming it as coverage of the defect. ## Verification (measured) - new suite **4/4**; with `pr-automerge-cleanup` **9 passed / 2 files** - `tsc --noEmit`, `eslint` — clean - `check-lane-wiring` (8, none added), `lifecycle-column-census --strict`, `check-sql-column-literals`, `check-fnxc-future-dates` — green **Changeset added** (`patch`). `packages/cli` is the published `@runfusion/fusion` and this changes user-facing merge behaviour, so AGENTS.md requires one. My first pass hedged and left it to a maintainer — that was wrong, the rule is not discretionary, and it is now in the branch. --- .../cli-pr-merge-renamed-review-lane.md | 7 ++ .../__tests__/pr-merge-review-lane.test.ts | 107 ++++++++++++++++++ packages/cli/src/commands/task-lifecycle.ts | 35 +++++- scripts/lib/lane-wiring-baseline.json | 3 +- 4 files changed, 147 insertions(+), 5 deletions(-) create mode 100644 .changeset/cli-pr-merge-renamed-review-lane.md create mode 100644 packages/cli/src/__tests__/pr-merge-review-lane.test.ts 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 } }