mirror of
https://github.com/aaronsb/obsidian-mcp-plugin.git
synced 2026-07-22 06:45:14 +00:00
fix(bridge): start under Claude Desktop's loader, not just node server.js
0.11.34 shipped a broken bridge: #243 gated startup on `require.main === module`, but Claude Desktop runs the .mcpb with its built-in Node ("Using built-in Node.js for MCP server") via a loader that does NOT make server.js the main module. So main() never ran, stdin was never read, the client's `initialize` went unanswered, and Desktop timed out after 60s ("Request timed out", -32001) — the extension "couldn't connect at all". (Local `node mcpb/server.js` worked, which is why it passed review: that path makes require.main === module true.) Gate on the absence of Jest's worker env instead. main() then runs in every real launch — system `node server.js` and Desktop's built-in Node alike — while the require()-based test seam stays inert under Jest. Reproduced via `node -e "require('./mcpb/server.js')"` (hangs before, responds after). Add tests/bridge-bootstrap.test.ts: spawns the bridge against a stub HTTP server and asserts it answers `initialize` in BOTH launch shapes (`node server.js` AND non-main require). The existing unit tests `require()` the module — which by design skips main() — so they could never catch a boot failure; this is that missing coverage. Verified non-vacuous: reverting the guard fails the non-main case. Refs #243. The GitHub 0.11.34 release .mcpb has this bug and must not be promoted; ship the fix in 0.11.35. Claude-Session: https://claude.ai/code/session_011yowKCMFCBp61DRBu5xQm4
This commit is contained in:
parent
d8860751fb
commit
d79d4b0666
2 changed files with 119 additions and 4 deletions
|
|
@ -229,9 +229,9 @@ async function dispatch(message, isReplay = false) {
|
|||
}
|
||||
}
|
||||
|
||||
// Stdio bootstrap. Guarded so the module can be `require`d in tests (which
|
||||
// drive `dispatch` directly) without validating env, claiming stdin, or
|
||||
// exiting the process. When launched as the bridge, `require.main === module`.
|
||||
// Stdio bootstrap. Invoked unless the Jest test suite is importing this module
|
||||
// to drive `dispatch` directly (see the JEST_WORKER_ID guard at the bottom), so
|
||||
// tests don't validate env, claim stdin, or exit the process.
|
||||
function main() {
|
||||
if (!MCP_URL) {
|
||||
process.stderr.write('[obsidian-mcp-bridge] MCP_URL not set\n');
|
||||
|
|
@ -283,7 +283,18 @@ function main() {
|
|||
});
|
||||
}
|
||||
|
||||
if (require.main === module) {
|
||||
// Start the bridge unless we're being imported by the Jest test suite.
|
||||
//
|
||||
// We intentionally do NOT gate on `require.main === module`. Claude Desktop
|
||||
// runs the bundle with its built-in Node ("Using built-in Node.js for MCP
|
||||
// server" in its logs) via a loader that does NOT make server.js the main
|
||||
// module, so that check is false in production: main() never runs, stdin is
|
||||
// never read, the client's `initialize` goes unanswered, and Desktop times out
|
||||
// after 60s ("Request timed out", -32001). That regression shipped in 0.11.34.
|
||||
// Gating on the absence of Jest's worker env runs main() in every real launch
|
||||
// — system `node server.js` and Desktop's built-in Node alike — while keeping
|
||||
// the require()-based test seam below inert.
|
||||
if (!process.env.JEST_WORKER_ID) {
|
||||
main();
|
||||
}
|
||||
|
||||
|
|
|
|||
104
tests/bridge-bootstrap.test.ts
Normal file
104
tests/bridge-bootstrap.test.ts
Normal file
|
|
@ -0,0 +1,104 @@
|
|||
/**
|
||||
* Regression harness for the bridge bootstrap (0.11.34 boot bug).
|
||||
*
|
||||
* The .mcpb bridge (mcpb/server.js) is bundled as a single file and launched as
|
||||
* a process. The unit tests (bridge-self-heal) `require()` it and drive
|
||||
* `dispatch` directly — which by design skips `main()` — so they cannot catch a
|
||||
* failure to *start*. That gap shipped: #243 gated startup on
|
||||
* `require.main === module`, but Claude Desktop loads the bundle with its
|
||||
* built-in Node via a loader that does NOT make server.js the main module, so
|
||||
* main() never ran, stdin was never read, and `initialize` hung until Desktop's
|
||||
* 60s timeout.
|
||||
*
|
||||
* This test actually spawns the bridge against a stub HTTP server and asserts it
|
||||
* answers `initialize` — in BOTH launch shapes:
|
||||
* 1. `node server.js` (require.main === module)
|
||||
* 2. `node -e "require('server.js')"` (require.main !== module — Desktop-like)
|
||||
* Mode 2 is the one that regressed.
|
||||
*/
|
||||
import { spawn } from 'node:child_process';
|
||||
import http from 'node:http';
|
||||
import path from 'node:path';
|
||||
import type { AddressInfo } from 'node:net';
|
||||
|
||||
const SERVER = path.resolve(__dirname, '../mcpb/server.js');
|
||||
const INIT = JSON.stringify({
|
||||
jsonrpc: '2.0', id: 0, method: 'initialize',
|
||||
params: { protocolVersion: '2025-06-18', capabilities: {}, clientInfo: { name: 't', version: '0' } },
|
||||
});
|
||||
|
||||
// Minimal MCP HTTP stub: answers initialize with a session id + result, drains
|
||||
// the GET notify stream, and 200s everything else. Enough for the bridge to
|
||||
// complete a handshake and emit the initialize result to stdout.
|
||||
function startStub(): Promise<{ url: string; close: () => Promise<void> }> {
|
||||
return new Promise((resolve) => {
|
||||
const server = http.createServer((req, res) => {
|
||||
if (req.method === 'GET' || req.method === 'DELETE') { res.writeHead(200).end(); return; }
|
||||
let body = '';
|
||||
req.on('data', (c) => { body += c; });
|
||||
req.on('end', () => {
|
||||
const msg = JSON.parse(body || '{}');
|
||||
if (msg.method === 'initialize') {
|
||||
res.writeHead(200, { 'Content-Type': 'application/json', 'mcp-session-id': 'stub-session' });
|
||||
res.end(JSON.stringify({ jsonrpc: '2.0', id: msg.id, result: { serverInfo: { name: 'stub' } } }));
|
||||
} else {
|
||||
res.writeHead(202).end();
|
||||
}
|
||||
});
|
||||
});
|
||||
server.listen(0, '127.0.0.1', () => {
|
||||
const port = (server.address() as AddressInfo).port;
|
||||
resolve({
|
||||
url: `http://127.0.0.1:${port}/mcp`,
|
||||
close: () => new Promise((r) => server.close(() => r())),
|
||||
});
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
// Spawn the bridge with the given node args, feed it `initialize`, and resolve
|
||||
// with the first JSON object it writes to stdout (the initialize result).
|
||||
function bootAndInitialize(nodeArgs: string[], mcpUrl: string): Promise<Record<string, unknown>> {
|
||||
return new Promise((resolve, reject) => {
|
||||
// Strip JEST_WORKER_ID: the child IS a real bridge launch, and the bootstrap
|
||||
// guard deliberately skips main() when that var is present (test seam).
|
||||
const env: NodeJS.ProcessEnv = { ...process.env, MCP_URL: mcpUrl, MCP_API_KEY: 'k' };
|
||||
delete env.JEST_WORKER_ID;
|
||||
|
||||
const child = spawn(process.execPath, nodeArgs, { env, stdio: ['pipe', 'pipe', 'pipe'] });
|
||||
let out = '';
|
||||
const timer = setTimeout(() => { child.kill(); reject(new Error(`no initialize response (bridge hung). stderr: ${err}`)); }, 8000);
|
||||
let err = '';
|
||||
child.stderr.on('data', (c) => { err += c; });
|
||||
child.stdout.on('data', (c) => {
|
||||
out += c;
|
||||
const line = out.split('\n').find((l) => l.trim());
|
||||
if (line) {
|
||||
clearTimeout(timer);
|
||||
child.kill();
|
||||
try { resolve(JSON.parse(line)); } catch (e) { reject(e as Error); }
|
||||
}
|
||||
});
|
||||
child.on('error', (e) => { clearTimeout(timer); reject(e); });
|
||||
child.stdin.write(INIT + '\n');
|
||||
});
|
||||
}
|
||||
|
||||
describe('mcpb bridge bootstrap — starts under both launch shapes (0.11.34 boot bug)', () => {
|
||||
let stub: { url: string; close: () => Promise<void> };
|
||||
beforeAll(async () => { stub = await startStub(); });
|
||||
afterAll(async () => { await stub.close(); });
|
||||
|
||||
test('launched as `node server.js` (require.main === module) answers initialize', async () => {
|
||||
const res = await bootAndInitialize([SERVER], stub.url);
|
||||
expect(res.id).toBe(0);
|
||||
expect((res as { result?: unknown }).result).toBeDefined();
|
||||
}, 12000);
|
||||
|
||||
test('launched as a non-main require (Desktop-style loader) answers initialize', async () => {
|
||||
const requireExpr = `require(${JSON.stringify(SERVER)})`;
|
||||
const res = await bootAndInitialize(['-e', requireExpr], stub.url);
|
||||
expect(res.id).toBe(0);
|
||||
expect((res as { result?: unknown }).result).toBeDefined();
|
||||
}, 12000);
|
||||
});
|
||||
Loading…
Reference in a new issue