diff --git a/.changeset/review-gates-in-review-column.md b/.changeset/review-gates-in-review-column.md new file mode 100644 index 0000000000..14ff5bc057 --- /dev/null +++ b/.changeset/review-gates-in-review-column.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": minor +--- + +summary: Code Review and Browser Verification now run with the card in In review, showing the step as a card badge. +category: feature +dev: Moves the `code-review` / `browser-verification` optional-group nodes to `column: "in-review"` in the shared stepwise coding IR (inherited by `builtin:coding`, `builtin:stepwise-coding`, `builtin:brainstorming`, `builtin:coding-ideas`); their remediation nodes stay in `in-progress`, so a changes-requested verdict sends the card back to implementation. The dashboard badge was already lane-gated on `column === "in-review"`. Because `in-review` has no `wip` trait the slot is released during review, so the remediation crossing back into `in-progress` can hit the non-bypassable in-transaction capacity check; `workflow-column-boundary.onNodeEntry` now PARKS the run on a `capacity-exhausted` rejection instead of failing it, preserving the failed gate result and worktree so the next graph run retries once a slot frees. Non-capacity rejections still propagate. The legacy `builtin:legacy-coding` IR keeps its historical placement. diff --git a/docs/workflow-steps.md b/docs/workflow-steps.md index 1bdd833339..82f226bd7f 100644 --- a/docs/workflow-steps.md +++ b/docs/workflow-steps.md @@ -700,7 +700,7 @@ Review Level `0` and `>=2` run in warn-only telemetry mode (never block). Not all workflow failures are revision requests: -- **Revision requested**: Implementation needs changes → routes back to executor in-place while keeping the task in `in-progress` +- **Revision requested**: Implementation needs changes → routes back to the executor for a fresh remediation pass. For the pre-merge gates (Code Review, Browser Verification) the card is sitting in `in-review` while the gate runs, so the paired remediation node (`code-review-remediation` / `browser-verification-remediation`) moves it back to `in-progress` on entry. - **Hard failure**: Treated as remediable until retries are exhausted; the executor injects feedback and sends the task through `todo → in-progress` for a fresh remediation pass #### Pre-merge hard failure remediation flow @@ -712,7 +712,9 @@ For pre-merge gate hard failures, the graph executor drives remediation through 3. Reopen the terminal verification/delivery suffix plus the nearest preceding implementation step (`pending`) so the resumed pass can address feedback without discarding unrelated completed work 4. Schedule `todo → in-progress` after guard unwind, triggering a fresh executor remediation run that must complete every reopened step before the workflow step re-evaluates -Tasks are not parked in `in-review` for this remediable path unless additional terminal failures occur. +Tasks are not *parked* in `in-review` for this remediable path unless additional terminal failures occur — they pass through it. The pre-merge review gates run with the card in `in-review` (which is what surfaces the running step as a card badge on the board), and remediation moves it back to `in-progress`; the card only stays in `in-review` when a terminal failure blocks the merge gate. + +Because `in-review` carries no WIP trait, a card releases its concurrency/worktree slot while a review gate runs, even though its agent and checkout are still live. The pool can therefore be full when the remediation node crosses back into `in-progress`, and capacity is enforced in-transaction and is never bypassable — so that move can be rejected. The workflow column boundary treats a capacity rejection as a **park, not a failure**: the run unwinds cleanly, the card keeps its failed pre-merge gate result and its worktree, and the next graph run retries the crossing once a slot frees. Non-capacity rejections (invariant violations) still surface as real errors. ## Workflow Interpreter Dual-Observe (retired parity instrumentation) diff --git a/packages/core/src/__tests__/builtin-code-review-group.test.ts b/packages/core/src/__tests__/builtin-code-review-group.test.ts index 2b6ed14bc2..d7b3007df9 100644 --- a/packages/core/src/__tests__/builtin-code-review-group.test.ts +++ b/packages/core/src/__tests__/builtin-code-review-group.test.ts @@ -75,16 +75,26 @@ describe("codeReviewOptionalGroupNode", () => { }); describe("built-in coding + stepwise workflows wire code-review as a default-ON optional group", () => { + /* + FNXC:WorkflowReviewGates 2026-07-26-11:40: + The column is parametrized because the two built-ins deliberately disagree: the stepwise graph + (and everything cloned from it, incl. the default `builtin:coding`) runs Code Review in + "in-review" so the card shows the running gate as a badge, while the frozen legacy coding IR + keeps its historical "in-progress" placement. The paired remediation node stays "in-progress" + in BOTH — a gate that requests changes must send the card back to implementation. + */ it.each([ - ["builtin coding", BUILTIN_CODING_WORKFLOW_IR], - ["builtin stepwise", BUILTIN_STEPWISE_CODING_WORKFLOW_IR], - ])("%s includes the default-ON code-review optional-group and still parses/round-trips", (_name, ir) => { + ["builtin coding", BUILTIN_CODING_WORKFLOW_IR, "in-progress"], + ["builtin stepwise", BUILTIN_STEPWISE_CODING_WORKFLOW_IR, "in-review"], + ] as const)("%s includes the default-ON code-review optional-group and still parses/round-trips", (_name, ir, expectedColumn) => { const byId = new Map(ir.nodes.map((n) => [n.id, n])); const group = byId.get("code-review"); expect(group?.kind).toBe("optional-group"); expect(group?.config?.name).toBe("Code Review"); expect(group?.config?.defaultOn).toBe(true); - expect(group?.column).toBe("in-progress"); + expect(group?.column).toBe(expectedColumn); + // Changes-requested always routes back to implementation. + expect(byId.get("code-review-remediation")?.column).toBe("in-progress"); // Pre-merge wiring: ... → browser-verification → code-review → completion-summary; failure → remediation node. expect(ir.edges).toEqual( diff --git a/packages/core/src/__tests__/builtin-workflows.test.ts b/packages/core/src/__tests__/builtin-workflows.test.ts index 6e07a997ce..b32c783e95 100644 --- a/packages/core/src/__tests__/builtin-workflows.test.ts +++ b/packages/core/src/__tests__/builtin-workflows.test.ts @@ -599,11 +599,23 @@ describe("built-in workflows", () => { }); expect(byId.get("parse")?.column).toBe("in-progress"); expect(byId.get("steps")?.column).toBe("in-progress"); - // U6: the legacy `workflow-step` seam is replaced by the pre-merge - // `browser-verification` optional-group, placed in the implementation column. + /* + FNXC:WorkflowReviewGates 2026-07-26-11:40: + U6 replaced the legacy `workflow-step` seam with the pre-merge `browser-verification` + optional-group. Both pre-merge review gates sit in the REVIEW column, not the implementation + column: while a gate runs the card belongs in "In review" with the running step shown as a + card badge (the dashboard badge is lane-gated on `column === "in-review"`). Their paired + remediation nodes stay in "In progress" so a changes-requested verdict visibly sends the card + back to implementation. + */ expect(byId.get("workflow-step")).toBeUndefined(); expect(byId.get("browser-verification")?.kind).toBe("optional-group"); - expect(byId.get("browser-verification")?.column).toBe("in-progress"); + expect(byId.get("browser-verification")?.column).toBe("in-review"); + expect(byId.get("browser-verification-remediation")?.column).toBe("in-progress"); + expect(byId.get("code-review")?.kind).toBe("optional-group"); + expect(byId.get("code-review")?.column).toBe("in-review"); + expect(byId.get("code-review-remediation")?.column).toBe("in-progress"); + expect(byId.get("completion-summary")?.column).toBe("in-review"); expect(browserVerificationInnerConfig(ir)).toMatchObject({ toolMode: "coding", gateMode: "advisory", diff --git a/packages/core/src/__tests__/default-workflow-hooks.test.ts b/packages/core/src/__tests__/default-workflow-hooks.test.ts index e46d4fa6ce..91d37bdb51 100644 --- a/packages/core/src/__tests__/default-workflow-hooks.test.ts +++ b/packages/core/src/__tests__/default-workflow-hooks.test.ts @@ -122,3 +122,76 @@ describe("default-workflow-hooks registry wiring", () => { expect(defaultCtx.task.pausedReason).toBeUndefined(); }); }); + +/* +FNXC:WorkflowReviewGates 2026-07-26-14:40: +The pre-merge review gates (Code Review, Browser Verification) run with the card in `in-review`, so +the graph's crossing into the paired remediation node is a routine `in-review -> in-progress` move +that lands immediately after the gate wrote its `failed` result. The reopen clear used to wipe +`workflowStepResults` on every such move, destroying the remediation input — and, worse, making +`getTaskMergeBlocker`'s pending/failed branches vacuously false so a card could return to +`in-review` and be mergeable with its gate never re-run. + +These cases pin BOTH directions of the gate, because a fix that simply stopped clearing on +`in-progress` would silently change operator-reopen semantics that other recovery paths depend on +(`executor.performWorkflowRerunBounce` documents that `moveTask(in-review -> todo)` clears results +for it). Only a graph-owned in-review -> in-progress crossing is exempt. +*/ +describe("applyReopenFieldClears — graph-owned review-gate remediation crossing", () => { + beforeEach(() => { + __resetTraitRegistryForTests(); + __resetDefaultWorkflowHooksForTests(); + registerBuiltinTraits(); + registerDefaultWorkflowHooks(); + }); + + function withResults(overrides: Partial): DefaultWorkflowMoveContext { + const ctx = makeCtx(overrides); + ctx.task.workflowStepResults = [ + { workflowStepId: "code-review", workflowStepName: "Code Review", status: "failed", phase: "pre-merge" }, + { workflowStepId: "browser-verification", workflowStepName: "Browser Verification", status: "passed", phase: "pre-merge" }, + ] as Task["workflowStepResults"]; + return ctx; + } + + it("RETAINS workflowStepResults on the graph's in-review -> in-progress remediation crossing", () => { + const ctx = withResults({ + fromColumn: "in-review", + toColumn: "in-progress", + moveSource: "engine", + workflowMoveSource: "workflow-graph", + options: { preserveProgress: true }, + }); + applyDefaultWorkflowMoveEffects(ctx); + expect(ctx.task.workflowStepResults).toHaveLength(2); + expect(ctx.task.workflowStepResults?.find((r) => r.workflowStepId === "code-review")?.status).toBe("failed"); + }); + + it("still CLEARS on an operator reopen in-review -> in-progress (no graph provenance)", () => { + const ctx = withResults({ + fromColumn: "in-review", + toColumn: "in-progress", + moveSource: "user", + }); + applyDefaultWorkflowMoveEffects(ctx); + expect(ctx.task.workflowStepResults).toBeUndefined(); + }); + + it("still CLEARS on in-review -> todo even when the graph owns the move (bounce invariant)", () => { + const ctx = withResults({ + fromColumn: "in-review", + toColumn: "todo", + moveSource: "engine", + workflowMoveSource: "workflow-graph", + options: { preserveProgress: true }, + }); + applyDefaultWorkflowMoveEffects(ctx); + expect(ctx.task.workflowStepResults).toBeUndefined(); + }); + + it("still CLEARS on done -> todo reopen", () => { + const ctx = withResults({ fromColumn: "done", toColumn: "todo", moveSource: "user" }); + applyDefaultWorkflowMoveEffects(ctx); + expect(ctx.task.workflowStepResults).toBeUndefined(); + }); +}); diff --git a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts index 6532c78e61..752a9e4fde 100644 --- a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts +++ b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts @@ -158,7 +158,28 @@ const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { // after every step-instance completes — never per step-instance — and when // disabled the group passes through inert. Both the normal foreach-success path // and the rework-exhausted manual-release path flow through this node. - browserVerificationOptionalGroupNode("in-progress"), + /* + FNXC:WorkflowReviewGates 2026-07-26-11:05: + Review gates belong in the "In review" column, not "In progress". Browser Verification and + Code Review are review surfaces: while one runs the operator should see the card sitting in + In review with the running step name as a card badge (the badge is lane-gated on + `column === "in-review"` in dashboard `taskProgress.getRunningOptionalGateBadge`, so the + column IS the badge switch). This mirrors the Coding (Ideas) preset, which already re-homed + `code-review` to in-review. The paired remediation nodes stay in "In progress": a gate that + requests changes must visibly send the card back to implementation. + + Capacity consequence: `in-review` carries no `wip` trait, so a card under review releases its + concurrency/worktree slot even though its agent and checkout are still live. The pool can + therefore be full when the remediation node tries to cross back into `in-progress`, and + capacity is enforced in-transaction and is never bypassable — that move CAN be rejected. The + boundary controller handles it by PARKING the run on a capacity rejection rather than failing + it (`workflow-column-boundary.ts` onNodeEntry), so the card keeps its failed gate result and + the next graph run retries the crossing once a slot frees. Holding the slot through review via + occupancy accounting was tried and rejected: it cannot cover the failure -> remediation window + (occupancy keys on a `pending` lease that is already terminal by then) and it mis-assigns slots + on operator moves out of the review lane. + */ + browserVerificationOptionalGroupNode("in-review"), browserVerificationRemediationNode("in-progress"), // FNXC:CodeReviewStep 2026-06-25-15:00: // Pre-merge Code Review as a DEFAULT-ON optional-group (blocking gate), on the post-foreach @@ -167,7 +188,9 @@ const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { // (never per step-instance); both the foreach-success and rework-exhausted manual- // release paths flow through it. Runs for every task by default (defaultOn:true) but is // toggleable off per task; disabled → byte-inert pass-through. - codeReviewOptionalGroupNode("in-progress"), + // FNXC:WorkflowReviewGates 2026-07-26-11:05: in-review placement — see the note on + // browser-verification above. + codeReviewOptionalGroupNode("in-review"), codeReviewRemediationNode("in-progress"), completionSummaryNode("in-review"), { id: "merge-gate", kind: "merge-gate", column: "in-review", config: { gate: "auto-merge" } }, diff --git a/packages/core/src/default-workflow-hooks.ts b/packages/core/src/default-workflow-hooks.ts index 3f63225c77..b0524c9818 100644 --- a/packages/core/src/default-workflow-hooks.ts +++ b/packages/core/src/default-workflow-hooks.ts @@ -71,6 +71,16 @@ export interface DefaultWorkflowMoveContext { fromColumn: string; toColumn: string; moveSource: "user" | "engine" | "scheduler"; + /* + FNXC:WorkflowReviewGates 2026-07-26-14:20: + Provenance of the move, distinct from `moveSource` (which only says user/engine/scheduler). + `"workflow-graph"` is set at exactly one call site — the graph column boundary in + `executor.buildColumnBoundaryHooks` — so it uniquely identifies a graph-owned lifecycle crossing + as opposed to an operator reopen, a merge bounce, or a self-healing rebound. Needed because the + pre-merge review gates now live in `in-review`, making graph-owned `in-review -> in-progress` + routine; see `applyReopenFieldClears`. + */ + workflowMoveSource?: string; /** True when guards + abort-on-exit are bypassed (engine/recovery, KTD-9). */ bypassGuards: boolean; movedAt: string; @@ -202,9 +212,31 @@ export function applyInReviewEnterEffects(ctx: DefaultWorkflowMoveContext): void /** Reopen-from-review/done field clears (branch/summary/workflowStepResults). */ export function applyReopenFieldClears(ctx: DefaultWorkflowMoveContext): void { const { task, fromColumn, toColumn } = ctx; + /* + FNXC:WorkflowReviewGates 2026-07-26-14:25: + The GRAPH's own in-review -> in-progress crossing must NOT wipe `workflowStepResults`. + Since the pre-merge review gates moved into `in-review`, entering the paired remediation node + (in-progress) is a routine graph-owned crossing that happens immediately after the gate wrote its + `failed` result — so the ungated clear destroyed the remediation input. Three concrete breakages: + - `routeRetryableRemediationGraphFailureToPreMergeFix` and `recoverFailedPreMergeWorkflowStep` + select via `latestFailedPreMergeWorkflowStep` and silently no-op on an empty array, so the + auto-recovery for a parked remediation failure never fires. + - `getTaskMergeBlocker` reads pending/failed results; an empty array makes both branches + vacuously false, so a card can return to `in-review` and be MERGEABLE with its gate never + re-run. That is a safety regression, not just lost history. + - FN-7727 `priorAttempts` history restarts at attempt zero every remediation cycle. + Scoped deliberately to the graph-owned in-progress crossing: operator board drags, the in-review + comment re-engagement, merge bounces, and every `-> todo`/`-> triage` rebound still clear, so the + executor's documented bounce invariant ("moveTask(in-review->todo) already clears ALL results") + survives unchanged. + */ + const graphOwnedReviewToWip = ctx.workflowMoveSource === "workflow-graph" + && fromColumn === "in-review" + && toColumn === "in-progress"; if ( - (fromColumn === "in-review" && (toColumn === "todo" || toColumn === "in-progress" || toColumn === "triage")) || - (fromColumn === "done" && (toColumn === "todo" || toColumn === "triage")) + !graphOwnedReviewToWip + && ((fromColumn === "in-review" && (toColumn === "todo" || toColumn === "in-progress" || toColumn === "triage")) + || (fromColumn === "done" && (toColumn === "todo" || toColumn === "triage"))) ) { task.workflowStepResults = undefined; } diff --git a/packages/core/src/task-store/moves.ts b/packages/core/src/task-store/moves.ts index e9b152bfdc..4a4c4b99ee 100644 --- a/packages/core/src/task-store/moves.ts +++ b/packages/core/src/task-store/moves.ts @@ -734,6 +734,9 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum fromColumn, toColumn, moveSource, + // FNXC:WorkflowReviewGates 2026-07-26-14:25: graph-owned-crossing discriminator consumed + // by applyReopenFieldClears; set only by the graph column boundary. + workflowMoveSource: options?.workflowMoveSource, bypassGuards, movedAt, settings: undefined, @@ -865,9 +868,21 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum task.overlapBlockedBy = undefined; } + /* + FNXC:WorkflowReviewGates 2026-07-26-14:25: + Parity mirror of the gate in `applyReopenFieldClears` (default-workflow-hooks.ts) — these two + blocks are deliberately kept byte-equivalent in behavior. The graph's own + in-review -> in-progress crossing (the remediation node entry, routine now that the pre-merge + review gates live in `in-review`) must retain `workflowStepResults`; every other reopen still + clears. See the hook for the full rationale. + */ + const graphOwnedReviewToWip = options?.workflowMoveSource === "workflow-graph" + && fromColumn === "in-review" + && toColumn === "in-progress"; if ( - (fromColumn === "in-review" && (toColumn === "todo" || toColumn === "in-progress" || toColumn === "triage")) - || (fromColumn === "done" && (toColumn === "todo" || toColumn === "triage")) + !graphOwnedReviewToWip + && ((fromColumn === "in-review" && (toColumn === "todo" || toColumn === "in-progress" || toColumn === "triage")) + || (fromColumn === "done" && (toColumn === "todo" || toColumn === "triage"))) ) { task.workflowStepResults = undefined; } diff --git a/packages/engine/src/__tests__/executor-implicit-task-done-budget.test.ts b/packages/engine/src/__tests__/executor-implicit-task-done-budget.test.ts index cc6324c5bc..29bfbd7a9d 100644 --- a/packages/engine/src/__tests__/executor-implicit-task-done-budget.test.ts +++ b/packages/engine/src/__tests__/executor-implicit-task-done-budget.test.ts @@ -155,6 +155,13 @@ describe("FN-4946 implicit refusal budget handling", () => { The in-review handoff is now the graph's merge boundary, so the move carries the workflow-graph provenance of the node that made it instead of being a bare 2-arg completion-path move. + + FNXC:WorkflowReviewGates 2026-07-26-12:20: + The pre-merge review gates (browser-verification, code-review) now live in `in-review` too, so + the FIRST crossing into that column is whichever gate the graph reaches first rather than + `completion-summary`. This assertion is about the handoff carrying graph provenance and + `preserveProgress`, not about which node owns the boundary, so it pins the invariant (one + provenance-carrying move into in-review) and leaves the node id to the graph's shape. */ expect(store.moveTask).toHaveBeenCalledWith( "FN-4946-B3", @@ -162,7 +169,7 @@ describe("FN-4946 implicit refusal budget handling", () => { expect.objectContaining({ preserveProgress: true, workflowMoveSource: "workflow-graph", - workflowMoveMetadata: expect.objectContaining({ nodeId: "completion-summary" }), + workflowMoveMetadata: expect.objectContaining({ fromColumn: "in-progress" }), }), ); const retryBumpCalls = store.updateTask.mock.calls.filter(([, patch]: [string, Record]) => typeof patch.taskDoneRetryCount === "number" && patch.taskDoneRetryCount > 1); diff --git a/packages/engine/src/__tests__/workflow-column-boundary-capacity.test.ts b/packages/engine/src/__tests__/workflow-column-boundary-capacity.test.ts new file mode 100644 index 0000000000..b974b44def --- /dev/null +++ b/packages/engine/src/__tests__/workflow-column-boundary-capacity.test.ts @@ -0,0 +1,117 @@ +/* +FNXC:WorkflowReviewGates 2026-07-26-13:55: +The pre-merge review gates (Code Review, Browser Verification) run with the card in `in-review`, so +their paired remediation nodes cross `in-review -> in-progress` — a real crossing back INTO a +capacity-bearing column. Capacity is enforced in-transaction and is never bypassable, so if the +pool filled while the gate ran, that move is rejected. Before this fix the controller rethrew, the +graph run died at the remediation node, and the card was stranded in `in-review` behind a failed +pre-merge step with nothing scheduled to fix it. + +The invariant these tests pin: a CAPACITY rejection parks (suspends) so the run unwinds cleanly and +the next graph run retries the move, while a NON-capacity rejection (an invariant violation) still +propagates as a real error. Regression direction matters — a change that makes every rejection park +would silently swallow invariant violations, so both halves are asserted. +*/ +import { describe, expect, it, vi } from "vitest"; +import { TransitionRejectionError, type WorkflowIr } from "@fusion/core"; +import { createWorkflowColumnBoundary } from "../workflow-column-boundary.js"; + +/** Minimal two-column IR: a wip column and a review column, plus the remediation target. */ +function ir(): WorkflowIr { + return { + version: "v2", + name: "review-gate-capacity", + columns: [ + { id: "in-progress", name: "In progress", traits: [{ trait: "wip" }] }, + { id: "in-review", name: "In review", traits: [{ trait: "human-review" }, { trait: "merge-blocker" }] }, + ], + nodes: [ + { id: "code-review", kind: "optional-group", column: "in-review", config: {} }, + { id: "code-review-remediation", kind: "prompt", column: "in-progress", config: {} }, + ], + edges: [{ from: "code-review", to: "code-review-remediation", condition: "failure" }], + } as unknown as WorkflowIr; +} + +const remediationNode = () => ir().nodes.find((n) => n.id === "code-review-remediation")!; + +function capacityError() { + return new TransitionRejectionError( + { code: "capacity-exhausted", messageKey: "transition.rejected.capacityExhausted", retryable: true }, + "Column 'in-progress' is at capacity (4/4)", + ); +} + +function invariantError() { + return new TransitionRejectionError( + { code: "merge-blocked", messageKey: "transition.rejected.mergeBlocked", retryable: false }, + "merge blocked", + ); +} + +describe("workflow column boundary — capacity rejection on the remediation crossing", () => { + it("parks (suspends) instead of failing the run when in-progress is at capacity", async () => { + const moveTask = vi.fn().mockRejectedValue(capacityError()); + const onSuspend = vi.fn(); + const boundary = createWorkflowColumnBoundary({ + taskId: "FN-CAP1", + workflowId: "builtin:coding", + ir: ir(), + initialColumn: "in-review", + moveTask, + onSuspend, + }); + + const result = await boundary.onNodeEntry(remediationNode()); + + expect(result).toMatchObject({ + kind: "suspended", + reason: "capacity", + nodeId: "code-review-remediation", + fromColumn: "in-review", + toColumn: "in-progress", + }); + // The suspension must be persisted so the run has a durable continuation. + expect(onSuspend).toHaveBeenCalledTimes(1); + // The card did NOT move: the controller must keep reporting the review column, + // otherwise a later node entry would compute its boundary from a phantom column. + expect(boundary.currentColumn()).toBe("in-review"); + }); + + it("still propagates a non-capacity rejection so invariant violations are not swallowed", async () => { + const moveTask = vi.fn().mockRejectedValue(invariantError()); + const onSuspend = vi.fn(); + const boundary = createWorkflowColumnBoundary({ + taskId: "FN-CAP1", + workflowId: "builtin:coding", + ir: ir(), + initialColumn: "in-review", + moveTask, + onSuspend, + }); + + await expect(boundary.onNodeEntry(remediationNode())).rejects.toThrow(TransitionRejectionError); + expect(onSuspend).not.toHaveBeenCalled(); + expect(boundary.currentColumn()).toBe("in-review"); + }); + + it("advances normally when capacity allows the remediation move", async () => { + const moveTask = vi.fn().mockResolvedValue(undefined); + const boundary = createWorkflowColumnBoundary({ + taskId: "FN-CAP1", + workflowId: "builtin:coding", + ir: ir(), + initialColumn: "in-review", + moveTask, + }); + + const result = await boundary.onNodeEntry(remediationNode()); + + expect(result).toMatchObject({ kind: "entered" }); + expect(moveTask).toHaveBeenCalledWith("in-progress", expect.objectContaining({ + fromColumn: "in-review", + nodeId: "code-review-remediation", + })); + expect(boundary.currentColumn()).toBe("in-progress"); + }); +}); diff --git a/packages/engine/src/scheduler.ts b/packages/engine/src/scheduler.ts index 49db82a182..6fd3767c51 100644 --- a/packages/engine/src/scheduler.ts +++ b/packages/engine/src/scheduler.ts @@ -1501,6 +1501,14 @@ export class Scheduler { const maxConcurrent = settings.maxConcurrent ?? this.options.maxConcurrent ?? 2; const maxWorktrees = settings.maxWorktrees ?? this.options.maxWorktrees ?? 4; + /* + FNXC:WorkflowReviewGates 2026-07-26-13:10: + DEAD CODE — deliberately NOT carrying the review-gate WIP-occupancy fix here. + `shouldRunWorkflowColumnScheduler()` returns an unconditional `true` and the branch above it + always returns, so this legacy dispatcher is unreachable. The live capacity accounting is in + `runHoldReleaseSweepPass` (`reservedWorktreeSlots`/`reservedConcurrentSlots`). Mirroring the + fix into this block would only imply coverage that never executes. + */ // Count only in-progress tasks toward the worktree limit. // In-review tasks with worktrees are idle (waiting to merge) and // should not block new tasks from starting. diff --git a/packages/engine/src/workflow-column-boundary.ts b/packages/engine/src/workflow-column-boundary.ts index a3c689feb2..be21a84478 100644 --- a/packages/engine/src/workflow-column-boundary.ts +++ b/packages/engine/src/workflow-column-boundary.ts @@ -39,6 +39,7 @@ import { findWorkflowColumn, isHoldToWipBoundary, resolveColumnFlags, + TransitionRejectionError, } from "@fusion/core"; /** Run-audit event emitted by the boundary controller (KTD-12, ids/counts only). */ @@ -317,9 +318,41 @@ export function createWorkflowColumnBoundary( try { await deps.moveTask(toColumn, { fromColumn, nodeId: node.id }); } catch (err) { - // A rejected move (capacity, invariant) leaves the card in its current - // column; routing/parking is U4/U5. Do not advance `column` and do not - // emit a transition audit for a move that did not happen. + /* + FNXC:WorkflowReviewGates 2026-07-26-13:40: + A CAPACITY rejection on a real (non-hold→wip) boundary is transient, not a graph failure. + This became reachable once the pre-merge review gates moved into `in-review`: the paired + remediation node crosses in-review → in-progress, and that crossing re-enters a + capacity-bearing column. Capacity is enforced in-transaction and is never bypassable, so + if the pool filled while the gate ran, the move is rejected — and rethrowing here killed + the run at the remediation node, stranding the card in `in-review` behind a failed + pre-merge step with nothing scheduled to fix it. + Park it the same way the hold→wip seam parks instead: a `suspended` result unwinds to a + clean `outcome: "success"` with a suspension marker (no failure recorded, worktree/branch + and the durable failed gate result preserved), so the next graph re-run retries the + remediation move once a slot frees. Non-capacity rejections (invariant violations) are + real errors and still propagate. + */ + if (err instanceof TransitionRejectionError && err.rejection.code === "capacity-exhausted") { + warn("graph column move parked — column at capacity (will retry on next run)", { + fromColumn, + toColumn, + nodeId: node.id, + }); + const suspension = { + kind: "suspended", + reason: "capacity", + nodeId: node.id, + fromColumn, + toColumn, + irHash: computeWorkflowIrPin(deps.ir, node.id).irHash, + } as const; + await deps.onSuspend?.(suspension); + return suspension; + } + // A rejected move (invariant) leaves the card in its current column; + // routing/parking is U4/U5. Do not advance `column` and do not emit a + // transition audit for a move that did not happen. warn("graph column move rejected", { fromColumn, toColumn,