U8 PR4: declare the pending-review park as a graph node (inert) — and why the behavior move is blocked on the step-session chain (#2519)
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) <noreply@anthropic.com>
This commit is contained in:
7
.changeset/u8-review-pending-park-node.md
Normal file
7
.changeset/u8-review-pending-park-node.md
Normal file
@@ -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.
|
||||
@@ -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" },
|
||||
|
||||
@@ -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 },
|
||||
|
||||
Reference in New Issue
Block a user