From 72dc84a5163ce79e9b4e796b1c9b67439a2a9001 Mon Sep 17 00:00:00 2001 From: Byron-2452 Date: Sat, 17 Oct 2026 23:38:53 -0400 Subject: [PATCH 1/2] fix(mcpl,api): read a JSON-RPC envelope once: answer id null, refuse an invalid request with -32600 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #475. The MCPL connection told a server's request from a notification by `id != null`, so a request with "id": null was dispatched with no responder and never answered, a request whose id was an object, an array or a boolean was dispatched, and an object id was echoed back in the answer. The MCP server (src/api/mcp-server-core.ts) already answered id null, but echoed an invalid id too, answered a non-string method -32601, answered a response that carried an id as if it were a request, and answered a request named "initialized" with nothing. JSON-RPC 2.0 (§4, §4.1, §5) and MCPL §4.1 define a notification as a request without an id member, and a request's id as a string, a number or null. src/jsonrpc-envelope.ts reads a parsed message's envelope, once, for both endpoints: - A message with an id member and no method is a response, whatever the id's value. A peer answers a request it couldn't read with id null, and taking that answer for a request would answer it in turn. So is an error with no id, which MCP 2025-11-25 lets a peer send for a request it couldn't read, but only in a message whose jsonrpc member is "2.0", since a server's own JSON log lines can carry an error key. Any other object with neither a method nor an id isn't JSON-RPC. - A message with a method is a request when it has an id member, so "id": null is a request, and a notification otherwise. - A message whose method isn't a string, or whose id isn't a string, a number or null, isn't a Request object, and so isn't a notification either, with or without an id: it's answered once with -32600 "Invalid Request: ", carrying its id where that's a string or a number and null otherwise, and isn't dispatched. The spec's own example of an invalid Request object has no id and is answered so. - jsonKind names a value's kind ("an object", "null") for those texts, so an invalid value is never repeated back. It replaces describeMethod. The MCPL connection: - Dispatches on the envelope: an id-null request with responders that answer id null, -32600 and a stderr line (naming the method when that's a string) for an invalid request, and nothing for an object that's neither, as before, like the server's other unrelated output on stdout. Until now a non-string method got -32601 (#208); its test of that case now leaves it to the envelope. - Answers MCP's ping (#392) by the same rule: a ping with id null gets its empty result with id null, on a connected transport and during the initialize handshake. During the handshake anything else, an invalid request included, is still ignored, as #392 has it. - Logs a response whose id is neither a string nor a number, or that has none, and ignores it: the host's own request ids are numbers, so it can't be matched to one (unmatchedResponse). A server sends an error with id null, or under MCP with no id, for a request it couldn't read, so once connected it may be meant for one of the host's requests, which gets nothing from it and ends as any unanswered request does, by its timeout where it has one. Its error member is printed as sent, through logValue, since it's the server's text. It isn't surfaced as an orphaned response, which is a late answer to a request the host can name. - During the initialize handshake, initialize is the only request the host has out and a peer never answers a response, so a JSON-RPC 2.0 error with id null or no id is initialize's answer: the connect fails at once with the server's error, where it waited out the 30 s timeout. Requiring "jsonrpc": "2.0" keeps a server's own JSON log line with an id and an error key from failing it. Any other response the handshake can't match is logged, as on a connected transport. - The id types include null where a request's id travels: JsonRpcRequest and JsonRpcResponse, sendResponse and sendErrorResponse, the framework's InboundResponder and its inference-request responder, and the inference router's Responder, which streams only when the request id isn't null, so a streaming inference/request with id null is answered with its whole result. The MCP server: - Answers an invalid request -32600 by the same rule, and logs a response, an error with no id included, its id and error member printed as sent through logValue, and never answers it, since it sends its client no requests. - Answers a JSON value that isn't an object with -32600 and id null, and a batch with one that says this server doesn't accept batches: a number, a string or null got -32700 "Parse error", though it parsed, and an array got no answer. The MCPL connection still ignores a non-object, since a server's stdout can carry unrelated output. - Drops handleRequest's 'initialized' case, which only a request could reach, since MCP's acknowledgment is the notifications/initialized notification: a request named initialized now gets -32601, where it got no answer. Tests: - test/mcpl-request-envelope.test.ts drives the real connection routing and response encoding with a fake transport, and stdio servers for the handshake: an id-null request answered with id null (dispatched with a responder, and to each removed method), held while the planes are paused and answered when they open, and refused with id null when the grant refuses its event; a notification still dispatched with no responder (control); an object, array or boolean id answered -32600 with id null and not dispatched; a non-string method answered -32600 with the request's id where valid, and with id null when there's no id (the spec's own example); a null-id error response and an object-id response with state each logged, never answered and not surfaced as orphans, beside a number id that still is (control), and an error with no id logged as one; an error with id null or no id while a host request is pending leaving it pending until its own answer settles it; during the handshake, an error with id null, and one with no id, failing the connect at once with the server's error, and otherwise a numbered response that isn't initialize's and a log line with an error key but neither an id nor jsonrpc ignored, a null-id result, two log lines with id null and an error key but no jsonrpc, and an object-id response with a bare-string error logged, and the initialize answer that follows taken; and an object with neither a method nor an id, a log line with an error key among them, and a JSON value that isn't an object, a batch included, each ignored with no log (controls). - test/mcp-server-envelope.test.ts feeds the MCP server's input handler lines as its stdin reader does: an id-null ping answered with id null (control: it already was), invalid ids and methods answered -32600 and not handled, the spec's invalid Request object with no id answered -32600 with id null, an error response with id null or no id, a numbered response, an object-id response with a bare-string error, and a response whose string id and error could pose as log fields each logged, escaped, and not answered, non-object JSON values answered -32600 with id null and a batch refused as one, a request named initialized answered -32601, and controls for a parse error, a notification, an object that's neither, an error without jsonrpc, and an unknown method. - test/mcpl-ping.test.ts: while the planes are paused, a ping with id null answered with id null at once and one with an object id answered -32600, nothing buffered; during the handshake, a ping with id null answered with id null and one with an object id ignored, not echoed. - test/mcpl-inference-router.test.ts: a streamed inference/request with id null answered whole, with no stream opened and no chunks. - test/mcpl-request-handler-errors.test.ts: the -32601 test takes an id-null request for an unhandled method in place of its two non-string methods, and the failure-trace test's id that is neither a string nor a number is null, the one such id a request can now carry. - test/mcp-server-bin.test.ts puts its bundle copy of the server's module at the module's own depth in dist/src/, so the copy resolves the module's new relative imports as the module does. Co-Authored-By: Claude Opus 5.5 --- changelog.d/475-mcpl-request-ids.fixed.md | 9 + src/api/mcp-server-core.ts | 68 +++-- src/framework.ts | 5 +- src/jsonrpc-envelope.ts | 77 ++++++ src/mcpl/inference-router.ts | 6 +- src/mcpl/server-connection.ts | 124 ++++++--- src/mcpl/types.ts | 6 +- test/mcp-server-bin.test.ts | 9 +- test/mcp-server-envelope.test.ts | 133 ++++++++++ test/mcpl-inference-router.test.ts | 22 +- test/mcpl-ping.test.ts | 33 ++- test/mcpl-request-envelope.test.ts | 304 ++++++++++++++++++++++ test/mcpl-request-handler-errors.test.ts | 20 +- 13 files changed, 735 insertions(+), 81 deletions(-) create mode 100644 changelog.d/475-mcpl-request-ids.fixed.md create mode 100644 src/jsonrpc-envelope.ts create mode 100644 test/mcp-server-envelope.test.ts create mode 100644 test/mcpl-request-envelope.test.ts diff --git a/changelog.d/475-mcpl-request-ids.fixed.md b/changelog.d/475-mcpl-request-ids.fixed.md new file mode 100644 index 000000000..e52b78552 --- /dev/null +++ b/changelog.d/475-mcpl-request-ids.fixed.md @@ -0,0 +1,9 @@ +- MCPL tells a server's request from its notification by whether the message has an `id` member, as JSON-RPC 2.0 and MCPL §4.1 define them (#475). So a request with `"id": null` is now dispatched and answered, with `id: null`, where it was taken for a notification and never answered. That includes a ping, on a connected transport and during the initialize handshake. + - **A message with an `id` and no `method` is a response,** whatever the id's value, and is never answered. So is an error with no `id`, which MCP lets a server send for a request it couldn't read, in a message whose `jsonrpc` is `"2.0"`; it was ignored silently before. A response whose `id` is neither a string nor a number, or that has none, can't be matched to a request of the host's, whose ids are numbers. So it's logged, with its error member if it has one, and it's no longer surfaced as an orphaned response. A server sends an error with `id: null`, or under MCP with no `id`, for a request it couldn't read. Once connected, the request it was meant for gets nothing from it, ending as any unanswered request does, by its timeout where it has one. During the initialize handshake, `initialize` is the only request the host has out, so such an error, in a message whose `jsonrpc` is `"2.0"`, is its answer: the connect fails at once, with the server's error, even if the server answers `initialize` after it. Before, it waited out the 30 s timeout, or connected on that later answer. Any other response the handshake can't match is logged, as on a connected transport, a server's own JSON log line with an `id` and an `error` key included. Any other object with neither a `method` nor an `id` isn't JSON-RPC, and is ignored, as before. + - **An invalid request is answered once with `-32600` "Invalid Request",** saying what was wrong, and isn't dispatched. That's a message whose `id` isn't a string, a number or null, which was echoed back in the answer, or whose `method` isn't a string, which got -32601 "Method not found" with an `id` and no answer without one. One with no `id` is answered too, since it isn't a valid request, and so isn't a notification either, as JSON-RPC 2.0's own example shows. The answer carries the request's `id` where that's a string or a number, and `null` otherwise. During the initialize handshake, which answers only pings, it's ignored, as other requests are. A ping with an invalid `id` was answered there before, with that `id`. + - **A streaming `inference/request` with `id: null`** is answered with its whole result, since its chunks name the request by its id. +- The MCP server, `McpServer`, which the `agent-framework-mcp` command runs and the package root exports, reads a message's envelope by the same rule (#475). + - **An invalid request now gets one `-32600` "Invalid Request",** saying what was wrong, and isn't handled. Before, a request whose `id` isn't a string, a number or null had that `id` echoed back in the answer, one whose `method` isn't a string got -32601 "Method not found", and one of those with no `id` got no answer. + - **A message with an `id` and no `method` is a response,** and so is a JSON-RPC 2.0 error with no `id`. This server sends no requests, so it logs a response and never answers it. Before, one with an `id` was answered -32601, and an error with no `id` was ignored. + - **A JSON value that isn't an object gets `-32600` with `id: null`,** and a batch's answer says this server doesn't accept batches. Before, a number, a string or `null` got -32700 "Parse error", and an array got no answer. + - **A request named `initialized` gets -32601 "Method not found"**, as any unknown method does, where it got no answer. MCP's acknowledgment is the `notifications/initialized` notification, which needs none. diff --git a/src/api/mcp-server-core.ts b/src/api/mcp-server-core.ts index 7c3b743ad..666152be5 100644 --- a/src/api/mcp-server-core.ts +++ b/src/api/mcp-server-core.ts @@ -14,6 +14,8 @@ import WebSocket from 'ws'; import { createInterface } from 'node:readline'; import type { ApiRequest, ApiResponse, ApiEvent } from './types.js'; +import { jsonKind, readEnvelope } from '../jsonrpc-envelope.js'; +import { logValue } from '../mcpl/log-value.js'; // ============================================================================ // MCP Protocol Types @@ -21,7 +23,8 @@ import type { ApiRequest, ApiResponse, ApiEvent } from './types.js'; interface JsonRpcRequest { jsonrpc: '2.0'; - id: string | number; + /** null is allowed, though JSON-RPC 2.0 discourages it. */ + id: string | number | null; method: string; params?: unknown; } @@ -188,20 +191,55 @@ export class McpServer { } private handleInput(line: string): void { + let message: unknown; try { - const message = JSON.parse(line); - - // Check if this is a notification (no id) or a request (has id) - if (!('id' in message) || message.id === undefined) { - // This is a notification - handle silently without response - // Common notifications: notifications/initialized, notifications/cancelled - return; - } - - // This is a request - process and respond - this.handleRequest(message as JsonRpcRequest); + message = JSON.parse(line); } catch { this.sendError(null, -32700, 'Parse error'); + return; + } + // A request is an object (JSON-RPC 2.0 §4), so any other JSON value is an + // invalid request, answered with id null (§5). An array is JSON-RPC's + // batch, which this server doesn't take. + if (Array.isArray(message)) { + this.sendError(null, -32600, "Invalid Request: this server doesn't accept batches, so send each message on a line of its own"); + return; + } + if (message === null || typeof message !== 'object') { + this.sendError(null, -32600, `Invalid Request: a message must be an object, not ${jsonKind(message)}`); + return; + } + + // The envelope is read once, here (src/jsonrpc-envelope.ts says how). + const envelope = readEnvelope(message as Record); + switch (envelope.kind) { + case 'request': + void this.handleRequest(message as JsonRpcRequest); + return; + case 'invalid': + this.sendError(envelope.id, envelope.error.code, envelope.error.message); + return; + case 'response': { + // This server sends its client no requests, so a response can't be + // matched to one, and it's never answered. A client that couldn't + // read a line this server sent, whatever it was, may answer it with + // id null, or under MCP with no id. The id and the error member, if + // it has one, are printed as sent, through logValue, since they're + // the client's text. + const { id, error } = message as { id?: unknown; error?: unknown }; + const which = !Object.hasOwn(message as object, 'id') ? 'with no id' + : `whose id is ${typeof id === 'string' || typeof id === 'number' ? logValue(id) : jsonKind(id)}`; + console.error(`[agent-framework-mcp] ignored a response ${which}, since this server sends no requests` + + (error !== undefined ? `; its error: ${logValue(error)}` : '')); + return; + } + case 'notification': + // notifications/initialized, notifications/cancelled and the like + // need no answer. + return; + case 'neither': + // Not JSON-RPC, and nothing to answer. + return; } } @@ -221,10 +259,6 @@ export class McpServer { }); break; - case 'initialized': - // Client acknowledgment, no response needed - break; - case 'tools/list': this.sendResult(request.id, { tools: this.getTools() }); break; @@ -639,7 +673,7 @@ export class McpServer { }); } - private sendResult(id: string | number, result: unknown): void { + private sendResult(id: string | number | null, result: unknown): void { const response: JsonRpcResponse = { jsonrpc: '2.0', id, diff --git a/src/framework.ts b/src/framework.ts index 77e31efe6..95d9308d8 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -641,7 +641,8 @@ const bySeq = (a: { seq: number }, b: { seq: number }): number => a.seq - b.seq; /** The reply helpers an MCPL connection hands an inbound request's handler. */ type InboundResponder = { - id: string | number; + /** The request's id: null for a request that the server sent with id null. */ + id: string | number | null; respond: (result: unknown) => void; respondError: (code: number, message: string, data?: unknown) => void; }; @@ -22576,7 +22577,7 @@ export class AgentFramework { // Handle server-initiated inference requests (Step 6) onRequest('inference-request', async ( params: McplInferenceRequestParams, - responder?: { id: string | number; respond: (result: unknown) => void; respondError: (code: number, message: string) => void }, + responder?: { id: string | number | null; respond: (result: unknown) => void; respondError: (code: number, message: string) => void }, ) => { // RFC-006 §5.3 / SPEC §10.7: no inference from inside a render. if (this.pushCoalescer?.isRendering(connection.id)) { diff --git a/src/jsonrpc-envelope.ts b/src/jsonrpc-envelope.ts new file mode 100644 index 000000000..1ccea6eac --- /dev/null +++ b/src/jsonrpc-envelope.ts @@ -0,0 +1,77 @@ +/** + * A JSON-RPC 2.0 message's envelope, read once where a message arrives, by + * each of the framework's endpoints that answers requests: the MCPL + * connection, for its servers' requests, and the MCP server + * (src/api/mcp-server-core.ts), for its client's. + * + * A message with an id member and no method is a response, whatever the + * id's value: a peer answers a request it couldn't read with id null (§5), + * and taking that answer for a request would answer it in turn. So is an + * error with no id, which MCP lets a peer send for a request it couldn't + * read (MCP 2025-11-25, Basic, Error Responses), but only from a message + * whose jsonrpc member is "2.0": a peer's own JSON log lines, which an MCPL + * server's stdout can carry, can have an error key. Any other object with + * neither a method nor an id isn't JSON-RPC. A message with a method is + * a request when it has an id member, whatever its value, and a notification + * when it has none (§4.1), so `"id": null` is a request. A request's method + * must be a string and its id a string, a number or null (§4). A message + * that breaks either isn't a Request object, and so isn't a notification + * either, with or without an id: the endpoints answer it once with -32600, + * carrying its id where that's a string or a number and null otherwise (§5, + * and the spec's own example of an invalid Request object, which has no id), + * and don't dispatch it. The MCPL handshake, which answers only pings, + * ignores it, as it ignores every other request then. Apart from the error + * with no id, the `jsonrpc` member isn't checked: a peer that leaves it out + * is served, as before. + */ + +/** A JSON value's kind, as a log line or an error names what a peer sent + * without repeating it: "null", "an array", "an object", "a number". */ +export function jsonKind(value: unknown): string { + if (value === null) return 'null'; + const kind = Array.isArray(value) ? 'array' : typeof value; + return `${/^[aeiou]/.test(kind) ? 'an' : 'a'} ${kind}`; +} + +export type JsonRpcEnvelope = + /** An id member and no method, or a JSON-RPC 2.0 error with neither: an + * answer, which the endpoint matches to one of its own requests by the + * id where it can. */ + | { kind: 'response' } + /** Neither a method nor an id member, and not such an error: not a + * JSON-RPC message. */ + | { kind: 'neither' } + | { kind: 'request'; method: string; id: string | number | null } + | { kind: 'notification'; method: string } + /** Not a Request object, with or without an id: answered with `error`, + * carrying `id`, and not dispatched. `method` is its method where that's + * a string, for a log line. */ + | { kind: 'invalid'; id: string | number | null; error: { code: -32600; message: string }; method?: string; problems: string }; + +/** Read a parsed message's envelope. What to do with a value that isn't an + * object is the endpoint's own call. */ +export function readEnvelope(msg: Record): JsonRpcEnvelope { + if (!Object.hasOwn(msg, 'method')) { + const answers = Object.hasOwn(msg, 'id') || (msg.jsonrpc === '2.0' && Object.hasOwn(msg, 'error')); + return { kind: answers ? 'response' : 'neither' }; + } + const isRequest = Object.hasOwn(msg, 'id'); + const { method, id } = msg; + const problems = [ + ...(typeof method === 'string' ? [] : [`the method must be a string, not ${jsonKind(method)}`]), + ...(!isRequest || typeof id === 'string' || typeof id === 'number' || id === null + ? [] : [`the id must be a string, a number or null, not ${jsonKind(id)}`]), + ].join('; '); + if (problems) { + return { + kind: 'invalid', + id: typeof id === 'string' || typeof id === 'number' ? id : null, + error: { code: -32600, message: `Invalid Request: ${problems}` }, + ...(typeof method === 'string' ? { method } : {}), + problems, + }; + } + return isRequest + ? { kind: 'request', method: method as string, id: id as string | number | null } + : { kind: 'notification', method: method as string }; +} diff --git a/src/mcpl/inference-router.ts b/src/mcpl/inference-router.ts index 2cb69661f..3a6968238 100644 --- a/src/mcpl/inference-router.ts +++ b/src/mcpl/inference-router.ts @@ -35,8 +35,10 @@ import type { HookOrchestrator } from './hook-orchestrator.js'; interface Responder { respond(result: McplInferenceRequestResult): void; respondError(code: number, message: string, data?: unknown): void; - /** The JSON-RPC request ID, used for streaming chunk correlation. */ - requestId?: string | number; + /** The JSON-RPC request ID, used for streaming chunk correlation: a + * request sent with id null is answered whole, since its chunks couldn't + * name it. */ + requestId?: string | number | null; } // ============================================================================ diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index 0ef6bf244..c7cd5d09b 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -16,6 +16,7 @@ import { maskNegotiatedCapabilities } from './capability-mask.js'; import { CapabilityGrant, expandAdvertisementShorthand } from './capability-grant.js'; import { CAPABILITY_DISABLED, boundedForLog, describeCause } from './errors.js'; import { logDetail, logValue } from './log-value.js'; +import { jsonKind, readEnvelope } from '../jsonrpc-envelope.js'; import type { McplServerConfig, @@ -165,11 +166,19 @@ interface RequestOptions { surfaceOrphanedResponse?: boolean; } -/** A request's method for a log line, whatever the server sent there. */ -function describeMethod(method: unknown): string { - return typeof method === 'string' - ? `method ${logValue(method)}` - : `a method of type ${method === null ? 'null' : typeof method}`; +/** The log line for a response whose id is neither a string nor a number, + * or that has none: the host's own request ids are numbers, so it can't be + * matched to one. A server sends an error with id null, or under MCP with + * no id, when it couldn't read a request (JSON-RPC 2.0 §5), so once + * connected it may be the answer to a request of ours, which gets nothing + * from it and ends as any unanswered request does, by its timeout where it + * has one. (During the handshake such an error answers initialize.) Its + * error member, if it has one, is printed as sent, through logValue, since + * it's the server's text. */ +function unmatchedResponse(serverId: string, response: { id?: unknown; error?: unknown }): string { + const which = Object.hasOwn(response, 'id') ? `whose id is ${jsonKind(response.id)}` : 'with no id'; + return `[mcpl] ${serverId}: ignored a response ${which}, which can't be matched to a request of ours` + + (response.error !== undefined ? `; its error: ${logValue(response.error)}` : ''); } /** @@ -735,15 +744,30 @@ export class McplServerConnection extends EventEmitter { // initialize response, whatever its id: a server's own ids can start // at 0, this handshake's. MCP lets a server ping before // initialization completes, and the ping gets its empty result, as - // on a connected transport. Anything else it sends now is ignored. - if ('method' in msg) { - const request = msg as unknown as JsonRpcRequest; - if (request.method === 'ping' && request.id != null) { - transport.writeLine(JSON.stringify({ jsonrpc: '2.0', id: request.id, result: {} })); - } + // on a connected transport, `"id": null` included + // (src/jsonrpc-envelope.ts). Anything else it sends now is ignored, + // an invalid request included. Of its responses, the one with + // initialize's id answers it, and so does a JSON-RPC 2.0 error with + // id null or with no id: a peer answers a request it couldn't read + // that way (JSON-RPC 2.0 §5; MCP 2025-11-25, Basic), initialize is + // the only request this host has out, and a peer never answers a + // response. Such an error means the server can't read this host's + // messages, so the handshake fails at once, in the server's words. + // `"jsonrpc": "2.0"` keeps a server's own JSON log line, which can + // carry an id and an error key, from failing it. Any other response + // whose id can't be matched is logged, as on a connected transport. + const envelope = readEnvelope(msg as unknown as Record); + if (envelope.kind === 'request' && envelope.method === 'ping') { + transport.writeLine(JSON.stringify({ jsonrpc: '2.0', id: envelope.id, result: {} })); + return; + } + if (envelope.kind !== 'response') return; + const namesNoRequest = msg.id === null || !Object.hasOwn(msg, 'id'); + const answersInitialize = msg.id === initId || (namesNoRequest && msg.jsonrpc === '2.0' && Boolean(msg.error)); + if (!answersInitialize) { + if (typeof msg.id !== 'string' && typeof msg.id !== 'number') console.error(unmatchedResponse(config.id, msg)); return; } - if (msg.id !== initId) return; cleanup(); if (msg.error) { // describeCause rather than the template's own conversion: the @@ -1460,7 +1484,7 @@ export class McplServerConnection extends EventEmitter { */ private setupMessageRouting(transport: McplTransport): void { transport.on('line', (line: string) => { - let msg: JsonRpcRequest | JsonRpcResponse; + let msg: unknown; try { msg = JSON.parse(line); } catch { @@ -1472,7 +1496,7 @@ export class McplServerConnection extends EventEmitter { // before property access, just as malformed JSON lines are ignored. if (msg === null || typeof msg !== 'object' || Array.isArray(msg)) return; try { - this.dispatchMessage(msg); + this.dispatchMessage(msg as Record); } catch (error) { const request = msg as { method?: unknown; id?: unknown }; this.reportInboundFailure(error, { @@ -1484,15 +1508,31 @@ export class McplServerConnection extends EventEmitter { } /** Contain synchronous dispatch failures; async RPC handlers own their promises. */ - private dispatchMessage(msg: JsonRpcRequest | JsonRpcResponse): void { - // Is this a response to one of our outbound requests? - if ('id' in msg && msg.id != null && !('method' in msg)) { - this.handleResponse(msg as JsonRpcResponse); - return; + private dispatchMessage(msg: Record): void { + // The envelope is read once, here (src/jsonrpc-envelope.ts says how). + const envelope = readEnvelope(msg); + switch (envelope.kind) { + case 'response': + // Never answered, so a server that answers what it can't read, with + // id null, can't start an exchange of errors with us. + this.handleResponse(msg as unknown as JsonRpcResponse); + return; + case 'neither': + // Not JSON-RPC: ignored like the server's other unrelated output. + return; + case 'invalid': { + this.sendErrorResponse(envelope.id, envelope.error.code, envelope.error.message); + const forMethod = envelope.method !== undefined ? ` for method ${logValue(envelope.method)}` : ''; + console.error(`[mcpl] ${this.id}: answered -32600 to an invalid request${forMethod}: ${envelope.problems}`); + return; + } } - // It is an inbound request or notification from the server - const request = msg as JsonRpcRequest; + // It is an inbound request or notification from the server, its method + // a string. A request's id is a string, a number or null, and `"id": null` + // is answered with id null; a notification has none. + const request = msg as unknown as JsonRpcRequest; + const requestId = envelope.kind === 'request' ? envelope.id : undefined; // MCP's ping, which either side may send: its receiver answers promptly // with an empty result. It's answered as it arrives, before admission and @@ -1500,11 +1540,11 @@ export class McplServerConnection extends EventEmitter { // paused plane mustn't hold that answer. A ping sent as a notification // has nothing to answer. The handshake answers one too. if (request.method === 'ping') { - if (request.id != null) this.sendResponse(request.id, {}); + if (requestId !== undefined) this.sendResponse(requestId, {}); return; } - const eventName = typeof request.method === 'string' && Object.hasOwn(McplServerConnection.METHOD_TO_EVENT, request.method) + const eventName = Object.hasOwn(McplServerConnection.METHOD_TO_EVENT, request.method) ? McplServerConnection.METHOD_TO_EVENT[request.method] : undefined; // §7 is removed in 0.5.0: scope/elevate is no longer a method. Answer @@ -1516,8 +1556,8 @@ export class McplServerConnection extends EventEmitter { // routed, control-plane classified, and mutating declarations — a // live ungated self-attestation path (PR #79 review blocker 4). if (request.method === McplMethod.FeatureSetsChanged) { - if (request.id != null) { - this.sendErrorResponse(request.id, -32601, 'featureSets/changed removed in MCPL 0.5.0 (superseded by mcpl/manifestChanged, SPEC §17)'); + if (requestId !== undefined) { + this.sendErrorResponse(requestId, -32601, 'featureSets/changed removed in MCPL 0.5.0 (superseded by mcpl/manifestChanged, SPEC §17)'); } else { console.error(`[mcpl] ${this.id}: discarded featureSets/changed — removed in 0.5.0 (§6.7/§17)`); } @@ -1525,8 +1565,8 @@ export class McplServerConnection extends EventEmitter { } if (request.method === McplMethod.ScopeElevate) { - if (request.id != null) { - this.sendErrorResponse(request.id, -32601, 'scope/elevate removed in MCPL 0.5.0 (SPEC §7)'); + if (requestId !== undefined) { + this.sendErrorResponse(requestId, -32601, 'scope/elevate removed in MCPL 0.5.0 (SPEC §7)'); } return; } @@ -1538,24 +1578,23 @@ export class McplServerConnection extends EventEmitter { if (!eventName) { // §6.6: a request that will never be answered MUST get an error, so a - // request (it has an id) whose method isn't a string, or isn't one this - // host handles, is answered -32601. A notification nobody handles is - // ignored, as JSON-RPC asks. - if (request.id != null) { - this.sendErrorResponse(request.id, -32601, 'Method not found'); - console.error(`[mcpl] ${this.id}: answered -32601 to a request for ${describeMethod(request.method)}, which this host doesn't handle`); + // request whose method isn't one this host handles is answered -32601. + // A notification nobody handles is ignored, as JSON-RPC asks. + if (requestId !== undefined) { + this.sendErrorResponse(requestId, -32601, 'Method not found'); + console.error(`[mcpl] ${this.id}: answered -32601 to a request for method ${logValue(request.method)}, which this host doesn't handle`); } return; } // Emit the typed event with params and (for requests) a respond callback - if (request.id != null) { + if (requestId !== undefined) { // Server expects a response — provide a respond helper this.dispatchInboundEvent(eventName, request.params, { - id: request.id, - respond: (result: unknown) => this.sendResponse(request.id!, result), + id: requestId, + respond: (result: unknown) => this.sendResponse(requestId, result), respondError: (code: number, message: string, data?: unknown) => - this.sendErrorResponse(request.id!, code, message, data), + this.sendErrorResponse(requestId, code, message, data), }); } else { // Notification — no response expected @@ -1616,6 +1655,13 @@ export class McplServerConnection extends EventEmitter { * Handle a JSON-RPC response by resolving/rejecting the corresponding pending request. */ private handleResponse(response: JsonRpcResponse): void { + // A response whose id is neither a string nor a number, or that has + // none, can't be matched to a request of ours. It isn't an orphan, which + // is a late answer to one, so it's logged and ignored. + if (typeof response.id !== 'string' && typeof response.id !== 'number') { + console.error(unmatchedResponse(this.id, response)); + return; + } const pending = this.pendingRequests.get(response.id); if (!pending) { // Orphaned response — the request already timed out (or was settled) and @@ -1693,7 +1739,7 @@ export class McplServerConnection extends EventEmitter { /** * Send a successful JSON-RPC response back to the server. */ - private sendResponse(id: string | number, result: unknown): void { + private sendResponse(id: string | number | null, result: unknown): void { if (this.closed) return; const response: JsonRpcResponse = { jsonrpc: '2.0', id, result }; this.transport?.writeLine(JSON.stringify(response)); @@ -1702,7 +1748,7 @@ export class McplServerConnection extends EventEmitter { /** * Send a JSON-RPC error response back to the server. */ - private sendErrorResponse(id: string | number, code: number, message: string, data?: unknown): void { + private sendErrorResponse(id: string | number | null, code: number, message: string, data?: unknown): void { if (this.closed) return; const response: JsonRpcResponse = { jsonrpc: '2.0', diff --git a/src/mcpl/types.ts b/src/mcpl/types.ts index d84c655ed..77e062e9a 100644 --- a/src/mcpl/types.ts +++ b/src/mcpl/types.ts @@ -22,7 +22,8 @@ export interface JsonRpcRequest { jsonrpc: '2.0'; method: string; - id?: string | number; + /** A request's id; null is allowed, though JSON-RPC 2.0 discourages it. */ + id?: string | number | null; params?: Record; } @@ -31,7 +32,8 @@ export interface JsonRpcRequest { */ export interface JsonRpcResponse { jsonrpc: '2.0'; - id: string | number; + /** The request's id, or null when the request's own couldn't be read. */ + id: string | number | null; result?: unknown; error?: JsonRpcError; } diff --git a/test/mcp-server-bin.test.ts b/test/mcp-server-bin.test.ts index 8cac1d580..1cf20a8dd 100644 --- a/test/mcp-server-bin.test.ts +++ b/test/mcp-server-bin.test.ts @@ -108,10 +108,11 @@ test('importing the package, or running its server module under the command\'s n const entry = join(dir, 'entry.mjs'); writeFileSync(entry, `const { McpServer } = await import(${rootUrl});\nif (typeof McpServer !== 'function') process.exit(2);\n`); // An application bundling the package carries the server's module, never - // the command: this copy, under the command's file name and beside the - // original so it resolves 'ws' as the module does, is that bundle run as - // the entry point. It used to start the server. - const bundleDir = join(dirname(CORE), `bundle-${process.pid}`); + // the command: this copy, under the command's file name and as deep in + // dist/src/ as the original, so it resolves 'ws' and its own relative + // imports as the module does, is that bundle run as the entry point. It + // used to start the server. + const bundleDir = join(dirname(dirname(CORE)), `bundle-${process.pid}`); mkdirSync(bundleDir); made.push(bundleDir); const bundle = join(bundleDir, 'mcp-server.js'); diff --git a/test/mcp-server-envelope.test.ts b/test/mcp-server-envelope.test.ts new file mode 100644 index 000000000..87bd8502e --- /dev/null +++ b/test/mcp-server-envelope.test.ts @@ -0,0 +1,133 @@ +/** + * The MCP server reads a message's envelope as the MCPL connection does + * (#475, src/jsonrpc-envelope.ts): `"id": null` is a request, answered with + * id null; a message whose id isn't a string, a number or null, or whose + * method isn't a string, with or without an id, is answered once with + * -32600, carrying its id where that's a string or a number and null + * otherwise, and isn't dispatched; a message with an id and no method is a + * response, and so is a JSON-RPC 2.0 error with no id, as MCP allows: this + * server, sending no requests, logs one and never answers it; and a request + * named 'initialized' gets -32601 like any unknown method. A JSON + * value that isn't an object is an invalid request too, and a batch is + * refused as one. Lines go in as the server's stdin reader hands them + * over, to a server that's never started, since starting connects it to its + * WebSocket API first. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; + +import { McpServer } from '../src/api/mcp-server-core.js'; + +type Reply = { jsonrpc: string; id: unknown; result?: unknown; error?: { code: number; message: string } }; + +function serve(t: { mock: { method: (o: object, m: string, f: (...a: unknown[]) => void) => unknown } }) { + const server = new McpServer() as unknown as { handleInput(line: string): void; send(message: unknown): void }; + const replies: Reply[] = []; + server.send = (message: unknown) => { replies.push(JSON.parse(JSON.stringify(message))); }; + const logged: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }); + return { + line: (text: string) => server.handleInput(text), + send: (msg: unknown) => server.handleInput(JSON.stringify(msg)), + replies, + logged, + }; +} + +test('a request with id null is answered, with id null', (t) => { + const { send, replies } = serve(t); + send({ jsonrpc: '2.0', id: null, method: 'ping' }); + assert.deepEqual(replies, [{ jsonrpc: '2.0', id: null, result: {} }]); +}); + +test("an id that isn't a string, a number or null: -32600 with id null, and the request isn't handled", (t) => { + const { send, replies, logged } = serve(t); + send({ jsonrpc: '2.0', id: { nested: [1] }, method: 'initialize', params: {} }); + send({ jsonrpc: '2.0', id: [1], method: 'ping' }); + send({ jsonrpc: '2.0', id: true, method: 'ping' }); + assert.deepEqual(replies, [ + { jsonrpc: '2.0', id: null, error: { code: -32600, message: 'Invalid Request: the id must be a string, a number or null, not an object' } }, + { jsonrpc: '2.0', id: null, error: { code: -32600, message: 'Invalid Request: the id must be a string, a number or null, not an array' } }, + { jsonrpc: '2.0', id: null, error: { code: -32600, message: 'Invalid Request: the id must be a string, a number or null, not a boolean' } }, + ]); + assert.deepEqual(logged, [], 'the answer says what was wrong'); +}); + +test("a method that isn't a string: -32600, carrying the request's id where it's a string or a number", (t) => { + const { send, replies } = serve(t); + send({ jsonrpc: '2.0', id: 9, method: 7 }); + send({ jsonrpc: '2.0', id: 'a', method: ['ping'] }); + send({ jsonrpc: '2.0', id: null, method: null }); + assert.deepEqual(replies.map((r) => [r.id, r.error?.code, r.error?.message, 'result' in r]), [ + [9, -32600, 'Invalid Request: the method must be a string, not a number', false], + ['a', -32600, 'Invalid Request: the method must be a string, not an array', false], + [null, -32600, 'Invalid Request: the method must be a string, not null', false], + ]); +}); + +test("a method that isn't a string, with no id, isn't a notification either: -32600 with id null", (t) => { + const { send, replies, logged } = serve(t); + // JSON-RPC 2.0's own example of an invalid Request object. + send({ jsonrpc: '2.0', method: 1, params: 'bar' }); + assert.deepEqual(replies, [ + { jsonrpc: '2.0', id: null, error: { code: -32600, message: 'Invalid Request: the method must be a string, not a number' } }, + ]); + assert.deepEqual(logged, []); +}); + +test('a message without a method is a response: this server sends no requests, so it logs one and never answers it', (t) => { + const { send, replies, logged } = serve(t); + // What a client sends for a line it couldn't read (JSON-RPC 2.0 §5), and + // the form MCP allows for it, with no id (MCP 2025-11-25, Basic). + send({ jsonrpc: '2.0', id: null, error: { code: -32700, message: 'Parse error' } }); + send({ jsonrpc: '2.0', error: { code: -32700, message: 'Parse error' } }); + send({ jsonrpc: '2.0', id: 3, result: {} }); + // An error member that isn't an error object is printed as sent too. + send({ jsonrpc: '2.0', id: { x: 1 }, error: 'boom' }); + // A string id and an error that could pose as log fields are escaped. + send({ jsonrpc: '2.0', id: 'a b=c', error: 'reason=stolen' }); + assert.deepEqual(replies, [], 'a response is never answered'); + assert.deepEqual(logged, [ + String.raw`[agent-framework-mcp] ignored a response whose id is null, since this server sends no requests; its error: "{\"code\":-32700,\"message\":\"Parse\u0020error\"}"`, + String.raw`[agent-framework-mcp] ignored a response with no id, since this server sends no requests; its error: "{\"code\":-32700,\"message\":\"Parse\u0020error\"}"`, + '[agent-framework-mcp] ignored a response whose id is 3, since this server sends no requests', + '[agent-framework-mcp] ignored a response whose id is an object, since this server sends no requests; its error: boom', + String.raw`[agent-framework-mcp] ignored a response whose id is "a\u0020b\u003dc", since this server sends no requests; its error: "reason\u003dstolen"`, + ]); +}); + +test("a JSON value that isn't an object is an invalid request, answered with id null, and a batch is refused as one", (t) => { + const { line, replies, logged } = serve(t); + for (const text of ['5', '"ping"', 'null', '[]', '[{"jsonrpc":"2.0","id":1,"method":"ping"}]']) line(text); + const batch = "Invalid Request: this server doesn't accept batches, so send each message on a line of its own"; + assert.deepEqual(replies.map((r) => [r.id, r.error?.code, r.error?.message]), [ + [null, -32600, 'Invalid Request: a message must be an object, not a number'], + [null, -32600, 'Invalid Request: a message must be an object, not a string'], + [null, -32600, 'Invalid Request: a message must be an object, not null'], + [null, -32600, batch], + [null, -32600, batch], + ]); + assert.deepEqual(logged, []); +}); + +test('controls: a line that isn\'t JSON is a parse error; a notification, an object with neither a method nor an id, and an unknown method are handled as before', (t) => { + const { line, send, replies, logged } = serve(t); + line('{"jsonrpc":'); + send({ jsonrpc: '2.0', method: 'notifications/initialized' }); + send({}); + // An error member alone, without saying it's JSON-RPC, is no response. + send({ error: { code: -32700, message: 'Parse error' } }); + send({ jsonrpc: '2.0', id: 4, method: 'no/such-method' }); + assert.deepEqual(replies, [ + { jsonrpc: '2.0', id: null, error: { code: -32700, message: 'Parse error' } }, + { jsonrpc: '2.0', id: 4, error: { code: -32601, message: 'Method not found: no/such-method' } }, + ]); + assert.deepEqual(logged, []); +}); + +test("a request named 'initialized' gets -32601, where it got no answer: MCP's acknowledgment is the notifications/initialized notification", (t) => { + const { send, replies } = serve(t); + send({ jsonrpc: '2.0', id: 5, method: 'initialized' }); + send({ jsonrpc: '2.0', method: 'notifications/initialized' }); + assert.deepEqual(replies, [{ jsonrpc: '2.0', id: 5, error: { code: -32601, message: 'Method not found: initialized' } }]); +}); diff --git a/test/mcpl-inference-router.test.ts b/test/mcpl-inference-router.test.ts index 66417ccb7..b801e3fc1 100644 --- a/test/mcpl-inference-router.test.ts +++ b/test/mcpl-inference-router.test.ts @@ -115,7 +115,7 @@ interface Answer { /** Ask the router for one inference, played by `script` on a real Membrane. */ async function ask( script: Script, - opts: { stream: boolean; mode: Mode; policy?: InferenceRoutingPolicy }, + opts: { stream: boolean; mode: Mode; policy?: InferenceRoutingPolicy; requestId?: string | number | null }, ): Promise { const membrane = new Membrane(new ScriptedAdapter(script), { formatter: opts.mode === 'xml' ? new AnthropicXmlFormatter({ toolMode: 'xml' }) : new NativeFormatter(), @@ -126,15 +126,17 @@ async function ask( answer.stream = streamYielding(request); return answer.stream; }; - return askWith(membrane, opts.stream, answer, opts.policy); + return askWith(membrane, opts.stream, answer, opts.policy, opts.requestId); } -/** Ask a router built on `membrane`, routing by `policy` (none: `default`), for one inference. */ +/** Ask a router built on `membrane`, routing by `policy` (none: `default`), for one inference, + * as the request `requestId` (7 unless given). */ async function askWith( membrane: Membrane, stream: boolean, answer: Answer = { chunks: [], sent: [], traces: [] }, policy?: InferenceRoutingPolicy, + requestId: string | number | null = 7, ): Promise { const router = new InferenceRouter( membrane, @@ -155,7 +157,7 @@ async function askWith( stream, messages: [{ role: 'user', content: 'Summarize these memories.' }], }, { - requestId: 7, + requestId, respond: (result) => { answer.result = result; answer.sent.push(`result:${result.content}`); @@ -374,6 +376,18 @@ for (const mode of ['native', 'xml'] as const) { }); } +describe('MCPL inference router: a request whose id is null (#475)', () => { + it('answers a streamed request whole, with no chunks, since a chunk names its request by id', async () => { + const answer = await ask({ chunks: ['Hello ', 'world'], stopReason: 'end_turn' }, { stream: true, mode: 'native', requestId: null }); + + assert.strictEqual(answer.error, undefined); + assert.strictEqual(answer.stream, undefined, 'no stream was opened'); + assert.deepStrictEqual(answer.chunks, []); + assert.deepStrictEqual(answer.sent, ['result:Hello world']); + assert.deepStrictEqual(answer.traces.at(-1), completeTrace(false)); + }); +}); + // A native tool call arrives as a typed tool_use block. membrane's yielding // stream lists it in toolCalls only when the stop is tool_use, so a streamed // block cut off by another stop is only in the content, and only the router's diff --git a/test/mcpl-ping.test.ts b/test/mcpl-ping.test.ts index abb569663..f9e865767 100644 --- a/test/mcpl-ping.test.ts +++ b/test/mcpl-ping.test.ts @@ -42,6 +42,19 @@ test('a ping on a connected transport is answered with an empty result; a ping n ]); }); +test('a ping with id null is answered with id null, and one with an invalid id gets -32600, at once while the planes are paused (#475)', (t) => { + const { connection, receive, written } = bareConnection(); + t.mock.method(console, 'error', () => {}); + // Before ready() both planes are closed, as at startup. + receive({ jsonrpc: '2.0', id: null, method: 'ping' }); + receive({ jsonrpc: '2.0', id: { x: 1 }, method: 'ping' }); + assert.deepEqual(written(), [ + { jsonrpc: '2.0', id: null, result: {} }, + { jsonrpc: '2.0', id: null, error: { code: -32600, message: 'Invalid Request: the id must be a string, a number or null, not an object' } }, + ]); + assert.equal((connection as any).bufferedEvents.length, 0); +}); + test('a ping is answered at once while the planes are paused, and nothing is held for it', () => { const { connection, receive, written } = bareConnection(); // Before ready() both planes are closed, as at startup; a quiesce or the @@ -61,6 +74,9 @@ test('a ping is answered at once while the planes are paused, and nothing is hel // that isn't a ping, a request whose method isn't a string, a ping sent as // a notification, a log notification, its initialize response, and a ping // with the id "late", which the connected transport answers. +// - null-ids: as initialize arrives, pings with id null and with an object +// id, and answers initialize, then pings with the id "late", once its pong +// for id null arrives. const HANDSHAKE_SERVER = String.raw` const mode = process.argv[1]; let buf = ''; @@ -80,6 +96,8 @@ process.stdin.on('data', (c) => { init = m; if (mode === 'pings-first') { process.stdout.write(line({ jsonrpc: '2.0', id: 0, method: 'ping' })); + } else if (mode === 'null-ids') { + process.stdout.write(line({ jsonrpc: '2.0', id: null, method: 'ping' }) + line({ jsonrpc: '2.0', id: { x: 1 }, method: 'ping' })); } else { process.stdout.write( line({ jsonrpc: '2.0', id: 0, method: 'roots/list' }) + @@ -96,7 +114,10 @@ process.stdin.on('data', (c) => { process.stdout.write(line(initResult(init.id))); report(); } - if (mode === 'one-chunk' && m.id === 'late') report(); + if (mode === 'null-ids' && m.id === null && pong) { + process.stdout.write(line(initResult(init.id)) + line({ jsonrpc: '2.0', id: 'late', method: 'ping' })); + } + if ((mode === 'one-chunk' || mode === 'null-ids') && m.id === 'late') report(); } } }); @@ -105,7 +126,7 @@ setInterval(() => {}, 1 << 30); /** Connects to a HANDSHAKE_SERVER and returns the capabilities the host took * from the handshake, and the server's stderr report. */ -async function handshakeWith(mode: 'pings-first' | 'one-chunk') { +async function handshakeWith(mode: 'pings-first' | 'one-chunk' | 'null-ids') { const connection = await McplServerConnection.connect( { id: 'pinger', command: process.execPath, args: ['-e', HANDSHAKE_SERVER, mode], requestTimeoutMs: 5_000 }, { version: '0.5' }, @@ -144,3 +165,11 @@ test('during the handshake nothing that carries a method is taken for the initia // subscribed, was answered by the connected transport. assert.equal(report, 'responses:["\\"late\\":pong"]'); }); + +test('during the handshake a ping with id null is answered with id null, and one with an invalid id is ignored (#475)', async () => { + const { capabilities, report } = await handshakeWith('null-ids'); + // The server answered initialize only once its pong for id null arrived. + assert.equal(capabilities?.pushEvents, true); + // The ping with an object id got nothing, and isn't echoed. + assert.equal(report, 'responses:["null:pong","\\"late\\":pong"]'); +}); diff --git a/test/mcpl-request-envelope.test.ts b/test/mcpl-request-envelope.test.ts new file mode 100644 index 000000000..4e8887a38 --- /dev/null +++ b/test/mcpl-request-envelope.test.ts @@ -0,0 +1,304 @@ +/** + * An inbound message's envelope (#475): a request is a message with an id + * member, whatever its value, so `"id": null` is a request and is answered + * with id null, held and flushed or refused as any request is; a message + * with an id and no method is a response, whatever the id's value, and so + * is a JSON-RPC 2.0 error with no id, as MCP allows; a response is never + * answered, and one whose id can't be matched is logged, during the + * handshake too, except that an error with id null or none then answers + * initialize, and the connect fails at once; any other object with neither + * a method nor an id isn't JSON-RPC, so it's ignored, as a JSON value that + * isn't an object is, a batch included. A message whose id isn't a string, + * a number or null, or whose method isn't a string, with or without an id, + * is answered once with -32600, carrying its id where that's a string or a + * number and null otherwise, and isn't dispatched. Real connection routing + * and response encoding, and stdio servers for the handshake. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; + +import { McplServerConnection } from '../src/mcpl/server-connection.js'; +import { McplTransport } from '../src/mcpl/transport.js'; +import { CapabilityGrant, ALL_CAPABILITY_PATHS } from '../src/mcpl/capability-grant.js'; + +class ResponseTransport extends McplTransport { + constructor() { super(); } + readonly kind = 'stdio' as const; + readonly lines: string[] = []; + writeLine(line: string): void { this.lines.push(line); } + async close(): Promise {} +} + +type Reply = { jsonrpc: string; id: unknown; result?: unknown; error?: { code: number; message: string } }; + +/** A ready connection with every capability granted, a channels/list + * handler that answers, and what it was handed. */ +function wire(t: { mock: { method: (o: object, m: string, f: (...a: unknown[]) => void) => unknown } }) { + const transport = new ResponseTransport(); + const connection = new (McplServerConnection as any)('srv', null, transport) as McplServerConnection; + connection.establishGrant(new CapabilityGrant(new Set(ALL_CAPABILITY_PATHS), [])); + connection.ready(); + const handled: Array<{ params: unknown; responder: { id: unknown; respond?: unknown } | undefined }> = []; + connection.on('channels-list', (params: unknown, responder?: { id: unknown; respond: (r: unknown) => void }) => { + handled.push({ params, responder }); + responder?.respond({ channels: [] }); + }); + const logged: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }); + const send = (msg: unknown) => transport.emit('line', JSON.stringify(msg)); + const replies = (): Reply[] => transport.lines.map((line) => JSON.parse(line)); + return { connection, transport, send, replies, handled, logged }; +} + +test('a request with id null is answered, with id null', (t) => { + const { send, replies, handled } = wire(t); + send({ jsonrpc: '2.0', id: null, method: 'channels/list', params: {} }); + send({ jsonrpc: '2.0', id: null, method: 'scope/elevate', params: {} }); + send({ jsonrpc: '2.0', id: null, method: 'featureSets/changed', params: {} }); + assert.equal(handled.length, 1); + assert.equal(typeof handled[0]!.responder?.respond, 'function', 'dispatched as a request, with a responder'); + assert.equal(handled[0]!.responder?.id, null); + assert.deepEqual(replies(), [ + { jsonrpc: '2.0', id: null, result: { channels: [] } }, + { jsonrpc: '2.0', id: null, error: { code: -32601, message: 'scope/elevate removed in MCPL 0.5.0 (SPEC §7)' } }, + { jsonrpc: '2.0', id: null, error: { code: -32601, message: 'featureSets/changed removed in MCPL 0.5.0 (superseded by mcpl/manifestChanged, SPEC §17)' } }, + ]); +}); + +test('a request with id null held while the planes are paused is answered with id null when they open', async () => { + const transport = new ResponseTransport(); + const connection = new (McplServerConnection as any)('srv', null, transport) as McplServerConnection; + connection.establishGrant(new CapabilityGrant(new Set(ALL_CAPABILITY_PATHS), [])); + connection.on('channels-list', (_params: unknown, responder?: { respond: (r: unknown) => void }) => responder?.respond({ channels: [] })); + // Before ready() both planes are closed, as at startup. + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: null, method: 'channels/list', params: {} })); + assert.deepEqual(transport.lines, [], 'held, not answered or dropped'); + connection.ready(); + await new Promise((resolve) => setImmediate(resolve)); + assert.deepEqual(transport.lines.map((line) => JSON.parse(line)), [{ jsonrpc: '2.0', id: null, result: { channels: [] } }]); +}); + +test('a request with id null for an event the grant refuses gets the refusal, with id null', (t) => { + const transport = new ResponseTransport(); + const connection = new (McplServerConnection as any)('srv', null, transport) as McplServerConnection; + connection.establishGrant(new CapabilityGrant(new Set(), [])); + connection.ready(); + t.mock.method(console, 'error', () => {}); + let handled = false; + connection.on('channels-list', () => { handled = true; }); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: null, method: 'channels/list', params: {} })); + const replies = transport.lines.map((line) => JSON.parse(line)); + assert.equal(handled, false); + assert.deepEqual(replies.map((r) => [r.id, r.error?.code]), [[null, -32002]]); +}); + +test('a message with no id member is a notification: dispatched with no responder, and not answered', (t) => { + const { send, replies, handled } = wire(t); + send({ jsonrpc: '2.0', method: 'channels/list', params: { n: 1 } }); + assert.deepEqual(handled, [{ params: { n: 1 }, responder: undefined }]); + assert.deepEqual(replies(), []); +}); + +test("an id that isn't a string, a number or null: one -32600 with id null, and nothing is dispatched", (t) => { + const { send, replies, handled, logged } = wire(t); + const ids = [{ nested: [1] }, [1], true, false]; + for (const id of ids) send({ jsonrpc: '2.0', id, method: 'channels/list', params: {} }); + assert.equal(handled.length, 0); + assert.deepEqual(replies().map((r) => [r.id, r.error?.code, r.error?.message, 'result' in r]), [ + [null, -32600, 'Invalid Request: the id must be a string, a number or null, not an object', false], + [null, -32600, 'Invalid Request: the id must be a string, a number or null, not an array', false], + [null, -32600, 'Invalid Request: the id must be a string, a number or null, not a boolean', false], + [null, -32600, 'Invalid Request: the id must be a string, a number or null, not a boolean', false], + ]); + assert.deepEqual(logged, [ + '[mcpl] srv: answered -32600 to an invalid request for method channels/list: the id must be a string, a number or null, not an object', + '[mcpl] srv: answered -32600 to an invalid request for method channels/list: the id must be a string, a number or null, not an array', + '[mcpl] srv: answered -32600 to an invalid request for method channels/list: the id must be a string, a number or null, not a boolean', + '[mcpl] srv: answered -32600 to an invalid request for method channels/list: the id must be a string, a number or null, not a boolean', + ]); +}); + +test("a method that isn't a string: -32600, carrying the request's id where it's a string or a number", (t) => { + const { send, replies, handled, logged } = wire(t); + send({ jsonrpc: '2.0', id: 5, method: 7 }); + send({ jsonrpc: '2.0', id: 'a', method: ['channels/list'] }); + send({ jsonrpc: '2.0', id: null, method: null }); + send({ jsonrpc: '2.0', id: { x: 1 }, method: { y: 2 } }); + assert.equal(handled.length, 0); + assert.deepEqual(replies().map((r) => [r.id, r.error?.code, r.error?.message]), [ + [5, -32600, 'Invalid Request: the method must be a string, not a number'], + ['a', -32600, 'Invalid Request: the method must be a string, not an array'], + [null, -32600, 'Invalid Request: the method must be a string, not null'], + [null, -32600, 'Invalid Request: the method must be a string, not an object; the id must be a string, a number or null, not an object'], + ]); + assert.deepEqual(logged, [ + '[mcpl] srv: answered -32600 to an invalid request: the method must be a string, not a number', + '[mcpl] srv: answered -32600 to an invalid request: the method must be a string, not an array', + '[mcpl] srv: answered -32600 to an invalid request: the method must be a string, not null', + '[mcpl] srv: answered -32600 to an invalid request: the method must be a string, not an object; the id must be a string, a number or null, not an object', + ]); +}); + +test("a method that isn't a string, with no id, isn't a notification either: -32600 with id null", (t) => { + const { send, replies, handled, logged } = wire(t); + // JSON-RPC 2.0's own example of an invalid Request object. + send({ jsonrpc: '2.0', method: 1, params: 'bar' }); + send({ jsonrpc: '2.0', method: ['channels/list'], params: {} }); + assert.equal(handled.length, 0); + assert.deepEqual(replies(), [ + { jsonrpc: '2.0', id: null, error: { code: -32600, message: 'Invalid Request: the method must be a string, not a number' } }, + { jsonrpc: '2.0', id: null, error: { code: -32600, message: 'Invalid Request: the method must be a string, not an array' } }, + ]); + assert.deepEqual(logged, [ + '[mcpl] srv: answered -32600 to an invalid request: the method must be a string, not a number', + '[mcpl] srv: answered -32600 to an invalid request: the method must be a string, not an array', + ]); +}); + +test('a message without a method is a response, whatever its id, and is never answered', (t) => { + const { connection, send, replies, handled, logged } = wire(t); + const orphans: unknown[] = []; + connection.on('orphaned-response', (info) => { orphans.push(info); }); + // What a server sends for a request it couldn't read (JSON-RPC 2.0 §5), + // and the form MCP allows for it, with no id (MCP 2025-11-25, Basic). + send({ jsonrpc: '2.0', id: null, error: { code: -32700, message: 'Parse error' } }); + send({ jsonrpc: '2.0', error: { code: -32700, message: 'Parse error' } }); + // An id none of ours can have, with the state an orphan would carry. + send({ jsonrpc: '2.0', id: { x: 1 }, result: { state: {} } }); + assert.deepEqual(replies(), [], 'a response is never answered'); + assert.equal(handled.length, 0); + assert.deepEqual(orphans, [], "neither is a late answer to a request of ours"); + // The error member is the server's text, so it prints as log-value prints a + // server's value: escaped and quoted unless it's a plain token. + assert.deepEqual(logged, [ + String.raw`[mcpl] srv: ignored a response whose id is null, which can't be matched to a request of ours; its error: "{\"code\":-32700,\"message\":\"Parse\u0020error\"}"`, + String.raw`[mcpl] srv: ignored a response with no id, which can't be matched to a request of ours; its error: "{\"code\":-32700,\"message\":\"Parse\u0020error\"}"`, + "[mcpl] srv: ignored a response whose id is an object, which can't be matched to a request of ours", + ]); + // The control: a number id that matches no pending request, carrying state, + // is still surfaced as an orphan. + send({ jsonrpc: '2.0', id: 999, result: { state: {} } }); + assert.equal(orphans.length, 1); + assert.deepEqual(replies(), []); +}); + +test("a null-id error while a request of the host's is pending leaves that request pending, and its own answer settles it", async (t) => { + const { connection, transport, send, logged } = wire(t); + const orphans: unknown[] = []; + connection.on('orphaned-response', (info) => { orphans.push(info); }); + // Its own timeout is the hang guard: it rejects the request, failing the + // test, if the answer below never settles it. + const pending = (connection as any).sendRequest('tools/list', {}, { timeoutMs: 30_000 }) as Promise; + let settled = false; + pending.then(() => { settled = true; }, () => { settled = true; }); + const request = JSON.parse(transport.lines.at(-1)!); + // A server that couldn't read the request may answer it so (JSON-RPC 2.0 + // §5), or under MCP with no id. + send({ jsonrpc: '2.0', id: null, error: { code: -32700, message: 'Parse error' } }); + send({ jsonrpc: '2.0', error: { code: -32700, message: 'Parse error' } }); + await new Promise((resolve) => setImmediate(resolve)); + assert.equal(settled, false, 'still pending'); + assert.deepEqual(orphans, []); + assert.equal(logged.length, 2); + send({ jsonrpc: '2.0', id: request.id, result: { tools: [] } }); + assert.deepEqual(await pending, { tools: [] }); +}); + +/** A stdio server that answers initialize with the given lines, `INIT` in + * each standing for initialize's id, and stays up until the host closes it. */ +function initializeAnswers(...answers: string[]): string { + return String.raw` +let buf = ''; +process.stdin.on('data', (c) => { + buf += c; + let i; + while ((i = buf.indexOf('\n')) >= 0) { + const text = buf.slice(0, i); buf = buf.slice(i + 1); + if (!text.trim()) continue; + const m = JSON.parse(text); + if (m.method === 'initialize') { + for (const answer of ${JSON.stringify(answers)}) process.stdout.write(answer.replaceAll('"INIT"', JSON.stringify(m.id)) + '\n'); + } + } +}); +setInterval(() => {}, 1 << 30); +`; +} + +for (const [form, answer] of [ + ['with id null', { jsonrpc: '2.0', id: null, error: { code: -32700, message: 'Parse error' } }], + ['with no id, as MCP allows', { jsonrpc: '2.0', error: { code: -32700, message: 'Parse error' } }], +] as const) { + test(`during the handshake an error ${form} answers initialize, the one request out: the connect fails at once, in the server's words`, async () => { + // A server that couldn't read initialize, and so answers it naming no + // request. Its error is the handshake's, rather than a timeout 30 s later. + const server = initializeAnswers(JSON.stringify(answer)); + await assert.rejects( + McplServerConnection.connect({ id: 'unreadable', command: process.execPath, args: ['-e', server] }, { version: '0.5' }), + { message: 'MCPL server "unreadable" initialize error: Parse error' }, + ); + }); +} + +test("during the handshake any other response that can't be matched is logged, one with another number is ignored, and the handshake goes on", async (t) => { + const logged: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }); + // Answers initialize first with a number that isn't initialize's, then with + // id null and a result, which no peer sends for a request it couldn't read, + // then with log lines of its own that carry an error, two with id null, + // but don't say they're JSON-RPC, then with an object id and a bare string + // for its error, and then properly. + const server = initializeAnswers( + JSON.stringify({ jsonrpc: '2.0', id: 5, result: {} }), + JSON.stringify({ jsonrpc: '2.0', id: null, result: {} }), + JSON.stringify({ level: 50, msg: 'request failed', error: { code: -32700, message: 'Parse error' } }), + JSON.stringify({ level: 50, msg: 'request failed', id: null, error: { code: -32700, message: 'Parse error' } }), + JSON.stringify({ level: 'error', id: null, error: 'db unavailable' }), + JSON.stringify({ jsonrpc: '2.0', id: { x: 1 }, error: 'boom' }), + JSON.stringify({ jsonrpc: '2.0', id: 'INIT', result: { protocolVersion: '2024-11-05', capabilities: { experimental: { mcpl: { version: '0.5', pushEvents: true } } } } }), + ); + const connection = await McplServerConnection.connect( + { id: 'unmatched', command: process.execPath, args: ['-e', server], requestTimeoutMs: 5_000 }, + { version: '0.5' }, + ); + try { + assert.equal(connection.capabilities?.pushEvents, true, 'the initialize answer that followed was taken'); + // The numbered response that isn't initialize's is neither logged nor taken. + assert.deepEqual(logged.filter((entry) => entry.startsWith('[mcpl] unmatched: ignored a response')), [ + "[mcpl] unmatched: ignored a response whose id is null, which can't be matched to a request of ours", + String.raw`[mcpl] unmatched: ignored a response whose id is null, which can't be matched to a request of ours; its error: "{\"code\":-32700,\"message\":\"Parse\u0020error\"}"`, + String.raw`[mcpl] unmatched: ignored a response whose id is null, which can't be matched to a request of ours; its error: "db\u0020unavailable"`, + "[mcpl] unmatched: ignored a response whose id is an object, which can't be matched to a request of ours; its error: boom", + ]); + } finally { + await connection.close(); + } +}); + +test("an object with neither a method nor an id isn't JSON-RPC: it's ignored, with no log", (t) => { + const { connection, send, replies, handled, logged } = wire(t); + const orphans: unknown[] = []; + connection.on('orphaned-response', (info) => { orphans.push(info); }); + // A server's own log lines written to stdout, one of them carrying an error + // as a JSON-RPC error does but not saying it's JSON-RPC, and a result that + // left out its id. + send({ level: 30, msg: 'listening' }); + send({ level: 50, msg: 'request failed', error: { code: -32700, message: 'Parse error' } }); + send({ jsonrpc: '2.0', result: { state: {} } }); + assert.deepEqual(replies(), []); + assert.equal(handled.length, 0); + assert.deepEqual(orphans, []); + assert.deepEqual(logged, []); +}); + +test("a JSON value that isn't an object, a batch included, is ignored like a server's other output on stdout", (t) => { + const { transport, replies, handled, logged } = wire(t); + // Unlike the MCP server, whose stdin carries only its client's messages, + // a server's stdout can carry output that isn't JSON-RPC. + for (const line of ['7', '"listening"', 'null', 'true', '[]', JSON.stringify([{ jsonrpc: '2.0', id: 1, method: 'channels/list', params: {} }])]) { + transport.emit('line', line); + } + assert.deepEqual(replies(), []); + assert.equal(handled.length, 0); + assert.deepEqual(logged, []); +}); diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index a7c3e0693..b1992072f 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -450,8 +450,9 @@ test('a throwing orphaned-response listener is contained, and later messages sti assert.equal(traces.filter((e) => e.type === 'mcpl:server-error').length, 1); }); -test('a method that is not a string is not dispatched, even if it stringifies to a known method', () => { +test('a method that is not a string is not dispatched, even if it stringifies to a known method', (t) => { const { connection, transport } = wireHarness(); + t.mock.method(console, 'error', () => {}); let received = false; connection.removeAllListeners('tools-list-changed'); connection.on('tools-list-changed', () => { received = true; }); @@ -598,20 +599,19 @@ test('a request for a method nothing handles is answered -32601; such a notifica const logged: string[] = []; t.mock.method(console, 'error', (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }); transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 31, method: 'no/such-method' })); - transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 32, method: ['host/command'] })); - transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 33, method: 7 })); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: null, method: 'no/such-method' })); transport.emit('line', JSON.stringify({ jsonrpc: '2.0', method: 'no/such-notification' })); const replies = transport.lines.map((line) => JSON.parse(line)); + // A method that isn't a string is an invalid request, answered -32600 + // (test/mcpl-request-envelope.test.ts). assert.deepEqual(replies.map((r) => [r.id, r.error?.code, r.error?.message]), [ [31, -32601, 'Method not found'], - [32, -32601, 'Method not found'], - [33, -32601, 'Method not found'], + [null, -32601, 'Method not found'], ]); // One line per answered request, naming the method; none for the notification. assert.deepEqual(logged, [ "[mcpl] srv: answered -32601 to a request for method no/such-method, which this host doesn't handle", - "[mcpl] srv: answered -32601 to a request for a method of type object, which this host doesn't handle", - "[mcpl] srv: answered -32601 to a request for a method of type number, which this host doesn't handle", + "[mcpl] srv: answered -32601 to a request for method no/such-method, which this host doesn't handle", ]); }); @@ -948,13 +948,15 @@ test('a failed request is answered -32603 with its own id, while its trace bound const { framework, traces, transport } = wireHarness(); t.mock.method(console, 'error', () => {}); framework.channelRegistry = { async handleRegister() { throw new Error('register failed'); } }; - for (const id of ['a\u0000b\nc\u202e', true]) { + // null is the one id a request can carry that is neither a string nor a + // number; any other is refused before dispatch (mcpl-request-envelope). + for (const id of ['a\u0000b\nc\u202e', null]) { transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id, method: 'channels/register', params: { channels: [] } })); } await settle(); assert.deepEqual( transport.lines.map((line) => JSON.parse(line)).map((reply) => [reply.id, reply.error?.code]), - [['a\u0000b\nc\u202e', -32603], [true, -32603]], + [['a\u0000b\nc\u202e', -32603], [null, -32603]], ); assert.deepEqual(failureTraces(traces).map((trace) => trace.requestId), ['a?b?c?', undefined]); }); From 1dd81e459b2f4099ff95e7589dae1206243dbcf0 Mon Sep 17 00:00:00 2001 From: Byron-2452 Date: Fri, 23 Oct 2026 14:53:21 -0400 Subject: [PATCH 2/2] MCPL #475: say what the handshake logs, and pin the kept redaction of an unmatched response The review of the merge found text that said more than the code does: - The handshake's comment, the changelog fragment and the envelope test's header said any response the handshake can't match is logged. Only one whose id is neither a string nor a number, or that has none, is logged. One with another string or number id is ignored, as on main. They now say so, and so does a test's name. 72dc84a's message says the same as the old comment, and stays as pushed. - The handshake's comment said "jsonrpc": "2.0" keeps a server's own JSON log line, with an id and an error key, from failing the connect. That holds for id null or no id. A line with initialize's own id is taken for its answer whatever its jsonrpc, as on main. The comment now says so, and says the connect fails with the error's message rather than "in the server's words", since an error with no message reads "undefined" there. - unmatchedResponse's doc now says an error naming no request answers initialize only when its jsonrpc is "2.0". - The MCP server's comment said a response's id and error member are printed as sent. logValue escapes and bounds what it prints, and an id that isn't a string or a number is named by its kind. A new test pins the merge's use of the connection's kept redactor for the line of a response that can't be matched. Three such responses read a getter env once, and each line is redacted. Building the redactor afresh for each response fails it. The redactor field's doc is rewrapped. Co-Authored-By: Claude Opus 5.5 --- changelog.d/475-mcpl-request-ids.fixed.md | 2 +- src/api/mcp-server-core.ts | 7 ++-- src/mcpl/server-connection.ts | 35 +++++++++++--------- test/mcpl-request-envelope.test.ts | 40 ++++++++++++++++------- 4 files changed, 52 insertions(+), 32 deletions(-) diff --git a/changelog.d/475-mcpl-request-ids.fixed.md b/changelog.d/475-mcpl-request-ids.fixed.md index e52b78552..db660bf65 100644 --- a/changelog.d/475-mcpl-request-ids.fixed.md +++ b/changelog.d/475-mcpl-request-ids.fixed.md @@ -1,5 +1,5 @@ - MCPL tells a server's request from its notification by whether the message has an `id` member, as JSON-RPC 2.0 and MCPL §4.1 define them (#475). So a request with `"id": null` is now dispatched and answered, with `id: null`, where it was taken for a notification and never answered. That includes a ping, on a connected transport and during the initialize handshake. - - **A message with an `id` and no `method` is a response,** whatever the id's value, and is never answered. So is an error with no `id`, which MCP lets a server send for a request it couldn't read, in a message whose `jsonrpc` is `"2.0"`; it was ignored silently before. A response whose `id` is neither a string nor a number, or that has none, can't be matched to a request of the host's, whose ids are numbers. So it's logged, with its error member if it has one, and it's no longer surfaced as an orphaned response. A server sends an error with `id: null`, or under MCP with no `id`, for a request it couldn't read. Once connected, the request it was meant for gets nothing from it, ending as any unanswered request does, by its timeout where it has one. During the initialize handshake, `initialize` is the only request the host has out, so such an error, in a message whose `jsonrpc` is `"2.0"`, is its answer: the connect fails at once, with the server's error, even if the server answers `initialize` after it. Before, it waited out the 30 s timeout, or connected on that later answer. Any other response the handshake can't match is logged, as on a connected transport, a server's own JSON log line with an `id` and an `error` key included. Any other object with neither a `method` nor an `id` isn't JSON-RPC, and is ignored, as before. + - **A message with an `id` and no `method` is a response,** whatever the id's value, and is never answered. So is an error with no `id`, which MCP lets a server send for a request it couldn't read, in a message whose `jsonrpc` is `"2.0"`; it was ignored silently before. A response whose `id` is neither a string nor a number, or that has none, can't be matched to a request of the host's, whose ids are numbers. So it's logged, with its error member if it has one, and it's no longer surfaced as an orphaned response. A server sends an error with `id: null`, or under MCP with no `id`, for a request it couldn't read. Once connected, the request it was meant for gets nothing from it, ending as any unanswered request does, by its timeout where it has one. During the initialize handshake, `initialize` is the only request the host has out, so such an error, in a message whose `jsonrpc` is `"2.0"`, is its answer: the connect fails at once, with the server's error, even if the server answers `initialize` after it. Before, it waited out the 30 s timeout, or connected on that later answer. Of the handshake's other responses, one whose `id` is neither a string nor a number, or that has none, is logged, as on a connected transport, a server's own JSON log line with `"id": null` and an `error` key included, and one with another string or number `id` is ignored, as before. Any other object with neither a `method` nor an `id` isn't JSON-RPC, and is ignored, as before. - **An invalid request is answered once with `-32600` "Invalid Request",** saying what was wrong, and isn't dispatched. That's a message whose `id` isn't a string, a number or null, which was echoed back in the answer, or whose `method` isn't a string, which got -32601 "Method not found" with an `id` and no answer without one. One with no `id` is answered too, since it isn't a valid request, and so isn't a notification either, as JSON-RPC 2.0's own example shows. The answer carries the request's `id` where that's a string or a number, and `null` otherwise. During the initialize handshake, which answers only pings, it's ignored, as other requests are. A ping with an invalid `id` was answered there before, with that `id`. - **A streaming `inference/request` with `id: null`** is answered with its whole result, since its chunks name the request by its id. - The MCP server, `McpServer`, which the `agent-framework-mcp` command runs and the package root exports, reads a message's envelope by the same rule (#475). diff --git a/src/api/mcp-server-core.ts b/src/api/mcp-server-core.ts index 666152be5..12e606587 100644 --- a/src/api/mcp-server-core.ts +++ b/src/api/mcp-server-core.ts @@ -223,9 +223,10 @@ export class McpServer { // This server sends its client no requests, so a response can't be // matched to one, and it's never answered. A client that couldn't // read a line this server sent, whatever it was, may answer it with - // id null, or under MCP with no id. The id and the error member, if - // it has one, are printed as sent, through logValue, since they're - // the client's text. + // id null, or under MCP with no id. The error member, if it has one, + // is printed through logValue, which escapes and bounds the client's + // text, and so is the id when it's a string or a number; any other + // id is named by its kind. const { id, error } = message as { id?: unknown; error?: unknown }; const which = !Object.hasOwn(message as object, 'id') ? 'with no id' : `whose id is ${typeof id === 'string' || typeof id === 'number' ? logValue(id) : jsonKind(id)}`; diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index 2239c6b7a..eb24a3a12 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -321,11 +321,11 @@ interface HeldItem { * no id, when it couldn't read a request (JSON-RPC 2.0 §5), so once * connected it may be the answer to a request of ours, which gets nothing * from it and ends as any unanswered request does, by its timeout where it - * has one. (During the handshake such an error answers initialize.) Its - * error member, if it has one, is the server's text, whatever JSON value - * it is, so it's printed redacted with `redact`, the server's redaction - * (logValueRedacted), as handleResponse prints a matched error's code and - * message. */ + * has one. (During the handshake such an error answers initialize, when + * its jsonrpc is "2.0".) Its error member, if it has one, is the server's + * text, whatever JSON value it is, so it's printed redacted with `redact`, + * the server's redaction (logValueRedacted), as handleResponse prints a + * matched error's code and message. */ function unmatchedResponse( serverId: string, response: { id?: unknown; error?: unknown }, @@ -614,13 +614,13 @@ export class McplServerConnection extends EventEmitter { private config: McplServerConfig | null = null; /** The redactor of the server's text for `config`, made at its first error * answer or response it can't match (handleResponse), by when `config` - * is set, and kept: a framework-launched child's - * `env` is a getter that reads the awareness ledger, so a redactor built - * per answer would read it per answer (#540's review, R1-2). An answer - * takes one build of it for its words, data and log line (`now()`), so - * it reads the ledger at most once. It builds again only after a build - * that failed, which withholds that answer's text, or once a dial notes - * something new (serverTextRedactor). */ + * is set, and kept: a framework-launched child's `env` is a getter that + * reads the awareness ledger, so a redactor built per answer would read + * it per answer (#540's review, R1-2). An answer takes one build of it + * for its words, data and log line (`now()`), so it reads the ledger at + * most once. It builds again only after a build that failed, which + * withholds that answer's text, or once a dial notes something new + * (serverTextRedactor). */ private redactor: ServerTextRedactor | null = null; private hostCapabilities: McplHostCapabilities | null = null; private reconnectEnabled = false; @@ -1270,10 +1270,13 @@ export class McplServerConnection extends EventEmitter { // that way (JSON-RPC 2.0 §5; MCP 2025-11-25, Basic), initialize is // the only request this host has out, and a peer never answers a // response. Such an error means the server can't read this host's - // messages, so the handshake fails at once, in the server's words. - // `"jsonrpc": "2.0"` keeps a server's own JSON log line, which can - // carry an id and an error key, from failing it. Any other response - // whose id can't be matched is logged, as on a connected transport. + // messages, so the handshake fails at once, with the error's message. + // `"jsonrpc": "2.0"` keeps a server's own JSON log line with an + // error key and id null, or no id, from failing it; one with + // initialize's id is taken for the answer, whatever its jsonrpc. Of + // the other responses, one whose id is neither a string nor a number, + // or that has none, is logged, as on a connected transport, and one + // with another string or number id is ignored. const envelope = readEnvelope(msg as unknown as Record); if (envelope.kind === 'request' && envelope.method === 'ping') { transport.writeLine(JSON.stringify({ jsonrpc: '2.0', id: envelope.id, result: {} })); diff --git a/test/mcpl-request-envelope.test.ts b/test/mcpl-request-envelope.test.ts index da8b076e5..806ed8687 100644 --- a/test/mcpl-request-envelope.test.ts +++ b/test/mcpl-request-envelope.test.ts @@ -5,16 +5,17 @@ * the transport it came on, an invalid one included; a message with an id * and no method is a response, whatever the id's value, and so is a * JSON-RPC 2.0 error with no id, as MCP allows; a response is never - * answered, and one whose id can't be matched is logged, its error - * redacted, during the handshake too, except that an error with id null or - * none then answers initialize, and the connect fails at once; any other - * object with neither a method nor an id isn't JSON-RPC, so it's ignored, - * as a JSON value that isn't an object is, a batch included. A message - * whose id isn't a string, a number or null, or whose method isn't a - * string, with or without an id, is answered once with -32600, carrying its - * id where that's a string or a number and null otherwise, and isn't - * dispatched. Real connection routing and response encoding, and stdio - * servers for the handshake. + * answered, and one whose id is neither a string nor a number, or that has + * none, can't be matched, so it's logged, its error redacted, during the + * handshake too, except that a JSON-RPC 2.0 error with id null or none then + * answers initialize, and the connect fails at once; any other object with + * neither a method nor an id isn't JSON-RPC, so it's ignored, as a JSON + * value that isn't an object is, a batch included. A message whose id isn't + * a string, a number or null, or whose method isn't a string, with or + * without an id, is answered once with -32600, carrying its id where that's + * a string or a number and null otherwise, and isn't dispatched. Real + * connection routing and response encoding, and stdio servers for the + * handshake. */ import { test } from 'node:test'; import assert from 'node:assert/strict'; @@ -231,6 +232,21 @@ test("an error that can't be matched is logged with a credential its server's co ]); }); +test("responses that can't be matched read their connection's configuration once for its redaction, and keep it (#540's review, R1-2)", (t) => { + const { connection, send, logged } = wire(t); + const token = 'tok-0123456789abcdef'; + // A framework-launched server's env is a getter that reads the awareness + // ledger each time. + let reads = 0; + const config = { id: 'srv', command: 'server' } as McplServerConfig; + Object.defineProperty(config, 'env', { enumerable: true, get: () => { reads++; return { API_TOKEN: token }; } }); + (connection as unknown as { config: McplServerConfig }).config = config; + for (let i = 0; i < 3; i++) send({ jsonrpc: '2.0', id: null, error: { code: -32000, message: `bad token ${token}` } }); + assert.equal(reads, 1, 'read for the first of these responses only'); + assert.equal(logged.length, 3); + for (const line of logged) assert.ok(line.includes(String.raw`bad\u0020token\u0020redacted`) && !line.includes(token), line); +}); + test("a null-id error while a request of the host's is pending leaves that request pending, and its own answer settles it", async (t) => { const { connection, transport, send, logged } = wire(t); const orphans: unknown[] = []; @@ -278,7 +294,7 @@ for (const [form, answer] of [ ['with id null', { jsonrpc: '2.0', id: null, error: { code: -32700, message: 'Parse error' } }], ['with no id, as MCP allows', { jsonrpc: '2.0', error: { code: -32700, message: 'Parse error' } }], ] as const) { - test(`during the handshake an error ${form} answers initialize, the one request out: the connect fails at once, in the server's words`, async () => { + test(`during the handshake an error ${form} answers initialize, the one request out: the connect fails at once, with the error's message`, async () => { // A server that couldn't read initialize, and so answers it naming no // request. Its error is the handshake's, rather than a timeout 30 s later. const server = initializeAnswers(JSON.stringify(answer)); @@ -289,7 +305,7 @@ for (const [form, answer] of [ }); } -test("during the handshake any other response that can't be matched is logged, one with another number is ignored, and the handshake goes on", async (t) => { +test("during the handshake a response whose id isn't a string or a number, and that doesn't answer initialize, is logged, one with another number is ignored, and the handshake goes on", async (t) => { const logged: string[] = []; t.mock.method(console, 'error', (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }); // Answers initialize first with a number that isn't initialize's, then with