From 5de083ef08087e06571e3a05f2a5d6ff8cf3ee58 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Tue, 28 Jul 2026 22:19:28 -0700 Subject: [PATCH] =?UTF-8?q?U8=20PR4:=20declare=20the=20pending-review=20pa?= =?UTF-8?q?rk=20as=20a=20graph=20node=20(inert)=20=E2=80=94=20and=20why=20?= =?UTF-8?q?the=20behavior=20move=20is=20blocked=20on=20the=20step-session?= =?UTF-8?q?=20chain=20(#2519)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth PR of **U8 — the graph owns execution**. This is the IR half of the pending-review routing move. **Inert: no behavior change.** The behavior half is deliberately NOT in this PR, for a measured reason below. ## What lands A `review-handoff` seam node (`review-pending-handoff`, column `in-review`) in `BUILTIN_CODING_WORKFLOW_IR`, with: ``` execute --outcome:review-pending--> review-pending-handoff --success--> end ``` An implementation session can end 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). Today the **executor** performs that transition inline, mid-session, and the graph finds out afterwards — which is why `handleGraphFailure` carries `alreadyFinalizedToReview`, a classifier whose only job is recognising a move the graph did not make. Two design points worth recording, both verified against the interpreter rather than assumed: - **The edge goes to `end`, not to `review`.** Routing to the ordinary `review` node would have continued the run into `merge-gate` and `merge-attempt` on work whose steps are incomplete. "Hand off and stop" is what the inline handoff does; the edge to `end` is what preserves it. - **`outcome:` edges match on the node's VALUE and take priority over generic `success`/`failure` edges** (`shouldTraverseEdge` / `traverseChildren`). So this claims only the pending-review ending, and a workflow that does not declare the edge falls through to its generic `failure` edge — exactly today's behavior. That is what makes the eventual move safe for user-authored graphs. ## Why the behavior half is not here — a measured finding I implemented it, and backed it out. The record matters more than the diff: 1. **`BUILTIN_CODING_WORKFLOW_IR` is not the default workflow.** It backs `builtin:legacy-coding`; `builtin:coding` uses the *stepwise-final-review* IR, which has no `execute` node — its implementation runs as a `foreach` of `step-execute`. 2. **The foreach mechanism would work.** `runForeach` propagates a failing instance's `value` up as the foreach node's own value, so a `steps` node could carry an `outcome:review-pending` edge. 3. **But `stepExecute` flattens it first.** The seam returns `value: result.outcome === "success" ? "step-done" : "step-failed"`, discarding the exit before it can reach any edge. So on the default workflow the exit cannot reach an edge, and a compat classifier in `handleGraphFailure` keyed on the failure value cannot see it either. **Removing the inline handoff therefore regressed the default path**: the card stopped reaching `in-review` at all. `executor-step-session.test.ts`'s FN-5436 case caught it — ``` FAIL FN-5436: pending-review skip on no-fn_task_done exit > parks in-review when review request has no subsequent verdict expected "moveTask" to be called with [ 'FN-5436-B', 'in-review' ] Number of calls: 0 ``` I could have made that green by relaxing the assertion. That would have been appeasement of a test that was telling the truth, so the behavior commit came out instead. **Also caught, and worth noting as the ratchets earning their keep:** the PR1 ownership ledger flagged the change as `runImplementation` 3 → 2 review handoffs and `handleGraphFailure` 0 → 1 — i.e. a *relocation*, not an elimination, for every non-plain-`execute` shape. That number is what turned "this move is good" into "this move is only good for one workflow shape". And PR3's routing-unchanged pin plus its out-of-band adjacency ratchet both fired, forcing the routing change to be declared rather than slipping in. ## PR5 Thread the implementation exit through the step-session chain (`runImplementationPhase` → `graphStepRunOnce` → `runGraphTaskStep` → `runProjectedGraphTaskStep` → `stepExecute`) so the seam can return `review-pending` instead of flattening to `step-failed`; add the node + edge to the stepwise IRs; then flip the execute seam and delete the inline handoff **in one correct step** for every built-in shape at once. The compat path for user-authored graphs is then a single named classifier rather than a call buried two thousand lines into a session loop. ## Verification - `builtin-coding-workflow-ir` + `builtin-workflows` — 76 tests green (the layout-completeness contract required a layout entry for the new node; it is placed off the main line because the park is an exit, not a stage) - `executor-step-session` + ownership ledger + exit events — 50 tests green, unchanged - `pnpm test:gate` green (309/10/71); `pnpm lint` clean - Changeset included (`patch`, `internal`) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- .changeset/u8-review-pending-park-node.md | 7 ++++ .../core/src/builtin-coding-workflow-ir.ts | 34 +++++++++++++++++++ packages/core/src/builtin-workflows.ts | 2 ++ 3 files changed, 43 insertions(+) create mode 100644 .changeset/u8-review-pending-park-node.md diff --git a/.changeset/u8-review-pending-park-node.md b/.changeset/u8-review-pending-park-node.md new file mode 100644 index 0000000000..f740d0174d --- /dev/null +++ b/.changeset/u8-review-pending-park-node.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Declare the pending-review park as a step in the Legacy coding workflow, so it is visible in the editor. +category: internal +dev: Adds a `review-handoff` seam node (`review-pending-handoff`) plus `execute --outcome:review-pending--> review-pending-handoff --success--> end` to BUILTIN_CODING_WORKFLOW_IR. Inert — no seam returns `review-pending` yet; the behavior move lands separately once the step-session chain can surface the exit. diff --git a/packages/core/src/builtin-coding-workflow-ir.ts b/packages/core/src/builtin-coding-workflow-ir.ts index ec1966c8f2..cd2448a46f 100644 --- a/packages/core/src/builtin-coding-workflow-ir.ts +++ b/packages/core/src/builtin-coding-workflow-ir.ts @@ -100,6 +100,29 @@ const RAW_BUILTIN_CODING_WORKFLOW_IR: WorkflowIr = { codeReviewRemediationNode("in-progress"), completionSummaryNode("in-review"), { id: "review", kind: "prompt", column: "in-review", config: builtinPromptConfig("review", "Review") }, + /* + FNXC:WorkflowExecutionOwnership 2026-07-29-09:20 (U8 / R4 — workflow-owned lifecycle): + THE PENDING-REVIEW PARK, as a graph node. + + An implementation session can end 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 (marking it + failed deadlocks a row that is both `in-review` and `failed`). Until now the EXECUTOR + performed that transition inline, mid-session, and the graph found out afterwards — which is + why `handleGraphFailure` carries `alreadyFinalizedToReview`, a classifier whose only job is + recognising a move the graph did not make. + + Declaring it here makes the ending a routed outcome instead of a side effect. It is a + `review-handoff` seam (a pure lifecycle handoff, no reviewer invocation) and its only edge is + to `end`, which is what preserves today's semantics exactly: hand the card to review and STOP. + Routing to the ordinary `review` node instead would have been wrong in a way worth recording — + the run would continue into merge-gate and merge-attempt on work whose steps are incomplete. + */ + { + id: "review-pending-handoff", + kind: "prompt", + column: "in-review", + config: builtinPromptConfig("review-handoff", "Park for pending 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 } }, { id: "merge-manual-hold", kind: "manual-merge-hold", column: "in-review", config: { release: "manual" } }, @@ -149,6 +172,17 @@ const RAW_BUILTIN_CODING_WORKFLOW_IR: WorkflowIr = { { from: "planning", to: "end", condition: "failure" }, { from: "plan-review", to: "plan-replan", condition: "failure" }, { from: "plan-replan", to: "plan-review", condition: "success", kind: "rework" }, + /* + FNXC:WorkflowExecutionOwnership 2026-07-29-09:25 (U8 / R4): + `outcome:` edges are matched on the node's VALUE and take priority over the generic + success/failure edges (`shouldTraverseEdge` / `traverseChildren`), so this claims the + pending-review ending without disturbing the `failure -> end` edge below it for every other + failure value. A workflow that does NOT declare this edge falls through to that generic + failure edge — i.e. exactly the pre-existing behavior — which is what lets custom workflows + keep working unchanged while the built-in routes the ending properly. + */ + { from: "execute", to: "review-pending-handoff", condition: "outcome:review-pending" }, + { from: "review-pending-handoff", to: "end", condition: "success" }, { from: "execute", to: "end", condition: "failure" }, { from: "browser-verification", to: "browser-verification-remediation", condition: "failure" }, { from: "browser-verification-remediation", to: "browser-verification", condition: "success", kind: "rework" }, diff --git a/packages/core/src/builtin-workflows.ts b/packages/core/src/builtin-workflows.ts index 8f44dc79e0..d52c5e0107 100644 --- a/packages/core/src/builtin-workflows.ts +++ b/packages/core/src/builtin-workflows.ts @@ -490,6 +490,8 @@ export const BUILTIN_WORKFLOWS: WorkflowDefinition[] = [ "code-review-remediation": { x: 910, y: 320 }, "completion-summary": { x: 1080, y: 160 }, review: { x: 1250, y: 160 }, + /* U8: the pending-review park sits off the main line — it is an exit, not a stage. */ + "review-pending-handoff": { x: 570, y: 320 }, "merge-gate": { x: 1420, y: 160 }, "branch-group-member-integration": { x: 1590, y: 80 }, "branch-group-promotion": { x: 1760, y: 80 },