mirror of
https://github.com/thedotmack/claude-mem.git
synced 2026-08-03 13:52:49 +08:00
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:
File diff suppressed because one or more lines are too long
File diff suppressed because one or more lines are too long
@@ -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.`
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user