feat(workflow): run pre-merge review gates in the In review column
Code Review and Browser Verification now run with the card in `in-review` instead of `in-progress`, so the board shows the card under review with the running step as a badge (matching the Coding (Ideas) preset). Their paired remediation nodes stay in `in-progress`, so a changes-requested verdict visibly sends the card back to implementation. The column move IS the badge switch: the dashboard badge was already lane-gated on `column === "in-review"`. Applied to the shared stepwise coding IR, so it is inherited by builtin:coding (the default), builtin:stepwise-coding, builtin:brainstorming and builtin:coding-ideas; builtin:legacy-coding keeps its historical placement. Two consequences handled: - Capacity: `in-review` has no `wip` trait, so the slot is released during review and the remediation crossing back into `in-progress` can hit the non-bypassable in-transaction capacity check. The column boundary 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. - Reopen clears: `applyReopenFieldClears` wiped `workflowStepResults` on every in-review -> in-progress move, which the remediation crossing now performs routinely. That destroyed the remediation input, made `routeRetryableRemediationGraphFailureToPreMergeFix` and `recoverFailedPreMergeWorkflowStep` silently no-op, and — worse — made both `getTaskMergeBlocker` branches vacuously false, so a card could return to `in-review` and be mergeable with its gate never re-run. Now exempted for graph-owned in-review -> in-progress crossings only; operator reopens, merge bounces and every -> todo/triage rebound still clear, so the executor's documented bounce invariant is unchanged. Adds regression coverage for both (there was previously none for the reopen clear in either direction), and annotates the unreachable legacy scheduler dispatch block rather than mirroring the fix into dead code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
7
.changeset/review-gates-in-review-column.md
Normal file
7
.changeset/review-gates-in-review-column.md
Normal file
@@ -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.
|
||||
@@ -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)
|
||||
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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>): 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();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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" } },
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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<string, unknown>]) => typeof patch.taskDoneRetryCount === "number" && patch.taskDoneRetryCount > 1);
|
||||
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
@@ -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.
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user