root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

U8 PR5: thread the implementation exit to the step seam, and declare the stepwise pending-review park (inert) (#2546)

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-07-29 09:54:39 -07:00
committed by GitHub
parent 3cef9c226e
commit 8578a1d27d
5 changed files with 103 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
.changeset/u8-step-session-exit-threading.md Normal file
View File

@@ -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.

28
packages/core/src/builtin-stepwise-coding-workflow-ir.ts
View File

@@ -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" },

8
packages/core/src/builtin-workflows.ts
View File

@@ -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 },

44
packages/engine/src/executor.ts
View File

@@ -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<string, Promise<{ taskDone: boolean; modifiedFiles: string[] }>>();
private graphStepRunOnce = new Map<string, Promise<{ taskDone: boolean; modifiedFiles: string[]; exit?: ImplementationExit }>>();
/** 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,
},

24
packages/engine/src/step-runner.ts
View File

@@ -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 ──────────────────────────────────────────────────