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>

.changeset/u8-review-pending-graph-owned.md

@@ -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`.

packages/engine/src/__tests__/executor-implementation-exit-events.test.ts

@@ -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);

packages/engine/src/__tests__/executor-lifecycle-ownership-ledger.test.ts

@@ -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 });
   });
 });

packages/engine/src/__tests__/executor-primitive-exit-events.test.ts

@@ -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 () => {
+  it("routes the pending-review ending, and leaves every other ending's value alone", async () => {
-    const { primitives, ctx } = harness({ taskDone: false, modifiedFiles: [], exit: "review-handoff-pending-review" });
+    /*
+    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);
+    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(result).toMatchObject({ outcome: "failure", value: "implementation-incomplete" });
+    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();
+  });
+});

packages/engine/src/__tests__/executor-step-session.test.ts

@@ -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 () => {

packages/engine/src/executor.ts

@@ -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;
               }

packages/engine/src/executor/implementation-exit.ts

@@ -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 {

packages/engine/src/workflow-node-handlers.ts

@@ -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,
         },