From 131feb243c7c2162f613304455980a0214fe8c03 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 29 Jul 2026 20:30:54 -0700 Subject: [PATCH] =?UTF-8?q?U8:=20the=20exit=20announcement=20was=20on=20a?= =?UTF-8?q?=20dead=20code=20path=20=E2=80=94=20move=20it=20to=20the=20hand?= =?UTF-8?q?ler=20the=20engine=20actually=20runs=20(#2578)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A merged behavior of mine has never executed. This fixes it and adds the ratchet that would have caught it. ## The finding `createDefaultNodeHandlers` chooses the prompt-node handler like this: ```ts const promptLike = deps?.primitives ? createPrimitivePromptLikeHandler(deps.primitives, runCustomNode) : createPromptLikeHandler(seams, runCustomNode); ``` `executeWorkflowGraph` always passes `primitives: this.createAuthoritativeWorkflowPrimitives(settings)` (`executor.ts:6051`). **So `createPromptLikeHandler` — and with it every `execute` / `step-execute` function in `createAuthoritativeWorkflowSeams` — is unreachable for prompt nodes.** Both objects are passed to the graph executor and only one is consulted. The `NodeCompleted.exit` announcement added in #2507 was wired into that seam. It type-checks, its tests pass (they call the seam object directly), and it has never run in production. `runCodingSession` in the primitives is the live twin, and that is where it emits now. ## How it was found — and why the negative is trustworthy Instrumenting `createAuthoritativeWorkflowSeams.stepExecute` produced no output for a run that demonstrably visits `steps#0:step-execute`. So did instrumenting `createPromptLikeHandler`'s dispatch. A negative result from instrumentation is worthless until the instrumentation is shown to be observable, so: a `process.stderr.write` at module load of the same file **did** appear, exactly once, in the same run. The two negatives were real, not swallowed output. This is also the answer to the open question I left in #2546 — the pending-review routing move kept failing because the seam value it depends on is never produced. **That move is still not landed here.** This commit only relocates the announcement, so it stays small and separately revertable; the routing move follows once its value originates on the live path. ## The ratchet A source assertion pins the dispatch rule: `deps?.primitives ? createPrimitivePromptLikeHandler` and the executor's wiring of `primitives`. Inverting or conditionalising that preference would silently disable every behavior attached to the primitives path — the same failure in the other direction — and **a seam-level unit test cannot tell the two apart**, which is precisely how this survived review twice. ## Red-green Removing the emit fails 2 of the 4 new tests (`Tests 2 failed | 2 passed (4)`). The other two are the regression floor: an ordinary completion emits `success` with no `exit`, and the returned routing outcome is unchanged — announcing must not reroute. ## Scope note I did **not** delete the now-known-dead seam wiring in this PR. `createAuthoritativeWorkflowSeams` is still passed to the graph executor and its non-prompt entries (`stepReview`, `merge`) are reached through other handlers, so deciding what is genuinely dead there is a deletion audit of its own — and this program's rule is that deletions never ride along with behavior changes. Filed as the next slice. ## Verification - 4 new tests + exit-events + step-session + triage audit + ownership ledger — **54 tests green** - `pnpm test:gate` green (10 / 414 / 71); `pnpm lint` clean; `tsc --noEmit` clean - Changeset included (`patch`, `fix`) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- .changeset/u8-primitive-exit-events.md | 7 + .../executor-primitive-exit-events.test.ts | 135 ++++++++++++++++++ packages/engine/src/executor.ts | 24 +++- 3 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 .changeset/u8-primitive-exit-events.md create mode 100644 packages/engine/src/__tests__/executor-primitive-exit-events.test.ts diff --git a/.changeset/u8-primitive-exit-events.md b/.changeset/u8-primitive-exit-events.md new file mode 100644 index 0000000000..de8ec89195 --- /dev/null +++ b/.changeset/u8-primitive-exit-events.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Report how an implementation session ended on the code path the engine actually runs. +category: fix +dev: `createDefaultNodeHandlers` prefers `createPrimitivePromptLikeHandler` whenever `deps.primitives` is set, and `executeWorkflowGraph` always sets it — so `createAuthoritativeWorkflowSeams.execute` is unreachable for prompt nodes. The `NodeCompleted.exit` announcement was wired only there and never fired; it now emits from `runCodingSession`. Adds a ratchet asserting the primitives-preferred dispatch rule. diff --git a/packages/engine/src/__tests__/executor-primitive-exit-events.test.ts b/packages/engine/src/__tests__/executor-primitive-exit-events.test.ts new file mode 100644 index 0000000000..0de858740b --- /dev/null +++ b/packages/engine/src/__tests__/executor-primitive-exit-events.test.ts @@ -0,0 +1,135 @@ +/* +FNXC:WorkflowExecutionOwnership 2026-07-29-16:40 (U8 / R4, R5, R12): + +The exit announcement was wired into `createAuthoritativeWorkflowSeams.execute` — which is NOT +the handler production runs. `createDefaultNodeHandlers` picks the PRIMITIVES prompt-like handler +whenever `deps.primitives` is set, and `executeWorkflowGraph` always sets it, so the legacy-seams +prompt handler is unreachable for prompt nodes. Everything wired only there is dead code that +type-checks, passes its own unit tests against the seam object, and never runs. + +That is why this file exists at all: a seam-level test cannot tell the two apart. These assert the +announcement on the LIVE primitive, and — more importantly — assert the wiring rule itself, so the +next person adding behavior to a seam finds out from a red test rather than from an operator. +*/ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { readFileSync } from "node:fs"; +import type { Settings, TaskDetail } from "@fusion/core"; +import { + getWorkflowEventBus, + resetWorkflowEventBusForTesting, + type WorkflowLifecycleEvent, +} from "@fusion/core"; +import "./executor-test-helpers.js"; +import { TaskExecutor } from "../executor.js"; +import { createDefaultNodeHandlers } from "../workflow-node-handlers.js"; +import { createMockStore, resetExecutorMocks } from "./executor-test-helpers.js"; + +const TASK = { id: "FN-PRIM-EXIT", column: "in-progress", steps: [], paused: false } as unknown as TaskDetail; + +function harness(phase: { taskDone: boolean; modifiedFiles: string[]; exit?: string }) { + const store = createMockStore(); + store.getTask.mockResolvedValue(TASK); + const executor = new TaskExecutor(store, "/tmp/test"); + vi.spyOn(executor as never as { runImplementationPhase: () => unknown }, "runImplementationPhase") + .mockResolvedValue(phase); + const primitives = executor.createAuthoritativeWorkflowPrimitives({} as Settings); + const ctx = { run: {}, node: { node: { id: "execute", kind: "prompt" }, context: {} } } as never; + return { executor, primitives, ctx }; +} + +function captured(): { events: WorkflowLifecycleEvent[]; drain: () => Promise } { + const events: WorkflowLifecycleEvent[] = []; + getWorkflowEventBus().subscribe((e) => { events.push(e); }, { name: "prim-exit" }); + return { events, drain: () => getWorkflowEventBus().drain() }; +} + +describe("the LIVE implementation primitive announces the exit", () => { + beforeEach(() => { resetExecutorMocks(); resetWorkflowEventBusForTesting(); }); + afterEach(() => resetWorkflowEventBusForTesting()); + + it("emits NodeCompleted with the exit from runCodingSession", async () => { + const { primitives, ctx } = harness({ taskDone: false, modifiedFiles: [], exit: "review-handoff-pending-review" }); + const bus = captured(); + + await primitives.runCodingSession(ctx, TASK, { worktreePath: "/tmp/wt", branchName: "b" } as never); + await bus.drain(); + + const completed = bus.events.filter((e) => e.type === "NodeCompleted"); + expect(completed).toHaveLength(1); + expect(completed[0]).toMatchObject({ taskId: TASK.id, outcome: "failure", exit: "review-handoff-pending-review" }); + }); + + it("emits success without an exit for an ordinary completion", async () => { + const { primitives, ctx } = harness({ taskDone: true, modifiedFiles: [] }); + const bus = captured(); + + await primitives.runCodingSession(ctx, TASK, { worktreePath: "/tmp/wt", branchName: "b" } as never); + await bus.drain(); + + const completed = bus.events.filter((e) => e.type === "NodeCompleted"); + expect(completed).toHaveLength(1); + expect(completed[0]).toMatchObject({ outcome: "success" }); + expect(completed[0]).not.toHaveProperty("exit"); + }); + + it("returns the unchanged routing outcome — announcing must not reroute", async () => { + const { primitives, ctx } = harness({ taskDone: false, modifiedFiles: [], exit: "review-handoff-pending-review" }); + + const result = await primitives.runCodingSession(ctx, TASK, { worktreePath: "/tmp/wt", branchName: "b" } as never); + + expect(result).toMatchObject({ outcome: "failure", value: "implementation-incomplete" }); + }); + + /* + The rule, asserted rather than remembered. `createDefaultNodeHandlers` prefers the primitives + handler whenever primitives are supplied; `executeWorkflowGraph` always supplies them. If that + preference is ever inverted or made conditional, every behavior wired to the primitives path + silently stops running — the same failure that put the announcement on a dead seam. + */ + /* + FNXC:WorkflowExecutionOwnership 2026-07-29-21:10 (U8 / R12, PR #2578 review — greptile): + BEHAVIOURAL, not textual. The first version grepped for `deps?.primitives ? createPrimitive...` + and for the executor's wiring string. Those fragments can both survive while an earlier fallback, + an extracted dispatch helper, or a reordered condition stops selecting the primitive handler — + so the ratchet would stay green through exactly the regression it exists to catch, which is the + same "proves syntax, not behaviour" mistake that put a shipped behaviour on a dead seam. + + This builds the real handler map from spied seams AND spied primitives and asserts, per seam, + which one ran. It fails if dispatch ever stops preferring primitives, however that happens. + */ + it("dispatches every seam through the PRIMITIVES handler, never the legacy seams", async () => { + const calls: string[] = []; + const seamSpy = (name: string) => async () => { calls.push(`seam:${name}`); return { outcome: "success" as const }; }; + const seams = { + planning: seamSpy("planning"), + execute: seamSpy("execute"), + review: seamSpy("review"), + "review-handoff": seamSpy("review-handoff"), + merge: seamSpy("merge"), + schedule: seamSpy("schedule"), + stepExecute: seamSpy("step-execute"), + } as never; + const primitives = { + prepareWorktree: async () => { calls.push("prim:prepareWorktree"); return { outcome: "success", data: { worktreePath: "/tmp/wt", branchName: "b" } }; }, + runPlanningSession: async () => { calls.push("prim:runPlanningSession"); return { outcome: "success" }; }, + runCodingSession: async () => { calls.push("prim:runCodingSession"); return { outcome: "success", value: "implemented" }; }, + requestReviewHandoff: async () => { calls.push("prim:requestReviewHandoff"); return { outcome: "success" }; }, + requestReview: async () => { calls.push("prim:requestReview"); return { outcome: "success" }; }, + requestMerge: async () => { calls.push("prim:requestMerge"); return { outcome: "success" }; }, + scheduleWork: async () => { calls.push("prim:scheduleWork"); return { outcome: "success" }; }, + runTaskStep: async () => { calls.push("prim:runTaskStep"); return { outcome: "success" }; }, + } as never; + + const handlers = createDefaultNodeHandlers(seams, undefined, { primitives }); + for (const seam of ["planning", "execute", "review", "review-handoff", "merge", "schedule"]) { + await handlers.prompt!( + { id: `${seam}-node`, kind: "prompt", column: "in-progress", config: { seam } } as never, + { task: { id: "FN-DISPATCH" }, context: {} } as never, + ).catch(() => undefined); + } + + /* The assertion that matters: no seam function ran, for any seam name. */ + expect(calls.filter((c) => c.startsWith("seam:"))).toEqual([]); + expect(calls.some((c) => c.startsWith("prim:"))).toBe(true); + }); +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 09f2bef8a4..6a0e21aae2 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -7298,12 +7298,34 @@ export class TaskExecutor { if (typeof governingNodeId === "string") { this.graphSeamGoverningNodeId.set(task.id, governingNodeId); } - let result: { taskDone: boolean; modifiedFiles: string[] }; + let result: { taskDone: boolean; modifiedFiles: string[]; exit?: ImplementationExit }; try { result = await this.runImplementationPhase(task, prepared); } finally { this.graphSeamGoverningNodeId.delete(task.id); } + /* + FNXC:WorkflowExecutionOwnership 2026-07-29-16:20 (U8 / R4, R5): + THIS is the live implementation node, not the identically-shaped `execute` entry in + `createAuthoritativeWorkflowSeams`. `createDefaultNodeHandlers` prefers the PRIMITIVES + handler whenever `deps.primitives` is set, and `executeWorkflowGraph` always sets it — so + the legacy-seams prompt handler is unreachable for prompt nodes and anything wired only + there never runs. The exit announcement was wired only there; it is announced here now. + + Measured, not assumed: instrumenting the seam and `createPromptLikeHandler` produced no + output for a graph run that demonstrably visited `steps#0:step-execute`, while a + module-load write from the same file appeared — so the negative was real and not swallowed + output. + */ + emitWorkflowLifecycleEvent({ + type: "NodeCompleted", + taskId: task.id, + at: new Date().toISOString(), + runId: this.getRunContextFor(task.id)?.runId, + nodeId: typeof governingNodeId === "string" ? governingNodeId : ctx.node.node.id, + outcome: result.taskDone ? "success" : "failure", + ...(result.exit ? { exit: result.exit } : {}), + }); if (result.taskDone) { return { outcome: "success", value: "implemented", data: result }; }