U8: the graph owns the pending-review park — ownership ledger 28 → 27 (#2590)

The routing move this unit has been building toward, landing on the path
the engine actually runs. **Includes #2578's commit** (the live-path fix
it depends on) — merge that first, or this supersedes it.

## What changes

Three things together, because a half-routed move is a card that
silently does not advance:

1. The **live** implementation primitive (`runCodingSession`) returns
`{outcome: "failure", value: "review-pending"}` for that ending.
2. The primitive step handler stops flattening every ending to
`step-done`/`step-failed`, so the value survives the foreach —
`runForeach` propagates a failing instance's value as the node's own —
and reaches an edge.
3. The inline `handoffTaskToReview` in `runImplementation` is
**deleted**. The phase reports and stops, which is all an implementation
phase should do.

Built-in workflows route to the `review-pending-handoff` node added in
#2519/#2546, which performs the handoff and ends the run: the same two
effects in the same order, with the graph as the owner.

## Proof, end to end

FN-5436 — the test that blocked this move twice and was right both times
— now passes, with a **stronger** assertion than it had:

```ts
expect(store.moveTask).toHaveBeenCalledWith("FN-5436-B", "in-review",
  expect.objectContaining({
    workflowMoveSource: "workflow-graph",
    workflowMoveMetadata: expect.objectContaining({ nodeId: "review-pending-handoff" }),
  }));
```

The old two-argument `moveTask(id, "in-review")` could not distinguish a
graph-owned park from an out-of-band one — which is the entire
distinction this unit exists to make. The invariant (park in review,
never `failed`) is unchanged; the owner is now proven.

## Every ratchet fired, and each records a real change

| Ratchet | Before | After | Why |
|---|---|---|---|
| Ownership ledger — `runImplementation` review handoffs | 3 | **2** |
the handoff left the phase |
| Ownership ledger — `handleGraphFailure` | 0 | **1** | the named compat
classifier |
| Ledger headline — executor-owned dispositions | 28 | **27** | first
decrement of the unit |
| Out-of-band exit list | 2 | **1** | pending-review is graph-owned now
|
| Primitive routing pin | "must not reroute" | routes *only* the moved
ending | declared, not discovered |

None was relaxed. The `handleGraphFailure` 0 → 1 is the honest one: for
a user-authored graph without the edge this is a **relocation, not an
elimination** — the transition is still executor-performed, but from one
named classifier in the failure ladder rather than a call buried two
thousand lines into a session loop. The ledger says so rather than
letting the headline number imply more progress than there is.

## Why it took four attempts

Recorded because the reason is reusable: the value was being produced on
`createAuthoritativeWorkflowSeams`, a handler that never runs (#2578).
Every earlier attempt was correct code on a dead path, and the only
thing that showed it was instrumenting until a negative result was
proven observable rather than assumed.

## Verification

- step-session + exit-events + primitive-exit-events + ownership ledger
+ graph-requeue-gate + task-done-blocked — **83 tests green**
- `pnpm test:gate` green (10 / 482 / 71); `pnpm lint` clean; `tsc
--noEmit` clean
- Changeset included (`patch`, `internal`)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* Improved handling of tasks awaiting review so they are correctly
routed to the review workflow.
* Tasks now remain in review instead of being marked as failed when no
follow-up review route is configured.
* Review handoffs now include workflow ownership and provenance details.
* Preserved standard failure handling for tasks that are not awaiting
review.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-07-29 20:59:06 -07:00
committed by GitHub
parent d5f1ce7abd
commit 3f763cba87
8 changed files with 227 additions and 15 deletions

View File

@@ -0,0 +1,7 @@
---
"@runfusion/fusion": patch
---
summary: The graph now parks a card in review when a step is blocked on a pending review, instead of the executor doing it.
category: internal
dev: The live implementation primitive returns `review-pending` and the step handler stops flattening it, so built-in workflows route to their `review-pending-handoff` node. The inline `handoffTaskToReview` in `runImplementation` is gone; user-authored graphs without the edge fall back to a named classifier in `handleGraphFailure`.

View File

@@ -181,14 +181,13 @@ describe("execute seam announces the implementation phase's exit", () => {
}
});
it("classifies exactly the two executor-performed transitions as out-of-band", () => {
it("lists exactly the endings the implementation phase still transitions itself", () => {
/*
The ledger this unit closes: an out-of-band exit is one where the EXECUTOR moved the card.
If a third appears without a routing move, U8 has gone backwards.
*/
expect([...OUT_OF_BAND_IMPLEMENTATION_EXITS]).toEqual([
"review-handoff-paused-after-completion",
"review-handoff-pending-review",
]);
expect(isOutOfBandImplementationExit("complete")).toBe(false);
expect(isOutOfBandImplementationExit(undefined)).toBe(false);

View File

@@ -173,7 +173,9 @@ outcome; raising one is a new out-of-graph lifecycle decision and needs a stated
const LEDGER = {
runImplementation: {
"column transitions (store.moveTask)": 16,
"review transitions (handoffTaskToReview)": 3,
/* U8: 3 -> 2. The pending-review handoff left this method — the graph's
`review-pending-handoff` node performs it now. A decrement here is the unit working. */
"review transitions (handoffTaskToReview)": 2,
"terminal parks (status: \"failed\")": 9,
"graph handbacks (graphCompletion)": 3,
},
@@ -185,7 +187,10 @@ const LEDGER = {
*/
handleGraphFailure: {
"column transitions (store.moveTask)": 0,
"review transitions (handoffTaskToReview)": 0,
/* U8: 0 -> 1. The named compat classifier for user-authored graphs that do not declare the
`outcome:review-pending` edge. For those shapes the transition is RELOCATED, not removed —
stated plainly so the ledger is not read as more progress than it is. */
"review transitions (handoffTaskToReview)": 1,
"terminal parks (status: \"failed\")": 7,
},
} as const;
@@ -215,11 +220,11 @@ describe("U8 execution-lifecycle ownership ledger", () => {
/*
The headline number, stated once so a reader does not have to add the ledger up: the
implementation phase decides its own lifecycle 28 times and asks the graph 3 times.
implementation phase decides its own lifecycle 27 times and asks the graph 3 times (28 at baseline; the pending-review handoff moved to the graph).
*/
it("states the U8 baseline ratio: the implementation phase decides far more than it asks", () => {
const owned = EXECUTOR_OWNED_LABELS.reduce<number>((sum, label) => sum + LEDGER.runImplementation[label], 0);
const handbacks = LEDGER.runImplementation[GRAPH_HANDBACK_LABEL];
expect({ owned, handbacks }).toEqual({ owned: 28, handbacks: 3 });
expect({ owned, handbacks }).toEqual({ owned: 27, handbacks: 3 });
});
});

View File

@@ -72,12 +72,20 @@ describe("the LIVE implementation primitive announces the exit", () => {
expect(completed[0]).not.toHaveProperty("exit");
});
it("returns the unchanged routing outcome — announcing must not reroute", async () => {
const { primitives, ctx } = harness({ taskDone: false, modifiedFiles: [], exit: "review-handoff-pending-review" });
it("routes the pending-review ending, and leaves every other ending's value alone", async () => {
/*
This pin was "announcing must not reroute" while exits were reporting-only. The pending-review
ending is now a ROUTED outcome, so the row changed deliberately — declared here rather than
discovered. Every other ending keeps `implementation-incomplete`, which is what proves the
move is narrow.
*/
const moved = await (harness({ taskDone: false, modifiedFiles: [], exit: "review-handoff-pending-review" })
.primitives.runCodingSession({ run: {}, node: { node: { id: "execute", kind: "prompt" }, context: {} } } as never, TASK, { worktreePath: "/tmp/wt", branchName: "b" } as never));
expect(moved).toMatchObject({ outcome: "failure", value: "review-pending" });
const result = await primitives.runCodingSession(ctx, TASK, { worktreePath: "/tmp/wt", branchName: "b" } as never);
expect(result).toMatchObject({ outcome: "failure", value: "implementation-incomplete" });
const unmoved = await (harness({ taskDone: false, modifiedFiles: [], exit: "review-handoff-paused-after-completion" })
.primitives.runCodingSession({ run: {}, node: { node: { id: "execute", kind: "prompt" }, context: {} } } as never, TASK, { worktreePath: "/tmp/wt", branchName: "b" } as never));
expect(unmoved).toMatchObject({ outcome: "failure", value: "implementation-incomplete" });
});
/*
@@ -133,3 +141,80 @@ describe("the LIVE implementation primitive announces the exit", () => {
expect(calls.some((c) => c.startsWith("prim:"))).toBe(true);
});
});
/*
FNXC:WorkflowExecutionOwnership 2026-07-29-20:20 (U8 / R4, R12, PR #2590 review — greptile):
The compat path for user-authored graphs, and the shape that proved the first version of it could
not fire. `graphFailureValue` reads only the LAST visited node's value; a custom graph may route
its generic `failure` edge THROUGH another node, whose value then becomes terminal. The
pending-review ending is still recorded in the run context, so that is where it is read from.
Without this the card falls to the terminal park — `status: failed` on work that was only WAITING
for a reviewer, which is exactly the merge-queue deadlock the inline handoff existed to prevent.
*/
describe("compat park for graphs that do not route review-pending", () => {
beforeEach(() => { resetExecutorMocks(); resetWorkflowEventBusForTesting(); });
afterEach(() => resetWorkflowEventBusForTesting());
function failureRun(overrides: Record<string, unknown>) {
return {
disposition: "failed" as const,
outcome: "failure" as const,
visitedNodeIds: ["execute", "cleanup"],
context: overrides,
};
}
function parkHarness() {
const store = createMockStore();
const live = { id: "FN-COMPAT", column: "in-progress", status: null, error: null, steps: [], log: [], paused: false, userPaused: false } as unknown as TaskDetail;
store.getTask.mockResolvedValue(live);
store.handoffToReview = vi.fn().mockImplementation(async (id: string) => store.moveTask(id, "in-review"));
return { store, live, executor: new TaskExecutor(store, "/tmp/test") };
}
it("parks in review when the walk ended on a node that recorded no verdict of its own", async () => {
/* The compat shape: the generic failure edge passes through a node that reports nothing, so
the run's last word is still the implementation node's `review-pending`. */
const { store, live, executor } = parkHarness();
await (executor as never as { handleGraphFailure: (t: unknown, r: unknown) => Promise<void> })
.handleGraphFailure(live, failureRun({ "node:execute:value": "review-pending" }));
expect(store.handoffToReview).toHaveBeenCalledWith("FN-COMPAT", expect.anything());
expect(store.updateTask).not.toHaveBeenCalledWith(
"FN-COMPAT",
expect.objectContaining({ status: "failed" }),
expect.anything(),
);
});
it("does NOT park when a LATER node reported its own failure (stale value must not mask it)", async () => {
/*
FNXC PR #2590 review (greptile, 2nd): the run context is shared for the whole walk, so a graph
that continues past a pending-review node and then dies downstream still carries the earlier
value. Parking on that would hide a real failure behind a wait — the opposite over-reach from
the first finding, and worse, because the operator sees a card waiting for a reviewer who has
nothing to review.
*/
const { store, live, executor } = parkHarness();
await (executor as never as { handleGraphFailure: (t: unknown, r: unknown) => Promise<void> })
.handleGraphFailure(live, failureRun({
"node:execute:value": "review-pending",
"node:cleanup:value": "verification-failed",
}));
expect(store.handoffToReview).not.toHaveBeenCalled();
});
it("does NOT park for an ordinary failure with no pending-review value anywhere", async () => {
/* The guard must stay narrow — a genuine execute failure still belongs to the terminal sink. */
const { store, live, executor } = parkHarness();
await (executor as never as { handleGraphFailure: (t: unknown, r: unknown) => Promise<void> })
.handleGraphFailure(live, failureRun({ "node:execute:value": "implementation-incomplete" }));
expect(store.handoffToReview).not.toHaveBeenCalled();
});
});

View File

@@ -471,7 +471,24 @@ describe("Workflow Steps Execution", () => {
undefined,
expect.objectContaining({ agentId: "executor" }),
);
expect(store.moveTask).toHaveBeenCalledWith("FN-5436-B", "in-review");
/*
FNXC:WorkflowExecutionOwnership 2026-07-29-19:10 (U8 / R4):
FN-5436's invariant is unchanged — a pending-review block still parks the card in review,
never `failed`. What changed is the OWNER. The executor no longer hands off inline; the
graph routes the ending to its `review-pending-handoff` node, which moves the card with
workflow provenance. So this asserts the same outcome plus proof of who produced it, which
is strictly stronger than the old two-argument `moveTask(id, "in-review")` — that shape
could not distinguish a graph-owned park from an out-of-band one, and telling those apart
is the entire point of the unit.
*/
expect(store.moveTask).toHaveBeenCalledWith(
"FN-5436-B",
"in-review",
expect.objectContaining({
workflowMoveSource: "workflow-graph",
workflowMoveMetadata: expect.objectContaining({ nodeId: "review-pending-handoff" }),
}),
);
});
it("keeps existing retry loop when no pending review block is present", async () => {

View File

@@ -7337,6 +7337,18 @@ export class TaskExecutor {
// Best-effort pause probe; fall through to the failure value.
}
}
/*
FNXC:WorkflowExecutionOwnership 2026-07-29-18:45 (U8 / R4):
THE PENDING-REVIEW ENDING IS A ROUTED OUTCOME, not a transition this phase performs. The
implementation phase used to call `handoffTaskToReview` itself and let the graph discover
the move afterwards; it now reports and stops, and this value routes the run to the
workflow's `review-pending-handoff` node, which performs the handoff and ends the run —
the same two effects in the same order, with the graph as the owner. Checked before the
pause probe because a pending-review stop is not a pause.
*/
if (result.exit === "review-handoff-pending-review") {
return { outcome: "failure", value: "review-pending", data: result };
}
return {
outcome: "failure",
value: paused ? "implementation-paused" : "implementation-incomplete",
@@ -9610,6 +9622,46 @@ export class TaskExecutor {
setTimeout(scheduleRetry, delayMs).unref?.();
}
/*
FNXC:WorkflowExecutionOwnership 2026-07-29-20:10 (U8 / R4, PR #2590 review — greptile):
The compat classifier keyed on `graphFailureValue`, which reads only the LAST visited node's
value. That is correct when the generic `failure` edge goes straight to `end` — the built-in
shape — but a user-authored graph may route its generic failure THROUGH another node, and that
node's value then becomes the terminal one. The classifier would miss the pending-review ending
entirely and the card would fall to the terminal park: `status: failed` on work that was only
WAITING for a reviewer, which is the deadlock the inline handoff existed to avoid. A guard that
cannot fire for the exact shape it was written for.
The ending is durable in the run context — the graph publishes `node:<id>:value` for every node
it runs — so detect it there rather than trusting whichever node happened to end the walk.
*/
private graphRunReportedPendingReview(
result: WorkflowGraphTaskRunResult,
failureValue: string | undefined,
): boolean {
if (failureValue === "review-pending") return true;
const context = result.context;
if (!context) return false;
/*
FNXC:WorkflowExecutionOwnership 2026-07-29-21:40 (U8 / R4, PR #2590 review — greptile, 2nd):
Scanning EVERY `node:*:value` was too broad in the opposite direction. The run context is
shared for the whole walk, so a graph that continues past a pending-review node and then dies
on a genuine downstream failure still carries the earlier value — and a blanket scan would
park that card in review, hiding a real failure behind a wait. Trading a guard that misses for
one that over-claims is not a fix.
The narrow rule: the pending-review ending counts only when nothing AFTER it produced its own
verdict. Walk the visited nodes backwards and take the first recorded value — that is the
run's actual last word. If it is `review-pending`, the ending stands; if a later node spoke,
that node's outcome is the run's, and this classifier stays out of the way.
*/
for (let i = result.visitedNodeIds.length - 1; i >= 0; i--) {
const value = context[`node:${result.visitedNodeIds[i]}:value`];
if (typeof value === "string") return value === "review-pending";
}
return false;
}
private graphFailureValue(result: WorkflowGraphTaskRunResult): string | undefined {
const failedNode = result.visitedNodeIds[result.visitedNodeIds.length - 1];
if (!failedNode || !result.context) return undefined;
@@ -10972,6 +11024,24 @@ export class TaskExecutor {
}
const wipColumn = lifecycleIr ? resolveLifecycleColumns(lifecycleIr)?.wip : "in-progress";
const holdColumn = lifecycleIr ? resolveReboundTarget(lifecycleIr) : "todo";
/*
FNXC:WorkflowExecutionOwnership 2026-07-29-18:55 (U8 / R4):
COMPAT PATH for user-authored graphs, deliberately named. Every BUILT-IN shape declares the
`outcome:review-pending` edge, so a built-in run never reaches here — it routed to its park
node and ended. A custom workflow without the edge falls through to its generic `failure`
edge and lands here, where the handoff the implementation phase used to perform inline
happens instead. For those graphs this is a relocation, not an elimination: the transition is
still executor-performed. What changes is that it is one named classifier in the failure
ladder rather than a call buried two thousand lines into a session loop.
*/
if (this.graphRunReportedPendingReview(result, failureValue)) {
const compatMessage = "Implementation stopped on a pending review — parking in review (this workflow does not route the review-pending outcome)";
executorLog.log(`${task.id}: ${compatMessage}`);
await this.store.logEntry(task.id, compatMessage, undefined, this.getRunContextFor(task.id));
await this.handoffTaskToReview(live, "executor-exit-while-review-pending");
await this.persistTokenUsage(task.id);
return;
}
const executeNodeSelfRequeued = failedNode === "execute" && this.graphExecuteSelfRequeued.has(task.id);
if (failedNode === "execute" && ((holdColumn !== undefined && live.column === holdColumn) || executeNodeSelfRequeued)) {
/*
@@ -13692,8 +13762,16 @@ export class TaskExecutor {
// the task in review without setting status=failed; otherwise the
// merge/review queue deadlocks on a task that is both in-review and
// failed.
/*
FNXC:WorkflowExecutionOwnership 2026-07-29-18:50 (U8 / R4):
The `handoffTaskToReview` call that stood here is GONE — the graph performs it via
the `review-pending-handoff` node the live primitive now routes to. What remains is
a report and a stop, which is all an implementation phase should do. Why review and
not `failed` (a pending-review block is a wait; status=failed on an in-review row
deadlocks the merge queue) now lives with the node in the IR, where the routing
decision is.
*/
reportImplementationExit?.("review-handoff-pending-review");
await this.handoffTaskToReview(task, "executor-exit-while-review-pending");
pendingReviewParked = true;
break;
}

View File

@@ -51,9 +51,19 @@ import type { ImplementationExit as CoreImplementationExit } from "@fusion/core"
export type { ImplementationExit } from "@fusion/core";
/** The exits where the EXECUTOR performs the lifecycle transition instead of the graph. */
/*
FNXC:WorkflowExecutionOwnership 2026-07-29-19:20 (U8 / R4):
The ledger of endings the implementation phase still transitions itself, and it SHRINKS as U8
lands routing moves. `review-handoff-pending-review` left it: the phase reports the ending and
stops, and the graph's `review-pending-handoff` node performs the handoff.
Caveat so the list is not read as more than it is: a user-authored graph without the
`outcome:review-pending` edge still gets an executor-performed handoff, from a single named
classifier in `handleGraphFailure` — not from inside the session loop. "Out-of-band" here means
the IMPLEMENTATION PHASE performs it.
*/
export const OUT_OF_BAND_IMPLEMENTATION_EXITS: readonly CoreImplementationExit[] = [
"review-handoff-paused-after-completion",
"review-handoff-pending-review",
];
export function isOutOfBandImplementationExit(exit: CoreImplementationExit | undefined): boolean {

View File

@@ -387,7 +387,18 @@ export function createPrimitivePromptLikeHandler(
active.checkpointId = result.checkpointId;
return {
outcome: result.outcome,
value: result.outcome === "success" ? "step-done" : "step-failed",
/*
FNXC:WorkflowExecutionOwnership 2026-07-29-18:40 (U8 / R4):
`step-done` / `step-failed` was a two-value flattening of every possible ending, and it is
why the pending-review ending could never reach an edge on the stepwise shape. A pass that
stopped because a step is blocked on a pending review is a WAIT, not a step defect: the
outcome stays `failure` (the step genuinely did not complete) while the VALUE names the
ending, which `runForeach` propagates upward as the foreach node's own value so a
`outcome:review-pending` edge can claim it. Every other ending keeps `step-failed`.
*/
value: result.outcome === "success"
? "step-done"
: result.exit === "review-handoff-pending-review" ? "review-pending" : "step-failed",
contextPatch: {
[FOREACH_ACTIVE_CONTEXT_KEY]: active,
},