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..db660bf65 --- /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. 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). + - **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 623cae22d..afb1217c6 100644 --- a/src/api/mcp-server-core.ts +++ b/src/api/mcp-server-core.ts @@ -14,6 +14,8 @@ import WebSocket from 'ws'; import { escapeLineSeparators, readLines } from '../line-reader.js'; 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; } @@ -177,20 +180,56 @@ 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 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)}`; + 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; } } @@ -210,10 +249,6 @@ export class McpServer { }); break; - case 'initialized': - // Client acknowledgment, no response needed - break; - case 'tools/list': this.sendResult(request.id, { tools: this.getTools() }); break; @@ -628,7 +663,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 e683eccd2..7d86c0e6d 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -999,7 +999,8 @@ class McpConfigurationRefusal extends Error { /** 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; }; @@ -31837,7 +31838,7 @@ export class AgentFramework { onRequest('inference-request', async ( params: McplInferenceRequestParams, responder?: { - id: string | number; + id: string | number | null; respond: (result: unknown) => void; respondError: (code: number, message: string) => void; sendChunk?: (chunk: InferenceChunkParams) => void; 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 b58aeabde..c359f0078 100644 --- a/src/mcpl/inference-router.ts +++ b/src/mcpl/inference-router.ts @@ -36,8 +36,10 @@ import { logValue } from './log-value.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; /** Sends a streamed chunk on the transport the request came on, as its * answer is sent. */ sendChunk?(params: InferenceChunkParams): void; diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index 91504c158..05024c1ef 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, logValueRedacted, loggedCode } from './log-value.js'; +import { jsonKind, readEnvelope } from '../jsonrpc-envelope.js'; import { LEGACY_MCP_PROTOCOL_VERSION, MODERN_MCP_PROTOCOL_VERSION, @@ -364,11 +365,25 @@ interface HeldItem { kept?: true; } -/** 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, 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 }, + redact: (text: string) => string, +): 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: ${logValueRedacted(response.error, redact)}` : ''); } /** @@ -693,13 +708,14 @@ export class McplServerConnection extends EventEmitter { // Reconnect state (adapted from Anarchid/agent-framework@mcpl-module-proto) private config: McplServerConfig | null = null; /** The redactor of the server's text for `config`, made at its first error - * answer, 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). */ + * 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). */ private redactor: ServerTextRedactor | null = null; private hostCapabilities: McplHostCapabilities | null = null; private reconnectEnabled = false; @@ -1337,15 +1353,35 @@ 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, 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: {} })); + 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, (text) => redactServerText(text, config))); } return; } - if (msg.id !== initId) return; cleanup(); if (msg.error) { // describeCause rather than the template's own conversion: the @@ -2372,10 +2408,10 @@ export class McplServerConnection extends EventEmitter { // passed is the dead process's (afterBoundary). const epoch = this.transportEpoch; transport.on('line', (line: string) => { - const msg = messageFromLine(this.id, line, transport) as JsonRpcRequest | JsonRpcResponse | undefined; + const msg = messageFromLine(this.id, line, transport); if (!msg) return; try { - this.dispatchMessage(msg, epoch); + this.dispatchMessage(msg as Record, epoch); } catch (error) { const request = msg as { method?: unknown; id?: unknown }; this.reportInboundFailure(error, { @@ -2387,25 +2423,47 @@ export class McplServerConnection extends EventEmitter { } /** Contain synchronous dispatch failures; async RPC handlers own their promises. */ - private dispatchMessage(msg: JsonRpcRequest | JsonRpcResponse, epoch: number): void { - // Is this a response to one of our outbound requests? - if ('id' in msg && msg.id != null && !('method' in msg)) { - const late = epoch !== this.transportEpoch; - // RFC-014: its hints apply as it's read, whether or not anyone still - // waits for it. A dead transport's revs are its own, so a response - // read after its boundary is never tapped. - if (!late) this.tapResponse(msg as JsonRpcResponse); - this.handleResponse(msg as JsonRpcResponse, late); - return; + private dispatchMessage(msg: Record, epoch: number): 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. One read after + // its transport's boundary is late (handleResponse). + const late = epoch !== this.transportEpoch; + // RFC-014: its hints apply as it's read, whether or not anyone still + // waits for it. A dead transport's revs are its own, so a response + // read after its boundary is never tapped. + if (!late) this.tapResponse(msg as unknown as JsonRpcResponse); + this.handleResponse(msg as unknown as JsonRpcResponse, late); + return; + } + case 'neither': + // Not JSON-RPC: ignored like the server's other unrelated output. + return; + case 'invalid': { + // Answered on the transport it came on, as every request is. One + // read after that transport's boundary is the dead process's and + // isn't answered: sendErrorResponse logs the answer it didn't send. + this.sendErrorResponse(envelope.id, envelope.error.code, envelope.error.message, undefined, epoch); + if (epoch !== this.transportEpoch) return; + 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; // Read after its transport's boundary, it is the dead process's: the // boundary's rule applies (afterBoundary). if (epoch !== this.transportEpoch) { - this.afterBoundary(request, epoch); + this.afterBoundary(request, requestId, epoch); return; } @@ -2415,7 +2473,7 @@ 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; } @@ -2426,11 +2484,11 @@ export class McplServerConnection extends EventEmitter { if (request.method === 'push/event') this.tap({ kind: 'push', params: request.params }); else if (request.method === 'tools/hints' && this.readTap) { this.tap({ kind: 'hints', params: request.params }); - 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 @@ -2442,8 +2500,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)`); } @@ -2451,8 +2509,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; } @@ -2464,20 +2522,19 @@ 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, this.responderFor(request.id, epoch)); + this.dispatchInboundEvent(eventName, request.params, this.responderFor(requestId, epoch)); } else { // Notification — no response expected this.dispatchInboundEvent(eventName, request.params); @@ -2487,9 +2544,10 @@ export class McplServerConnection extends EventEmitter { /** * A request's responder, bound to the transport it came on (`epoch`): an * answer, or a streamed inference's chunk, sent after that transport has - * closed reaches nothing, never a server that didn't ask it. + * closed reaches nothing, never a server that didn't ask it. A request + * sent with id null is answered with id null. */ - private responderFor(id: string | number, epoch: number) { + private responderFor(id: string | number | null, epoch: number) { return { id, respond: (result: unknown) => this.sendResponse(id, result, epoch), @@ -2513,17 +2571,17 @@ export class McplServerConnection extends EventEmitter { * it while its writer lives, with no grant to come, so it drops and logs * all of it; hold's bounds cap what any other keeps. */ - private afterBoundary(request: JsonRpcRequest, epoch: number): void { - const event = typeof request.method === 'string' && Object.hasOwn(McplServerConnection.METHOD_TO_EVENT, request.method) + private afterBoundary(request: JsonRpcRequest, requestId: string | number | null | undefined, epoch: number): void { + const event = Object.hasOwn(McplServerConnection.METHOD_TO_EVENT, request.method) ? McplServerConnection.METHOD_TO_EVENT[request.method] : undefined; if (this.closedForGood || event === undefined || !McplServerConnection.WORLD_EVENTS.has(event)) { console.error( `[mcpl] ${this.id}: dropped a message read after its transport closed` + - `${this.closedForGood ? ', with no transport to come' : ''}: ${describeMethod(request.method)}`, + `${this.closedForGood ? ', with no transport to come' : ''}: method ${logValue(request.method)}`, ); return; } - this.hold(event, request.id != null ? [request.params, this.responderFor(request.id, epoch)] : [request.params], true); + this.hold(event, requestId !== undefined ? [request.params, this.responderFor(requestId, epoch)] : [request.params], true); } /** Live delivery and delayed plane flushes share the same containment boundary. */ @@ -2578,7 +2636,7 @@ export class McplServerConnection extends EventEmitter { /** Tap a response to a hinted request (RFC-014): its result's hints, or a * list's statement, apply now, awaited or not. */ private tapResponse(response: JsonRpcResponse): void { - if (!this.readTap) return; + if (!this.readTap || response.id === null) return; const sent = this.hintedRequests.get(response.id); if (!sent) return; this.hintedRequests.delete(response.id); @@ -2591,6 +2649,14 @@ export class McplServerConnection extends EventEmitter { * Handle a JSON-RPC response by resolving/rejecting the corresponding pending request. */ private handleResponse(response: JsonRpcResponse, late = false): 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, on this transport or an + // earlier one. 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, (this.redactor ??= serverTextRedactor(this.config ?? {})).now())); + return; + } // One read after its transport's boundary answers a request the close // ended. It's matched against none of this connection's request records, // since the next transport numbers its requests afresh, and goes the way @@ -2688,7 +2754,7 @@ export class McplServerConnection extends EventEmitter { /** * Send a successful JSON-RPC response back to the server. */ - private sendResponse(id: string | number, result: unknown, epoch = this.transportEpoch): void { + private sendResponse(id: string | number | null, result: unknown, epoch = this.transportEpoch): void { // A closed connection's epoch is past every request's, so a refusal // while it is closed is logged too. if (!this.sameTransport(id, epoch) || this.closed) return; @@ -2699,7 +2765,7 @@ export class McplServerConnection extends EventEmitter { /** Whether a response to a request received on `epoch` may go out now. * The id is the server's, so it's logged as a field; `answer`, which the * caller has formatted for the log, ends the line. */ - private sameTransport(id: string | number, epoch: number, answer?: string): boolean { + private sameTransport(id: string | number | null, epoch: number, answer?: string): boolean { if (epoch === this.transportEpoch) return true; console.error( `[mcpl] ${this.id}: not answering request ${logValue(id)}: the transport it came on has closed${answer ? `, with ${answer}` : ''}`, @@ -2711,7 +2777,7 @@ export class McplServerConnection extends EventEmitter { * Send a JSON-RPC error response back to the server. */ private sendErrorResponse( - id: string | number, + id: string | number | null, code: number, message: string, data?: unknown, diff --git a/src/mcpl/types.ts b/src/mcpl/types.ts index 8df65098a..ee71b1f4d 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 d213dd0f5..a5bd8c5dc 100644 --- a/test/mcp-server-bin.test.ts +++ b/test/mcp-server-bin.test.ts @@ -20,7 +20,6 @@ import { WebSocketServer } from 'ws'; // The compiled test sits in dist/test/, beside dist/src/. const BIN = fileURLToPath(new URL('../src/api/mcp-server.js', import.meta.url)); const CORE = fileURLToPath(new URL('../src/api/mcp-server-core.js', import.meta.url)); -const LINE_READER = fileURLToPath(new URL('../src/line-reader.js', import.meta.url)); const ROOT_MODULE = fileURLToPath(new URL('../src/index.js', import.meta.url)); // The repository root, from which mcp-wrapper.sh runs the built file. const ROOT = fileURLToPath(new URL('../../', import.meta.url)); @@ -109,16 +108,15 @@ 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, with the module it - // imports at the path it imports it from, and inside the package 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}`); - mkdirSync(join(bundleDir, 'api'), { recursive: true }); + // 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, 'api', 'mcp-server.js'); + const bundle = join(bundleDir, 'mcp-server.js'); copyFileSync(CORE, bundle); - copyFileSync(LINE_READER, join(bundleDir, 'line-reader.js')); const cases: Array<[string, string[], string | undefined]> = [ ['a host that imports the package root, from an entry script of its own', [entry], undefined], ['the package root imported under `node -e`', ['--input-type=module', '-e', `await import(${rootUrl});`], undefined], 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 3a6b08ca0..b3c012ea7 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; noChunkSender?: boolean }, + opts: { stream: boolean; mode: Mode; policy?: InferenceRoutingPolicy; noChunkSender?: boolean; requestId?: string | number | null }, ): Promise { const membrane = new Membrane(new ScriptedAdapter(script), { formatter: opts.mode === 'xml' ? new AnthropicXmlFormatter({ toolMode: 'xml' }) : new NativeFormatter(), @@ -126,17 +126,19 @@ async function ask( answer.stream = streamYielding(request); return answer.stream; }; - return askWith(membrane, opts.stream, answer, opts.policy, opts.noChunkSender); + return askWith(membrane, opts.stream, answer, opts.policy, opts.noChunkSender, opts.requestId); } /** Ask a router built on `membrane`, routing by `policy` (none given: a policy - * whose default is `default`), for one inference. */ + * whose default is `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, noChunkSender = false, + requestId: string | number | null = 7, ): Promise { const router = new InferenceRouter( membrane, @@ -151,7 +153,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}`); @@ -385,6 +387,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 b49cfd633..752451222 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 holds 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..94615e914 --- /dev/null +++ b/test/mcpl-request-envelope.test.ts @@ -0,0 +1,401 @@ +/** + * 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, and only on + * 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 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'; + +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'; +import type { McplServerConfig } from '../src/mcpl/types.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 tools/hints request with id null, read while a read tap listens, is answered with id null', (t) => { + const { connection, send, replies } = wire(t); + const reads: string[] = []; + connection.setReadTap((read) => { reads.push(read.kind); }); + send({ jsonrpc: '2.0', id: null, method: 'tools/hints', params: { rev: 1, tools: [] } }); + assert.deepEqual(reads, ['hints']); + assert.deepEqual(replies(), [{ jsonrpc: '2.0', id: null, result: {} }]); +}); + +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, or an invalid one, is answered only on the transport it came on', (t) => { + const { connection, transport, send, logged } = wire(t); + const incoming: Array<{ id: unknown; respond: (r: unknown) => void }> = []; + connection.on('channels-incoming', (_params: unknown, responder: { id: unknown; respond: (r: unknown) => void }) => { + incoming.push(responder); + }); + // One the handler answers only after the transport it came on has closed. + send({ jsonrpc: '2.0', id: null, method: 'channels/incoming', params: { messages: [{ id: 'live-1' }] } }); + assert.equal(incoming.length, 1); + // The transport's boundary passes, and the next transport serves. + (connection as unknown as { resetPolicyForTransportBoundary(): void }).resetPolicyForTransportBoundary(); + const next = new ResponseTransport(); + (connection as unknown as { transport: McplTransport }).transport = next; + incoming[0]!.respond({ ok: true }); + // What the closed transport still sends: a world message, kept for the next + // grant, a request about its own process, dropped, and an invalid request. + send({ jsonrpc: '2.0', id: null, method: 'channels/incoming', params: { messages: [{ id: 'kept-1' }] } }); + send({ jsonrpc: '2.0', id: null, method: 'channels/list', params: {} }); + send({ jsonrpc: '2.0', id: {}, method: 'channels/list' }); + connection.establishGrant(new CapabilityGrant(new Set(ALL_CAPABILITY_PATHS), [])); + connection.ready(); + assert.equal(incoming.length, 2, 'the kept message is admitted under the next grant'); + assert.equal(incoming[1]!.id, null); + incoming[1]!.respond({ ok: true }); + assert.deepEqual(next.lines, [], 'nothing is answered on the next transport'); + assert.deepEqual(transport.lines, [], 'nor on the closed one'); + assert.deepEqual(logged, [ + '[mcpl] srv: not answering request null: the transport it came on has closed', + '[mcpl] srv: dropped a message read after its transport closed: method channels/list', + '[mcpl] srv: not answering request null: the transport it came on has closed, with error -32600 "Invalid Request: the id must be a string, a number or null, not an object"', + '[mcpl] srv: not answering request null: the transport it came on has closed', + ]); +}); + +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("an error that can't be matched is logged with a credential its server's configuration holds redacted", (t) => { + const { connection, send, logged } = wire(t); + const token = 'tok-0123456789abcdef'; + (connection as unknown as { config: McplServerConfig }).config = { id: 'srv', command: 'server', env: { API_TOKEN: token } }; + send({ jsonrpc: '2.0', id: null, error: { code: -32000, message: `bad token ${token}` } }); + send({ jsonrpc: '2.0', error: `bad token ${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\":-32000,\"message\":\"bad\u0020token\u0020redacted\"}"`, + String.raw`[mcpl] srv: ignored a response with no id, which can't be matched to a request of ours; its error: "bad\u0020token\u0020redacted"`, + ]); +}); + +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[] = []; + 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, 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)); + 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 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 + // 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("during the handshake an error that can't be matched is logged with a credential its server's configuration holds redacted", async (t) => { + const logged: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }); + const token = 'tok-0123456789abcdef'; + // A log line of the server's own, with id null and an error, that doesn't + // say it's JSON-RPC, so it doesn't answer initialize; then the answer. + const server = initializeAnswers( + JSON.stringify({ level: 'error', id: null, error: `bad token ${token}` }), + JSON.stringify({ jsonrpc: '2.0', id: 'INIT', result: { protocolVersion: '2024-11-05', capabilities: { experimental: { mcpl: { version: '0.5' } } } } }), + ); + const connection = await McplServerConnection.connect( + { id: 'unmatched', command: process.execPath, args: ['-e', server], requestTimeoutMs: 5_000, env: { API_TOKEN: token } }, + { version: '0.5' }, + ); + try { + assert.deepEqual(logged.filter((entry) => entry.startsWith('[mcpl] unmatched: ignored a response')), [ + String.raw`[mcpl] unmatched: ignored a response whose id is null, which can't be matched to a request of ours; its error: "bad\u0020token\u0020redacted"`, + ]); + } 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.filter((line) => !line.includes('so it was dropped')), []); +}); diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index 6ca620574..0d65b9c1c 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -484,8 +484,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; }); @@ -632,20 +633,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", ]); }); @@ -959,13 +959,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]); });