fix(core): resolve the task's own terminal node in the node-override guard (#2812)

## What

Fixes the defect **#2793 measured** but deliberately did not fix. That
PR's two characterization tests are flipped here to assert the correct
behaviour — which is what they were written to do.

## The bug

`updateTask({ nodeId })` passes through `validateNodeOverrideChange`
**twice**, and both calls were wrong in different ways:

| call | how it answered "is this terminal?" |
|---|---|
| `branch-and-pr-entities.ts:568` | `resolveTaskWorkflowIrSync` — the
**default** workflow for every task under PostgreSQL |
| `task-update.ts:53` | no options at all → `defaultIsTerminalNodeId`,
the bare literal `nodeId === "end"` |

On a board whose terminal node is not named `end`, the FN-7641 guard
inverted in both directions:

- an override to a **non-terminal** node that happens to be named `end`
was **rejected** with a merge-proof error about finalizing a card the
operator was not finalizing;
- an override to the board's **real** terminal node was **written
verbatim**, no error, card unadvanced — the silent no-op FN-7641 exists
to prevent.

## The fix

A new `isTaskTerminalNodeIdAsync` resolves the task's own workflow, with
the **identical** literal fail-soft for an unresolvable one. Both call
sites use it — pre-resolved, because `validateNodeOverrideChange`'s
callback is synchronous and it asks the question at most once.

**Nothing forced the sync call at either site**: both frames are already
`async` and already awaiting. That is the same finding as #2809's
review, one file over.

The sync helper is **deleted, not kept as a fallback**. Keeping both
would re-create the half-conversion this program keeps finding — one
caller resolved, one not, and no way to tell from a call site which it
got. `branch-and-pr-entities.ts` also leaves the sync-resolver call-site
allow-list (ratchet green, 3/3), the **second** of the six allow-listed
sites to close.

## Why both halves were needed — and how that is proven

#2793's mutation matrix showed the rejected-`end` case is
**over-determined**: both guards independently called it terminal, so
correcting either one alone changed nothing an operator could see. That
is why fixing only the allow-listed sync site would have looked like
progress and delivered none.

Re-measured here, on the fixed tree:

| state | result |
|---|---|
| both guards fixed | **3/3 pass** |
| inner guard reverted to no-options | **1 fails** |
| outer guard reverted to the literal | **1 fails** |

## Tests

#2793's two cases now assert the fixed behaviour and keep their
reasoning:

- the non-terminal `end` override is **written**, and the card stays in
review — a routing change, not a finalize;
- the real terminal `finish` override is **refused** without merge
proof, **and** the field is not written on the way to refusing.

The fixture-integrity case (`finish` is the end node, `end` is not) is
unchanged — it is what stops both assertions passing for the wrong
reason.

## Verification

- terminal-node suite — **3/3**, both-halves matrix above
- sync-resolver call-site allow-list ratchet — **3/3**
- `pnpm test:gate` — **exit 0**
- full live-PG E2E surface — **151/151**
- `pnpm lint` — clean

Changeset included (`patch`, category `fix`).

🤖 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 13:15:04 -07:00
committed by GitHub
parent 7c30b54255
commit e5c9ea3870
6 changed files with 120 additions and 48 deletions

View File

@@ -0,0 +1,7 @@
---
"@runfusion/fusion": patch
---
summary: Fix node-override handling on workflows whose terminal node is not named "end".
category: fix
dev: `updateTask({ nodeId })` passes through `validateNodeOverrideChange` twice. The outer call resolved terminality via `resolveTaskWorkflowIrSync` (the default workflow under PostgreSQL); the inner call passed no options and fell to the literal `nodeId === "end"`. Both now resolve the task's own workflow via the new `isTaskTerminalNodeIdAsync`, and `branch-and-pr-entities.ts` leaves the sync-resolver call-site allow-list.

View File

@@ -35,10 +35,6 @@ reason; a sync-resolved lifecycle guard is a guard that cannot fire.
*/ */
const ALLOWED_CALL_SITES: ReadonlyMap<string, string> = new Map([ const ALLOWED_CALL_SITES: ReadonlyMap<string, string> = new Map([
[
"packages/core/src/task-store/branch-and-pr-entities.ts",
"Inside a synchronous store entity read.",
],
[ [
"packages/core/src/task-store/lifecycle-ops.ts", "packages/core/src/task-store/lifecycle-ops.ts",
"Synchronous lifecycle bookkeeping inside a transaction.", "Synchronous lifecycle bookkeeping inside a transaction.",

View File

@@ -20,6 +20,7 @@ import { getWorkflowWorkItem as getWorkflowWorkItemAsync } from "./async-workflo
import { MergeRequestRow, PrEntityRow, WorkflowWorkItemRow } from "./row-types.js"; import { MergeRequestRow, PrEntityRow, WorkflowWorkItemRow } from "./row-types.js";
import { BranchGroup, BranchGroupCreateInput, ColumnId, MergeRequestRecord, MergeRequestState, PrEntity, PrEntityCreateInput, PrThreadOutcome, PrThreadState, RunMutationContext, Task, TaskLogEntry, TaskPriority, TaskVerificationRequest, TaskVerificationResultSummary, TaskVerificationStatus, WorkflowWorkItem, WorkflowWorkItemKind, WorkflowWorkItemState, WorkflowWorkItemTransitionPatch } from "../types.js"; import { BranchGroup, BranchGroupCreateInput, ColumnId, MergeRequestRecord, MergeRequestState, PrEntity, PrEntityCreateInput, PrThreadOutcome, PrThreadState, RunMutationContext, Task, TaskLogEntry, TaskPriority, TaskVerificationRequest, TaskVerificationResultSummary, TaskVerificationStatus, WorkflowWorkItem, WorkflowWorkItemKind, WorkflowWorkItemState, WorkflowWorkItemTransitionPatch } from "../types.js";
import { validateNodeOverrideChange, resolveNodeOverrideLanes } from "../node-override-guard.js"; import { validateNodeOverrideChange, resolveNodeOverrideLanes } from "../node-override-guard.js";
import { isTaskTerminalNodeIdAsync } from "../workflow-ir-resolver.js";
import { WorkflowMovePolicyInput } from "../workflow-extension-types.js"; import { WorkflowMovePolicyInput } from "../workflow-extension-types.js";
import { resolveWorkflowIrById } from "../workflow-ir-resolver.js"; import { resolveWorkflowIrById } from "../workflow-ir-resolver.js";
import { resolveTaskLifecycleColumns } from "../workflow-lifecycle-traits.js"; import { resolveTaskLifecycleColumns } from "../workflow-lifecycle-traits.js";
@@ -607,8 +608,19 @@ export async function updateTaskImpl(store: TaskStore,
const overrideLanes = await resolveNodeOverrideLanes(store, id); const overrideLanes = await resolveNodeOverrideLanes(store, id);
const currentTask = await store.getTask(id).catch(() => null); const currentTask = await store.getTask(id).catch(() => null);
if (currentTask) { if (currentTask) {
/*
FNXC:StateMachine 2026-07-31-20:15 (PR #2793's finding, fixed):
RESOLVED BEFORE the guard, because the guard's callback is synchronous and the real answer
needs an await. `validateNodeOverrideChange` asks the question at most once, for
`updates.nodeId`, so pre-resolving that single answer is equivalent — and this frame is
already async.
*/
const terminal = updates.nodeId == null
? false
: await isTaskTerminalNodeIdAsync(store, id, updates.nodeId);
const validation = validateNodeOverrideChange(currentTask, updates.nodeId ?? null, { const validation = validateNodeOverrideChange(currentTask, updates.nodeId ?? null, {
isTerminalNodeId: (nodeId) => isTaskTerminalNodeIdImpl(store, id, nodeId), /* Resolved above; `overrideLanes` stays exactly as main computes it. */
isTerminalNodeId: () => terminal,
...overrideLanes, ...overrideLanes,
}); });
if (!validation.allowed) { if (!validation.allowed) {
@@ -632,23 +644,16 @@ export async function updateTaskImpl(store: TaskStore,
return store.withTaskLock(id, () => store.updateTaskUnlocked(id, updates, runContext)); return store.withTaskLock(id, () => store.updateTaskUnlocked(id, updates, runContext));
} }
/** /*
* FNXC:StateMachine 2026-07-07-12:00: FNXC:StateMachine 2026-07-31-20:15 (PR #2793's finding, fixed):
* Resolve whether `nodeId` is the task's resolved workflow terminal `end` node (kind === "end"), `isTaskTerminalNodeIdImpl` LIVED HERE and is deleted, not kept as a fallback. It resolved the task's
* for the nodeId='end' finalize-on-proof-or-error contract (FN-7641 Signature 2). Falls back to graph through `store.resolveTaskWorkflowIrSync`, which answers with the DEFAULT workflow for every
* the literal id check when the workflow IR cannot be resolved or does not contain the node, which task under PostgreSQL — so it reported on a board the card is not on. Its replacement,
* still matches every built-in workflow's terminal node id. `isTaskTerminalNodeIdAsync`, keeps the identical literal fail-soft for an unresolvable workflow.
*/
function isTaskTerminalNodeIdImpl(store: TaskStore, taskId: string, nodeId: string): boolean { Keeping both would have re-created the half-conversion this program keeps finding: one caller
try { resolved, one not, and no way to tell from a call site which it got.
const ir = store.resolveTaskWorkflowIrSync(taskId); */
const node = ir.nodes.find((n) => n.id === nodeId);
if (node) return node.kind === "end";
} catch {
// Fall through to the literal-id fallback below.
}
return nodeId === "end";
}
export function mergeCustomFieldPatchImpl(store: TaskStore, export function mergeCustomFieldPatchImpl(store: TaskStore,
current: Record<string, unknown> | undefined, current: Record<string, unknown> | undefined,

View File

@@ -17,6 +17,7 @@ import {validateCustomFieldPatch, CustomFieldRejectionError} from "../task-field
import "../builtin-traits.js"; import "../builtin-traits.js";
import {normalizeTaskPriority} from "../task-priority.js"; import {normalizeTaskPriority} from "../task-priority.js";
import {validateNodeOverrideChange, resolveNodeOverrideLanes} from "../node-override-guard.js"; import {validateNodeOverrideChange, resolveNodeOverrideLanes} from "../node-override-guard.js";
import {isTaskTerminalNodeIdAsync} from "../workflow-ir-resolver.js";
import {extractTaskIdTokens, normalizeTitleForTaskId} from "../task-title-id-drift.js"; import {extractTaskIdTokens, normalizeTitleForTaskId} from "../task-title-id-drift.js";
import {buildBootstrapPrompt} from "../mesh-task-replication.js"; import {buildBootstrapPrompt} from "../mesh-task-replication.js";
import {validateFileScopeInPromptContent} from "../task-store/file-scope.js"; import {validateFileScopeInPromptContent} from "../task-store/file-scope.js";
@@ -79,7 +80,26 @@ export async function updateTaskUnlockedImpl(store: TaskStore, id: string, updat
loaded above if the re-read fails, which is no worse than before. loaded above if the re-read fails, which is no worse than before.
*/ */
const freshForGuard = await store.readTaskJson(dir).catch(() => null); const freshForGuard = await store.readTaskJson(dir).catch(() => null);
const validation = validateNodeOverrideChange(freshForGuard ?? task, updates.nodeId ?? null, overrideLanes); /*
FNXC:StateMachine 2026-08-01-10:20 (PR #2793's finding — the INNER half, merged with #2821):
THIS GUARD RUNS SECOND AND USED TO OVERRIDE THE FIRST. `updateTaskImpl` resolves the terminal
question and passes it in; this call passed no `isTerminalNodeId`, so it fell to
`defaultIsTerminalNodeId` — the bare literal `nodeId === "end"`. An unconverted literal behind
a converted call site, which meant converting the outer guard alone changed nothing an
operator could see. PR #2793 measured exactly that: correcting either guard on its own left
the rejected-`end` case unmoved, because both independently called it terminal.
Resolved here too, from the task's own workflow, and threaded ALONGSIDE #2821's
`overrideLanes` rather than in place of them — the two answer different questions about the
same call, and dropping either re-opens a defect the other did not cover.
*/
const terminal = updates.nodeId == null
? false
: await isTaskTerminalNodeIdAsync(store, id, updates.nodeId);
const validation = validateNodeOverrideChange(freshForGuard ?? task, updates.nodeId ?? null, {
...overrideLanes,
isTerminalNodeId: () => terminal,
});
if (!validation.allowed) { if (!validation.allowed) {
throw new Error(validation.message); throw new Error(validation.message);
} }

View File

@@ -388,3 +388,46 @@ export async function resolveWorkflowIrForTask(
*/ */
return (await resolveWorkflowIrForTaskWithProvenance(store, taskId, irCache)).ir; return (await resolveWorkflowIrForTaskWithProvenance(store, taskId, irCache)).ir;
} }
/*
FNXC:StateMachine 2026-07-31-20:10 (PR #2793's finding, fixed):
IS `nodeId` THE TASK'S OWN TERMINAL NODE? Resolved from the task's REAL workflow.
The previous answer came from `store.resolveTaskWorkflowIrSync`, which returns the DEFAULT workflow
IR for every task under PostgreSQL, so it answered about a board the card is not on. On a workflow
whose terminal node is not called `end` that inverted the FN-7641 guard in both directions: a
legitimate override to a non-terminal node named `end` was rejected as a terminal finalize, and an
override to the board's REAL terminal node was written verbatim — the silent no-op FN-7641 exists to
prevent. Proven end to end in `workflow-terminal-node-sync-resolution-live-e2e.pg.test.ts`.
FAIL-SOFT TO THE LITERAL, deliberately: an unresolvable workflow keeps exactly the pre-conversion
answer rather than losing the guard. `end` is also every builtin's terminal node id, so the fallback
is correct wherever it can still be reached.
RESIDUAL, stated because a review raised it and the obvious remedy does not work yet (PR #2812):
`resolveWorkflowIrForTask` DEGRADES to the default workflow instead of throwing, so the `catch` above
does not cover a failed lookup — a task on a custom board whose definition cannot be loaded is judged
against the default graph. The natural fix is to gate on
`resolveWorkflowIrForTaskWithProvenance(...).source === "selection"`, and that signal is currently
unusable: `createWorkflowDefinition` stores an authored IR VERBATIM, so its `ir.id` keeps whatever the
author wrote while the store allocates its own `WF-NNN`. The provenance identity check then compares
those two and reports `source: "default"` for a workflow it resolved CORRECTLY. Measured, not
assumed — gating on it here turned the "non-terminal override is written" case red.
So the residual is knowingly left: it is the pre-existing fail-soft, not a regression this change
introduces, and narrowing it depends on fixing the provenance signal first.
*/
export async function isTaskTerminalNodeIdAsync(
store: WorkflowIrResolverStore,
taskId: string,
nodeId: string,
): Promise<boolean> {
try {
const ir = await resolveWorkflowIrForTask(store, taskId);
const node = ir.nodes.find((candidate) => candidate.id === nodeId);
if (node) return node.kind === "end";
} catch {
// Fall through to the literal-id fallback below.
}
return nodeId === "end";
}

View File

@@ -1,5 +1,6 @@
/* /*
FNXC:StateMachine 2026-07-30-23:55 (E2E evidence — the terminal-node guard fires on the wrong node): FNXC:StateMachine 2026-07-30-23:55 (E2E evidence — the terminal-node guard fires on the wrong node)
FNXC:StateMachine 2026-07-31-20:45 (FIXED in PR #2811 — the two cases below now assert the CORRECT behaviour):
Fourth in the inert-sync-resolution series (#2789 scheduler, #2791 planner lanes, #2792 custom 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 fields). This one does not merely answer with the wrong vocabulary: it makes a SAFETY GUARD fire on a
@@ -131,49 +132,49 @@ pgDescribe("terminal-node resolution for a live task", () => {
expect(nodes.find((n) => n.id === "end")?.kind).not.toBe("end"); 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 () => { it("a legitimate override to the non-terminal `end` node is WRITTEN", async () => {
/* /*
On this board `end` is an ordinary planning node, so per the contract's own words ("non-terminal FIXED. On this board `end` is an ordinary planning node, so per the contract's own words
nodeId overrides ... are untouched") this override should simply be written. Instead the operator ("non-terminal nodeId overrides ... are untouched") the override is simply written. Before the
gets a merge-proof error about finalizing a card they were not finalizing, and the routing change fix it was rejected with a merge-proof error about finalizing a card the operator was not
they asked for does not happen. finalizing, because BOTH guards independently called `end` terminal — the default IR said so, and
`defaultIsTerminalNodeId` is the literal `"end"`.
OVER-DETERMINED, so read the mutation results carefully: the default IR calls `end` terminal AND That over-determination is why this case is the one that proves the fix is COMPLETE: it could not
`defaultIsTerminalNodeId` is the literal `"end"`. Either guard alone rejects this write, so this move until both the outer resolution and the inner literal were corrected. See the mutation
case survives a mutation of either one and fails only when both are corrected. It is not a weak matrix in PR #2793 for the measurement, and PR #2811 for the fix.
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 store = h.store();
const taskId = await taskOnShiftedBoard(store, "wf-false-positive"); const taskId = await taskOnShiftedBoard(store, "wf-false-positive");
await expect(store.updateTask(taskId, { nodeId: "end" } as never)) await store.updateTask(taskId, { nodeId: "end" } as never);
.rejects.toThrow(/does not finalize a card by itself|durable merge proof/);
store.taskCache.delete(taskId); store.taskCache.delete(taskId);
expect((await store.getTask(taskId))?.nodeId).not.toBe("end"); const row = await store.getTask(taskId);
expect(row?.nodeId).toBe("end");
expect(row?.column).toBe(RENAMED_VOCAB.review); // a routing change, not a finalize
}); });
it("CHARACTERIZATION — and an override to the REAL terminal node silently no-ops", async () => { it("an override to the REAL terminal node is REFUSED without merge proof", async () => {
/* /*
The half that matters more, because it is the original FN-7641 bug restored. `finish` IS this FIXED, and this is the half that matters more: it was the original FN-7641 bug restored on every
board's `end`-kind node, so this write must either finalize the card (with merge proof) or be custom board. `finish` IS this board's `end`-kind node, so the write must either finalize the card
rejected (without). Instead NEITHER guard recognises the id: the field is written verbatim, no (with durable merge proof) or be refused. Before the fix neither guard recognised the id, so the
error is raised, and the card stays in review with nothing advanced — "no error and no field was written verbatim with no error and the card sat unadvanced — "no error and no
advancement", exactly as the contract's comment describes the behaviour it replaced. 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 Refusal is the correct outcome here because the fixture has no `mergeDetails.mergeConfirmed`.
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 store = h.store();
const taskId = await taskOnShiftedBoard(store, "wf-false-negative"); const taskId = await taskOnShiftedBoard(store, "wf-false-negative");
await store.updateTask(taskId, { nodeId: "finish" } as never); await expect(store.updateTask(taskId, { nodeId: "finish" } as never))
.rejects.toThrow(/does not finalize a card by itself|durable merge proof/);
/* And the field was not written on the way to refusing. */
store.taskCache.delete(taskId); store.taskCache.delete(taskId);
const row = await store.getTask(taskId); const row = await store.getTask(taskId);
expect(row?.nodeId).toBe("finish"); expect(row?.nodeId).not.toBe("finish");
expect(row?.column).toBe(RENAMED_VOCAB.review); // not finalized, not rejected — just written expect(row?.column).toBe(RENAMED_VOCAB.review);
}); });
}); });