ratchet the planner-liveness gate so a fourth door fails CI (FN-6756) (#2540)

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) <noreply@anthropic.com>
2026-07-29 22:42:26 -07:00
parent 55ce01335c
commit 26c82ebc18
1 changed files with 223 additions and 0 deletions
--- a/packages/engine/src/tests/liveness-gate-ratchet.test.ts
+++ 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);
+  });
+});