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;