refactor(mcp): close the gaps a cleanup pass found in the #2802 review fixes

Quality pass over the review-response series (reuse / simplification /
efficiency / altitude). No behaviour change except where noted.

The two that mattered:

- **The cfg/emit fix had no guard.** `FORBIDDEN_RE` covers
  `core/ingestion/languages/` and `FORBIDDEN_GROUP_RE` covers
  `core/group/extractors/|tree-sitter`; neither matches `core/ingestion/cfg/`.
  Because `emit.ts` re-exports the constants, pointing `pdg-impact.ts` back at
  `cfg/emit.js` typechecks identically and silently restores all 7 modules.
  Verified: with the import reverted, `tsc --noEmit` still exits 0 and every
  test stayed green before this commit; after it, 3 rows go red naming the
  offenders. Written as an ALLOWLIST of genuine leaves rather than a denylist of
  the 7 already-suffered modules, because the next regression is a module nobody
  has thought of yet.

- **`FORBIDDEN_GROUP_RE`'s parser matcher was forward-slash only** while both
  sibling probe regexes spell the separator `[\\/]`. Native bindings arrive via
  the `require.cache` channel as absolute paths and `toRepoRelativePosix` only
  normalises paths inside the repo root, so a hoisted `node_modules` renders as
  `…\node_modules\tree-sitter\…` on Windows and matched nothing. The same series
  put this file on the Windows matrix, where that half of the assertion would
  have been vacuous.

Reuse — three re-implementations of existing helpers:

- `removeTempDirRecursive` re-rolled `fs.rmSync` retries; it now delegates to
  `cleanupTempDirSync` (`test-db.ts`), the repo's Windows-lock-aware remover.
  The copy had already drifted on both knobs that matter — 3 retries at 50 ms
  vs 5 at 100–400 ms, and warn-on-everything vs swallow-lock-codes-rethrow-rest
  — which is how one half of a suite goes green-with-a-warning on the same
  `EBUSY` the other half fails on. The per-directory try/warn loop, which is the
  actual fix, is unchanged.
- `errorChainText` re-rolled the cause-chain walk that `causeChain`
  (`src/lib/utils.ts`) exists to be the single copy of — its own doc asks
  callers not to.
- The SIGKILL escalation (a timer, an `unref`, and two `clearTimeout`s) is
  `spawn`'s own `killSignal` option, which Node's `timeout` already delivers.

Simplification and altitude:

- `'callee-ids-unrecorded'` documented ONE of its three producer paths. The
  unnamed common one is a call site that did not RESOLVE — exactly the
  receiver gaps this repo pins (#2807) — so on a real index the reason fires
  broadly, driven by resolution quality rather than a missing `--pdg` layer,
  and "re-run analyze --pdg" is the wrong remedy for it. Doc now names all
  three and states the consequence: `examinedComplete: true` is the strong,
  rare signal.
- The derived policy-entry list was re-pinned against a hand-written 3-element
  literal, reinstating one layer down the list the derivation removes. Now
  asserts the properties that are actually at risk — non-emptiness (a policy
  going silent) and `cli/mcp.js` staying excluded (a row that cannot fail).
- A test fixture spread `ascentBlockCell: 'idless'` and then overrode it to
  `'capped'` in both runs, so the id-less shape never reached the mock while
  reading as though it did.
- `idlessCallSites` is sticky, so its per-row string allocation now
  short-circuits once set.
- Dropped an unused `export` on `CleanupWarner`.

Refs #2802

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

gitnexus/src/mcp/local/pdg-impact.ts

@@ -587,11 +587,30 @@ export type PdgImpactEvidence =
  *    `parseCalleeIdsCell` strips the sentinel, so the dropped ids are invisible to
  *    BOTH the summary scan and the counters.
  *  - `'callee-ids-unrecorded'` — a slice block records CALL SITES (a non-empty
- *    `callees` name cell) but NO resolved callee ids. The emitter writes an empty
+ *    `callees` name cell) but NO resolved callee ids. An empty `calleeIds` cell
- *    `calleeIds` cell for a whole file whose resolved-id map is absent, and an
+ *    carries no sentinel, so those call sites are invisible to the summary scan
- *    empty cell carries no sentinel, so those call sites are invisible to the
+ *    without raising `'callee-list-capped'`. Distinct from the cap: nothing was
- *    summary scan without raising `'callee-list-capped'`. Distinct from the cap:
+ *    dropped at emit — the ids were never recorded.
- *    nothing was dropped at emit — the ids were never recorded.
+ *
+ *    THREE producer paths yield it, and the consumer cannot tell them apart —
+ *    do not read this code as naming any one of them (`cfg/emit.ts`,
+ *    `calleeIdsOfBlock`):
+ *      1. the file's resolved-id map is absent entirely (`fileMap === undefined`);
+ *      2. a call site has no position anchor;
+ *      3. a call site's position IS in the map but did not RESOLVE.
+ *    (3) is the ordinary one — it is exactly the receiver-resolution gaps this
+ *    repo pins (e.g. #2807's inference-typed field receivers, where `calleeIds`
+ *    empties while `calleesOfBlock` still writes the leaf names). So on a real
+ *    index this fires broadly and is driven by resolution quality, NOT by a
+ *    missing `--pdg` layer: "re-run analyze --pdg" is the wrong remedy for it,
+ *    and `examinedComplete: false` here is a statement about how much of the
+ *    call graph resolved, not about the traversal giving up.
+ *
+ *    Consequence worth knowing before branching on it: because (3) is common,
+ *    `examinedComplete: true` is the strong, rare signal and `false` is close to
+ *    the default on a large repo. Distinguishing the three needs a marker at
+ *    emit time, which would move the persisted cell format — deliberately out of
+ *    scope here, and tracked separately.
  */
 export type PdgAscentIncompleteReason =
   | 'traversal-truncated'
@@ -1763,7 +1782,14 @@ async function calleeIdsByBlock(
     // Ids absent while NAMES are present ⇒ recorded call sites with no resolved
     // id. Gated on `!truncated` so a capped-to-nothing cell keeps reporting the
     // cap (the more specific mechanism) rather than both.
-    if (calleeIds.length === 0 && !truncated && String(r['callees'] ?? '').trim().length > 0) {
+    // `!idlessCallSites` first: the flag is sticky, so once it is set the string
+    // allocation below is pure waste on every remaining row of every later hop.
+    if (
+      !idlessCallSites &&
+      calleeIds.length === 0 &&
+      !truncated &&
+      String(r['callees'] ?? '').trim().length > 0
+    ) {
       idlessCallSites = true;
     }
     if (calleeIds.length > 0) out.push({ blockId, calleeIds });

gitnexus/test/helpers/module-load-probe.ts

@@ -74,16 +74,6 @@ const END = '<<<END_GITNEXUS_PROBE>>>';
  */
 const PROBE_TIMEOUT_MS = 60_000;
 
-/**
- * Grace between `spawn`'s own timeout SIGTERM and an unconditional SIGKILL.
- * `spawn({ timeout })` signals ONCE; a child wedged inside synchronous native
- * code (a grammar binding's init, an addon `dlopen`) never reaches a point
- * where it can handle the signal, so without escalation it is orphaned rather
- * than reaped and the vitest worker waits on a dead probe. Nothing asserts on
- * either duration — both are wedge-breakers.
- */
-const PROBE_KILL_GRACE_MS = 5_000;
-
 const PROBE_SOURCE = `
   import { createRequire, registerHooks } from 'node:module';
 
@@ -236,14 +226,12 @@ function spawnProbe(targetUrl: string, extraEnv: Readonly<Record<string, string>
       // exists to make impossible.
       env: { ...process.env, NODE_OPTIONS: '', ...extraEnv, PROBE_TARGET: targetUrl },
       timeout: PROBE_TIMEOUT_MS,
+      // SIGKILL rather than the default SIGTERM, which is catchable and
+      // ignorable — a child wedged in synchronous native code (the failure this
+      // guards) would survive it. Same reasoning as `lbug-config.ts`'s spawn.
+      // Node's own `timeout` delivers this, so no second timer to keep in step.
+      killSignal: 'SIGKILL',
     });
-    // `timeout` above sends a single SIGTERM. Escalate unconditionally so a
-    // child stuck in synchronous native code is reaped rather than orphaned.
-    // `unref` keeps a healthy sub-second probe from holding the event loop.
-    const escalate = setTimeout(() => {
-      child.kill('SIGKILL');
-    }, PROBE_TIMEOUT_MS + PROBE_KILL_GRACE_MS);
-    escalate.unref();
     let stdout = '';
     let stderr = '';
     child.stdout.setEncoding('utf8');
@@ -255,11 +243,9 @@ function spawnProbe(targetUrl: string, extraEnv: Readonly<Record<string, string>
       stderr += chunk;
     });
     child.on('error', (error) => {
-      clearTimeout(escalate);
       reject(error);
     });
     child.on('close', (status, signal) => {
-      clearTimeout(escalate);
       resolve({ status, signal, stdout, stderr });
     });
   });

gitnexus/test/helpers/temp-dir-pool.ts

@@ -30,6 +30,7 @@ import fs from 'fs';
 import os from 'os';
 import path from 'path';
 import { afterAll } from 'vitest';
+import { cleanupTempDirSync } from './test-db.js';
 
 export interface TempDirPool {
   /** A fresh empty temp dir, registered for cleanup. Seed it yourself. */
@@ -42,26 +43,31 @@ export interface TempDirPool {
 export type TempDirRemover = (dir: string) => void;
 
 /** Reports one cleanup failure. A seam, for the same reason. */
-export type CleanupWarner = (message: string) => void;
+type CleanupWarner = (message: string) => void;
 
 /**
- * `force` suppresses only `ENOENT`. A handle a pipeline test left open on the
+ * Delegates to `cleanupTempDirSync`, the repo's existing Windows-lock-aware
- * directory surfaces on Windows as `EBUSY`/`EPERM`, which `force` does not
+ * remover — do NOT re-roll `fs.rmSync` here. It already encodes the whole
- * suppress — hence `maxRetries`, Node's own mitigation for exactly that class
+ * problem this pool hit: `force` suppresses only `ENOENT`, while a handle a
- * (it retries `EBUSY`/`EMFILE`/`ENFILE`/`ENOTEMPTY`/`EPERM` with a linear
+ * pipeline test left open surfaces as `EBUSY`/`EPERM`, so it retries 5× with a
- * backoff). The retries only ever run on the failing path.
+ * 100–400 ms backoff and then swallows exactly the Windows lock codes and
+ * `ENOTEMPTY` — rethrowing anything else, so a genuine bug still surfaces
+ * through `removeTempDirs`' per-directory catch below.
+ *
+ * A second copy here had already drifted from it on both knobs that matter
+ * (3 retries at 50 ms, and warn-on-everything), which is how one half of the
+ * suite ends up green-with-a-warning on the same `EBUSY` the other half fails on.
  *
  * Exported so the cleanup pin can inject a failure for ONE directory while the
  * others still go through the removal that actually ships — a proof against a
  * stand-in `fs.rmSync` call in the test would not be one.
  */
 export const removeTempDirRecursive: TempDirRemover = (dir) => {
-  fs.rmSync(dir, { recursive: true, force: true, maxRetries: 3, retryDelay: 50 });
+  cleanupTempDirSync(dir);
 };
 
-const warnToConsole: CleanupWarner = (message) => {
+// Node's console methods are bound, so this can be the default directly.
-  console.warn(message);
+const warnToConsole: CleanupWarner = console.warn;
-};
 
 /**
  * Remove every registered directory, best-effort: one failure must not abort

gitnexus/test/integration/group/group-service-sync-lazy-import.test.ts

@@ -32,6 +32,7 @@ import { createTempDirPool } from '../../helpers/temp-dir-pool.js';
 import { makeGroupToolPort } from '../../unit/group/fixtures.js';
 import { GroupService } from '../../../src/core/group/service.js';
 import { readContractRegistry } from '../../../src/core/group/storage.js';
+import { causeChain } from '../../../src/lib/utils.js';
 
 const tempDirs = createTempDirPool('gn-group-lazy-sync-');
 
@@ -90,15 +91,11 @@ matching:
  * assertion about the failure that happened, not about how vitest wraps it.
  */
 function errorChainText(err: unknown): string {
-  const messages: string[] = [];
+  // `causeChain` is the repo's single cause-chain traversal — its own doc asks
-  const seen = new Set<unknown>();
+  // callers not to re-roll the loop, because every hand-rolled copy re-decides
-  let current: unknown = err;
+  // the bound and they disagree. Its default depth is 5; real chains here are
-  while (current instanceof Error && !seen.has(current)) {
+  // the runner's wrapper plus the original, so 2.
-    seen.add(current);
+  return [...causeChain(err)].map((link) => link.message).join(' | ');
-    messages.push(current.message);
-    current = current.cause;
-  }
-  return messages.join(' | ');
 }
 
 /**

gitnexus/test/integration/mcp/startup-language-closure.test.ts

@@ -78,8 +78,18 @@ const FORBIDDEN_RE = /(^|\/)core\/ingestion\/languages\//;
  *
  * Matching the parser by its package prefix rather than a bare substring so a
  * source file that merely mentions the word cannot satisfy or trip this.
+ *
+ * The separator is `[\\/]`, matching the sibling probes' `ANY_GRAMMAR_RE` and
+ * `OPTIONAL_GRAMMAR_RE`, NOT a bare `/`. Native bindings reach the probe through
+ * the `require.cache` channel as absolute paths, and `toRepoRelativePosix` only
+ * POSIX-normalises paths INSIDE the repo root — a hoisted `node_modules` renders
+ * verbatim, so on Windows this is `…\node_modules\tree-sitter\…` and a
+ * forward-slash-only pattern silently matches nothing. This file now runs on the
+ * Windows matrix, where that would have made the parser half of the assertion
+ * vacuous. The `core/group/extractors/` half is first-party `dist/**`, always
+ * in-repo and therefore already normalised.
  */
-const FORBIDDEN_GROUP_RE = /(^|\/)core\/group\/extractors\/|(^|\/)node_modules\/tree-sitter/;
+const FORBIDDEN_GROUP_RE = /(^|\/)core\/group\/extractors\/|[\\/]node_modules[\\/]tree-sitter/;
 
 /**
  * The chain the group policy above polices, and therefore ITS non-vacuity
@@ -94,6 +104,33 @@ const FORBIDDEN_GROUP_RE = /(^|\/)core\/group\/extractors\/|(^|\/)node_modules\/
  */
 const GROUP_ANCHOR = 'dist/core/group/service.js';
 
+/**
+ * Third instance of the same defect class, and the one this file could not see.
+ *
+ * `pdg-impact.ts` imported two format constants from `core/ingestion/cfg/emit.ts`.
+ * ESM evaluates a module to import any binding from it, so those two strings
+ * pulled the whole analyze-only CFG closure — `emit`, `reaching-defs`,
+ * `reaching-defs-graph`, `control-dependence`, `post-dominators`,
+ * `synthetic-escape`, `call-site-harvest` — into every MCP start. The constants
+ * moved to the leaf `cfg/callee-cell-format.ts`, but `emit.ts` still RE-EXPORTS
+ * them, so pointing the import back at `emit.js` typechecks identically and
+ * restores all seven modules. Neither existing policy matches
+ * `core/ingestion/cfg/`, so nothing was stopping that.
+ *
+ * Allowlist rather than a denylist of the seven: the failure mode is a module
+ * nobody has thought of yet, and a denylist only ever names the regressions
+ * already suffered. Everything here is a genuine LEAF — zero imports — which is
+ * why it can sit on the startup path at all; that is a real convention in
+ * `core/ingestion` (each file's header calls itself the one shared codec), and
+ * this is the only thing enforcing it.
+ */
+const CFG_ANCHOR = 'dist/core/ingestion/cfg/callee-cell-format.js';
+const INGESTION_CFG_RE = /(^|\/)core\/ingestion\/cfg\//;
+const CFG_LEAVES_ALLOWED: ReadonlySet<string> = new Set([
+  CFG_ANCHOR,
+  'dist/core/ingestion/cfg/reaching-def-reason-codec.js',
+]);
+
 // Observed on Node 22.18 against a clean build at the tip of this branch:
 // server.js 380 distinct modules, local-backend.js 156, cli/mcp.js 4. Treat
 // these as a snapshot, not a contract — they moved twice inside this branch
@@ -120,17 +157,17 @@ const GROUP_ANCHOR = 'dist/core/group/service.js';
 const ENTRIES = [
   {
     entry: 'mcp/server.js',
-    anchor: ['dist/mcp/resources.js', GROUP_ANCHOR],
+    anchor: ['dist/mcp/resources.js', GROUP_ANCHOR, CFG_ANCHOR],
     minModules: 100,
   },
   {
     entry: 'mcp/http-transport.js',
-    anchor: ['dist/mcp/server.js', GROUP_ANCHOR],
+    anchor: ['dist/mcp/server.js', GROUP_ANCHOR, CFG_ANCHOR],
     minModules: 100,
   },
   {
     entry: 'mcp/local/local-backend.js',
-    anchor: ['dist/mcp/local/pdg-impact.js', GROUP_ANCHOR],
+    anchor: ['dist/mcp/local/pdg-impact.js', GROUP_ANCHOR, CFG_ANCHOR],
     minModules: 50,
   },
   // The one row with a single anchor, because it is subject to ONE policy. Its
@@ -153,6 +190,11 @@ const GROUP_POLICY_ENTRIES = ENTRIES.filter((request) =>
   anchorsOf(request.anchor).includes(GROUP_ANCHOR),
 ).map((request) => request.entry);
 
+/** Same derivation for the CFG-leaf policy. */
+const CFG_POLICY_ENTRIES = ENTRIES.filter((request) =>
+  anchorsOf(request.anchor).includes(CFG_ANCHOR),
+).map((request) => request.entry);
+
 describe('MCP startup module-load closure (#2802)', () => {
   let probes: ModuleLoadProbes;
 
@@ -187,17 +229,49 @@ describe('MCP startup module-load closure (#2802)', () => {
     },
   );
 
-  // Pins the derivation above: if a future edit drops `GROUP_ANCHOR` from every
+  // Pins the two derivations above. The risk each guards is a policy going
-  // entry, `GROUP_POLICY_ENTRIES` empties and the `it.each` below silently
+  // SILENT, not its exact membership: drop an anchor from every entry and the
-  // registers zero cases — a whole policy disappearing without one red test.
+  // derived list empties, so the `it.each` registers zero cases and the whole
-  it('runs the group policy over exactly the entries that reach core/group/service.js', () => {
+  // policy disappears without one red test. Asserting non-emptiness catches
-    expect(GROUP_POLICY_ENTRIES).toEqual([
+  // exactly that; asserting the literal list would reinstate, one layer down,
-      'mcp/server.js',
+  // the hand-maintained list the derivation exists to remove — every entry
-      'mcp/http-transport.js',
+  // added or removed would then need editing in two places.
-      'mcp/local/local-backend.js',
+  //
-    ]);
+  // `cli/mcp.js` is pinned OUT of both policies deliberately. It is a 4-module
+  // leaf closure that cannot reach either policed chain, so listing it would
+  // give each policy a row that cannot fail — the vacuity this file exists to
+  // prevent. That exclusion is a real property, so it is asserted rather than
+  // left to the comment above.
+  it.each([
+    ['group', GROUP_POLICY_ENTRIES],
+    ['cfg-leaf', CFG_POLICY_ENTRIES],
+  ])('the %s policy runs over a non-empty entry set that excludes cli/mcp.js', (_name, entries) => {
+    expect(entries.length).toBeGreaterThan(0);
+    expect(entries).not.toContain('cli/mcp.js');
   });
 
+  // The #2802 defect class, third instance: an analyze-only closure reached
+  // through a constant. Allowlist, not denylist — see CFG_LEAVES_ALLOWED.
+  it.each(CFG_POLICY_ENTRIES)(
+    'importing dist/%s loads no non-leaf core/ingestion/cfg module',
+    (entry) => {
+      const probe = probes.get(entry);
+      const offenders = probe
+        .matching(INGESTION_CFG_RE)
+        .filter((module) => !CFG_LEAVES_ALLOWED.has(module));
+
+      expect(
+        offenders,
+        `${probe.label} eagerly loads analyze-only CFG modules. ESM evaluates a ` +
+          `module to import ANY binding from it, so importing a constant from ` +
+          `\`cfg/emit.js\` drags its whole closure onto startup — take format ` +
+          `constants from the leaf \`cfg/callee-cell-format.js\` instead, and add ` +
+          `a new module here only if it genuinely imports nothing (see #2802). ` +
+          `Offending modules:\n${offenders.join('\n')}`,
+      ).toEqual([]);
+    },
+  );
+
   it.each(GROUP_POLICY_ENTRIES)(
     'importing dist/%s loads no group contract extractor or native parser',
     (entry) => {

gitnexus/test/unit/impact-pdg-ascent-note.test.ts

@@ -856,11 +856,11 @@ describe('runImpactPDG — structured ascent coverage (pdgEvidence.ascent)', ()
   it('a block reached only by the ascent contributes its call sites to the scan', async () => {
     const cell = {
       ascentBlockCallees: [hiddenCalleeId(FILE)],
-      ascentBlockCell: 'idless',
+      ascentBlockCell: 'capped',
     } as const;
     const [ascended, withheld] = await Promise.all([
-      run(FILE, { maxDepth: 1, summary: flow([0]), ...cell, ascentBlockCell: 'capped' }),
+      run(FILE, { maxDepth: 1, summary: flow([0]), ...cell }),
-      run(FILE, { maxDepth: 1, ...cell, ascentBlockCell: 'capped' }),
+      run(FILE, { maxDepth: 1, ...cell }),
     ]);
     // Premise: the block below is in the slice ONLY because the ascent fired —
     // withhold the return-flow and it is gone.