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

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

ProcessManager.ts: improve actionability of "Bun not found" errors

Both Windows and Unix branches of spawnDaemon previously logged a vague
"Failed to locate Bun runtime" message when resolveWorkerRuntimePath()
returned null. Replaced with an actionable message that names the install
URL and explains *why* Bun is required (worker uses bun:sqlite). The
existing null-guard at the call sites already prevents passing null to
child_process.spawn — only the error text changed.

scripts/build-hooks.js: refine bun:sqlite guardrail to match actual
require() calls only

The previous coarse `includes('bun:sqlite')` check tripped on its own
improved error message, which legitimately mentions "bun:sqlite" by name.
Switched to a regex that matches `require("bun:sqlite")` /
`require('bun:sqlite')` (with optional whitespace, handles both quote
styles, handles minified output) so error messages and inline comments
can reference the module name without false positives. Verified the
regex still trips on real violations (both spaced and minified forms)
and correctly ignores string-literal mentions.

Other round-3 items (verified, not changed):

- TOOL_ENDPOINT_MAP: reviewer flagged as dead code, but it IS used at
  lines 250 and 263 by the search and timeline tool handlers. False
  positive — kept as-is.
- if (!pid) callsites: grepped src/, zero offenders. The Windows `0`
  PID sentinel contract is safe; only the in-line documentation comment
  in ProcessManager.ts mentions the anti-pattern.
- callWorkerAPIPost double-wrapping: pre-existing intentional behavior
  (only used by /api/observations/batch which returns raw data, not
  the MCP {content:[...]} shape). Unrelated to this regression.
- Snap path / startParentHeartbeat / main().catch / test for non-
  existent workerScriptPath / etc — pre-existing or out of scope for
  this hotfix, deferred per established disposition.

Verified: build clean, guardrail still trips on real violations,
mcp-server.cjs has 0 require("bun:sqlite") calls, JSON-RPC tools/list
returns the 7-tool surface, ProcessManager tests 43/43.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Alex Newman
2026-04-07 16:43:26 -07:00
parent 7a96b3b9fc
commit 193286f9c5
4 changed files with 16 additions and 6 deletions

File diff suppressed because one or more lines are too long

File diff suppressed because one or more lines are too long

View File

@@ -249,10 +249,14 @@ async function buildHooks() {
// 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.
//
// 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.
const mcpBundleContent = fs.readFileSync(`${hooksDir}/${MCP_SERVER.name}.cjs`, 'utf-8');
if (mcpBundleContent.includes('bun:sqlite')) {
if (/require\(\s*["']bun:sqlite["']\s*\)/.test(mcpBundleContent)) {
throw new Error(
`mcp-server.cjs contains a 'bun:sqlite' reference. 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 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.`
);
}

View File

@@ -668,7 +668,10 @@ export function spawnDaemon(
const runtimePath = resolveWorkerRuntimePath();
if (!runtimePath) {
logger.error('SYSTEM', 'Failed to locate Bun runtime for Windows worker spawn');
logger.error(
'SYSTEM',
'Bun runtime not found — install from https://bun.sh and ensure it is on PATH or set BUN env var. The worker daemon requires Bun because it uses bun:sqlite.'
);
return undefined;
}
@@ -707,7 +710,10 @@ export function spawnDaemon(
// the caller is the MCP server. See resolveWorkerRuntimePath() for lookup.
const unixRuntimePath = resolveWorkerRuntimePath();
if (!unixRuntimePath) {
logger.error('SYSTEM', 'Failed to locate Bun runtime for worker spawn');
logger.error(
'SYSTEM',
'Bun runtime not found — install from https://bun.sh and ensure it is on PATH or set BUN env var. The worker daemon requires Bun because it uses bun:sqlite.'
);
return undefined;
}