From 26c82ebc182b57dda2b4bb8a6452dcafd990cf0f Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 29 Jul 2026 22:42:26 -0700 Subject: [PATCH] ratchet the planner-liveness gate so a fourth door fails CI (FN-6756) (#2540) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Test-only follow-up to the merged P0 (#2531). No production change, no changeset (internal). ## Why This bug reached users **three times**, each as the same mistake in a new place: | | What happened | |---|---| | FN-8600 | the reclaim sweep removed a worktree a live **planner** was using — fixed by registering planning paths and teaching *that* sweep `isPathActive` | | FN-6756 | the leaked-slot reaper never got the same signal; its last line of defense computed liveness from four TaskExecutor-owned maps, so a triage planner matched none of them | | (same PR) | fixing that was not enough — `recoverPausedAbortFailures` **discarded** the refusal and still logged `"Auto-recovered…"`, audited and counted it. The whole bug again, while reporting success | The shared cause is not any one sweep: **“liveness” was re-derived per call site**, so closing one door left the next open and nothing failed. Every one of those fixes was found by review, not by CI. This makes the next one a CI failure. ## Four properties, each written to fail on the exact defect that got through 1. **Every `clearPhantomExecutorBinding?.(` call site consumes its return** — a bare expression statement (including `void`/`await`-prefixed) is the signature of the pause-abort defect. 2. **The destructive path delegates to `hasLiveSessionSurface`** rather than inlining the session-map disjunction — a second copy can drift from the one callers gate on, which is precisely how each sweep got “fixed” without fixing the next. 3. **The probe is wired** in `in-process-runtime`. `self-healing.ts` already records `releaseExecutorWorktreeOwnership` as a declared-but-never-wired option that silently no-opped; an unwired *probe* is worse, since `?.() === true` is `false` when unwired and every gate would quietly stop deferring with nothing failing. 4. **The probe counts registered session paths**, not just executor maps — a triage planner appears in no executor-owned map, so that term is the only thing that sees it. Grep-level, comment-stripped, production source only; no engine boot and no fixtures (FN-5048). Fails closed on an empty/moved source file so a rename cannot make it silently check nothing. ## Proven, one injection at a time **The first draft of property 1 was worthless** — its filter chain was convoluted enough to discard every candidate, so the injected bare call passed. Caught by actually running the injection instead of trusting the green, and rewritten as a single “is this a bare expression statement” rule. | Injection | Result | |---|---| | discard the return value | fails, naming the call site | | re-derive liveness inline | fails on the delegation assertion | | unwire the probe | fails, naming `in-process-runtime` | | drop the registry term | fails, naming `activeSessionRegistry` | Clean tree passes 4/4; all three sources restored byte-identical (`git status` shows only the new file). **Verified:** `pnpm lint` clean · engine `tsc` clean · `pnpm test:gate` green (414 + 10 + 71). 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Summary by CodeRabbit * **Tests** * Added safeguards to ensure liveness checks remain consistently enforced. * Verified phantom executor cleanup uses shared session-liveness detection. * Added coverage for registered session paths to prevent false inactive states. * Added fail-closed checks when required runtime source is unavailable. --------- Co-authored-by: Claude Opus 5 (1M context) --- .../__tests__/liveness-gate-ratchet.test.ts | 223 ++++++++++++++++++ 1 file changed, 223 insertions(+) create mode 100644 packages/engine/src/__tests__/liveness-gate-ratchet.test.ts diff --git a/packages/engine/src/__tests__/liveness-gate-ratchet.test.ts b/packages/engine/src/__tests__/liveness-gate-ratchet.test.ts new file mode 100644 index 0000000000..73ba964954 --- /dev/null +++ b/packages/engine/src/__tests__/liveness-gate-ratchet.test.ts @@ -0,0 +1,223 @@ +import { describe, expect, it } from "vitest"; +import { readFileSync } from "node:fs"; +import { join, resolve } from "node:path"; + +/* +FNXC:NodeWorktreeIsolation 2026-07-29-07:10 (FN-6756 — make the fourth door a CI failure): +RATCHET for the "is an agent working this task?" gate. + +This bug reached users THREE times, each time as the same mistake in a new place: + + FN-8600 the self-owned-branch reclaim sweep removed a worktree a live PLANNER was + using. Fixed by registering planning paths in activeSessionRegistry and + teaching THAT sweep to consult isPathActive. + FN-6756 the leaked-slot reaper never got the same signal. Its last-line-of-defense, + clearPhantomExecutorBinding, computed liveness from four TaskExecutor-owned + maps, so a triage planner — owned by TriageProcessor — matched none of them. + (same) fixing that was not enough either: recoverPausedAbortFailures DISCARDED the + refusal and still logged "Auto-recovered…", emitted its audit and counted + the task, so it did the whole bug again while reporting success. + +The shared cause is not any one sweep. It is that "liveness" was RE-DERIVED at each +call site, so closing one door left the next one open and no test failed. These +assertions encode the three properties that keep the doors shut, and each is written +to fail on the exact defect that got through before — see the revert-proof notes in +PR #2531. + +Grep-level and comment-stripped, per the existing tombstone ratchet: no engine boot, +no fixtures (FN-5048 — do not add slow tests). Production source only. +*/ + +const REPO_ROOT = resolve(import.meta.dirname, "../../../.."); + +function readSource(relPath: string): string { + const source = readFileSync(join(REPO_ROOT, relPath), "utf8"); + // FAIL CLOSED: a moved/emptied file must not silently pass every assertion below. + expect(source.length, `${relPath} is empty or unreadable — the ratchet checked nothing`).toBeGreaterThan(1000); + return source; +} + +/** Strip comments so an explanatory FNXC note naming a pattern is not read as the pattern. */ +function stripComments(source: string): string { + return source + .replace(/\/\*[\s\S]*?\*\//g, "") + .replace(/(^|[^:])\/\/.*$/gm, "$1"); +} + + +/** Count call sites of `name`, regardless of receiver or formatting. */ +function countCalls(source: string, name: string): number { + return source.split(`${name}?.(`).length - 1; +} + +/* +FNXC:NodeWorktreeIsolation 2026-07-29-16:20 (PR #2540 review — greptile P2 + coderabbit): +Detect a DISCARDED return receiver-agnostically and across line breaks. + +The first version scanned line-by-line for a literal `this.options.` prefix, which +three separate evasions walked straight through: a call split across lines +(`this.options` / `.clearPhantomExecutorBinding?.(…)`), a local alias +(`options.clearPhantomExecutorBinding?.(…)`), and any other receiver spelling. A +ratchet that a reformat defeats is worse than no ratchet — it reports the invariant +is held while it is not, which is the exact failure this file exists to prevent. + +Instead of matching the receiver, walk LEFT from the call over its receiver chain and +any `await`/`void` prefix, then look at the first meaningful character before it. A +statement boundary (`;` `{` `}`) or start-of-file means the call is a bare expression +statement and its `false` goes nowhere. Anything else — `=`, `(`, `&&`, `||`, `!`, +`?`, `:`, `,`, `return` — means the value is consumed. +*/ +function findDiscardedCalls(source: string, name: string): string[] { + const marker = `${name}?.(`; + const discarded: string[] = []; + let from = 0; + for (;;) { + const at = source.indexOf(marker, from); + if (at === -1) break; + from = at + marker.length; + + /* + Walk left over the receiver chain INCLUDING the whitespace inside it, so a call + split across lines (`this.options` \n `.clearPhantomExecutorBinding?.(…)`) is + treated the same as the single-line form. Missing that was the greptile P2: the + first fix walked only chain characters, stopped at the newline, and read + `options` as a consuming context. + */ + let i = at - 1; + while (i >= 0 && /[A-Za-z0-9_$.?!\s]/.test(source[i]!)) i--; + // ...then over any await/void prefix, repeatedly. + for (;;) { + const prefix = source.slice(Math.max(0, i - 5), i + 1); + const matched = /(await|void)$/.exec(prefix); + if (!matched) break; + i -= matched[1]!.length; + while (i >= 0 && /\s/.test(source[i]!)) i--; + } + const preceding = i < 0 ? "" : source[i]!; + if (preceding === "" || preceding === ";" || preceding === "{" || preceding === "}") { + const lineNumber = source.slice(0, at).split("\n").length; + discarded.push(`${SELF_HEALING}:${lineNumber} bare ${name} call`); + } + } + return discarded; +} + +const SELF_HEALING = "packages/engine/src/self-healing.ts"; +const EXECUTOR = "packages/engine/src/executor.ts"; +const IN_PROCESS_RUNTIME = "packages/engine/src/runtimes/in-process-runtime.ts"; + +describe("FN-6756 liveness-gate ratchet", () => { + /* + PROPERTY 1 — every clearPhantomExecutorBinding call site CONSUMES its return value. + + The defect: `recoverPausedAbortFailures` called it bare and threw the boolean away, + so the refusal that the other two callers treat as a stop signal did nothing, and a + live planner lost its worktree while the sweep reported a clean recovery. + + A bare call is the signature of that mistake: the method's entire contract is that + `false` means "refused, do not proceed". Consuming it is `const x = …` or a direct + comparison; anything else is discarding a safety signal. + */ + it("every clearPhantomExecutorBinding call site consumes the return value", () => { + const source = stripComments(readSource(SELF_HEALING)); + const discarded = findDiscardedCalls(source, "clearPhantomExecutorBinding"); + + expect( + countCalls(source, "clearPhantomExecutorBinding"), + "no call sites found — the ratchet is scanning the wrong thing", + ).toBeGreaterThan(0); + expect( + discarded, + "a clearPhantomExecutorBinding call discards its return value — `false` means the release was REFUSED because an agent is live, and ignoring it is how FN-6756 pulled a worktree from under a running planner while logging success", + ).toEqual([]); + }); + + /* + PROPERTY 2 — the destructive path does not RE-DERIVE liveness. + + `clearPhantomExecutorBinding` must delegate to `hasLiveSessionSurface` rather than + inlining the session-map disjunction again. A probe that can disagree with the guard + it stands in for is worse than no probe: callers would gate on one answer and the + release would act on another, which is precisely the drift that let each successive + sweep be "fixed" without fixing the next. + */ + it("clearPhantomExecutorBinding delegates to the shared hasLiveSessionSurface probe", () => { + const source = stripComments(readSource(EXECUTOR)); + const start = source.indexOf("clearPhantomExecutorBinding(taskId: string"); + expect(start, "clearPhantomExecutorBinding not found in executor source").toBeGreaterThan(-1); + const body = source.slice(start, start + 1200); + + expect( + body.includes("this.hasLiveSessionSurface(taskId)"), + "clearPhantomExecutorBinding must call the shared hasLiveSessionSurface probe, not re-derive liveness inline — a second copy can drift from the one callers gate on", + ).toBe(true); + + expect( + /activeSessions\.has|activeStepExecutors\.has|activeWorkflowStepSessions\.has|activeCliTaskSessions\.has/.test(body), + "the session-map disjunction is inlined here again; it belongs only in hasLiveSessionSurface", + ).toBe(false); + }); + + /* + PROPERTY 3 — the probe is WIRED into the runtime. + + self-healing.ts records that `releaseExecutorWorktreeOwnership` was a + declared-but-never-wired option that silently no-opped. An unwired liveness probe is + strictly worse: `this.options.hasLiveSessionSurface?.(id) === true` is FALSE when + unwired, so every gate depending on it would quietly stop deferring for live + sessions and the FN-6756 fix would evaporate with no test failing. + */ + it("hasLiveSessionSurface is wired from the runtime to the self-healing options", () => { + /* + FNXC:NodeWorktreeIsolation 2026-07-29-16:20 (PR #2540 review — coderabbit): + Assert DELEGATION, not the presence of the option key. `hasLiveSessionSurface: + () => false` satisfies a key-presence regex while disabling the gate completely — + the same "declared but inert" shape as the never-wired option this property was + written to catch. Require the callback to forward its argument to the executor's + implementation. + */ + expect( + /hasLiveSessionSurface:\s*\((\w+)[^)]*\)\s*=>\s*this\.executor\?\.hasLiveSessionSurface\(\1/.test( + stripComments(readSource(IN_PROCESS_RUNTIME)), + ), + "in-process-runtime does not forward hasLiveSessionSurface to the executor — an absent or stubbed probe reads as `false` and silently disables every liveness gate that consumes it", + ).toBe(true); + + const selfHealing = stripComments(readSource(SELF_HEALING)); + expect( + selfHealing.includes("hasLiveSessionSurface?:"), + "the self-healing option declaration is gone", + ).toBe(true); + expect( + selfHealing.includes("this.options.hasLiveSessionSurface?.("), + "no self-healing sweep consults the liveness probe — FN-6756's pre-mutation gate is gone", + ).toBe(true); + }); + + /* + PROPERTY 4 — the registry is part of the liveness answer. + + A triage PLANNING session appears in NONE of the executor-owned maps; it registers + in the module-level activeSessionRegistry. Dropping the registry term from the probe + restores the exact blind spot FN-8600 and FN-6756 both went through. + */ + it("hasLiveSessionSurface counts registered session paths, not just executor maps", () => { + const source = stripComments(readSource(EXECUTOR)); + const start = source.indexOf("hasLiveSessionSurface(taskId: string): boolean"); + expect(start, "hasLiveSessionSurface not found — the probe was removed or renamed").toBeGreaterThan(-1); + /* + FNXC:NodeWorktreeIsolation 2026-07-29-16:20 (PR #2540 review — coderabbit): + FAIL CLOSED on a missing boundary. `indexOf` returning -1 made `slice(start, -1)` + scan nearly the whole of executor.ts, so an unrelated later `activeSessionRegistry` + reference could satisfy this assertion after the probe itself was deleted. + */ + const end = source.indexOf("\n }", start); + expect(end, "could not find the end of hasLiveSessionSurface — the ratchet would scan the whole file").toBeGreaterThan(start); + const body = source.slice(start, end); + + expect( + body.includes("activeSessionRegistry.pathsForTask(taskId)"), + "hasLiveSessionSurface no longer consults activeSessionRegistry — a triage planner is owned by TriageProcessor and appears in NO executor-owned map, so this term is the only thing that sees it", + ).toBe(true); + }); +});