U8: pin the completion-finalize ordering invariant before moving the last out-of-band exit (#2599)

Groundwork for moving `paused-after-completion`, the **last**
out-of-band exit. Stacked on #2590.

## What lands

1. **An indentation defect I introduced.** My bulk edit when the exit
vocabulary landed left the second `paused-after-completion` site
mis-indented inside a `finally` block. Cosmetic, but misleading
indentation in a `finally` is how a future reader misjudges scope.

2. **The adjacency ratchet now requires `markCompletionFinalized` before
the handoff, at every reporting site.** It previously checked only the
first occurrence, and only for the handoff itself.

That ordering is the invariant `handleGraphFailure` depends on and
**cannot check for itself**: `alreadyFinalizedToReview` /
`completionFinalized` exist to recognise this out-of-band move when a
later teardown re-marks the abort as `hard-cancel`. Without the durable
marker set first, a completed no-commit task is re-parked `failed` —
FN-6644/FN-6641.

It is asserted **structurally, and labelled as such in the test**. Both
call sites sit in pause and `finally` paths that cannot be driven
without mocking an entire agent session; presenting a source assertion
as behavioural coverage would repeat the overclaim I have been correctly
pulled up on twice in this unit.

Red-green: removing `markCompletionFinalized` from either site fails the
ratchet.

## Why the move itself is not in this PR

`paused-after-completion` is structurally harder than the pending-review
ending that #2590 moved, and the difference is worth recording before
someone assumes it is a copy-paste:

- it does **four** things, not one — `markCompletionFinalized`,
`handoffTaskToReview`,
`clearCompletedTaskWatchdog`/`signalTaskComplete`. Only the handoff is
lifecycle; the rest is substrate that must stay put.
- one of the two sites is inside a **`finally`**. Moving a transition
out of a `finally` is not the same operation as moving one out of a
branch: the graph may already be unwinding, so "report and let the graph
route" needs a defined answer for a run that is already ending.
- there is **no behavioural coverage of either site today** — the
closest tests only exercise the exit vocabulary. The pending-review move
succeeded on the fourth attempt precisely because FN-5436 existed to
catch each wrong version; this exit has no equivalent, so the move needs
that floor built first, and building it means real session mocking
rather than a shortcut.

## Verification

- exit-events + primitive-exit-events + step-session + ownership ledger
— green
- `pnpm lint` clean; `tsc --noEmit` clean
- No user-facing behaviour change, so no changeset

🤖 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 workflow steps that pause for review.
- Tasks now remain in review when a review request has no subsequent
decision.
  - Added clearer completion events for primitive prompt steps.
  - Preserved correct failure handling when later workflow steps fail.

- **Workflow Improvements**
- Built-in workflows now route pending reviews through a dedicated
review handoff.
- User-authored workflows retain compatible review parking behavior when
routing is unavailable.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

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

@@ -176,8 +176,30 @@ describe("execute seam announces the implementation phase's exit", () => {
     expect(missing).toEqual([]);
     /* Each out-of-band id must accompany an inline review handoff — that pairing IS its meaning. */
     for (const exit of OUT_OF_BAND_IMPLEMENTATION_EXITS) {
-      const idx = source.indexOf(`reportImplementationExit?.("${exit}")`);
-      expect(source.slice(idx, idx + 400)).toContain("handoffTaskToReview(");
+      const occurrences = [...source.matchAll(new RegExp(`reportImplementationExit\\?\\.\\("${exit}"\\)`, "g"))];
+      expect(occurrences.length, `${exit} should be reported at every site that performs it`).toBeGreaterThan(0);
+      for (const match of occurrences) {
+        const idx = match.index ?? 0;
+        /*
+        FNXC:WorkflowExecutionOwnership 2026-07-30-10:25 (PR #2599 review — greptile):
+        WINDOW-FREE. A fixed 400-character lookaround was wrong in both directions: an unrelated
+        occurrence inside it let a missing marker pass, and harmless intervening instrumentation
+        pushed a valid pairing out of range. Ordering is asserted by INDEX instead — the marker
+        that precedes this report must be nearer to it than any earlier handoff is, which is what
+        "this site's own marker" means, and the handoff must follow the report.
+        */
+        const markerIdx = source.lastIndexOf("markCompletionFinalized(", idx);
+        const priorHandoffIdx = source.lastIndexOf("handoffTaskToReview(", idx);
+        expect(markerIdx, `${exit} must set the durable completion-finalize marker before handing off`).toBeGreaterThan(-1);
+        expect(
+          markerIdx,
+          `${exit}'s completion-finalize marker must belong to this site, not an earlier one`,
+        ).toBeGreaterThan(priorHandoffIdx);
+        expect(
+          source.indexOf("handoffTaskToReview(", idx),
+          `${exit} must be followed by the review handoff it describes`,
+        ).toBeGreaterThan(idx);
+      }
     }
   });
 

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

@@ -180,10 +180,13 @@ const LEDGER = {
     "graph handbacks (graphCompletion)": 3,
   },
   /*
-  handleGraphFailure moves NO card itself and hands off to NO review — every disposition it
-  owns is a terminal park. That is a genuinely better starting position than the
-  implementation phase, and it is measured, not assumed: the moveTask calls that look like
-  they belong to this method sit past its closing brace, in the recovery helpers below it.
+  handleGraphFailure moves NO card itself, and owned NO review handoff at baseline — every
+  disposition was a terminal park. It now carries exactly ONE review handoff: the named compat
+  path for user-authored graphs that do not declare the `outcome:review-pending` edge. For those
+  shapes the pending-review transition is RELOCATED here from the session loop, not eliminated,
+  and this entry going 0 -> 1 while `runImplementation` went 3 -> 2 is the honest record of that.
+  The zero moveTask count is measured, not assumed: the calls that look like they belong to this
+  method sit past its closing brace, in the recovery helpers below it.
   */
   handleGraphFailure: {
     "column transitions (store.moveTask)": 0,

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

@@ -206,6 +206,18 @@ describe("compat park for graphs that do not route review-pending", () => {
       }));
 
     expect(store.handoffToReview).not.toHaveBeenCalled();
+    /*
+    FNXC:WorkflowExecutionOwnership 2026-07-30-10:40 (PR #2599 review — coderabbit):
+    Assert the DISPOSITION, not only the absence of a park. "No review handoff" passes just as
+    well for a run that silently did nothing, which is the failure mode this whole file exists to
+    catch. Verified against the real path rather than assumed: both cases reach the terminal sink
+    and park with the failure of the node that actually ended the walk.
+    */
+    expect(store.updateTask).toHaveBeenCalledWith(
+      "FN-COMPAT",
+      expect.objectContaining({ status: "failed", error: expect.stringContaining("terminated with failure at node 'cleanup'") }),
+      undefined,
+    );
   });
 
   it("does NOT park for an ordinary failure with no pending-review value anywhere", async () => {
@@ -216,5 +228,69 @@ describe("compat park for graphs that do not route review-pending", () => {
       .handleGraphFailure(live, failureRun({ "node:execute:value": "implementation-incomplete" }));
 
     expect(store.handoffToReview).not.toHaveBeenCalled();
+    /*
+    FNXC:WorkflowExecutionOwnership 2026-07-30-10:40 (PR #2599 review — coderabbit):
+    Assert the DISPOSITION, not only the absence of a park. "No review handoff" passes just as
+    well for a run that silently did nothing, which is the failure mode this whole file exists to
+    catch. Verified against the real path rather than assumed: both cases reach the terminal sink
+    and park with the failure of the node that actually ended the walk.
+    */
+    expect(store.updateTask).toHaveBeenCalledWith(
+      "FN-COMPAT",
+      expect.objectContaining({ status: "failed", error: expect.stringContaining("terminated with failure at node 'cleanup'") }),
+      undefined,
+    );
+  });
+});
+
+/*
+FNXC:WorkflowExecutionOwnership 2026-07-30-10:55 (PR #2599 review — coderabbit, major):
+A visited node id is not always the context key its value lives under. A foreach instance
+(`steps#0:step-execute`) records under the CONTAINER key `node:steps:value`. The default coding
+workflow IS a foreach, so a backward walk reading `node:<visitedId>:value` directly misses the
+pending-review ending on exactly the shape it was written for — and then keeps walking, adopting
+some earlier node's value as the run's verdict.
+*/
+describe("compat detection resolves foreach and optional-group context keys", () => {
+  beforeEach(() => { resetExecutorMocks(); resetWorkflowEventBusForTesting(); });
+  afterEach(() => resetWorkflowEventBusForTesting());
+
+  function parkHarness() {
+    const store = createMockStore();
+    const live = { id: "FN-FE", 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("finds a FOREACH instance's ending under its container key", async () => {
+    const { store, live, executor } = parkHarness();
+
+    await (executor as never as { handleGraphFailure: (t: unknown, r: unknown) => Promise<void> })
+      .handleGraphFailure(live, {
+        disposition: "failed" as const,
+        outcome: "failure" as const,
+        /* The walk must END elsewhere, or `graphFailureValue`'s own resolution answers first and
+           the backward walk is never exercised — which is how my first version of this test
+           passed with the fix reverted. */
+        visitedNodeIds: ["steps#0:step-execute", "cleanup"],
+        context: { "node:steps:value": "review-pending" },
+      });
+
+    expect(store.handoffToReview).toHaveBeenCalledWith("FN-FE", expect.anything());
+  });
+
+  it("finds an OPTIONAL-GROUP template ending under the group key", async () => {
+    const { store, live, executor } = parkHarness();
+
+    await (executor as never as { handleGraphFailure: (t: unknown, r: unknown) => Promise<void> })
+      .handleGraphFailure(live, {
+        disposition: "failed" as const,
+        outcome: "failure" as const,
+        visitedNodeIds: ["group::template", "cleanup"],
+        context: { "node:group:value": "review-pending" },
+      });
+
+    expect(store.handoffToReview).toHaveBeenCalledWith("FN-FE", expect.anything());
   });
 });

packages/engine/src/executor.ts

@@ -9656,12 +9656,40 @@ export class TaskExecutor {
     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`];
+      const value = this.recordedNodeValue(context, result.visitedNodeIds[i]);
       if (typeof value === "string") return value === "review-pending";
     }
     return false;
   }
 
+  /*
+  FNXC:WorkflowExecutionOwnership 2026-07-30-10:10 (U8, PR #2599 review — coderabbit, major):
+  A visited node id does NOT always name the context key its value is stored under, and the two
+  shapes that differ are the ones this unit cares about most. A foreach instance
+  (`steps#0:step-execute`) records under the CONTAINER key `node:steps:value`; an optional-group
+  template (`group::template`) records under the group key, then the template key. Reading
+  `node:<visitedId>:value` directly therefore misses a foreach ending and walks on to some
+  earlier node's value — and the default coding workflow IS a foreach, so the backward walk
+  would have misread precisely the shape it was written for.
+
+  Extracted from `graphFailureValue`, which already knew this, so the two cannot drift apart.
+  */
+  private recordedNodeValue(context: Record<string, unknown>, nodeId: string): string | undefined {
+    const direct = context[`node:${nodeId}:value`];
+    if (typeof direct === "string") return direct;
+    const groupDelimiter = nodeId.indexOf("::");
+    if (groupDelimiter !== -1) {
+      const groupValue = context[`node:${nodeId.slice(0, groupDelimiter)}:value`];
+      if (typeof groupValue === "string") return groupValue;
+      const templateValue = context[`node:${nodeId.slice(groupDelimiter + 2)}:value`];
+      return typeof templateValue === "string" ? templateValue : undefined;
+    }
+    const foreachDelimiter = nodeId.indexOf("#");
+    if (foreachDelimiter === -1) return undefined;
+    const containerValue = context[`node:${nodeId.slice(0, foreachDelimiter)}:value`];
+    return typeof containerValue === "string" ? containerValue : undefined;
+  }
+
   private graphFailureValue(result: WorkflowGraphTaskRunResult): string | undefined {
     const failedNode = result.visitedNodeIds[result.visitedNodeIds.length - 1];
     if (!failedNode || !result.context) return undefined;