From e5c9ea38709bc56a9f84e924bd180079178cfb9b Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 13:15:04 -0700 Subject: [PATCH] fix(core): resolve the task's own terminal node in the node-override guard (#2812) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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) --- .changeset/terminal-node-resolution.md | 7 +++ ...ync-workflow-ir-callsite-allowlist.test.ts | 4 -- .../src/task-store/branch-and-pr-entities.ts | 41 ++++++++------- packages/core/src/task-store/task-update.ts | 22 +++++++- packages/core/src/workflow-ir-resolver.ts | 43 ++++++++++++++++ ...l-node-sync-resolution-live-e2e.pg.test.ts | 51 ++++++++++--------- 6 files changed, 120 insertions(+), 48 deletions(-) create mode 100644 .changeset/terminal-node-resolution.md 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); }); });