From 319e051c654e99800b2a7cf1ebd5ef1a5c36eb97 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 27 Jul 2026 21:09:59 -0700 Subject: [PATCH] U8 PR1: pin the execution-lifecycle ownership ledger (measured: 28 executor-owned dispositions vs 3 graph handbacks) (#2490) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First PR of **U8 — the graph owns execution** (plan `docs/plans/2026-07-26-001-refactor-workflow-owned-lifecycle-plan.md`, line ~436). The plan states this unit "is expected to land as several commits; it must not be attempted as one sweep", and Execution-note-first: **characterization before ownership moves**. This is that floor. **No behavior change.** ## Why a ledger and not a refactor U8's goal is "the executor stops deciding *what happens next*" — and that had no measurable form. - **Executor line count does not measure it.** A 3,178-line `runImplementation` can shrink substantially with every lifecycle decision still exactly where it was. - **A green suite measures it least of all.** Every disposition counted below already has passing tests, because each one was *correct behavior* when it was written. What is wrong is the **owner**, not the behavior. So the unit needs a number, and the number has to exist *before* the migration — a ratchet written afterwards cannot prove the migration happened. ## The measured baseline Counted from source, comments stripped, method bodies extracted by brace matching: | Method | `store.moveTask` | `handoffTaskToReview` | terminal `status:"failed"` | `graphCompletion` handbacks | |---|---:|---:|---:|---:| | `runImplementation` (3,178 lines) | 16 | 3 | 9 | **3** | | `handleGraphFailure` (~930 lines) | 0 | 0 | 7 | — | **The implementation phase decides its own lifecycle 28 times and asks the graph 3 times.** These are measured, not estimated. My first `handleGraphFailure` estimate was **wrong** (2 moves / 4 parks); the extractor corrected it to 0 / 7 — the `moveTask` calls that read as belonging to that method sit past its closing brace, in the recovery helpers below it. The correction is in the ledger comment so the next reader does not repeat the misread. ## The finding this makes concrete `createAuthoritativeWorkflowSeams.execute` collapses that entire implementation phase to one boolean: ```ts if (result.taskDone) return { outcome: "success", value: "implemented" }; ``` The graph has no vocabulary for *"the agent stopped because a step is blocked on a pending review"* or *"the session paused after the work was already complete"*. So the implementation phase performs those transitions itself (`executor-exit-while-review-pending`, `paused-after-completion`) and the graph finds out afterwards. That is why `handleGraphFailure` carries `alreadyFinalizedToReview` / `completionFinalized` — **classifiers whose entire job is to recognise a move the graph did not make.** They are compensation for dual ownership, and they are U8's acceptance test: they become unreachable, and then deletable, exactly when the last out-of-band transition is gone. This PR records that contract in source at the seam (FNXC comment), which is where the next PR starts. ## Proof the guard fails on the defect A ratchet that reports success without checking anything is worse than no ratchet. Both failure modes were injected and observed: 1. **The defect it exists to catch** — injected one `await this.store.moveTask(task.id, "in-review", {})` into `runImplementation`'s completion path → ledger fails, `16 -> 17`. 2. **A broken guard** — injected a string literal containing `}` so naive brace matching ends the body early → the size self-check fails at **13 lines**, instead of silently reporting a comfortable zero for every count. Both injections were reverted; `git diff` against the pre-injection copy is empty. ## Direction of travel Executor-owned counts may only go **down**, and a decrement must land with the disposition visible as a **graph outcome** — not merely deleted. An increment is a new out-of-graph lifecycle decision and needs a stated justification in its PR, not a quiet edit to the constant. This is the precursor to U12's planned `no-out-of-graph-lifecycle-writes.test.ts`; when the counts reach their floor the assertion becomes "zero, outside the allowlist", and this file is where that allowlist grows up. ## Preserved behaviors Untouched, and re-run green as the regression floor for everything that follows: FN-8141 honest-blocked exit (`executor-task-done-blocked.test.ts`), FN-7996/FN-7998 tool-failure retry + escalation (`executor-tool-failure-retry.test.ts`), FN-7863 dispatch-loop terminalization and FN-7926 completed-blocked parking (`executor-graph-requeue-gate.test.ts`). ## Verification - `pnpm --filter @fusion/engine exec vitest run` on the ledger + the four preserved-behavior suites + `legacy-tombstones` — **6 files, 49 tests, green** - `pnpm test:gate` — **green** (2/10, 16/299, 1/71) - `pnpm lint` — clean; `tsc --noEmit` on `@fusion/engine` — clean No changeset: test-only plus a source comment, no `@runfusion/fusion` behavior change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Summary by CodeRabbit * **Tests** * Added a lifecycle-ownership “source-scanning” test that analyzes the executor’s task disposition patterns to ensure counts remain consistent across execution and graph-failure flows. * Added safeguards to catch unintended changes to lifecycle handling. * **Documentation** * Documented the lifecycle-ownership boundary for task disposition handling, including how completion and failure transitions are consolidated and how related failure classifiers are affected. --------- Co-authored-by: Claude Opus 5 (1M context) --- ...xecutor-lifecycle-ownership-ledger.test.ts | 225 ++++++++++++++++++ packages/engine/src/executor.ts | 20 ++ 2 files changed, 245 insertions(+) create mode 100644 packages/engine/src/__tests__/executor-lifecycle-ownership-ledger.test.ts diff --git a/packages/engine/src/__tests__/executor-lifecycle-ownership-ledger.test.ts b/packages/engine/src/__tests__/executor-lifecycle-ownership-ledger.test.ts new file mode 100644 index 0000000000..e4f775c155 --- /dev/null +++ b/packages/engine/src/__tests__/executor-lifecycle-ownership-ledger.test.ts @@ -0,0 +1,225 @@ +/* +FNXC:WorkflowExecutionOwnership 2026-07-27-16:10 (U8 / R4, R12 — workflow-owned lifecycle): + +U8's goal has one measurable form: **the executor stops deciding what happens next.** +"executor.ts got smaller" does not measure it (a 3,000-line method can shrink while every +lifecycle decision stays exactly where it was), and "the suite is green" measures it least of +all — every disposition counted below already has passing tests, because each one was correct +behavior when it was written. What is wrong is the OWNER, not the behavior. + +So this is a LEDGER, not an assertion about correctness. It counts, inside the two junction-box +methods, the call sites where the executor performs a lifecycle disposition ITSELF, against the +sites where it hands the decision back to the graph. Every number here is measured from source, +not estimated. + + runImplementation — the implementation phase (the agent session and every way out of it) + handleGraphFailure — the sink every non-success graph run drains into + +WHY THE COUNTS ARE THE POINT. The execute seam collapses that entire implementation phase to one +boolean (`result.taskDone` -> "implemented" | "implementation-incomplete"). The graph therefore +has no vocabulary for "the agent stopped because a review is pending" or "paused after +completing" — so the executor transitions the card itself and the graph finds out afterwards. +`handleGraphFailure`'s `alreadyFinalizedToReview` / `completionFinalized` classifiers exist for +exactly that reason: they are compensation for a transition performed behind the graph's back. +Widening the seam's outcome vocabulary is what lets those counts fall; deleting the compensation +is what proves they fell. + +DIRECTION OF TRAVEL. The executor-owned numbers may only go DOWN, and a decrement must land with +the disposition visible as a graph outcome — not merely deleted. An increment is a new +out-of-graph lifecycle decision and needs an explicit justification in its PR, not a quiet edit +to the constant below. + +RELATION TO U12. The plan's `no-out-of-graph-lifecycle-writes.test.ts` ratchet is the endpoint of +this ledger: once the counts reach their floor, the assertion becomes "zero, outside the +allowlist" and this file is where that allowlist grows up. Seeded here because a ratchet written +after the migration cannot prove the migration happened. + +SELF-CHECK DISCIPLINE. A source-scanning guard that silently matches nothing passes forever, so +two things hold it honest. The extraction is AST-based (`ts.createSourceFile`), not brace +matching over text: a `}` inside a string, a template literal, a regex, or a comment is a token +to the parser, never structure, so ordinary edits to literal content cannot truncate a body or +shift a count. And the extracted bodies are still range-checked below, because "the parser found +something" is not the same as "the parser found the 3.2k-line method we meant". + +WHY AST AND NOT GREP (greptile P2, PR #2490). The first cut brace-matched over comment-stripped +text. Its failure mode was demonstrable rather than theoretical — a single `const brace = "}"` +inside `runImplementation` truncated the body to 13 lines — and while the size guard caught that +loudly, the cost landed on whichever unrelated PR happened to add such a literal. A ratchet that +misfires on innocent edits gets deleted, and then it protects nothing. +*/ +import { describe, expect, it } from "vitest"; +import { readFileSync } from "node:fs"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import ts from "typescript"; + +const EXECUTOR_PATH = join(dirname(fileURLToPath(import.meta.url)), "..", "executor.ts"); + +const SOURCE_FILE = ts.createSourceFile( + EXECUTOR_PATH, + readFileSync(EXECUTOR_PATH, "utf8"), + ts.ScriptTarget.ESNext, + /* setParentNodes */ true, +); + +/** + * Find a class method's body by NAME through the AST. Throws rather than returning empty — a + * silent miss would make every count zero and report "U8 complete" while nothing had changed. + */ +function methodBody(name: string): ts.Block { + let found: ts.Block | undefined; + const visit = (node: ts.Node): void => { + if (ts.isMethodDeclaration(node) && ts.isIdentifier(node.name) && node.name.text === name && node.body) { + if (found) throw new Error(`ambiguous method name (declared more than once): ${name}`); + found = node.body; + } + ts.forEachChild(node, visit); + }; + visit(SOURCE_FILE); + if (!found) throw new Error(`method not found: ${name}`); + return found; +} + +/** + * Count call expressions of `this..…(…)` shapes inside a method body. + * `member` is the dotted path after `this.` — e.g. `store.moveTask` or `handoffTaskToReview`. + * Matching on the callee EXPRESSION (not text) is what makes a call inside a string impossible + * to miscount, and a renamed-but-equivalent call impossible to miss. + */ +function countCalls(body: ts.Block, member: string): number { + const path = member.split("."); + let total = 0; + const matchesPath = (expr: ts.Expression): boolean => { + let current: ts.Expression = expr; + for (let i = path.length - 1; i >= 0; i--) { + if (!ts.isPropertyAccessExpression(current) || current.name.text !== path[i]) return false; + current = current.expression; + } + return current.kind === ts.SyntaxKind.ThisKeyword; + }; + const visit = (node: ts.Node): void => { + if (ts.isCallExpression(node) && matchesPath(node.expression)) total++; + ts.forEachChild(node, visit); + }; + visit(body); + return total; +} + +/** Count calls to a plain identifier (a local callback such as `graphCompletion`). */ +function countIdentifierCalls(body: ts.Block, name: string): number { + let total = 0; + const visit = (node: ts.Node): void => { + if (ts.isCallExpression(node) && ts.isIdentifier(node.expression) && node.expression.text === name) total++; + ts.forEachChild(node, visit); + }; + visit(body); + return total; +} + +/** + * Count object-literal properties that write a terminal park, i.e. `status: "failed"`. + * A property assignment is a node, so `status: "failed"` appearing inside a log string or an + * FNXC note is structurally not a write and is never counted. + */ +function countTerminalParks(body: ts.Block): number { + let total = 0; + const visit = (node: ts.Node): void => { + if ( + ts.isPropertyAssignment(node) + && ts.isIdentifier(node.name) + && node.name.text === "status" + && ts.isStringLiteral(node.initializer) + && node.initializer.text === "failed" + ) total++; + ts.forEachChild(node, visit); + }; + visit(body); + return total; +} + +const RUN_IMPLEMENTATION = methodBody("runImplementation"); +const HANDLE_GRAPH_FAILURE = methodBody("handleGraphFailure"); + +/** The three ways the executor performs a lifecycle disposition itself. */ +const EXECUTOR_OWNED_LABELS = [ + "column transitions (store.moveTask)", + "review transitions (handoffTaskToReview)", + "terminal parks (status: \"failed\")", +] as const; + +/** The one way the implementation phase hands the decision back to the graph. */ +const GRAPH_HANDBACK_LABEL = "graph handbacks (graphCompletion)"; + +function bodyLineCount(body: ts.Block): number { + const { line: start } = SOURCE_FILE.getLineAndCharacterOfPosition(body.getStart(SOURCE_FILE)); + const { line: end } = SOURCE_FILE.getLineAndCharacterOfPosition(body.getEnd()); + return end - start + 1; +} + +function measure(body: ts.Block, includeHandbacks: boolean): Record { + const measured: Record = { + "column transitions (store.moveTask)": countCalls(body, "store.moveTask"), + "review transitions (handoffTaskToReview)": countCalls(body, "handoffTaskToReview"), + "terminal parks (status: \"failed\")": countTerminalParks(body), + }; + if (includeHandbacks) measured[GRAPH_HANDBACK_LABEL] = countIdentifierCalls(body, "graphCompletion"); + return measured; +} + +/* +Measured 2026-07-27 against 387e83643. Lower these as U8 moves a disposition behind a graph +outcome; raising one is a new out-of-graph lifecycle decision and needs a stated reason. +*/ +const LEDGER = { + runImplementation: { + "column transitions (store.moveTask)": 16, + "review transitions (handoffTaskToReview)": 3, + "terminal parks (status: \"failed\")": 9, + "graph handbacks (graphCompletion)": 3, + }, + /* + handleGraphFailure moves NO card itself and hands off to NO review — every disposition it + owns is a terminal park. That is a genuinely better starting position than the + implementation phase, and it is measured, not assumed: the moveTask calls that look like + they belong to this method sit past its closing brace, in the recovery helpers below it. + */ + handleGraphFailure: { + "column transitions (store.moveTask)": 0, + "review transitions (handoffTaskToReview)": 0, + "terminal parks (status: \"failed\")": 7, + }, +} as const; + +describe("U8 execution-lifecycle ownership ledger", () => { + /* + The extraction guard. Parsing cannot silently truncate a body the way brace matching could, + but it CAN silently find the wrong thing — a renamed method, a moved declaration, a parser + that gave up. `runImplementation` is ~3.2k lines and `handleGraphFailure` ~0.9k; anything far + outside that is not the method this ledger is about, and every count below would be measuring + something else while still reporting a comfortable pass. + */ + it("extracts both junction-box method bodies at their real size", () => { + expect(bodyLineCount(RUN_IMPLEMENTATION)).toBeGreaterThan(2000); + expect(bodyLineCount(RUN_IMPLEMENTATION)).toBeLessThan(4500); + expect(bodyLineCount(HANDLE_GRAPH_FAILURE)).toBeGreaterThan(500); + expect(bodyLineCount(HANDLE_GRAPH_FAILURE)).toBeLessThan(1600); + }); + + it("runImplementation: executor-owned dispositions match the ledger", () => { + expect(measure(RUN_IMPLEMENTATION, true)).toEqual(LEDGER.runImplementation); + }); + + it("handleGraphFailure: executor-owned dispositions match the ledger", () => { + expect(measure(HANDLE_GRAPH_FAILURE, false)).toEqual(LEDGER.handleGraphFailure); + }); + + /* + The headline number, stated once so a reader does not have to add the ledger up: the + implementation phase decides its own lifecycle 28 times and asks the graph 3 times. + */ + it("states the U8 baseline ratio: the implementation phase decides far more than it asks", () => { + const owned = EXECUTOR_OWNED_LABELS.reduce((sum, label) => sum + LEDGER.runImplementation[label], 0); + const handbacks = LEDGER.runImplementation[GRAPH_HANDBACK_LABEL]; + expect({ owned, handbacks }).toEqual({ owned: 28, handbacks: 3 }); + }); +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index fb391f2792..4fe57435be 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -7760,6 +7760,26 @@ export class TaskExecutor { this.graphSeamGoverningNodeId.delete(seamTask.id); this.graphSeamThinkingLevel.delete(seamTask.id); } + /* + FNXC:WorkflowExecutionOwnership 2026-07-27-16:25 (U8 / R4): + THIS BOOLEAN IS THE OWNERSHIP BOUNDARY, and it is too narrow. `runImplementation` has + 28 measured ways of disposing of a task (16 column moves, 3 review handoffs, 9 terminal + parks — counted by `executor-lifecycle-ownership-ledger.test.ts`) and exactly 3 ways of + telling the graph anything, all of which collapse to `taskDone: true` here. + + The consequence is not a missing feature, it is a second lifecycle owner. Because the + seam has no value for "the agent stopped because a step is blocked on a pending review" + or "the session was paused after the work was already complete", the implementation + phase performs those transitions ITSELF (`executor-exit-while-review-pending`, + `paused-after-completion`) and the graph learns about them afterwards — which is why + `handleGraphFailure` carries `alreadyFinalizedToReview` / `completionFinalized` + classifiers whose whole job is to recognise a move the graph did not make. + + U8's direction: widen this vocabulary so a disposition is REPORTED here and the graph + routes it, rather than performed upstream and compensated for downstream. The + compensating classifiers are the acceptance test — they become unreachable, and then + deletable, exactly when the last out-of-band transition is gone. + */ if (result.taskDone) { return { outcome: "success", value: "implemented" }; }