diff --git a/.changeset/terminal-node-resolution.md b/.changeset/terminal-node-resolution.md new file mode 100644 index 0000000000..cdd8d6865f --- /dev/null +++ b/.changeset/terminal-node-resolution.md @@ -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. diff --git a/packages/core/src/__tests__/sync-workflow-ir-callsite-allowlist.test.ts b/packages/core/src/__tests__/sync-workflow-ir-callsite-allowlist.test.ts index b4336f8890..aa7fc111ea 100644 --- a/packages/core/src/__tests__/sync-workflow-ir-callsite-allowlist.test.ts +++ b/packages/core/src/__tests__/sync-workflow-ir-callsite-allowlist.test.ts @@ -35,10 +35,6 @@ reason; a sync-resolved lifecycle guard is a guard that cannot fire. */ const ALLOWED_CALL_SITES: ReadonlyMap = 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", "Synchronous lifecycle bookkeeping inside a transaction.", diff --git a/packages/core/src/task-store/branch-and-pr-entities.ts b/packages/core/src/task-store/branch-and-pr-entities.ts index 144359ac6b..5d802b99a3 100644 --- a/packages/core/src/task-store/branch-and-pr-entities.ts +++ b/packages/core/src/task-store/branch-and-pr-entities.ts @@ -20,6 +20,7 @@ import { getWorkflowWorkItem as getWorkflowWorkItemAsync } from "./async-workflo 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 { validateNodeOverrideChange, resolveNodeOverrideLanes } from "../node-override-guard.js"; +import { isTaskTerminalNodeIdAsync } from "../workflow-ir-resolver.js"; import { WorkflowMovePolicyInput } from "../workflow-extension-types.js"; import { resolveWorkflowIrById } from "../workflow-ir-resolver.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 currentTask = await store.getTask(id).catch(() => null); 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, { - isTerminalNodeId: (nodeId) => isTaskTerminalNodeIdImpl(store, id, nodeId), + /* Resolved above; `overrideLanes` stays exactly as main computes it. */ + isTerminalNodeId: () => terminal, ...overrideLanes, }); if (!validation.allowed) { @@ -632,23 +644,16 @@ export async function updateTaskImpl(store: TaskStore, return store.withTaskLock(id, () => store.updateTaskUnlocked(id, updates, runContext)); } -/** - * FNXC:StateMachine 2026-07-07-12:00: - * Resolve whether `nodeId` is the task's resolved workflow terminal `end` node (kind === "end"), - * for the nodeId='end' finalize-on-proof-or-error contract (FN-7641 Signature 2). Falls back to - * the literal id check when the workflow IR cannot be resolved or does not contain the node, which - * still matches every built-in workflow's terminal node id. - */ -function isTaskTerminalNodeIdImpl(store: TaskStore, taskId: string, nodeId: string): boolean { - try { - 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"; -} +/* +FNXC:StateMachine 2026-07-31-20:15 (PR #2793's finding, fixed): +`isTaskTerminalNodeIdImpl` LIVED HERE and is deleted, not kept as a fallback. It resolved the task's +graph through `store.resolveTaskWorkflowIrSync`, which answers with the DEFAULT workflow for every +task under PostgreSQL — so it reported on a board the card is not on. Its replacement, +`isTaskTerminalNodeIdAsync`, keeps the identical literal fail-soft for an unresolvable workflow. + +Keeping both would have re-created the half-conversion this program keeps finding: one caller +resolved, one not, and no way to tell from a call site which it got. +*/ export function mergeCustomFieldPatchImpl(store: TaskStore, current: Record | undefined, diff --git a/packages/core/src/task-store/task-update.ts b/packages/core/src/task-store/task-update.ts index a710922d19..c46f973ceb 100644 --- a/packages/core/src/task-store/task-update.ts +++ b/packages/core/src/task-store/task-update.ts @@ -17,6 +17,7 @@ import {validateCustomFieldPatch, CustomFieldRejectionError} from "../task-field import "../builtin-traits.js"; import {normalizeTaskPriority} from "../task-priority.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 {buildBootstrapPrompt} from "../mesh-task-replication.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. */ 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) { throw new Error(validation.message); } diff --git a/packages/core/src/workflow-ir-resolver.ts b/packages/core/src/workflow-ir-resolver.ts index 0fb05d6ca7..a7ca3ed1c2 100644 --- a/packages/core/src/workflow-ir-resolver.ts +++ b/packages/core/src/workflow-ir-resolver.ts @@ -388,3 +388,46 @@ export async function resolveWorkflowIrForTask( */ 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 { + 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"; +} diff --git a/packages/engine/src/__tests__/workflow-terminal-node-sync-resolution-live-e2e.pg.test.ts b/packages/engine/src/__tests__/workflow-terminal-node-sync-resolution-live-e2e.pg.test.ts index c6b60e310c..ea19dcd72d 100644 --- a/packages/engine/src/__tests__/workflow-terminal-node-sync-resolution-live-e2e.pg.test.ts +++ b/packages/engine/src/__tests__/workflow-terminal-node-sync-resolution-live-e2e.pg.test.ts @@ -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 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"); }); - 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 - 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. + FIXED. On this board `end` is an ordinary planning node, so per the contract's own words + ("non-terminal nodeId overrides ... are untouched") the override is simply written. Before the + fix it was rejected with a merge-proof error about finalizing a card the operator was not + 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 - `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. + That over-determination is why this case is the one that proves the fix is COMPLETE: it could not + move until both the outer resolution and the inner literal were corrected. See the mutation + matrix in PR #2793 for the measurement, and PR #2811 for the fix. */ 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/); + await store.updateTask(taskId, { nodeId: "end" } as never); 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 - 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 + FIXED, and this is the half that matters more: it was the original FN-7641 bug restored on every + custom board. `finish` IS this board's `end`-kind node, so the write must either finalize the card + (with durable merge proof) or be refused. Before the fix neither guard recognised the id, so the + 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. - 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. + Refusal is the correct outcome here because the fixture has no `mergeDetails.mergeConfirmed`. */ const store = h.store(); 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); const row = await store.getTask(taskId); - expect(row?.nodeId).toBe("finish"); - expect(row?.column).toBe(RENAMED_VOCAB.review); // not finalized, not rejected — just written + expect(row?.nodeId).not.toBe("finish"); + expect(row?.column).toBe(RENAMED_VOCAB.review); }); });