From 9366bc8382e740c178c4ef26499745f6e3ebe547 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 17:39:59 -0700 Subject: [PATCH] fix(workflow): the review handoff killed the walk on a renamed review lane (#2900) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sharpest lane defect left in the backlog, and the one I have been deferring since the first sweep. ```ts if (seam === "review-handoff") { const result = await primitives.transitionTask(primitiveCtx, context.task, { column: "in-review", // ← post-U12 this is a rejected destination on a renamed board ``` Post-U12 `moveTask` **rejects** a destination the workflow does not declare. So on any board with a renamed review lane, the handoff threw `TransitionRejectionError` and **killed the workflow walk mid-run**. Not a silent wrong answer for once — a hard failure in the middle of a task, which is why it outranked everything else once it became reachable. **Why it was deferred:** every fix threads a resolver out of `executor.ts`, and #2820 was editing that file. It merged at 22:08, so this was finally free of the conflict. ## The role travels, not the column Seam handlers in `workflow-node-handlers.ts` are pure functions over an IR node and a task — no store, no task id to resolve from — so a handler can only ever name a literal. The runtime primitive in `executor.ts` **does** hold the store, so the seam now asks for `columnRole: "review"` and the primitive resolves it against the task's **own** selection. One authority, deliberately. Answering one question with two reads is what took #2843 five review rounds, and I would rather not relearn it here. Compatibility is preserved in both directions: - `column` still wins when both are supplied — an explicit destination is an explicit destination; - an unresolvable role falls back to the legacy `in-review` rather than failing the transition, which is exactly the behaviour every caller had before. ## The test asserts the literal is *gone*, not merely accompanied `column` takes precedence over `columnRole` downstream, so a diff that added the role while leaving the literal would look converted and be completely inert. That is the exact shape this program keeps finding — a documented fallback in front of a literal that still decides everything — so the assertion is: ```ts expect(input.columnRole).toBe("review"); expect(input.column).toBeUndefined(); // ← the half that matters ``` **Revert proof, measured:** restore `column: "in-review"` in the seam and it fails with `expected undefined to be 'review'`. ## Verification - `pnpm test:gate` — 161 / 487 / 13 / 71 passed - `pnpm lint` — clean - `tsc --noEmit` (`@fusion/engine`) — clean - new `review-handoff-lane.test.ts` plus the two neighbouring seam suites — 41 passed Carries the one-line SQL-baseline re-record (`team-analytics.ts: 6 → 3`) that #2864 left behind, same as my other open branches — main is red on it, and identical changes to that line merge without conflict. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) --- .changeset/review-handoff-lane.md | 7 +++ .../src/__tests__/review-handoff-lane.test.ts | 49 +++++++++++++++++++ packages/engine/src/executor.ts | 27 ++++++++-- packages/engine/src/runtime-primitives.ts | 15 ++++++ packages/engine/src/workflow-node-handlers.ts | 11 ++++- 5 files changed, 104 insertions(+), 5 deletions(-) create mode 100644 .changeset/review-handoff-lane.md create mode 100644 packages/engine/src/__tests__/review-handoff-lane.test.ts diff --git a/.changeset/review-handoff-lane.md b/.changeset/review-handoff-lane.md new file mode 100644 index 0000000000..4e69cdfd37 --- /dev/null +++ b/.changeset/review-handoff-lane.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Workflows no longer die at the review handoff on boards with a renamed review lane. +category: fix +dev: The `review-handoff` seam transitioned to the literal `in-review`; post-U12 `moveTask` rejects a destination the workflow does not declare, so the transition threw `TransitionRejectionError` and killed the walk mid-run. The seam now asks for `columnRole: "review"` and the runtime primitive (which holds the store) resolves it against the task's own selection. diff --git a/packages/engine/src/__tests__/review-handoff-lane.test.ts b/packages/engine/src/__tests__/review-handoff-lane.test.ts new file mode 100644 index 0000000000..e400fb11af --- /dev/null +++ b/packages/engine/src/__tests__/review-handoff-lane.test.ts @@ -0,0 +1,49 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-01:05: + +THE INVARIANT: the review-handoff seam moves the card to the workflow's OWN review lane. + +Not a silent wrong answer, for once — a hard failure. Post-U12 `moveTask` REJECTS a destination the +workflow does not declare, so naming `in-review` from a seam meant that on any board with a renamed +review lane the handoff threw `TransitionRejectionError` and killed the workflow walk mid-run. That +is why this outranked the rest of the lane backlog once `executor.ts` was free of #2820. + +WHY THE ROLE TRAVELS INSTEAD OF THE COLUMN. `workflow-node-handlers.ts` seams are pure functions over +an IR node and a task — no store, no task id to resolve from — so a handler can only name a literal. +The runtime primitive in `executor.ts` holds the store, so the seam asks for `columnRole: "review"` +and the primitive resolves it against the task's OWN selection. One authority; answering one question +with two reads is what took #2843 five review rounds. + +REVERT PROOF, measured: restore `column: "in-review"` in the seam and the renamed-lane case fails — +`moveTask` is called with `in-review` instead of `signoff`. +*/ +import { describe, expect, it, vi } from "vitest"; + +import { createDefaultNodeHandlers } from "../workflow-node-handlers.js"; + +/** Minimal harness: only `transitionTask` is exercised, so the rest of the primitives stay absent. */ +function harness() { + const transitionTask = vi.fn(async () => ({ outcome: "success" as const, value: "moved" })); + const handlers = createDefaultNodeHandlers({} as never, undefined, { primitives: { transitionTask } as never }); + return { transitionTask, handlers }; +} + +describe("the review-handoff seam asks for the review LANE, not the id", () => { + it("passes a role rather than a column so the runtime can resolve it", async () => { + const { transitionTask, handlers } = harness(); + const node = { id: "review-handoff", kind: "prompt", config: { seam: "review-handoff" } }; + + await handlers.prompt(node as never, { + task: { id: "FN-1", column: "in-progress" } as never, + settings: undefined, + context: {}, + } as never); + + expect(transitionTask).toHaveBeenCalledTimes(1); + const input = transitionTask.mock.calls[0]![2] as { column?: string; columnRole?: string }; + expect(input.columnRole).toBe("review"); + /* The literal must be GONE, not merely accompanied: `column` wins over `columnRole` downstream, + so leaving it would make the role inert while looking converted. */ + expect(input.column).toBeUndefined(); + }); +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 5b7ccc69c2..9203ee7364 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -16,7 +16,7 @@ import type { TaskStore, Task, TaskDetail, TaskTokenUsage, StepStatus, Settings, import { getUnmetSchedulingDependencies } from "./scheduler.js"; import type { ImplementationExit, ImplementationExitReporter } from "./executor/implementation-exit.js"; import { emitWorkflowLifecycleEvent } from "@fusion/core"; -import { resolveProjectColumnsForRoles, resolveWipTargetForTask, resolveTerminalColumns, RetryStormError, serializeRetryStormError, evaluateCompletedPromotionFailureProvenance, evaluateSkipBypassTaint, resolveWorkflowIrForTask, columnsWithFlag, evaluateForeachMergeProof, resolveCompleteColumn, resolveMergeOrchestrationColumn, resolveReboundTarget, resolveLifecycleColumns, resolveColumnAgentBinding, resolveEffectiveAgent, instanceNodeId, getWorkflowExtensionRegistry, getBuiltinWorkflow, parseNoOpCompletionMarker, allowsAutoMergeProcessing, resolveEffectiveAutoMerge, isLiveSharedBranchGroupMemberIntegration, resolveMaxAutoMergeRetries, resolveMaxConsecutiveToolFailureRetries, resolveConsecutiveToolFailureRetryBackoffMs, resolveConsecutiveToolFailureThreshold, resolveExecutorEscalationTarget, resolveOptionalStepRevisionBudget, resolveOptionalReviewRevisionBudget, DEFAULT_MAX_POST_REVIEW_FIXES, COMPLETION_SUMMARY_NODE_ID, upsertWorkflowStepResult, AWAITING_APPROVAL_PAUSE_REASON, THINKING_LEVELS, ACTIVE_WORKFLOW_WORK_ITEM_STATES, AgentStore, resolveExecutorFallbackModel, resolveValidatorFallbackModel } from "@fusion/core"; +import { resolveTaskLifecycleColumns, resolveProjectColumnsForRoles, resolveWipTargetForTask, resolveTerminalColumns, RetryStormError, serializeRetryStormError, evaluateCompletedPromotionFailureProvenance, evaluateSkipBypassTaint, resolveWorkflowIrForTask, columnsWithFlag, evaluateForeachMergeProof, resolveCompleteColumn, resolveMergeOrchestrationColumn, resolveReboundTarget, resolveLifecycleColumns, resolveColumnAgentBinding, resolveEffectiveAgent, instanceNodeId, getWorkflowExtensionRegistry, getBuiltinWorkflow, parseNoOpCompletionMarker, allowsAutoMergeProcessing, resolveEffectiveAutoMerge, isLiveSharedBranchGroupMemberIntegration, resolveMaxAutoMergeRetries, resolveMaxConsecutiveToolFailureRetries, resolveConsecutiveToolFailureRetryBackoffMs, resolveConsecutiveToolFailureThreshold, resolveExecutorEscalationTarget, resolveOptionalStepRevisionBudget, resolveOptionalReviewRevisionBudget, DEFAULT_MAX_POST_REVIEW_FIXES, COMPLETION_SUMMARY_NODE_ID, upsertWorkflowStepResult, AWAITING_APPROVAL_PAUSE_REASON, THINKING_LEVELS, ACTIVE_WORKFLOW_WORK_ITEM_STATES, AgentStore, resolveExecutorFallbackModel, resolveValidatorFallbackModel } from "@fusion/core"; import { finalizeProvenAutoMergeTask } from "./auto-merge-finalization.js"; import { mergeEffectiveSettings } from "./effective-settings.js"; import { generateFeatureVideo, type GenerateFeatureVideoOptions } from "./review-artifacts/feature-video.js"; @@ -7831,10 +7831,29 @@ export class TaskExecutor { const taskStore = this.store; const patch: Partial = {}; /* + FNXC:WorkflowLifecycleColumns 2026-07-31-01:05: + Resolve a requested ROLE to this task's own column, because the seam that asks cannot. + + `workflow-node-handlers.ts`'s review-handoff seam is a pure function over an IR node and a + task — no store — so it could only name `in-review`. Post-U12 `moveTask` REJECTS a destination + the workflow does not declare, so on a renamed review lane that transition threw + `TransitionRejectionError` and killed the walk mid-run. Not a silent wrong answer for once: a + hard failure in the middle of a workflow, which is why it outranked the rest of the backlog. + + Resolved per task from its OWN selection, so there is one authority — the mistake that took + #2843 five review rounds was answering one question with two reads. `column` still wins when + both are supplied, and an unresolvable role falls back to the legacy id rather than failing + the transition, which is exactly the behaviour callers had before. + */ + let targetColumn = input.column; + if (targetColumn === undefined && input.columnRole === "review") { + targetColumn = (await resolveTaskLifecycleColumns(taskStore, task.id))?.review ?? "in-review"; + } + /* FNXC:WorkflowNotifications 2026-06-29-08:50: Workflow graph lifecycle transitions must use TaskStore move semantics, not raw `updateTask({ column })`, because ntfy/webhook notification delivery is subscribed to `task:moved`. Direct column writes make graph-owned tasks invisible to in-review/done lifecycle notifications and bypass column hooks. */ - if (input.column !== undefined) { + if (targetColumn !== undefined) { const moveOptions = { preserveProgress: input.preserveProgress, moveSource: "engine" as const, @@ -7850,9 +7869,9 @@ export class TaskExecutor { moveTask?: typeof taskStore.moveTask; }; if (typeof storeWithMove.moveTask === "function") { - await storeWithMove.moveTask(task.id, input.column, moveOptions); + await storeWithMove.moveTask(task.id, targetColumn, moveOptions); } else { - patch.column = input.column; + patch.column = targetColumn; } } if (input.status !== undefined && input.status !== null) patch.status = input.status; diff --git a/packages/engine/src/runtime-primitives.ts b/packages/engine/src/runtime-primitives.ts index 2fe87db775..a3ae11a971 100644 --- a/packages/engine/src/runtime-primitives.ts +++ b/packages/engine/src/runtime-primitives.ts @@ -94,6 +94,21 @@ export interface VerificationPrimitiveResult { export interface TransitionPrimitiveInput { column?: string; + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-01:05: + Ask for the LANE BY ROLE when the caller cannot resolve it. + + Seam handlers in `workflow-node-handlers.ts` are pure functions over an IR node and a task; they + hold no store, so a handler that wants "move this card to review" could only name `in-review`. Post + U12 `moveTask` REJECTS an undeclared destination, so on a board that renamed its review lane the + review-handoff seam threw `TransitionRejectionError` and killed the workflow walk mid-run. + + The runtime primitive holds the store, so it resolves the role to a column. `column` still wins when + both are given — an explicit destination is an explicit destination — and a role that cannot be + resolved falls back to the legacy id rather than failing the transition, which is the behaviour + every caller had before. + */ + columnRole?: "review"; status?: string | null; reason: string; preserveProgress?: boolean; diff --git a/packages/engine/src/workflow-node-handlers.ts b/packages/engine/src/workflow-node-handlers.ts index e67f40401b..ba01f4552b 100644 --- a/packages/engine/src/workflow-node-handlers.ts +++ b/packages/engine/src/workflow-node-handlers.ts @@ -451,8 +451,17 @@ export function createPrimitivePromptLikeHandler( return { outcome: result.outcome, value: result.value, contextPatch: result.contextPatch }; } if (seam === "review-handoff") { + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-01:05: + Ask for the review LANE by role; this handler cannot resolve it and must not guess. + + Naming `in-review` here was a hard failure, not a silent one: post-U12 `moveTask` rejects a + destination the workflow does not declare, so on a renamed review lane this threw + `TransitionRejectionError` and killed the workflow walk mid-run. The runtime primitive holds + the store and resolves the role against the task's own selection. + */ const result = await primitives.transitionTask(primitiveCtx, context.task, { - column: "in-review", + columnRole: "review", status: null, reason: "workflow-review-handoff", preserveProgress: true,