From 45e8b5f7ac7e8b13319d90c50ee74cd82403e92f Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 29 Jul 2026 21:04:39 -0700 Subject: [PATCH] U8: pin the completion-finalize ordering invariant before moving the last out-of-band exit (#2599) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) ## 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. Co-authored-by: Claude Opus 5 (1M context) --- ...xecutor-implementation-exit-events.test.ts | 26 ++++++- ...xecutor-lifecycle-ownership-ledger.test.ts | 11 ++- .../executor-primitive-exit-events.test.ts | 76 +++++++++++++++++++ packages/engine/src/executor.ts | 30 +++++++- 4 files changed, 136 insertions(+), 7 deletions(-) diff --git a/packages/engine/src/__tests__/executor-implementation-exit-events.test.ts b/packages/engine/src/__tests__/executor-implementation-exit-events.test.ts index 28cef920d6..b391ed77b1 100644 --- a/packages/engine/src/__tests__/executor-implementation-exit-events.test.ts +++ b/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); + } } }); diff --git a/packages/engine/src/__tests__/executor-lifecycle-ownership-ledger.test.ts b/packages/engine/src/__tests__/executor-lifecycle-ownership-ledger.test.ts index f8038ecc02..81a8f226db 100644 --- a/packages/engine/src/__tests__/executor-lifecycle-ownership-ledger.test.ts +++ b/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, diff --git a/packages/engine/src/__tests__/executor-primitive-exit-events.test.ts b/packages/engine/src/__tests__/executor-primitive-exit-events.test.ts index 0d8d1e6fd9..0a7ed7e714 100644 --- a/packages/engine/src/__tests__/executor-primitive-exit-events.test.ts +++ b/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::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 }) + .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 }) + .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()); }); }); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 51e73bf415..66a529f351 100644 --- a/packages/engine/src/executor.ts +++ b/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::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, 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;