Skip to content

Commit e29e224

Browse files
simongdaviesCopilot
simongdavies
and
Copilot
committed
fix: address review comments on MCP server example
- Factor NDJSON transport helpers into shared tests/helpers.js module to eliminate duplicated reader logic across all 4 test files (#1) - Remove confusing BBP parenthetical comment; the implementation uses Machin's formula throughout (#9) - Replace Math.random() with seeded PRNG (mulberry32) in test code constants for deterministic, non-flaky test results (#10) - Tighten timing assertion from executeMs > 0 to > 1 for meaningful validation (#10) Note: Issues #2-8 and #11 from the review were already addressed in the current code (setScratchSize, safe JSON.stringify, shell script portability, PowerShell cleanup, timing log correctness, u32 bounds check, Node >= 20 in README + engines, correct JSDoc path). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Davies <simongdavies@users.noreply.github.com>
1 parent 06f93ba commit e29e224

6 files changed

Lines changed: 247 additions & 411 deletions

File tree

‎src/js-host-api/examples/mcp-server/package-lock.json‎

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/js-host-api/examples/mcp-server/tests/config.test.js‎

Lines changed: 1 addition & 164 deletions
Original file line numberDiff line numberDiff line change
@@ -14,170 +14,7 @@
1414
// ─────────────────────────────────────────────────────────────────────
1515

1616
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
17-
import { spawn } from 'node:child_process';
18-
import { fileURLToPath } from 'node:url';
19-
import { dirname, join } from 'node:path';
20-
21-
const __dirname = dirname(fileURLToPath(import.meta.url));
22-
const SERVER_PATH = join(__dirname, '..', 'server.js');
23-
const PROTOCOL_VERSION = '2025-11-25';
24-
25-
// ── NDJSON Utilities ────────────────────────────────────────────────
26-
27-
function send(proc, message) {
28-
proc.stdin.write(JSON.stringify(message) + '\n');
29-
}
30-
31-
/**
32-
* Shared per-process stdout state to correctly handle NDJSON streams.
33-
* Ensures that multiple messages in a single chunk are not dropped and
34-
* that leftover buffered data is preserved across waitForResponse calls.
35-
*/
36-
const stdoutState = new WeakMap();
37-
38-
function getStdoutState(proc) {
39-
let state = stdoutState.get(proc);
40-
if (state) return state;
41-
42-
state = {
43-
buffer: '',
44-
pendingLines: [],
45-
waiters: [],
46-
listening: false,
47-
listener: null,
48-
};
49-
50-
const listener = (chunk) => {
51-
state.buffer += chunk.toString();
52-
53-
while (true) {
54-
const idx = state.buffer.indexOf('\n');
55-
if (idx === -1) break;
56-
57-
let line = state.buffer.slice(0, idx);
58-
state.buffer = state.buffer.slice(idx + 1);
59-
60-
// Normalize Windows line endings and skip empty lines.
61-
line = line.replace(/\r$/, '');
62-
if (line.length === 0) continue;
63-
64-
if (state.waiters.length > 0) {
65-
const { resolve, reject } = state.waiters.shift();
66-
try {
67-
resolve(JSON.parse(line));
68-
} catch (_err) {
69-
reject(new Error(`Invalid JSON from server: ${line}`));
70-
}
71-
} else {
72-
state.pendingLines.push(line);
73-
}
74-
}
75-
};
76-
77-
state.listener = listener;
78-
stdoutState.set(proc, state);
79-
return state;
80-
}
81-
82-
function waitForResponse(proc) {
83-
const state = getStdoutState(proc);
84-
85-
return new Promise((resolve, reject) => {
86-
const deliverFromLine = (line) => {
87-
try {
88-
resolve(JSON.parse(line));
89-
} catch (_err) {
90-
reject(new Error(`Invalid JSON from server: ${line}`));
91-
}
92-
};
93-
94-
// If we already have a complete line buffered, use it immediately.
95-
if (state.pendingLines.length > 0) {
96-
const line = state.pendingLines.shift();
97-
deliverFromLine(line);
98-
return;
99-
}
100-
101-
// Otherwise, enqueue this waiter and let the shared listener fulfill it.
102-
state.waiters.push({ resolve, reject });
103-
104-
// Attach the shared listener once per process.
105-
if (!state.listening) {
106-
proc.stdout.on('data', state.listener);
107-
state.listening = true;
108-
}
109-
});
110-
}
111-
112-
/**
113-
* Spawn a server with the given env overrides, perform the MCP
114-
* handshake, and return { server, messageId, callTool, listTools }.
115-
*
116-
* @param {Record<string, string>} envOverrides
117-
* @returns {Promise<{server: import('node:child_process').ChildProcess, messageId: {value: number}, callTool: (code: string) => Promise<object>, listTools: () => Promise<object>}>}
118-
*/
119-
async function spawnServer(envOverrides = {}) {
120-
const messageId = { value: 1 };
121-
const server = spawn('node', [SERVER_PATH], {
122-
stdio: ['pipe', 'pipe', 'pipe'],
123-
env: { ...process.env, ...envOverrides },
124-
});
125-
126-
// Collect stderr for debugging (vitest captures it)
127-
const stderrChunks = [];
128-
server.stderr.on('data', (d) => {
129-
stderrChunks.push(d.toString());
130-
process.stderr.write(`[config-test] ${d}`);
131-
});
132-
133-
// MCP handshake — initialize
134-
send(server, {
135-
jsonrpc: '2.0',
136-
id: messageId.value++,
137-
method: 'initialize',
138-
params: {
139-
protocolVersion: PROTOCOL_VERSION,
140-
capabilities: {},
141-
clientInfo: { name: 'vitest-config-client', version: '1.0.0' },
142-
},
143-
});
144-
145-
const initResponse = await waitForResponse(server);
146-
expect(initResponse.result).toBeDefined();
147-
148-
// MCP handshake — initialized notification
149-
send(server, {
150-
jsonrpc: '2.0',
151-
method: 'notifications/initialized',
152-
});
153-
await new Promise((r) => setTimeout(r, 200));
154-
155-
/** Call execute_javascript and return the full response. */
156-
const callTool = async (code) => {
157-
send(server, {
158-
jsonrpc: '2.0',
159-
id: messageId.value++,
160-
method: 'tools/call',
161-
params: {
162-
name: 'execute_javascript',
163-
arguments: { code },
164-
},
165-
});
166-
return waitForResponse(server);
167-
};
168-
169-
/** Call tools/list and return the full response. */
170-
const listTools = async () => {
171-
send(server, {
172-
jsonrpc: '2.0',
173-
id: messageId.value++,
174-
method: 'tools/list',
175-
});
176-
return waitForResponse(server);
177-
};
178-
179-
return { server, messageId, callTool, listTools, stderrChunks };
180-
}
17+
import { spawnServer } from './helpers.js';
18118

18219
// ── Custom CPU Timeout ──────────────────────────────────────────────
18320

‎src/js-host-api/examples/mcp-server/tests/helpers.js‎

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,183 @@
1+
// ── Shared Test Utilities — NDJSON Transport Helpers ─────────────────
2+
//
3+
// Provides a persistent NDJSON line reader for MCP stdio transport tests.
4+
// Each test file imports from here instead of duplicating the reader logic.
5+
//
6+
// The reader maintains a per-process buffer via WeakMap, correctly handles
7+
// multiple messages arriving in a single stdout chunk, and queues complete
8+
// lines for consumption by waitForResponse callers.
9+
//
10+
// ─────────────────────────────────────────────────────────────────────
11+
12+
import { spawn } from 'node:child_process';
13+
import { fileURLToPath } from 'node:url';
14+
import { dirname, join } from 'node:path';
15+
import { expect } from 'vitest';
16+
17+
const __dirname = dirname(fileURLToPath(import.meta.url));
18+
19+
/** Path to server.js — one directory up from tests/ */
20+
export const SERVER_PATH = join(__dirname, '..', 'server.js');
21+
22+
/**
23+
* The protocol version the MCP SDK (v1.26.0) expects.
24+
* Must match LATEST_PROTOCOL_VERSION in the SDK.
25+
*/
26+
export const PROTOCOL_VERSION = '2025-11-25';
27+
28+
// ── NDJSON Framing ──────────────────────────────────────────────────
29+
//
30+
// MCP stdio transport uses newline-delimited JSON (NDJSON):
31+
// - Send: JSON.stringify(message) + '\n'
32+
// - Receive: read lines, parse each as JSON
33+
34+
/**
35+
* Send a JSON-RPC message to the server via stdin (NDJSON framing).
36+
*
37+
* @param {import('node:child_process').ChildProcess} proc
38+
* @param {object} message — JSON-RPC message object
39+
*/
40+
export function send(proc, message) {
41+
proc.stdin.write(JSON.stringify(message) + '\n');
42+
}
43+
44+
// Shared per-process line reader state: buffer, queued lines, and waiters.
45+
// Uses a WeakMap so the reader survives across multiple waitForResponse calls
46+
// without dropping lines that arrived in the same stdout chunk.
47+
const procLineState = new WeakMap();
48+
49+
function ensureLineReader(proc) {
50+
let state = procLineState.get(proc);
51+
if (state) return state;
52+
53+
state = {
54+
buffer: '',
55+
lines: [],
56+
waiters: [],
57+
};
58+
59+
const onData = (chunk) => {
60+
state.buffer += chunk.toString();
61+
let idx;
62+
while ((idx = state.buffer.indexOf('\n')) !== -1) {
63+
let line = state.buffer.slice(0, idx).replace(/\r$/, '');
64+
state.buffer = state.buffer.slice(idx + 1);
65+
if (line.length === 0) {
66+
continue;
67+
}
68+
69+
if (state.waiters.length > 0) {
70+
const { resolve, reject } = state.waiters.shift();
71+
try {
72+
resolve(JSON.parse(line));
73+
} catch (_err) {
74+
reject(new Error(`Invalid JSON from server: ${line}`));
75+
}
76+
} else {
77+
state.lines.push(line);
78+
}
79+
}
80+
};
81+
82+
proc.stdout.on('data', onData);
83+
procLineState.set(proc, state);
84+
return state;
85+
}
86+
87+
/**
88+
* Wait for the next JSON-RPC response from the server's stdout.
89+
* Reads newline-delimited JSON via a persistent per-process line buffer.
90+
*
91+
* @param {import('node:child_process').ChildProcess} proc
92+
* @returns {Promise<object>} — parsed JSON-RPC response
93+
*/
94+
export function waitForResponse(proc) {
95+
return new Promise((resolve, reject) => {
96+
const state = ensureLineReader(proc);
97+
98+
if (state.lines.length > 0) {
99+
const line = state.lines.shift();
100+
try {
101+
resolve(JSON.parse(line));
102+
} catch (_err) {
103+
reject(new Error(`Invalid JSON from server: ${line}`));
104+
}
105+
return;
106+
}
107+
108+
state.waiters.push({ resolve, reject });
109+
});
110+
}
111+
112+
/**
113+
* Spawn a server with the given env overrides, perform the MCP
114+
* handshake, and return a context object with helper methods.
115+
*
116+
* @param {Record<string, string>} envOverrides
117+
* @param {object} [options]
118+
* @param {string} [options.clientName] — client name for the handshake
119+
* @param {string} [options.stderrPrefix] — prefix for stderr debug output
120+
* @returns {Promise<{server: import('node:child_process').ChildProcess, messageId: {value: number}, callTool: (code: string) => Promise<object>, listTools: () => Promise<object>, stderrChunks: string[]}>}
121+
*/
122+
export async function spawnServer(envOverrides = {}, options = {}) {
123+
const { clientName = 'vitest-client', stderrPrefix = '[mcp-server]' } = options;
124+
const messageId = { value: 1 };
125+
const server = spawn('node', [SERVER_PATH], {
126+
stdio: ['pipe', 'pipe', 'pipe'],
127+
env: { ...process.env, ...envOverrides },
128+
});
129+
130+
const stderrChunks = [];
131+
server.stderr.on('data', (d) => {
132+
stderrChunks.push(d.toString());
133+
process.stderr.write(`${stderrPrefix} ${d}`);
134+
});
135+
136+
// MCP handshake — initialize
137+
send(server, {
138+
jsonrpc: '2.0',
139+
id: messageId.value++,
140+
method: 'initialize',
141+
params: {
142+
protocolVersion: PROTOCOL_VERSION,
143+
capabilities: {},
144+
clientInfo: { name: clientName, version: '1.0.0' },
145+
},
146+
});
147+
148+
const initResponse = await waitForResponse(server);
149+
expect(initResponse.result).toBeDefined();
150+
151+
// MCP handshake — initialized notification
152+
send(server, {
153+
jsonrpc: '2.0',
154+
method: 'notifications/initialized',
155+
});
156+
await new Promise((r) => setTimeout(r, 200));
157+
158+
/** Call execute_javascript and return the full response. */
159+
const callTool = async (code) => {
160+
send(server, {
161+
jsonrpc: '2.0',
162+
id: messageId.value++,
163+
method: 'tools/call',
164+
params: {
165+
name: 'execute_javascript',
166+
arguments: { code },
167+
},
168+
});
169+
return waitForResponse(server);
170+
};
171+
172+
/** Call tools/list and return the full response. */
173+
const listTools = async () => {
174+
send(server, {
175+
jsonrpc: '2.0',
176+
id: messageId.value++,
177+
method: 'tools/list',
178+
});
179+
return waitForResponse(server);
180+
};
181+
182+
return { server, messageId, callTool, listTools, stderrChunks };
183+
}

0 commit comments

Comments
 (0)