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