fix(cli): PR merges silently never ran on a renamed board — the blocker was asked about in-review (#2976)
## 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.
This commit is contained in:
7
.changeset/cli-pr-merge-renamed-review-lane.md
Normal file
7
.changeset/cli-pr-merge-renamed-review-lane.md
Normal file
@@ -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.
|
||||
107
packages/cli/src/__tests__/pr-merge-review-lane.test.ts
Normal file
107
packages/cli/src/__tests__/pr-merge-review-lane.test.ts
Normal file
@@ -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<string> {
|
||||
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 });
|
||||
});
|
||||
});
|
||||
@@ -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> },
|
||||
) => string | undefined;
|
||||
|
||||
/**
|
||||
* Process a single task through the PR merge workflow.
|
||||
@@ -809,7 +812,33 @@ export async function processPullRequestMergeTask(
|
||||
pool?: WorktreePool,
|
||||
): Promise<ProcessPullRequestResult> {
|
||||
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";
|
||||
}
|
||||
|
||||
|
||||
@@ -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
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user