fix(mcp): address PR #1645 review feedback (round 5)

Round 5 of Claude Code Review feedback on PR #1645:

src/services/worker-spawner.ts: drop `export` from internal helpers

`shouldSkipSpawnOnWindows`, `markWorkerSpawnAttempted`, and
`clearWorkerSpawnAttempted` were exported even though they were
private in worker-service.ts and nothing outside this module needs
them. Removing the `export` keyword keeps the public surface to just
`ensureWorkerStarted` and prevents future callers from bypassing the
spawn lifecycle.

scripts/build-hooks.js: broaden guardrail to all bun:* modules

Previously the regex only caught `require("bun:sqlite")`, but every
module in the `bun:` namespace (bun:ffi, bun:test, etc.) is Bun-only
and would crash mcp-server.cjs the same way under Node. Generalized
the regex to `require("bun:[a-z][a-z0-9_-]*")` so a transitive import
of any Bun-only module fails the build instead of shipping a broken
bundle. Verified the new regex still trips on bun:sqlite, bun:ffi,
bun:test, and correctly ignores string-literal mentions in error
messages.

src/servers/mcp-server.ts: attribute root cause when dirname resolution fails

Previously, if `__dirname`/`import.meta.url` resolution failed and we
fell back to `process.cwd()`, the user would see two warnings: an
error about the dirname fallback AND a separate warning about the
missing worker bundle. The second warning hides the root cause —
someone debugging would assume the install is broken when really it's
a dirname-resolution failure. Track the failure with a flag and emit
a single root-cause-attributing log line in the existence-check
branch instead. The dirname fallback paths are still functionally
unreachable in CJS deployment; this just makes the failure mode
unmistakable if it ever does fire.

Out of scope (consistent with prior rounds):
- darwin/linux split for non-Windows candidate paths (benign today)
- Integration test for non-existent workerScriptPath (test coverage
  gap deferred since rounds 1-2)
- Defer existsSync check to first ensureWorkerStarted call (current
  module-init check is the loud signal we want)

Already addressed in earlier rounds:
- resolveWorkerRuntimePath() called twice in spawnDaemon → hoisted in
  round 4 (b2c114b4)
- _originalLog dead code → removed in round 2 (7a96b3b9)

Verified: build clean, broadened guardrail trips on bun:sqlite,
bun:ffi, and bun:test (and ignores string literals), MCP server
serves the 7-tool surface, ProcessManager tests still 44/44.

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

scripts/build-hooks.js

@@ -244,19 +244,22 @@ async function buildHooks() {
     const mcpServerStats = fs.statSync(`${hooksDir}/${MCP_SERVER.name}.cjs`);
     console.log(`✓ mcp-server built (${(mcpServerStats.size / 1024).toFixed(2)} KB)`);
 
-    // GUARDRAIL (#1645): The MCP server runs under Node, but `bun:sqlite` is
-    // a Bun-only module. If any transitive import in mcp-server.ts ever pulls
-    // it back in, the bundle will crash on first require under Node — which
-    // is exactly the regression PR #1645 fixed. Fail the build instead of
-    // shipping a broken bundle so future contributors get an immediate signal.
+    // GUARDRAIL (#1645): The MCP server runs under Node, but the entire `bun:`
+    // module namespace (bun:sqlite, bun:ffi, bun:test, etc.) is Bun-only. If
+    // any transitive import in mcp-server.ts ever pulls one in, the bundle
+    // will crash on first require under Node — which is exactly the regression
+    // PR #1645 fixed for `bun:sqlite`. Fail the build instead of shipping a
+    // broken bundle so future contributors get an immediate signal.
     //
-    // Only flag actual `require("bun:sqlite")` / `require('bun:sqlite')` calls,
-    // not the bare string — error messages and inline comments may legitimately
-    // mention "bun:sqlite" by name without re-introducing the import.
+    // Only flag actual `require("bun:...")` / `require('bun:...')` calls, not
+    // the bare string — error messages and inline comments may legitimately
+    // mention `bun:sqlite` by name without re-introducing the import.
     const mcpBundleContent = fs.readFileSync(`${hooksDir}/${MCP_SERVER.name}.cjs`, 'utf-8');
-    if (/require\(\s*["']bun:sqlite["']\s*\)/.test(mcpBundleContent)) {
+    const bunRequireRegex = /require\(\s*["']bun:[a-z][a-z0-9_-]*["']\s*\)/;
+    const bunRequireMatch = mcpBundleContent.match(bunRequireRegex);
+    if (bunRequireMatch) {
       throw new Error(
-        `mcp-server.cjs contains a require("bun:sqlite") call. This means a transitive import in src/servers/mcp-server.ts pulled in code from worker-service.ts (or another module that touches DatabaseManager/ChromaSync). The MCP server runs under Node and cannot load bun:sqlite. Audit recent imports in src/servers/mcp-server.ts and src/services/worker-spawner.ts — the spawner module is intentionally lightweight and MUST NOT import anything that touches SQLite. See PR #1645 for context.`
+        `mcp-server.cjs contains a Bun-only ${bunRequireMatch[0]} call. This means a transitive import in src/servers/mcp-server.ts pulled in code from worker-service.ts (or another module that touches DatabaseManager/ChromaSync). The MCP server runs under Node and cannot load bun:* modules. Audit recent imports in src/servers/mcp-server.ts and src/services/worker-spawner.ts — the spawner module is intentionally lightweight and MUST NOT import anything that touches SQLite or other Bun-only modules. See PR #1645 for context.`
       );
     }
 

src/servers/mcp-server.ts

@@ -39,19 +39,22 @@ import { fileURLToPath } from 'node:url';
 // in the plugin's scripts directory. We need an explicit path because the MCP
 // server runs under Node while the worker must run under Bun, so we can't rely
 // on `__filename` pointing to a self-spawnable script.
+//
+// In the deployed CJS bundle, `__dirname` is always defined — the import.meta
+// fallback only exists to keep the source future-proof against an eventual
+// ESM port. Both fallback branches should be functionally unreachable today.
+let mcpServerDirResolutionFailed = false;
 const mcpServerDir = (() => {
   if (typeof __dirname !== 'undefined') return __dirname;
   try {
     return dirname(fileURLToPath(import.meta.url));
   } catch {
     // Last-ditch fallback: cwd is almost certainly wrong, but throwing here
-    // would crash the MCP server before it can serve a single request. Log
-    // at ERROR so the existence check below has a loud, searchable breadcrumb
-    // — a wrong WORKER_SCRIPT_PATH means worker auto-start will silently fail.
-    logger.error(
-      'SYSTEM',
-      'mcp-server: unable to resolve __dirname or import.meta.url; falling back to process.cwd() — WORKER_SCRIPT_PATH will almost certainly be wrong and worker auto-start will fail'
-    );
+    // would crash the MCP server before it can serve a single request. Mark
+    // the failure so the existence check below can produce a single, loud,
+    // root-cause-attributing log line instead of a confusing "missing worker
+    // bundle" warning that hides the dirname resolution failure.
+    mcpServerDirResolutionFailed = true;
     return process.cwd();
   }
 })();
@@ -60,12 +63,24 @@ const WORKER_SCRIPT_PATH = resolve(mcpServerDir, 'worker-service.cjs');
 // Surface a clear, actionable error early if the worker bundle isn't where
 // we expect. Without this check, a missing or partial install only fails
 // later inside spawnDaemon as a generic "failed to spawn" message.
+//
+// If dirname resolution itself failed (extremely unlikely in CJS), attribute
+// the missing-bundle warning to the root cause so the user doesn't waste time
+// looking for an install bug that doesn't exist.
 if (!existsSync(WORKER_SCRIPT_PATH)) {
-  logger.warn(
-    'SYSTEM',
-    'worker-service.cjs not found at expected path — auto-start will fail until it is built/installed',
-    { workerScriptPath: WORKER_SCRIPT_PATH, mcpServerDir }
-  );
+  if (mcpServerDirResolutionFailed) {
+    logger.error(
+      'SYSTEM',
+      'mcp-server: dirname resolution failed (both __dirname and import.meta.url are unavailable). Fell back to process.cwd() and the resolved WORKER_SCRIPT_PATH does not exist. This is the actual problem — the worker bundle is fine, but mcp-server cannot locate it. Worker auto-start will fail until the dirname-resolution path is fixed.',
+      { workerScriptPath: WORKER_SCRIPT_PATH, mcpServerDir }
+    );
+  } else {
+    logger.warn(
+      'SYSTEM',
+      'worker-service.cjs not found at expected path — auto-start will fail until it is built/installed',
+      { workerScriptPath: WORKER_SCRIPT_PATH, mcpServerDir }
+    );
+  }
 }
 
 /**

src/services/worker-spawner.ts

@@ -35,7 +35,11 @@ function getWorkerSpawnLockPath(): string {
   return path.join(SettingsDefaultsManager.get('CLAUDE_MEM_DATA_DIR'), '.worker-start-attempted');
 }
 
-export function shouldSkipSpawnOnWindows(): boolean {
+// Internal helpers — NOT exported. Only ensureWorkerStarted should be on the
+// public surface; callers must not bypass the lifecycle by calling these
+// directly. See PR #1645 review feedback for context.
+
+function shouldSkipSpawnOnWindows(): boolean {
   if (process.platform !== 'win32') return false;
   const lockPath = getWorkerSpawnLockPath();
   if (!existsSync(lockPath)) return false;
@@ -47,7 +51,7 @@ export function shouldSkipSpawnOnWindows(): boolean {
   }
 }
 
-export function markWorkerSpawnAttempted(): void {
+function markWorkerSpawnAttempted(): void {
   if (process.platform !== 'win32') return;
   try {
     const lockPath = getWorkerSpawnLockPath();
@@ -67,7 +71,7 @@ export function markWorkerSpawnAttempted(): void {
   }
 }
 
-export function clearWorkerSpawnAttempted(): void {
+function clearWorkerSpawnAttempted(): void {
   if (process.platform !== 'win32') return;
   try {
     const lockPath = getWorkerSpawnLockPath();