diff --git a/.changeset/u8-review-pending-graph-owned.md b/.changeset/u8-review-pending-graph-owned.md new file mode 100644 index 0000000000..d349e64609 --- /dev/null +++ b/.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`. 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 dd971845ed..28cef920d6 100644 --- a/packages/engine/src/__tests__/executor-implementation-exit-events.test.ts +++ b/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); 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 e4f775c155..f8038ecc02 100644 --- a/packages/engine/src/__tests__/executor-lifecycle-ownership-ledger.test.ts +++ b/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((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 }); }); }); 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 0de858740b..0d8d1e6fd9 100644 --- a/packages/engine/src/__tests__/executor-primitive-exit-events.test.ts +++ b/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 () => { - 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) { + 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 }) + .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 }) + .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 }) + .handleGraphFailure(live, failureRun({ "node:execute:value": "implementation-incomplete" })); + + expect(store.handoffToReview).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/engine/src/__tests__/executor-step-session.test.ts b/packages/engine/src/__tests__/executor-step-session.test.ts index 6d2f31ab49..b15292536a 100644 --- a/packages/engine/src/__tests__/executor-step-session.test.ts +++ b/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 () => { diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 6a0e21aae2..51e73bf415 100644 --- a/packages/engine/src/executor.ts +++ b/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::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; } diff --git a/packages/engine/src/executor/implementation-exit.ts b/packages/engine/src/executor/implementation-exit.ts index 2294f69777..cf463dc722 100644 --- a/packages/engine/src/executor/implementation-exit.ts +++ b/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 { diff --git a/packages/engine/src/workflow-node-handlers.ts b/packages/engine/src/workflow-node-handlers.ts index df71ffd2bb..e67f40401b 100644 --- a/packages/engine/src/workflow-node-handlers.ts +++ b/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, },