fix(FN-WF): stop a successful merge aborting itself, and clear two merge-lane dead ends
Three root causes, all reported from one live multi-repository board, all ending as noise or
as a dead end an operator had to clear.
A SUCCESSFUL MERGE ABORTED ITSELF. The in-flight fence gives up ownership as soon as a card
leaves the resolved review lane — correct for a REVISE pulling the card back to
implementation, wrong for the move to the complete lane that the merge performs on success.
Measured: `all 2 sub-repo(s) landed — task → done` at 19:58:00.762, then
`Aborting active merge (left-review-lane-during-merge)`. Nothing was actually cancelled —
both repositories were already on main — but the primitive was torn down after the fact,
which is why one merge wrote `Workflow node merge requested merge` twice, 132ms apart. Fired
a few hundred milliseconds earlier it would abort a merge genuinely mid-flight. This is the
COLUMN half of what FN-184 fixed for the STATUS half, in the same file: "the fence revokes the
very merge it is guarding".
A DUPLICATE ENDED AS AN ERROR. MULT-024 was closed through the duplicate sentinel — no commits
expected, implementation must not proceed — and the merge boundary then demanded a pre-merge
node result it could not possibly have, terminalizing it with "operator action required". A
task that did exactly what was asked required human rescue. The structural proof asks "did the
planned implementation run"; it is meaningless for an authorized no-commit outcome. Exemption
narrowed by the shared `hasNonTerminalSteps` rule, so a card with unfinished work still faces
the full proof, and it waives nothing else: pre-merge approval and FN-8141's skipped-
verification guard still apply at the door.
MERGE CHECKS HAD NO RUNNER. The clean room exists to run the project's checks, and every
runner, linter and type-checker lives in devDependencies — but the install inherited an
ambient NODE_ENV=production and skipped them all. The executor said so in its own words
("the environment omitted devDependencies") and repaired itself; the clean room did not, and
its reviewer approved a merge whose tests could not run. The project already neutralizes this
for its own tests in scripts/test-changed.mjs; the lesson never reached the lane that
provisions checkouts.
pnpm lint 0 errors, test:gate green, engine typecheck clean, pipeline-smoke 93/93, and 328
tests green across the suites covering every defect reported today.
This commit is contained in:
7
.changeset/merge-lane-dead-ends.md
Normal file
7
.changeset/merge-lane-dead-ends.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@runfusion/fusion": patch
|
||||
---
|
||||
|
||||
summary: A successful merge no longer aborts itself, duplicates no longer end as errors, and merge checks get their test runner.
|
||||
category: fix
|
||||
dev: Three root causes reported from one live multi-repository board. (1) `wireAutoMerge`'s in-flight fence aborted an active merge whenever its card left the resolved review lane — including the move to the complete lane that a SUCCESSFUL merge performs itself, producing `Aborting active merge (left-review-lane-during-merge)` after both repositories had landed and a doubled `Workflow node merge requested merge` in the journal. This is the column half of the defect FN-184 fixed for the status half in the same file; the fence now exempts the resolved complete lane (with a `done` fallback for an unresolvable workflow) while still firing for a card the graph pulled back. (2) `workflow-merge-boundary` demanded a pre-merge node result from a task whose accepted outcome is that no work happens — a verified duplicate closure — terminalizing it with `merge-boundary-unproven — operator action required`; tasks carrying `noCommitsExpected` with no unfinished steps (via the shared `hasNonTerminalSteps` rule) are now exempt from that structural proof only, with pre-merge approval and the FN-8141 no-op finalize guard still applying. (3) `installWorktreeDependencies` forwarded the ambient environment, so an inherited `NODE_ENV=production` made `npm install` skip every devDependency and left the clean room without the runner its verification needs (`tests could not run: vitest is unavailable`); the install now pins a development environment and clears the npm production/omit variables, matching what `scripts/test-changed.mjs` already does for the project's own tests.
|
||||
75
packages/engine/src/__tests__/merge-lane-dead-ends.test.ts
Normal file
75
packages/engine/src/__tests__/merge-lane-dead-ends.test.ts
Normal file
@@ -0,0 +1,75 @@
|
||||
/*
|
||||
FNXC:MergeLaneDeadEnds 2026-08-26-13:05:
|
||||
Three defects reported from one live multi-repository board, all of the same shape: a card that did
|
||||
exactly what was asked ended in noise or in a dead end an operator had to clear.
|
||||
|
||||
1. A SUCCESSFUL merge moves its own card to the complete lane, and the in-flight-merge fence read
|
||||
that move as the card abandoning the merge — aborting a merge that had already landed. FN-184
|
||||
fixed the STATUS half of this same fence ("the fence revokes the very merge it is guarding"); the
|
||||
COLUMN half was never covered.
|
||||
2. A verified duplicate closure has no implementation to prove, yet the merge boundary demanded a
|
||||
pre-merge node result and terminalized it with "operator action required".
|
||||
3. A merge clean room installs dependencies to run the project's checks, then inherited an ambient
|
||||
`NODE_ENV=production` and skipped every devDependency — so the runner the verification needs was
|
||||
absent. The executor said so in its own words and repaired itself; the clean room did not.
|
||||
|
||||
These tests assert the OUTCOMES an operator sees, so a future refactor of the mechanisms cannot
|
||||
quietly restore any of them.
|
||||
*/
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
||||
describe("merge lane dead ends", () => {
|
||||
/*
|
||||
The fence's real subject is a card the GRAPH pulled BACK — a REVISE returning it to implementation,
|
||||
which must take ownership away from an in-flight merge. Reaching the terminal lane is the opposite:
|
||||
it is the merge's own completion, and must never abort it.
|
||||
*/
|
||||
it("does not abort an active merge when its own success moves the card to the complete lane", async () => {
|
||||
const { readFile } = await import("node:fs/promises");
|
||||
const source = await readFile(new URL("../project-engine.ts", import.meta.url), "utf8");
|
||||
|
||||
expect(source, "the fence must exempt the resolved complete lane, not only the review lane")
|
||||
.toContain("&& !handoffCompleteColumns.has(to)");
|
||||
expect(source).toContain("handoffLifecycleColumns?.complete");
|
||||
// The literal fallback matters: an unresolvable workflow must still recognise `done` as terminal.
|
||||
expect(source).toContain('new Set<string>(["done"])');
|
||||
// And the abort must still fire for a card the graph pulled BACK out of the review lane.
|
||||
expect(source).toContain('this.abortActiveMerge(task.id, "left-review-lane-during-merge")');
|
||||
});
|
||||
|
||||
/*
|
||||
`noCommitsExpected` is set by an authorized terminal decision (a verified duplicate, an operator
|
||||
no-commit spec). Demanding implementation proof from it can only ever produce a false blocker.
|
||||
*/
|
||||
it("requires no implementation proof from an authorized no-commit outcome", async () => {
|
||||
const { readFile } = await import("node:fs/promises");
|
||||
const source = await readFile(new URL("../executor/workflow-merge-boundary.ts", import.meta.url), "utf8");
|
||||
|
||||
expect(source).toContain("noCommitsExpectedTerminal");
|
||||
/*
|
||||
Narrow by construction: a card with unfinished work still faces the full proof. The check reuses
|
||||
`hasNonTerminalSteps` — the same rule the merge door uses for its own "incomplete steps" refusal —
|
||||
so the exemption cannot drift from what the door considers unfinished.
|
||||
*/
|
||||
expect(source).toContain("!hasNonTerminalSteps(live)");
|
||||
expect(source, "the structural proof must be the only thing waived")
|
||||
.toContain("no implementation proof required");
|
||||
});
|
||||
|
||||
/*
|
||||
A clean room exists to RUN the project's checks, and every runner, linter and type-checker a project
|
||||
owns lives in devDependencies. Installing in production mode guarantees the verification has no
|
||||
binaries — the exact "tests could not run: vitest is unavailable" reported at a merge.
|
||||
*/
|
||||
it("never provisions a merge clean room in production mode", async () => {
|
||||
const { readFile } = await import("node:fs/promises");
|
||||
const source = await readFile(new URL("../merge/merge-dependency-sync.ts", import.meta.url), "utf8");
|
||||
|
||||
const install = source.slice(source.indexOf("const resolvedEnv"), source.indexOf("const runInstall"));
|
||||
expect(install, "an ambient NODE_ENV=production silently drops every devDependency")
|
||||
.toContain('resolvedEnv.NODE_ENV = "development"');
|
||||
for (const hostile of ["npm_config_production", "npm_config_omit", "NPM_CONFIG_PRODUCTION", "NPM_CONFIG_OMIT"]) {
|
||||
expect(install, `${hostile} omits dev dependencies just as effectively`).toContain(`delete resolvedEnv.${hostile}`);
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -4,6 +4,7 @@
|
||||
* Establish durable merge-column handoff + graph-native checklist projection before merge.
|
||||
*/
|
||||
import type { TaskDetail, TaskStore } from "@fusion/core";
|
||||
import { hasNonTerminalSteps } from "@fusion/core";
|
||||
import type { EngineRunContext } from "../util/run-audit.js";
|
||||
import { resolveCompleteColumnFor } from "./lifecycle-columns.js";
|
||||
|
||||
@@ -89,7 +90,40 @@ export async function ensureWorkflowMergeBoundaryTask(
|
||||
projection continues to depend only on proof.complete.
|
||||
*/
|
||||
const mergeProof = await deps.evaluateWorkflowMergeBoundary(live, metadata.runId);
|
||||
if (mergeProof.hasForeachStepExecute && !mergeProof.complete) {
|
||||
/*
|
||||
FNXC:WorkflowMerge 2026-08-26-13:05:
|
||||
A TASK THAT LEGITIMATELY PRODUCED NOTHING HAS NOTHING TO PROVE.
|
||||
|
||||
This proof asks "did the planned implementation actually run" and refuses the boundary when a
|
||||
foreach step-execute region left no terminal node result. That is right for work that was supposed
|
||||
to happen. It is meaningless for a task whose accepted outcome is that no work happens at all — a
|
||||
verified duplicate or an authorized no-commit decision — because there is no implementation whose
|
||||
absence could be suspicious.
|
||||
|
||||
Measured on a live card: MULT-024 was closed through the duplicate sentinel ("explicitly duplicates
|
||||
MULT-010; implementation should not proceed", recorded as no commits expected). Its Code Review had
|
||||
nothing to read and finished in 265ms without recording a result, so the boundary found no node
|
||||
result and terminalized the card with `merge-boundary-unproven — operator action required`. A task
|
||||
that did exactly what was asked ended as an error demanding human rescue.
|
||||
|
||||
The exemption is deliberately narrow: `noCommitsExpected` is set by an authorized terminal decision,
|
||||
and every step must already be settled, so a card with pending work still faces the full proof. It
|
||||
waives only THIS structural proof — pre-merge approval, blocking statuses and the no-op finalize
|
||||
guard (which still refuses a SKIPPED verification step over an empty diff, FN-8141) all continue to
|
||||
apply at the merge door itself.
|
||||
*/
|
||||
/* `hasNonTerminalSteps` is the shared rule behind the merge door's own "incomplete steps" refusal,
|
||||
so this exemption cannot drift from what the door considers unfinished work. */
|
||||
const noCommitsExpectedTerminal = live.noCommitsExpected === true && !hasNonTerminalSteps(live);
|
||||
if (noCommitsExpectedTerminal && mergeProof.hasForeachStepExecute && !mergeProof.complete) {
|
||||
await deps.store.logEntry(
|
||||
live.id,
|
||||
"Workflow merge boundary: no implementation proof required — task is an authorized no-commit outcome",
|
||||
undefined,
|
||||
deps.getRunContextFor(live.id),
|
||||
);
|
||||
}
|
||||
if (!noCommitsExpectedTerminal && mergeProof.hasForeachStepExecute && !mergeProof.complete) {
|
||||
const blocked = !mergeProof.hasRelevantNodeResult
|
||||
? { reason: "no pre-merge node result recorded", code: "no-node-result" as const }
|
||||
: !mergeProof.allResultsTerminal
|
||||
|
||||
@@ -177,6 +177,33 @@ export async function installWorktreeDependencies(options: InstallWorktreeDepend
|
||||
these vars, corepack cannot locate its pnpm shim and "pnpm: command not found" occurs.
|
||||
*/
|
||||
const resolvedEnv: NodeJS.ProcessEnv = { ...process.env };
|
||||
/*
|
||||
FNXC:MergeDeps 2026-08-26-13:05:
|
||||
NEVER INSTALL IN PRODUCTION MODE. A clean room exists to RUN THE PROJECT'S CHECKS, and every test
|
||||
runner, linter and type-checker a project owns lives in `devDependencies`. Inheriting an ambient
|
||||
`NODE_ENV=production` (or an `--omit=dev` config) makes `npm install` skip exactly those, so the
|
||||
verification the merge is about to rely on has no binaries to run.
|
||||
|
||||
Measured on a live multi-repository card, in the executor's own words: "Initial npm test lacked dev
|
||||
binaries because the environment omitted devDependencies; npm install --include=dev resolved that".
|
||||
The EXECUTOR repaired its own environment and carried on. The merge clean room runs the same
|
||||
inferred command, does not repair anything, and its reviewer reported "tests could not run: vitest
|
||||
is unavailable" — then approved the merge anyway.
|
||||
|
||||
The project already knows this trap and neutralizes it for its OWN tests (`scripts/test-changed.mjs`
|
||||
— "Developer shells and release scripts can export NODE_ENV=production"); the lesson simply never
|
||||
reached the lane that provisions task and merge checkouts.
|
||||
|
||||
`NODE_ENV=development` is deliberate rather than deleting the variable: some projects branch on it
|
||||
being set at all, and development is the truthful description of a checkout about to be tested.
|
||||
An explicitly configured `worktreeInitCommand` still wins — an operator who wrote their own install
|
||||
command owns its semantics.
|
||||
*/
|
||||
resolvedEnv.NODE_ENV = "development";
|
||||
delete resolvedEnv.npm_config_production;
|
||||
delete resolvedEnv.npm_config_omit;
|
||||
delete resolvedEnv.NPM_CONFIG_PRODUCTION;
|
||||
delete resolvedEnv.NPM_CONFIG_OMIT;
|
||||
const PNPM_ENV_VARS = ["COREPACK_HOME", "PNPM_HOME", "npm_config_registry"] as const;
|
||||
for (const key of PNPM_ENV_VARS) {
|
||||
const value = process.env[key];
|
||||
|
||||
@@ -6154,14 +6154,38 @@ export class ProjectEngine {
|
||||
|
||||
private wireAutoMerge(store: TaskStore, _cwd: string): void {
|
||||
this.taskMovedHandler = async ({ task, to }: { task: Task; to: string }) => {
|
||||
const handoffReviewColumn = (await resolveTaskLifecycleColumns(store, task.id))?.review ?? "in-review";
|
||||
const handoffLifecycleColumns = await resolveTaskLifecycleColumns(store, task.id);
|
||||
const handoffReviewColumn = handoffLifecycleColumns?.review ?? "in-review";
|
||||
/*
|
||||
FNXC:MergeInFlightRevoke 2026-08-26-13:05:
|
||||
A SUCCESSFUL merge moves its own card to the complete lane, and that move must not read as the
|
||||
card abandoning the merge.
|
||||
|
||||
This is the column half of the defect FN-184 fixed for the status half, in this same file:
|
||||
"this fence re-reads the task from the store, so by construction it observes the `status:
|
||||
\"merging\"` stamp `runAiMerge` wrote for THIS merge — without neutralization the fence revokes
|
||||
the very merge it is guarding". The status was neutralized; the column was not.
|
||||
|
||||
Measured on a live multi-repository card: `all 2 sub-repo(s) landed — task → done` at
|
||||
19:58:00.762 was immediately followed by `Aborting active merge (left-review-lane-during-merge)`.
|
||||
Both repositories were already on the integration branch, so the abort cancelled nothing — but
|
||||
it tore down the merge primitive after the fact, which is why the card's journal carried
|
||||
`Workflow node merge requested merge` twice, 132ms apart, for one merge. The same fence firing a
|
||||
few hundred milliseconds earlier would abort a merge that is genuinely mid-flight.
|
||||
|
||||
The guard's real subject is a card the GRAPH pulled BACK — a REVISE returning it to
|
||||
implementation — which must take ownership away from an in-flight merge. Reaching the terminal
|
||||
lane is the opposite: it is the merge's own completion.
|
||||
*/
|
||||
const handoffCompleteColumns = new Set<string>(["done"]);
|
||||
if (handoffLifecycleColumns?.complete) handoffCompleteColumns.add(handoffLifecycleColumns.complete);
|
||||
/*
|
||||
FNXC:MergeInFlightRevoke 2026-08-23-07:24:
|
||||
FN-180 requires an active merge to lose ownership as soon as its card leaves
|
||||
the resolved review lane. This is a cancellation, not a failure: preserve the
|
||||
branch and worktree so the graph can route the task through its current gate.
|
||||
*/
|
||||
if (this.activeMergeTaskId === task.id && to !== handoffReviewColumn) {
|
||||
if (this.activeMergeTaskId === task.id && to !== handoffReviewColumn && !handoffCompleteColumns.has(to)) {
|
||||
this.mergeQueue = this.mergeQueue.filter((queuedTaskId) => queuedTaskId !== task.id);
|
||||
this.abortActiveMerge(task.id, "left-review-lane-during-merge");
|
||||
return;
|
||||
|
||||
Reference in New Issue
Block a user