root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

test(engine): live-PG evidence that the terminal-node guard fires on the wrong node (#2793)

Browse Source 
## What

One new live-PostgreSQL E2E suite, 3 tests. **No production file is
touched** — evidence, per the E2E worker's remit. Fourth in the series
after #2789 (scheduler), #2791 (planner lanes), #2792 (custom fields).


`packages/engine/src/__tests__/workflow-terminal-node-sync-resolution-live-e2e.pg.test.ts`

## The finding

FN-7641 Signature 2 exists because setting `nodeId` to the terminal node
used to be written verbatim and silently do nothing — the card sat in
review with every step done, unadvanced and unexplained. The contract: a
terminal override **with** durable merge proof finalizes the card;
**without** proof it is rejected with an actionable error; non-terminal
overrides are untouched.

On a board whose terminal node is not called `end`, **both halves
invert**:

| write | contract says | actually observed |
|---|---|---|
| `nodeId: "end"` — an ordinary planning node here | written, untouched
| **rejected** with a merge-proof error about finalizing a card the
operator was not finalizing |
| `nodeId: "finish"` — this board's real `end`-kind node | finalize, or
reject | **written verbatim**, no error, card left in review |

The second row is the original FN-7641 bug, restored on every custom
board.

## The correction the mutation runs forced

My first draft blamed `isTaskTerminalNodeIdImpl`'s sync IR resolution
alone. Mutating it changed only one of the two cases, which is how I
found there are **two** guards:

```
branch-and-pr-entities.ts:568   validateNodeOverrideChange(task, nodeId, { isTerminalNodeId })
                                -> sync IR resolution (the default board, under PostgreSQL)
task-update.ts:53               validateNodeOverrideChange(task, nodeId)
                                -> NO options, so `defaultIsTerminalNodeId` — the bare literal
                                   `nodeId === "end"`
```

The inner one is an unconverted literal sitting behind a converted call
site, and it silently overrides it. **Converting the outer guard alone
changes nothing an operator can see.** A column census cannot find the
inner one either — `end` is a node id, not a column. This is the "a
guard survives in a branch of the same function" shape, one function
apart.

### Mutation matrix

| corrected | `end` rejected | `finish` silent |
|---|---|---|
| *(nothing — main)* | pass | pass |
| outer sync-IR guard only | pass | **fail** |
| inner `defaultIsTerminalNodeId` only | pass | **fail** |
| **both** | **fail** | **fail** |

Two different failure structures, which is why the cases are kept apart:

- **`end` rejected is over-determined** — both guards independently call
it terminal, so it survives a mutation of either one. Not a weak
assertion: a faithful record of a defect with two independent causes,
and the reason a partial fix here is invisible.
- **`finish` silent is under-determined** — both guards must miss the
id, so correcting either flips it. This is the arm that notices a
partial fix.

The fixture-integrity case exercises the async resolver by design and
correctly survives every mutation.

## Fixture

The shared builder's terminal node is `end`, so it cannot express this
shape. This file derives from it: one `lifecycleIr`, node ids shifted so
the `end`-kind node is `finish` and the non-terminal planning node takes
the name `end`. Columns, traits, edges and structure are otherwise the
builder's, so the only variable is which node ids carry which kind. The
first case asserts that shift really happened — both characterizations
are claims about which node is terminal and would read as defects if the
fixture had quietly kept the builder's ids.

## Evidence discipline

- **Observed state.** Whether `updateTask` throws, and what the re-read
row's `nodeId` and `column` actually are. No spies.
- **Characterization, not endorsement.** Both cases assert the
wrong-but-current behaviour deliberately, and the matrix above says
exactly which fix flips which.

## Not done, and why

**No fix.** It needs two coordinated edits in `@fusion/core` — threading
the resolved terminal check into `task-update.ts:53`, and making the
outer resolution async — and the second is the same synchronous-path
constraint as #2792. Both are behaviour decisions in another worker's
files. Worth flagging that fixing only the allow-listed sync site would
look like progress and deliver none, which the matrix above makes
checkable.

## Verification

- new suite — **3/3 passed**, mutation matrix above
- full live-PG E2E surface — **124/124 passed** (121 on main + 3)
- `pnpm lint` — clean

Lane: `.pg.test.ts`, skipped via `pgDescribe` when no PostgreSQL is
reachable, so the merge gate is unaffected. Throwaway per-file database;
never port 4040.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-07-30 10:29:04 -07:00
committed by GitHub
parent 59b5e61fa2
commit 755ada91ac
1 changed files with 179 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

179
packages/engine/src/__tests__/workflow-terminal-node-sync-resolution-live-e2e.pg.test.ts Normal file
View File

@@ -0,0 +1,179 @@
/*
FNXC:StateMachine 2026-07-30-23:55 (E2E evidence — the terminal-node guard fires on the wrong node):
Fourth in the inert-sync-resolution series (#2789 scheduler, #2791 planner lanes, #2792 custom
fields). This one does not merely answer with the wrong vocabulary: it makes a SAFETY GUARD fire on a
node that is not terminal and stay silent on the node that is.
`branch-and-pr-entities.ts`'s `isTaskTerminalNodeIdImpl` answers "is this the task's terminal node?"
by looking the id up in `store.resolveTaskWorkflowIrSync(taskId)` — which under PostgreSQL answers
`undefined` for every task and so resolves the DEFAULT workflow IR. It therefore answers about the
DEFAULT board's node graph, never the card's own, and its `catch` fallback (`nodeId === "end"`) is
unreachable because the lookup always succeeds against some IR.
It feeds FN-7641 Signature 2 in `updateTaskImpl`. That contract exists because setting `nodeId` to
the terminal node used to be written verbatim and silently do nothing — the card sat in review with
every step done, unadvanced and unexplained. The fix: a terminal override WITH durable merge proof
finalizes the card; WITHOUT proof it is rejected with an actionable error. Non-terminal overrides are
untouched.
Both halves invert on a board whose terminal node is not called `end`:
set nodeId to a NON-terminal node that happens to be named `end`
-> the default IR says `end` is terminal, so a legitimate routing override is REJECTED
set nodeId to the board's REAL terminal node
-> the default IR has no such node id, so the guard does not recognise it, the field is
written verbatim, and the card stays where it was — the exact silent no-op FN-7641 removed
So the guard is not weakened in one direction and strengthened in the other; it is aimed at the wrong
node in both. Restoring the original bug on every custom board is what makes this worth its own file.
TWO GUARDS, NOT ONE — a correction the mutation runs forced, and the reason this file says more than
"the sync resolver is inert". `updateTask` passes through the node-override guard TWICE:
branch-and-pr-entities.ts:568 validateNodeOverrideChange(task, nodeId, { isTerminalNodeId })
-> the sync-IR resolution described above
task-update.ts:53 validateNodeOverrideChange(task, nodeId)
-> NO options, so it falls to `defaultIsTerminalNodeId`,
which is the bare literal `nodeId === "end"`
The inner one is an unconverted literal sitting behind a converted call site, and it silently
overrides it: converting the outer guard alone changes NOTHING an operator can see. A column census
cannot find it either — `end` is a node id, not a column. This is the "a guard survives in a branch of
the same function" shape, one function apart.
Consequences, established by mutation rather than by reading (see the table in the PR body):
`end` REJECTED over-determined. BOTH guards independently call it terminal, so correcting either
one alone leaves the behaviour unchanged. Only correcting both flips it.
`finish` SILENT under-determined. BOTH guards must miss it, so correcting EITHER one flips it.
That asymmetry is why the two cases below are kept apart rather than merged into one round trip: they
bind to different failure structures.
FIXTURE. The shared builder cannot express this shape (its terminal node is `end`), so this file
derives from it: one `lifecycleIr`, with node ids shifted so the END node is `finish` and the
non-terminal planning node takes the name `end`. Columns, traits, edges and structure are otherwise
the builder's, so the only variable is which node ids carry which kind.
OBSERVED STATE. Whether `updateTask` throws, and what the re-read row's `nodeId` and `column`
actually are. No spies.
LANE. `.pg.test.ts`, skipped via `pgDescribe` when no PostgreSQL is reachable, so the merge gate is
unaffected. Throwaway per-file database; never port 4040.
*/
import { beforeAll, beforeEach, afterEach, afterAll, expect, it } from "vitest";
import "@fusion/core"; // registers the built-in column traits
import { resolveWorkflowIrForTask, type TaskStore } from "@fusion/core";
import {
pgDescribe,
createSharedPgTaskStoreTestHarness,
type SharedPgTaskStoreHarness,
} from "../../../core/src/__test-utils__/pg-test-harness.js";
import { RENAMED_VOCAB, lifecycleIr } from "./_workflow-vocabulary-fixture.js";
/**
* The shared lifecycle IR with two node ids swapped: the `end`-kind node becomes `finish`, and the
* non-terminal planning node takes the now-free name `end`. Edges are rewritten with the same map so
* the graph stays exactly the builder's shape.
*/
function shiftedTerminalIr(id: string) {
const base = lifecycleIr(RENAMED_VOCAB, id) as unknown as {
nodes: { id: string }[];
edges: { from: string; to: string }[];
};
const rename = (n: string) => (n === "end" ? "finish" : n === "plan" ? "end" : n);
base.nodes = base.nodes.map((n) => ({ ...n, id: rename(n.id) }));
base.edges = base.edges.map((e) => ({ ...e, from: rename(e.from), to: rename(e.to) }));
return base as never;
}
pgDescribe("terminal-node resolution for a live task", () => {
const h: SharedPgTaskStoreHarness = createSharedPgTaskStoreTestHarness({
prefix: "fusion_terminal_node",
});
beforeAll(h.beforeAll);
afterAll(h.afterAll);
beforeEach(async () => { await h.beforeEach(); });
afterEach(async () => { await h.afterEach(); });
/** A task on the shifted board, parked in its review column (the guard is a no-op once a card is
* already in `done`, and refuses outright while a card is in progress). */
async function taskOnShiftedBoard(store: TaskStore, key: string): Promise<string> {
const created = await store.createWorkflowDefinition({
name: `Terminal ${key}`,
kind: "workflow",
ir: shiftedTerminalIr(`custom:${key}`),
} as never);
const workflowId = (created as { id: string }).id;
const task = await store.createTask({ description: `terminal probe ${key}` });
await store.writeTaskWorkflowSelection(task.id, workflowId, []);
store.taskCache.delete(task.id);
await store.moveTask(task.id, RENAMED_VOCAB.review as never, { recoveryRehome: true } as never);
return task.id;
}
it("the board really is shifted: `finish` is the end node and `end` is not (fixture integrity)", async () => {
/* First, because both characterizations below are claims about which node is terminal and would
read as defects if the fixture had quietly kept the builder's node ids. */
const store = h.store();
const taskId = await taskOnShiftedBoard(store, "wf-integrity");
const ir = await resolveWorkflowIrForTask(store, taskId);
const nodes = (ir as { nodes: { id: string; kind: string }[] }).nodes;
expect(nodes.find((n) => n.id === "finish")?.kind).toBe("end");
expect(nodes.find((n) => n.id === "end")).toBeDefined();
expect(nodes.find((n) => n.id === "end")?.kind).not.toBe("end");
});
it("CHARACTERIZATION — a legitimate override to the non-terminal `end` node is REJECTED", async () => {
/*
On this board `end` is an ordinary planning node, so per the contract's own words ("non-terminal
nodeId overrides ... are untouched") this override should simply be written. Instead the operator
gets a merge-proof error about finalizing a card they were not finalizing, and the routing change
they asked for does not happen.
OVER-DETERMINED, so read the mutation results carefully: the default IR calls `end` terminal AND
`defaultIsTerminalNodeId` is the literal `"end"`. Either guard alone rejects this write, so this
case survives a mutation of either one and fails only when both are corrected. It is not a weak
assertion — it is a faithful record of a defect with two independent causes, which is exactly why
fixing the sync resolver here would produce no visible change.
*/
const store = h.store();
const taskId = await taskOnShiftedBoard(store, "wf-false-positive");
await expect(store.updateTask(taskId, { nodeId: "end" } as never))
.rejects.toThrow(/does not finalize a card by itself|durable merge proof/);
store.taskCache.delete(taskId);
expect((await store.getTask(taskId))?.nodeId).not.toBe("end");
});
it("CHARACTERIZATION — and an override to the REAL terminal node silently no-ops", async () => {
/*
The half that matters more, because it is the original FN-7641 bug restored. `finish` IS this
board's `end`-kind node, so this write must either finalize the card (with merge proof) or be
rejected (without). Instead NEITHER guard recognises the id: the field is written verbatim, no
error is raised, and the card stays in review with nothing advanced — "no error and no
advancement", exactly as the contract's comment describes the behaviour it replaced.
UNDER-DETERMINED, the mirror of the case above: because both guards must miss the id for the
write to slip through, correcting EITHER one is enough to flip this case. So this is the arm that
would notice a partial fix.
*/
const store = h.store();
const taskId = await taskOnShiftedBoard(store, "wf-false-negative");
await store.updateTask(taskId, { nodeId: "finish" } as never);
store.taskCache.delete(taskId);
const row = await store.getTask(taskId);
expect(row?.nodeId).toBe("finish");
expect(row?.column).toBe(RENAMED_VOCAB.review); // not finalized, not rejected — just written
});
});