3f763cba87
The routing move this unit has been building toward, landing on the path the engine actually runs. **Includes #2578's commit** (the live-path fix it depends on) — merge that first, or this supersedes it. ## What changes Three things together, because a half-routed move is a card that silently does not advance: 1. The **live** implementation primitive (`runCodingSession`) returns `{outcome: "failure", value: "review-pending"}` for that ending. 2. The primitive step handler stops flattening every ending to `step-done`/`step-failed`, so the value survives the foreach — `runForeach` propagates a failing instance's value as the node's own — and reaches an edge. 3. The inline `handoffTaskToReview` in `runImplementation` is **deleted**. The phase reports and stops, which is all an implementation phase should do. Built-in workflows route to the `review-pending-handoff` node added in #2519/#2546, which performs the handoff and ends the run: the same two effects in the same order, with the graph as the owner. ## Proof, end to end FN-5436 — the test that blocked this move twice and was right both times — now passes, with a **stronger** assertion than it had: ```ts expect(store.moveTask).toHaveBeenCalledWith("FN-5436-B", "in-review", expect.objectContaining({ workflowMoveSource: "workflow-graph", workflowMoveMetadata: expect.objectContaining({ nodeId: "review-pending-handoff" }), })); ``` The old two-argument `moveTask(id, "in-review")` could not distinguish a graph-owned park from an out-of-band one — which is the entire distinction this unit exists to make. The invariant (park in review, never `failed`) is unchanged; the owner is now proven. ## Every ratchet fired, and each records a real change | Ratchet | Before | After | Why | |---|---|---|---| | Ownership ledger — `runImplementation` review handoffs | 3 | **2** | the handoff left the phase | | Ownership ledger — `handleGraphFailure` | 0 | **1** | the named compat classifier | | Ledger headline — executor-owned dispositions | 28 | **27** | first decrement of the unit | | Out-of-band exit list | 2 | **1** | pending-review is graph-owned now | | Primitive routing pin | "must not reroute" | routes *only* the moved ending | declared, not discovered | None was relaxed. The `handleGraphFailure` 0 → 1 is the honest one: for a user-authored graph without the edge this is a **relocation, not an elimination** — the transition is still executor-performed, but from one named classifier in the failure ladder rather than a call buried two thousand lines into a session loop. The ledger says so rather than letting the headline number imply more progress than there is. ## Why it took four attempts Recorded because the reason is reusable: the value was being produced on `createAuthoritativeWorkflowSeams`, a handler that never runs (#2578). Every earlier attempt was correct code on a dead path, and the only thing that showed it was instrumenting until a negative result was proven observable rather than assumed. ## Verification - step-session + exit-events + primitive-exit-events + ownership ledger + graph-requeue-gate + task-done-blocked — **83 tests green** - `pnpm test:gate` green (10 / 482 / 71); `pnpm lint` clean; `tsc --noEmit` clean - Changeset included (`patch`, `internal`) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved handling of tasks awaiting review so they are correctly routed to the review workflow. * Tasks now remain in review instead of being marked as failed when no follow-up review route is configured. * Review handoffs now include workflow ownership and provenance details. * Preserved standard failure handling for tasks that are not awaiting review. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
75 lines
4.4 KiB
TypeScript
75 lines
4.4 KiB
TypeScript
/*
|
|
FNXC:WorkflowExecutionOwnership 2026-07-28-20:10 (U8 / R4, R5 — workflow-owned lifecycle):
|
|
|
|
THE IMPLEMENTATION PHASE'S EXIT VOCABULARY.
|
|
|
|
`runImplementation` can end in many ways, and the graph is told about exactly one bit of it:
|
|
`result.taskDone`. That is the whole language the execute seam has (`executor.ts` — the seam
|
|
maps it to `"implemented"` or `"implementation-incomplete"`), and it is why the implementation
|
|
phase transitions cards ITSELF for the endings the boolean cannot express:
|
|
|
|
- a session that paused AFTER the work was already complete finalizes to review inline;
|
|
- a session that stopped because a step is blocked on a pending review hands off to review
|
|
inline, because it cannot continue and review is not an error bucket.
|
|
|
|
The graph then sees `taskDone === false`, reports `implementation-incomplete`, and
|
|
`handleGraphFailure` compensates with `alreadyFinalizedToReview` / `completionFinalized` —
|
|
classifiers whose entire job is recognising a move the graph did not make. Dual ownership, and
|
|
today it is INVISIBLE: nothing anywhere records that the executor, not the graph, moved the card.
|
|
|
|
This module names those endings so they can be observed before they are moved. Each id is a
|
|
closed enum value, never prose — these ids travel on the U3 lifecycle bus to plugin subscribers
|
|
under its ids-only rule.
|
|
|
|
WHAT THIS DELIBERATELY DOES NOT DO. It does not change routing. The execute seam returns exactly
|
|
the outcome and value it returned before, for every exit, and `executor-implementation-exit-
|
|
events.test.ts` pins that. An exit id is a REACTION under R5 — a dropped event must cost a
|
|
notification and never a state change — so nothing downstream may branch on one until the
|
|
routing move lands with its own IR edges. Reporting first, moving second, is what keeps the two
|
|
changes independently revertable.
|
|
|
|
COVERAGE IS PARTIAL ON PURPOSE. `runImplementation` has ~28 lifecycle dispositions (measured by
|
|
`executor-lifecycle-ownership-ledger.test.ts`) and this instruments the six completion-adjacent
|
|
ones — the three graph handbacks and the three inline review handoffs. Those are the exits U8's
|
|
routing move needs; the rest report nothing yet and the ledger, not this enum, is the record of
|
|
that gap.
|
|
*/
|
|
|
|
/*
|
|
FNXC:WorkflowExecutionOwnership 2026-07-28-22:20 (U8, PR #2507 review — greptile):
|
|
THE UNION MOVED TO CORE. It was declared here and the public `NodeCompletedEvent.exit` was typed
|
|
`string`, so the contract permitted ids no consumer routes — and that failure is silent (the card
|
|
does not advance; nothing reports anything). A public contract cannot defer its vocabulary to one
|
|
of its producers, so `ImplementationExit` now lives beside the event that carries it, is checked
|
|
at the emit boundary against `IMPLEMENTATION_EXITS`, and is re-exported here for the call sites.
|
|
|
|
What stays in the engine is POLICY, not contract: which of those endings are ones the EXECUTOR
|
|
performed rather than the graph. That is a statement about this engine's current ownership split,
|
|
it changes as U8 lands its routing moves, and core has no business knowing it.
|
|
*/
|
|
import type { ImplementationExit as CoreImplementationExit } from "@fusion/core";
|
|
export type { ImplementationExit } from "@fusion/core";
|
|
|
|
/** The exits where the EXECUTOR performs the lifecycle transition instead of the graph. */
|
|
/*
|
|
FNXC:WorkflowExecutionOwnership 2026-07-29-19:20 (U8 / R4):
|
|
The ledger of endings the implementation phase still transitions itself, and it SHRINKS as U8
|
|
lands routing moves. `review-handoff-pending-review` left it: the phase reports the ending and
|
|
stops, and the graph's `review-pending-handoff` node performs the handoff.
|
|
|
|
Caveat so the list is not read as more than it is: a user-authored graph without the
|
|
`outcome:review-pending` edge still gets an executor-performed handoff, from a single named
|
|
classifier in `handleGraphFailure` — not from inside the session loop. "Out-of-band" here means
|
|
the IMPLEMENTATION PHASE performs it.
|
|
*/
|
|
export const OUT_OF_BAND_IMPLEMENTATION_EXITS: readonly CoreImplementationExit[] = [
|
|
"review-handoff-paused-after-completion",
|
|
];
|
|
|
|
export function isOutOfBandImplementationExit(exit: CoreImplementationExit | undefined): boolean {
|
|
return exit !== undefined && OUT_OF_BAND_IMPLEMENTATION_EXITS.includes(exit);
|
|
}
|
|
|
|
/** Reporter threaded into `runImplementation`; each instrumented exit calls it exactly once. */
|
|
export type ImplementationExitReporter = (exit: CoreImplementationExit) => void;
|