Third PR of **U8 — the graph owns execution**. Independent of everything
merged so far; small, green, revertable on its own.
## The problem this makes visible
`result.taskDone` is the entire language the execute seam has for
talking to the graph:
```ts
if (result.taskDone) return { outcome: "success", value: "implemented" };
return { outcome: "failure", value: paused ? "implementation-paused" : "implementation-incomplete" };
```
The endings that one bit cannot express are exactly the ones the
implementation phase **transitions itself**:
- 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 (a pending-review block is a wait, not a
failure; marking it failed deadlocks a row that is both `in-review` and
`failed`).
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.
**That was invisible.** An out-of-band transition and a genuine
implementation failure were indistinguishable in logs, in events, and in
tests. You cannot remove a transition you cannot see, and you cannot
prove you removed it either.
## What lands
A closed `ImplementationExit` enum
(`engine/executor/implementation-exit.ts`) reported from six
completion-adjacent exits in `runImplementation`, announced by the
execute seam as `NodeCompleted.exit` on the U3 lifecycle bus. Two ids
are flagged as out-of-band — the ones where the executor, not the graph,
performs the transition.
**Routing is unchanged, and that is the point.** The seam returns
byte-identically what it returned before for every exit, so this PR
cannot move a card. The routing move needs new IR edges and lands
separately; splitting them is what keeps both independently revertable.
Per R5 an exit id is a **reaction** — nothing branches on one, and
dropping every subscriber must change no outcome (a named U8 test
scenario, asserted here).
`NodeCompleted.exit` is added to the event key allow-list deliberately —
which is exactly what that allow-list is for — and carries closed enum
ids only, never prose.
## Revert-proofs, each observed failing
| Injected change | Result |
|---|---|
| Remove the emit entirely | **6 failures** |
| Let an exit change the returned outcome | **2 failures** (the
routing-unchanged pins) |
| Delete one `reportImplementationExit(...)` call site | **1 failure**
(the wiring ratchet) |
**The third proof exists because of a hole I found in my own tests.**
These tests stub `runImplementationPhase` — the only way to reach all
six exits deterministically — which means deleting a real call site left
the entire file **green**. A stubbed seam can only prove the seam. I'd
also written "every exit is reported — the signal is real, not a
placeholder" in the header, which the tests did not support. Both are
fixed: there is now a ratchet asserting every enum id is wired at a real
call site and that each out-of-band id sits adjacent to the handoff it
describes, and the header says what the tests actually prove.
## Scope
**6 of `runImplementation`'s ~28 dispositions** (per the ownership
ledger merged in #2490), chosen as the ones the routing move needs. The
remaining ~22 report nothing yet — the ledger, not this enum, stays the
record of that gap, and the module says so.
## Verification
- 15 new tests + ledger + graph-boundary + task-done-blocked +
graph-requeue-gate + step-session + review-verdicts + tool-failure-retry
— **9 files, 115 tests green**
- `@fusion/core` `workflow-events` — 20 tests green (allow-list change
covered)
- `pnpm test:gate` green (17/307, 2/10, 1/71); `pnpm lint` clean; `tsc
--noEmit` clean on both packages
- Changeset included (`patch`, `internal`), passes `check:changesets`
## Next
PR4 is the routing move itself: `review-handoff-pending-review` becomes
a graph outcome with its own IR edge, and `alreadyFinalizedToReview`
becomes provably unreachable for that path. The IR edge change will be
its own commit, separate from the seam change.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
197 lines
9.4 KiB
TypeScript
197 lines
9.4 KiB
TypeScript
/*
|
|
FNXC:WorkflowExecutionOwnership 2026-07-28-20:40 (U8 / R4, R5, R12 — workflow-owned lifecycle):
|
|
|
|
The execute seam tells the graph one bit: `result.taskDone`. The endings that bit cannot express
|
|
are exactly the ones the implementation phase transitions ITSELF — a session that paused after
|
|
the work was complete, and a session that stopped on a pending-review block. Both hand the card
|
|
to review inline; the graph then sees `taskDone === false`, reports `implementation-incomplete`,
|
|
and `handleGraphFailure` compensates with `alreadyFinalizedToReview`. Until now nothing anywhere
|
|
recorded which of those happened: an out-of-band transition and a genuine implementation failure
|
|
were indistinguishable in logs, in events, and in tests.
|
|
|
|
These tests pin the properties that make the exit signal safe to build the routing move on:
|
|
|
|
1. Every exit the phase reports is forwarded with its own id, AND every id in the enum has a
|
|
real call site in `runImplementation`. The second half is not pedantry: these tests stub
|
|
`runImplementationPhase`, so without it deleting a `reportImplementationExit(...)` call
|
|
leaves all of them green — verified by deleting one. A stubbed seam can only prove the seam.
|
|
2. ROUTING IS UNCHANGED. For every exit the seam returns byte-identically what it returned
|
|
before, so this PR cannot move a card. That is the property that lets the reporting and the
|
|
routing land as separate, independently revertable changes.
|
|
3. DROPPING EVERY SUBSCRIBER CHANGES NO OUTCOME (R5, and a named U8 scenario). An exit id is a
|
|
reaction; if anything downstream ever depends on one arriving, the bus has quietly become a
|
|
second source of truth, which is the failure mode this whole program exists to remove.
|
|
*/
|
|
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
|
import { readFileSync } from "node:fs";
|
|
import type { Settings, Task } from "@fusion/core";
|
|
import {
|
|
getWorkflowEventBus,
|
|
resetWorkflowEventBusForTesting,
|
|
type WorkflowLifecycleEvent,
|
|
} from "@fusion/core";
|
|
import "./executor-test-helpers.js";
|
|
import { TaskExecutor } from "../executor.js";
|
|
import { createMockStore, resetExecutorMocks } from "./executor-test-helpers.js";
|
|
import {
|
|
OUT_OF_BAND_IMPLEMENTATION_EXITS,
|
|
isOutOfBandImplementationExit,
|
|
type ImplementationExit,
|
|
} from "../executor/implementation-exit.js";
|
|
|
|
const SEAM_TASK = { id: "FN-U8-EXIT", title: "exit vocabulary", column: "in-progress" } as Task;
|
|
|
|
/**
|
|
* Drive the execute seam with a stubbed implementation phase. Stubbing the phase rather than
|
|
* running a real agent session is the only way to exercise all six exits deterministically; the
|
|
* exits themselves are wired at their real call sites in `runImplementation` and covered by the
|
|
* completion/handoff suites.
|
|
*/
|
|
function seamHarness(phaseResult: { taskDone: boolean; modifiedFiles: string[]; exit?: ImplementationExit }) {
|
|
const store = createMockStore();
|
|
store.getTask.mockResolvedValue({ ...SEAM_TASK, paused: false });
|
|
const executor = new TaskExecutor(store, "/tmp/test");
|
|
const runImplementationPhase = vi
|
|
.spyOn(executor as never as { runImplementationPhase: () => unknown }, "runImplementationPhase")
|
|
.mockResolvedValue(phaseResult);
|
|
const seams = executor.createAuthoritativeWorkflowSeams({} as Settings);
|
|
return { executor, store, seams, runImplementationPhase };
|
|
}
|
|
|
|
function captureEvents(): { events: WorkflowLifecycleEvent[]; drain: () => Promise<void> } {
|
|
const events: WorkflowLifecycleEvent[] = [];
|
|
getWorkflowEventBus().subscribe((event) => { events.push(event); }, { name: "exit-test" });
|
|
return { events, drain: () => getWorkflowEventBus().drain() };
|
|
}
|
|
|
|
/** Every exit, with the outcome/value the seam returned for it BEFORE this change. */
|
|
const EXITS: Array<{
|
|
exit: ImplementationExit;
|
|
taskDone: boolean;
|
|
expected: { outcome: string; value: string };
|
|
}> = [
|
|
{ exit: "complete", taskDone: true, expected: { outcome: "success", value: "implemented" } },
|
|
{ exit: "complete-after-retry", taskDone: true, expected: { outcome: "success", value: "implemented" } },
|
|
{ exit: "complete-from-live-files", taskDone: true, expected: { outcome: "success", value: "implemented" } },
|
|
{ exit: "review-handoff-paused-after-completion", taskDone: false, expected: { outcome: "failure", value: "implementation-incomplete" } },
|
|
{ exit: "review-handoff-pending-review", taskDone: false, expected: { outcome: "failure", value: "implementation-incomplete" } },
|
|
];
|
|
|
|
describe("execute seam announces the implementation phase's exit", () => {
|
|
beforeEach(() => {
|
|
resetExecutorMocks();
|
|
resetWorkflowEventBusForTesting();
|
|
});
|
|
afterEach(() => resetWorkflowEventBusForTesting());
|
|
|
|
it.each(EXITS)("reports $exit on the lifecycle bus", async ({ exit, taskDone }) => {
|
|
const { seams } = seamHarness({ taskDone, modifiedFiles: [], exit });
|
|
const bus = captureEvents();
|
|
|
|
await seams.execute!(SEAM_TASK, undefined);
|
|
await bus.drain();
|
|
|
|
const completed = bus.events.filter((e) => e.type === "NodeCompleted");
|
|
expect(completed).toHaveLength(1);
|
|
expect(completed[0]).toMatchObject({
|
|
type: "NodeCompleted",
|
|
taskId: SEAM_TASK.id,
|
|
nodeId: "execute",
|
|
outcome: taskDone ? "success" : "failure",
|
|
exit,
|
|
});
|
|
});
|
|
|
|
/*
|
|
The property that makes this PR safe to land ahead of the routing move: naming an exit must not
|
|
reroute anything. If any of these drift, a card is moving somewhere new and that belongs in the
|
|
PR that adds the IR edge, not in this one.
|
|
*/
|
|
it.each(EXITS)("returns the pre-existing routing outcome for $exit", async ({ exit, taskDone, expected }) => {
|
|
const { seams } = seamHarness({ taskDone, modifiedFiles: [], exit });
|
|
|
|
await expect(seams.execute!(SEAM_TASK, undefined)).resolves.toEqual(expected);
|
|
});
|
|
|
|
it("returns the same outcome when the phase reports no exit at all", async () => {
|
|
/* The ~22 uninstrumented dispositions still report nothing; they must be unaffected. */
|
|
const { seams } = seamHarness({ taskDone: false, modifiedFiles: [] });
|
|
const bus = captureEvents();
|
|
|
|
const outcome = await seams.execute!(SEAM_TASK, undefined);
|
|
await bus.drain();
|
|
|
|
expect(outcome).toEqual({ outcome: "failure", value: "implementation-incomplete" });
|
|
const completed = bus.events.filter((e) => e.type === "NodeCompleted");
|
|
expect(completed).toHaveLength(1);
|
|
expect(completed[0]).not.toHaveProperty("exit");
|
|
});
|
|
|
|
/*
|
|
R5, and a named U8 test scenario: "A dropped event subscriber changes no execution outcome
|
|
(proves reactions are non-authoritative)."
|
|
*/
|
|
it("produces identical outcomes with NO subscribers at all", async () => {
|
|
for (const { exit, taskDone, expected } of EXITS) {
|
|
resetWorkflowEventBusForTesting();
|
|
const { seams } = seamHarness({ taskDone, modifiedFiles: [], exit });
|
|
expect(getWorkflowEventBus().subscriberCount()).toBe(0);
|
|
|
|
await expect(seams.execute!(SEAM_TASK, undefined)).resolves.toEqual(expected);
|
|
}
|
|
});
|
|
|
|
it("is not derailed by a throwing subscriber", async () => {
|
|
const { seams } = seamHarness({ taskDone: true, modifiedFiles: [], exit: "complete" });
|
|
getWorkflowEventBus().subscribe(() => { throw new Error("subscriber blew up"); }, { name: "boom" });
|
|
|
|
await expect(seams.execute!(SEAM_TASK, undefined)).resolves.toEqual({
|
|
outcome: "success",
|
|
value: "implemented",
|
|
});
|
|
await getWorkflowEventBus().drain();
|
|
});
|
|
|
|
/*
|
|
FNXC:WorkflowExecutionOwnership 2026-07-28-21:05 (U8 / R12):
|
|
The wiring ratchet. Everything above drives a STUBBED implementation phase, which is the only
|
|
way to reach all six exits deterministically — but it means the real call sites are not
|
|
exercised. Deleting `reportImplementationExit?.("review-handoff-pending-review")` from
|
|
`runImplementation` left this whole file green (measured, not assumed), so the enum would have
|
|
drifted into a vocabulary that describes endings nothing actually reports. This asserts each id
|
|
is wired, and that the two out-of-band ids sit with the handoff they describe.
|
|
*/
|
|
it("every exit id has a real call site in runImplementation", () => {
|
|
const source = readFileSync(new URL("../executor.ts", import.meta.url), "utf8")
|
|
.replace(/\/\*[\s\S]*?\*\//g, " ")
|
|
.replace(/(^|[^:])\/\/[^\n]*/g, "$1 ");
|
|
const ALL_EXITS: ImplementationExit[] = [
|
|
"complete",
|
|
"complete-after-retry",
|
|
"complete-from-live-files",
|
|
"review-handoff-paused-after-completion",
|
|
"review-handoff-pending-review",
|
|
];
|
|
const missing = ALL_EXITS.filter((exit) => !source.includes(`reportImplementationExit?.("${exit}")`));
|
|
expect(missing).toEqual([]);
|
|
/* Each out-of-band id must accompany an inline review handoff — that pairing IS its meaning. */
|
|
for (const exit of OUT_OF_BAND_IMPLEMENTATION_EXITS) {
|
|
const idx = source.indexOf(`reportImplementationExit?.("${exit}")`);
|
|
expect(source.slice(idx, idx + 400)).toContain("handoffTaskToReview(");
|
|
}
|
|
});
|
|
|
|
it("classifies exactly the two executor-performed transitions as out-of-band", () => {
|
|
/*
|
|
The ledger this unit closes: an out-of-band exit is one where the EXECUTOR moved the card.
|
|
If a third appears without a routing move, U8 has gone backwards.
|
|
*/
|
|
expect([...OUT_OF_BAND_IMPLEMENTATION_EXITS]).toEqual([
|
|
"review-handoff-paused-after-completion",
|
|
"review-handoff-pending-review",
|
|
]);
|
|
expect(isOutOfBandImplementationExit("complete")).toBe(false);
|
|
expect(isOutOfBandImplementationExit(undefined)).toBe(false);
|
|
});
|
|
});
|