From 8578a1d27d075b8a4dd9767aef18a0d22cc4d730 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 29 Jul 2026 09:54:39 -0700 Subject: [PATCH] U8 PR5: thread the implementation exit to the step seam, and declare the stepwise pending-review park (inert) (#2546) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follows **#2519** (U8 PR4). Both halves are inert — **no behavior change** — and this removes the blocker PR4 documented. ## What was blocking PR4 could only land its IR half because the pending-review ending could not reach a graph edge on the **default** workflow. Three links in the chain: | Link | Problem | |---|---| | `runGraphTaskStep` | awaited the memoized implementation pass and **discarded** its result | | `RunTaskStepResult` / `RunSingleStep` | had nowhere to carry an exit | | `stepExecute` seam | flattened every ending to `step-done` / `step-failed` | All three are fixed. The outcome stays `failure` (the step genuinely did not complete) while the **value** now names the ending — which is what `runForeach` propagates upward, since it returns a failing instance's value as the foreach node's own. Every other ending keeps `step-failed` byte-identically. One design note: the exit is a property of the **pass**, not of a step. A single memoized pass serves every foreach instance, so all instances report the same ending — correct, because the ending is what stopped the whole session. With the value surviving, the stepwise IR declares the same `review-handoff` park node and `steps --outcome:review-pending--> review-pending-handoff --success--> end` edge the plain-`execute` shape got in PR4, inherited by the final-review and Ideas variants that clone it. ## A bug my own threading introduced, and what caught it The first threading commit covered **one of the two** paths out of `runProjectedGraphTaskStep`. The early-return branch carried the exit; the main path goes through `runTaskStep` in `step-runner.ts`, which builds its own result and dropped it — i.e. it worked on the path I happened to read, and not on the path the default workflow actually takes. **FN-5436's regression test caught it, not code review.** That is the second time this test has stood between this unit and a silent regression, which is worth recording somewhere durable: `executor-step-session.test.ts > FN-5436: pending-review skip on no-fn_task_done exit` is the load-bearing test for this area. ## Why the seam flip is still not here With the threading complete I applied the behavior half again — flip the execute seam to return `review-pending`, delete the inline `handoffTaskToReview`, add a named compat classifier for user-authored graphs. **FN-5436 still failed**: the card did not reach `in-review`, so something between the seam value and the park node is not routing under that harness. I have not isolated whether that is the mock store's IR resolution (it exposes no `getWorkflowDefinition`, so the run resolves the built-in through a different path), a foreach aggregation detail, or the park node's own seam. I stopped rather than keep guessing, and reverted the behavior edits so this lands green and inert. Shipping a half-routed move is exactly the failure this unit exists to remove — a lifecycle transition that silently does not happen. The alternative on offer was to relax FN-5436's assertion, which would have been appeasing a test that is telling the truth. ### What the instrumentation showed (done after opening this PR) I ran the bounded next step rather than leaving it as a note. Two facts, both measured: 1. **The IR is correct.** Resolving `BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR` at runtime shows the node and the edge survive the final-review variant's edge rewiring: ``` EDGES [{"from":"steps","to":"browser-verification","condition":"success"}, {"from":"steps","to":"review-pending-handoff","condition":"outcome:review-pending"}, {"from":"steps","to":"end","condition":"failure"}] HAS NODE true ``` That matters because the variant does `template.edges = [ ... ]` (a wholesale replacement) and filters outer edges touching `review` — `review-pending-handoff` is not `review`, so it survives. Worth knowing before anyone adds another node near it. 2. **The `stepExecute` seam is never invoked in that harness**, even though the run terminates at `steps#0:step-execute` and the implementation session demonstrably runs (`"Agent finished without calling fn_task_done but Step 0 is blocked on pending review"` is in the task log). A `console.log` at the seam's value computation produced no output. So the exit is threaded correctly and the IR can route it, but under this harness the value never originates. 3. **Nor is `createPromptLikeHandler`'s returned handler.** Instrumenting its dispatch (`node.id` + resolved seam) produced nothing either — so the node is not reaching the prompt-like path at all. **Control experiment, because a negative result from instrumentation is worthless until you prove the instrumentation is observable.** A `process.stderr.write` at module load of the same file appears exactly once in the same run, so writes from that module *are* captured under this harness and the two negatives above are real, not artifacts of swallowed output. That narrows the remaining work to one question — what actually drives `steps#0:step-execute` in this run, if neither the prompt-like handler nor the `stepExecute` seam does — and rules out the IR, the foreach propagation, the threading, and the instrumentation as suspects. **Next step, now much narrower:** find the handler registration this run resolves for a foreach instance node (the graph executor's handler map, not the seam table), then flip the seam, delete the inline handoff, and update the three ratchets that will correctly fire — PR3's routing pin, the out-of-band adjacency check, and PR1's ownership ledger (`runImplementation` 3 → 2; `handleGraphFailure` 0 → 1 for custom graphs only). ## Verification - `executor-step-session` + exit-events + ownership ledger + graph-boundary — **56 tests green** - `builtin-workflows` + `builtin-coding-workflow-ir` — green. The layout-completeness contract required a layout entry for the new node in all four stepwise-derived workflows; placed off the main line, because a park is an exit and not a stage. - `pnpm test:gate` green (10 / 309 / 71); `pnpm lint` clean; `tsc --noEmit` clean - Changeset included (`patch`, `internal`) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- .changeset/u8-step-session-exit-threading.md | 7 +++ .../builtin-stepwise-coding-workflow-ir.ts | 28 ++++++++++++ packages/core/src/builtin-workflows.ts | 8 ++++ packages/engine/src/executor.ts | 44 ++++++++++++++++--- packages/engine/src/step-runner.ts | 24 +++++++++- 5 files changed, 103 insertions(+), 8 deletions(-) create mode 100644 .changeset/u8-step-session-exit-threading.md diff --git a/.changeset/u8-step-session-exit-threading.md b/.changeset/u8-step-session-exit-threading.md new file mode 100644 index 0000000000..5345a6ff41 --- /dev/null +++ b/.changeset/u8-step-session-exit-threading.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Add a Park for pending review step to the stepwise coding workflows, visible in the workflow editor. +category: internal +dev: Threads `ImplementationExit` through `runGraphTaskStep` -> `RunTaskStepResult`/`RunSingleStep` -> `runProjectedGraphTaskStep` -> `stepExecute`, which no longer flattens every ending to `step-done`/`step-failed`; adds the `review-pending-handoff` node plus `steps --outcome:review-pending--> ... --> end` to the stepwise IR (inherited by the final-review and Ideas variants). Inert: no seam returns `review-pending` yet. diff --git a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts index e631df9412..44e80cd642 100644 --- a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts +++ b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts @@ -257,6 +257,25 @@ const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { // browser-verification above. codeReviewOptionalGroupNode("in-review"), codeReviewRemediationNode("in-progress"), + /* + FNXC:WorkflowExecutionOwnership 2026-07-29-11:40 (U8 / R4 — workflow-owned lifecycle): + THE PENDING-REVIEW PARK. An implementation pass can stop because a step is blocked on a + pending review: the agent cannot continue, and the card belongs in review rather than in an + error bucket (`status: failed` on an `in-review` row deadlocks the merge queue). The executor + used to perform that transition inline, mid-session, and the graph found out afterwards. + + A `review-handoff` seam (pure lifecycle handoff, no reviewer invocation) whose ONLY edge is to + `end` — hand off and STOP, which is what the inline handoff did. Routing to the merge path + instead would carry work whose steps are incomplete into merge-gate. + + Inherited by the final-review and Ideas variants, which clone this IR. + */ + { + id: "review-pending-handoff", + kind: "prompt", + column: "in-review", + config: builtinPromptConfig("review-handoff", "Park for pending review"), + }, completionSummaryNode("in-review"), { id: "merge-gate", kind: "merge-gate", column: "in-review", config: { gate: "auto-merge" } }, { id: "merge-retry", kind: "retry-backoff", column: "in-review", config: { policy: "merge", maxAttempts: 3 } }, @@ -310,6 +329,15 @@ const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { { from: "browser-verification-remediation", to: "browser-verification", condition: "success", kind: "rework" }, { from: "code-review", to: "code-review-remediation", condition: "failure" }, { from: "code-review-remediation", to: "code-review", condition: "success", kind: "rework" }, + /* + FNXC:WorkflowExecutionOwnership 2026-07-29-11:45 (U8 / R4): + `outcome:` edges match on the node's VALUE and take priority over the generic failure edge + below, so this claims ONLY the pending-review ending. `runForeach` returns a failing + instance's value as the foreach node's own, which is what carries `review-pending` from the + `step-execute` seam up to this edge. + */ + { from: "steps", to: "review-pending-handoff", condition: "outcome:review-pending" }, + { from: "review-pending-handoff", to: "end", condition: "success" }, { from: "steps", to: "end", condition: "failure" }, { from: "merge-gate", to: "branch-group-member-integration", condition: "outcome:auto-on" }, { from: "merge-gate", to: "merge-manual-hold", condition: "outcome:auto-off" }, diff --git a/packages/core/src/builtin-workflows.ts b/packages/core/src/builtin-workflows.ts index ce3f5f9977..5851f7466a 100644 --- a/packages/core/src/builtin-workflows.ts +++ b/packages/core/src/builtin-workflows.ts @@ -455,6 +455,8 @@ export const BUILTIN_WORKFLOWS: WorkflowDefinition[] = [ "plan-replan": { x: 400, y: 320 }, parse: { x: 570, y: 160 }, steps: { x: 740, y: 160 }, + /* U8: the pending-review park is an exit, not a stage — placed off the main line. */ + "review-pending-handoff": { x: 740, y: 320 }, "browser-verification": { x: 910, y: 160 }, "browser-verification-remediation": { x: 910, y: 320 }, "code-review": { x: 1080, y: 160 }, @@ -491,6 +493,8 @@ export const BUILTIN_WORKFLOWS: WorkflowDefinition[] = [ "plan-replan": { x: 400, y: 320 }, parse: { x: 570, y: 160 }, steps: { x: 740, y: 160 }, + /* U8: the pending-review park is an exit, not a stage — placed off the main line. */ + "review-pending-handoff": { x: 740, y: 320 }, "browser-verification": { x: 910, y: 160 }, "browser-verification-remediation": { x: 910, y: 320 }, "code-review": { x: 1080, y: 160 }, @@ -717,6 +721,8 @@ export const BUILTIN_WORKFLOWS: WorkflowDefinition[] = [ "plan-replan": { x: 400, y: 320 }, parse: { x: 570, y: 160 }, steps: { x: 740, y: 160 }, + /* U8: the pending-review park is an exit, not a stage — placed off the main line. */ + "review-pending-handoff": { x: 740, y: 320 }, "rework-hold": { x: 740, y: 320 }, "browser-verification": { x: 910, y: 160 }, "browser-verification-remediation": { x: 910, y: 320 }, @@ -855,6 +861,8 @@ export const BUILTIN_WORKFLOWS: WorkflowDefinition[] = [ "plan-replan": { x: 910, y: 320 }, parse: { x: 1080, y: 160 }, steps: { x: 1250, y: 160 }, + /* U8: the pending-review park is an exit, not a stage — placed off the main line. */ + "review-pending-handoff": { x: 1250, y: 320 }, "browser-verification": { x: 1420, y: 160 }, "browser-verification-remediation": { x: 1420, y: 320 }, "code-review": { x: 1590, y: 160 }, diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index b41579a7ea..967c44e4c4 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -5759,7 +5759,7 @@ export class TaskExecutor { * the (step-session) implementation exactly once per run and lets later step * instances observe the projection rather than re-running execute() per step. * Keyed by task id; cleared alongside the pin. */ - private graphStepRunOnce = new Map>(); + private graphStepRunOnce = new Map>(); /** Step-inversion (KTD-4): the foreach instance the step-execute seam is * currently driving for a graph-owned task, so `runGraphTaskStep` can honor @@ -7027,7 +7027,7 @@ export class TaskExecutor { governingNodeId?: string, thinkingLevel?: ThinkingLevel, skillName?: string, - ): Promise<{ success: boolean; error?: string }> { + ): Promise<{ success: boolean; error?: string; exit?: ImplementationExit }> { const active = this.foreachActiveForTask(task.id, instanceId); /* FNXC:WorkflowStepSessions 2026-06-30-00:00: @@ -7081,8 +7081,16 @@ export class TaskExecutor { } }); } + /* + FNXC:WorkflowExecutionOwnership 2026-07-29-11:20 (U8 / R4): + The memoized pass's result was awaited and DISCARDED here — which is exactly where the + implementation exit died. One pass serves every foreach instance, so the exit is a property + of the pass, not of a step: each instance reports the same ending, which is correct because + the ending is what stopped the whole session. + */ + let phaseResult: { taskDone: boolean; modifiedFiles: string[]; exit?: ImplementationExit } | undefined; try { - await phase; + phaseResult = await phase; } catch (err) { // Clear the poisoned memo so a rework cycle can retry the implementation // (only if it is still the same rejected promise — do not clobber a fresh @@ -7121,15 +7129,27 @@ export class TaskExecutor { }; } const status = live.steps[stepIndex]?.status; - if (status === "done" || status === "skipped") return { success: true }; + /* + FNXC:WorkflowExecutionOwnership 2026-07-29-14:10 (U8 / R4, PR #2546 review — greptile P2): + Carry the pass's ending on the SUCCESS returns too. One pass serves every foreach instance, + so "this step completed" and "the pass stopped on a pending-review block" are independent + facts and both can hold. Reporting only on failure made the exit branch-dependent: with + `deferDoneToReview` every instance returns success, so the ending would never reach the seam + and the graph-owned park would be unreachable for that shape. + + The seam still routes it only on FAILURE — a genuinely completed step must not be diverted + to the park — so this is inert today and correct once the seam flip lands. + */ + if (status === "done" || status === "skipped") return { success: true, exit: phaseResult?.exit }; // Step not terminal after the pass: when a review will author done-ness // (deferDoneToReview), the pass having RUN is the success signal — the review // gates the projection write. Otherwise the implementation pass failed to // complete this step, so report failure rather than masking it (FIX 3: the // prior code returned success on both branches, hiding step-session failures). - if (active?.deferDoneToReview === true) return { success: true }; + if (active?.deferDoneToReview === true) return { success: true, exit: phaseResult?.exit }; return { success: false, + exit: phaseResult?.exit, error: `step ${stepIndex} not completed by implementation pass (status: ${status ?? "unknown"})`, }; } catch (err) { @@ -7202,6 +7222,7 @@ export class TaskExecutor { outcome: result.success ? "success" : "failure", baselineSha: refreshed.baseCommitSha, checkpointId: undefined, + exit: result.exit, }; } @@ -8021,9 +8042,20 @@ export class TaskExecutor { // foreach sub-walk threads them to later template nodes (step-review/reset). active.baselineSha = result.baselineSha; active.checkpointId = result.checkpointId; + /* + FNXC:WorkflowExecutionOwnership 2026-07-29-11:30 (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 + blocked-on-pending-review pass is a WAIT, not a step defect: the outcome stays `failure` + (the step genuinely did not complete) while the VALUE names the ending, which is what the + foreach propagates upward — `runForeach` returns a failing instance's value as its own — + so the `steps` node can carry an `outcome:review-pending` edge to the park node. + Every other ending keeps `step-failed` exactly as before. + */ + const failureValue = result.exit === "review-handoff-pending-review" ? "review-pending" : "step-failed"; return { outcome: result.outcome, - value: result.outcome === "success" ? "step-done" : "step-failed", + value: result.outcome === "success" ? "step-done" : failureValue, contextPatch: { [FOREACH_ACTIVE_CONTEXT_KEY]: active, }, diff --git a/packages/engine/src/step-runner.ts b/packages/engine/src/step-runner.ts index 761dccc67c..3f69367430 100644 --- a/packages/engine/src/step-runner.ts +++ b/packages/engine/src/step-runner.ts @@ -30,6 +30,7 @@ import { existsSync, statSync } from "node:fs"; import { promisify } from "node:util"; import type { TaskStore } from "@fusion/core"; +import type { ImplementationExit } from "@fusion/core"; const execAsync = promisify(exec); @@ -56,7 +57,7 @@ export interface SessionRef { * for a single step (graph-owned runs force step-session physics, KTD-2/KTD-8); * tests inject a fake. Returns whether the step's session completed successfully. */ -export type RunSingleStep = (stepIndex: number) => Promise<{ success: boolean; error?: string }>; +export type RunSingleStep = (stepIndex: number) => Promise<{ success: boolean; error?: string; exit?: ImplementationExit }>; // ── runTaskStep ───────────────────────────────────────────────────────── @@ -109,6 +110,17 @@ export interface RunTaskStepResult { outcome: "success" | "failure"; baselineSha?: string; checkpointId?: string; + /* + FNXC:WorkflowExecutionOwnership 2026-07-29-11:10 (U8 / R4 — workflow-owned lifecycle): + How the shared implementation pass ENDED, when that is finer than this step's outcome. + A pass can stop because a step is blocked on a pending review: every instance then reports + `failure`, but the ending is a WAIT, not a step defect, and the graph routes the two + differently. Without carrying it here the distinction dies at the `stepExecute` seam, which + flattens every ending to `step-done` / `step-failed` — so no edge can ever see it and the + transition has to be performed out of band instead. + Absent for every ordinary step outcome; the value space is `@fusion/core`'s ImplementationExit. + */ + exit?: ImplementationExit; } /** @@ -199,7 +211,15 @@ export async function runTaskStep( return { outcome: "success", baselineSha, checkpointId }; } - return { outcome: "failure", baselineSha, checkpointId }; + /* + FNXC:WorkflowExecutionOwnership 2026-07-29-12:40 (U8 / R4): + Carry the pass's ending outward. `runStep` is the graph's step driver, and a failure here can + mean two different things — the step did not complete, or the whole implementation pass stopped + on a WAIT (blocked on a pending review). The `stepExecute` seam routes those differently, so + dropping the exit at this boundary is what previously forced the wait to be transitioned out of + band. Absent for every ordinary step failure. + */ + return { outcome: "failure", baselineSha, checkpointId, exit: result.exit }; } // ── resetStepToBaseline ──────────────────────────────────────────────────