fix(workflow): the review handoff killed the walk on a renamed review lane (#2900)
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) <noreply@anthropic.com>
This commit is contained in:
7
.changeset/review-handoff-lane.md
Normal file
7
.changeset/review-handoff-lane.md
Normal file
@@ -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.
|
||||
49
packages/engine/src/__tests__/review-handoff-lane.test.ts
Normal file
49
packages/engine/src/__tests__/review-handoff-lane.test.ts
Normal file
@@ -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();
|
||||
});
|
||||
});
|
||||
@@ -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<TaskDetail> = {};
|
||||
/*
|
||||
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;
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user