From ebfc3154487f98b597b7b984441837ea731b3191 Mon Sep 17 00:00:00 2001 From: Felix-299 Date: Fri, 2 Oct 2026 01:20:00 -0400 Subject: [PATCH 01/29] fix(mcpl): contain inbound request handler failures Plain EventEmitter listeners discard returned promises. Channel handlers acknowledge requests before reconciliation, so a later lifecycle-store exception used to become an unhandled rejection and could terminate the host. Wrap the inbound RPC listeners once at wireMcplEvents. Invoke handlers immediately to retain control/data-plane ordering, track result and error response attempts, consume synchronous throws and asynchronous rejections, and send -32603 only before any response attempt. A throwing write is outcome-unknown and is never followed by another reply. Emit a typed mcpl:request-handler-error trace, including a secondary error-response failure when present. Apply the same boundary to push, inference, incoming, host-command, and synchronous RPC listeners. Regression tests use the real framework wiring and channel registry with an injected Chronicle lifecycle append failure; cover pre/post-ACK errors, notifications, all async listener families, synchronous throws, failed transport responses, and preserved inference IDs/synchronous success. The suite was 1 pass / 12 fail on original source and 13 pass / 0 fail with the wrapper. Full compiled differential is being collected for the PR. Co-Authored-By: GPT-6 Astra --- CHANGELOG.md | 4 + src/framework.ts | 91 +++++++++--- src/types/trace.ts | 14 ++ test/mcpl-request-handler-errors.test.ts | 174 +++++++++++++++++++++++ 4 files changed, 265 insertions(+), 18 deletions(-) create mode 100644 test/mcpl-request-handler-errors.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b9a30b4c..49668fc0f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ Releases up to and including 0.7.3 predate this file; for their contents see ## Unreleased +### Fixed + +- MCPL request and notification handlers contain synchronous throws and async rejections (#186), including channel reconciliation failures after an acknowledgement. Failures emit `mcpl:request-handler-error`; requests receive JSON-RPC `-32603` only when no response write has been attempted. Failed error-response writes are included in the trace, and notifications receive no response. + ## 0.19.0 — 2026-09-28 ### Added diff --git a/src/framework.ts b/src/framework.ts index 510b9183a..9109ba93d 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -13542,6 +13542,66 @@ export class AgentFramework { * Push events and inference requests are deferred to Steps 6/7. */ private wireMcplEvents(connection: McplServerConnection): void { + type Responder = { + id: string | number; + respond: (result: unknown) => void; + respondError: (code: number, message: string, data?: unknown) => void; + }; + + // EventEmitter does not consume promises returned by listeners. Keep + // each RPC handler's rejection at this boundary, including failures after + // channels/register or channels/changed has acknowledged the request. + const onRequest = ( + event: string, + handler: (params: T, responder?: Responder) => void | Promise, + ): void => { + connection.on(event, (params: T, responder?: Responder) => { + let responseAttempted = false; + const tracked = responder && { + ...responder, + respond: (result: unknown) => { + // A throwing transport write may have partially sent the reply. + // Mark before writing so recovery never sends a second response. + responseAttempted = true; + responder.respond(result); + }, + respondError: (code: number, message: string, data?: unknown) => { + responseAttempted = true; + responder.respondError(code, message, data); + }, + }; + const failed = (error: unknown): void => { + const message = error instanceof Error ? error.message : String(error); + const attemptedBeforeFailure = responseAttempted; + let responseError: string | undefined; + if (tracked && !responseAttempted) { + try { + tracked.respondError(-32603, message); + } catch (sendError) { + responseError = sendError instanceof Error ? sendError.message : String(sendError); + } + } + this.emitTrace({ + type: 'mcpl:request-handler-error', + serverId: connection.id, + event, + requestId: responder?.id, + responseAttempted: attemptedBeforeFailure, + error: message, + ...(responseError === undefined ? {} : { responseError }), + }); + }; + try { + // Invoke immediately: control-plane responses must retain their + // ACK-before-reconcile ordering and data-plane barrier semantics. + const result = handler(params, tracked); + if (result) void result.catch(failed); + } catch (error) { + failed(error); + } + }); + }; + // Forward subprocess stderr lines as trace events so consumers (conhost, // log sinks, TUI badges) can persist and surface them. connection.on('stderr', (params: { line: string }) => { @@ -13559,7 +13619,7 @@ export class AgentFramework { // framework; the ScopeManager and config `scopes` wiring went with it. // Handle push events (Step 6) - connection.on('push-event', async ( + onRequest('push-event', async ( params: PushEventParams, responder?: { respond: (result: unknown) => void; respondError: (code: number, message: string) => void }, ) => { @@ -13569,7 +13629,7 @@ export class AgentFramework { }); // Handle server-initiated inference requests (Step 6) - connection.on('inference-request', async ( + onRequest('inference-request', async ( params: McplInferenceRequestParams, responder?: { id: string | number; respond: (result: unknown) => void; respondError: (code: number, message: string) => void }, ) => { @@ -13590,7 +13650,7 @@ export class AgentFramework { }); // Handle channel registration (Step 7) - connection.on('channels-register', async ( + onRequest('channels-register', async ( params: ChannelsRegisterParams, responder?: { respond: (result: unknown) => void }, ) => { @@ -13601,7 +13661,7 @@ export class AgentFramework { // present for the Request form and carries itemized added-descriptor // results; absent for Notifications, where rejection is itemwise // filtering plus a diagnostic trace. - connection.on('channels-changed', async ( + onRequest('channels-changed', async ( params: ChannelsChangedParams, responder?: { respond: (result: unknown) => void }, ) => { @@ -13619,7 +13679,7 @@ export class AgentFramework { }); }); - connection.on('channels-list', ( + onRequest('channels-list', ( _params: unknown, responder?: { respond: (result: unknown) => void }, ) => { @@ -13631,7 +13691,7 @@ export class AgentFramework { // RFC-007 §6: the server's filter over tools/lifecycle. Admission already // required toolLifecycle.observe (-32002 otherwise). Validation failures // leave the previous filter in force (§6.6). - connection.on('tools-observe', ( + onRequest('tools-observe', ( params: unknown, responder?: { respond: (result: unknown) => void; respondError?: (code: number, message: string, data?: unknown) => void }, ) => { @@ -13645,7 +13705,7 @@ export class AgentFramework { responder?.respond({}); }); - connection.on('model-info', ( + onRequest('model-info', ( _params: unknown, responder?: { respond: (result: unknown) => void; respondError?: (code: number, message: string, data?: unknown) => void }, ) => { @@ -13662,7 +13722,7 @@ export class AgentFramework { }); // Handle incoming channel messages (Step 7) - connection.on('channels-incoming', async ( + onRequest('channels-incoming', async ( params: ChannelsIncomingParams, responder?: { respond: (result: unknown) => void }, ) => { @@ -13672,20 +13732,15 @@ export class AgentFramework { }); // Handle host-level admin commands from a surface (e.g. Discord /undo) - connection.on('host-command', async ( + onRequest('host-command', async ( params: HostCommandParams, responder?: { respond: (result: unknown) => void; respondError: (code: number, message: string) => void }, ) => { if (!responder) return; - try { - const barrier = this.discordAwarenessBarrier; - if (barrier) await barrier.promise; - const result = await this.handleHostCommand(connection.id, params ?? {}); - responder.respond(result); - } catch (error) { - const err = error instanceof Error ? error : new Error(String(error)); - responder.respondError(-32603, err.message); - } + const barrier = this.discordAwarenessBarrier; + if (barrier) await barrier.promise; + const result = await this.handleHostCommand(connection.id, params ?? {}); + responder.respond(result); }); // Handle dynamic tool list changes (notifications/tools/list_changed) diff --git a/src/types/trace.ts b/src/types/trace.ts index e4a6bdba9..1fa4bfd55 100644 --- a/src/types/trace.ts +++ b/src/types/trace.ts @@ -428,6 +428,20 @@ export type TraceEvent = serverId: string; error: string; }) + | (TraceEventBase & { + /** An inbound RPC request/notification handler threw or rejected. */ + type: 'mcpl:request-handler-error'; + serverId: string; + /** Connection event name, e.g. channels-register or push-event. */ + event: string; + /** Absent for notifications. */ + requestId?: string | number; + /** True if a response write was attempted before the handler failed. */ + responseAttempted: boolean; + error: string; + /** Failure while trying to send an internal-error response. */ + responseError?: string; + }) | (TraceEventBase & { type: 'mcpl:orphaned-response'; serverId: string; diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts new file mode 100644 index 000000000..c6c4f84ec --- /dev/null +++ b/test/mcpl-request-handler-errors.test.ts @@ -0,0 +1,174 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { EventEmitter } from 'node:events'; + +import { AgentFramework } from '../src/framework.js'; +import { ChannelRegistry } from '../src/mcpl/channel-registry.js'; +import { CapabilityGrant, ALL_CAPABILITY_PATHS } from '../src/mcpl/capability-grant.js'; + +function harness() { + const traces: any[] = []; + const connection = Object.assign(new EventEmitter(), { id: 'srv' }); + const framework = Object.create(AgentFramework.prototype) as any; + framework.traceListeners = [(event: unknown) => traces.push(event)]; + framework.wireMcplEvents(connection); + return { framework, connection, traces }; +} + +function responder() { + const results: unknown[] = []; + const errors: unknown[] = []; + return { + results, + errors, + id: 17, + respond(result: unknown) { results.push(result); }, + respondError(code: number, message: string, data?: unknown) { + errors.push({ code, message, data }); + }, + }; +} + +const settle = () => new Promise((resolve) => setImmediate(resolve)); +const failureTraces = (traces: any[]) => traces.filter((t) => t.type === 'mcpl:request-handler-error'); + +for (const [event, field] of [['channels-register', 'channels'], ['channels-changed', 'added']] as const) { + test(event + ': lifecycle persistence failure after ACK is traced without another response', async () => { + const { framework, connection, traces } = harness(); + const server = { + grant: new CapabilityGrant(new Set(ALL_CAPABILITY_PATHS), []), + async sendChannelsOpen() { return {}; }, + async sendChannelsClose() { return {}; }, + }; + const registry = new ChannelRegistry( + { getServer: () => server } as never, + {} as never, + () => {}, + () => {}, + ); + // Inject the reported failure at the real Chronicle append boundary. + // A descriptor without a label avoids the unrelated label-history write. + (registry as any).store = { + appendToStateJson() { throw new Error('injected lifecycle-store failure'); }, + }; + registry.setSubscriptionPolicy('srv', 'auto'); + framework.channelRegistry = registry; + const reply = responder(); + connection.emit(event, { [field]: [{ id: 'room', type: 'test', direction: 'bidirectional' }] }, reply); + await settle(); + + assert.equal(reply.results.length, 1, 'ACK still precedes reconciliation'); + assert.equal(reply.errors.length, 0, 'an accepted registration must not get a second response'); + const failures = failureTraces(traces); + assert.equal(failures.length, 1); + assert.equal(failures[0].serverId, 'srv'); + assert.equal(failures[0].event, event); + assert.equal(failures[0].requestId, 17); + assert.equal(failures[0].responseAttempted, true); + assert.match(failures[0].error, /injected lifecycle-store failure/); + }); +} + +for (const [event, component, method] of [ + ['channels-register', 'channelRegistry', 'handleRegister'], + ['channels-changed', 'channelRegistry', 'handleChanged'], + ['channels-incoming', 'channelRegistry', 'handleIncoming'], + ['push-event', 'pushHandler', 'handlePushEvent'], + ['inference-request', 'inferenceRouter', 'handleInferenceRequest'], +] as const) { + test(event + ': rejection before a response gets one JSON-RPC internal error', async () => { + const { framework, connection, traces } = harness(); + framework[component] = { + async [method]() { + await Promise.resolve(); + throw new Error('handler failed before response'); + }, + }; + const reply = responder(); + connection.emit(event, {}, reply); + await settle(); + + assert.equal(reply.results.length, 0); + assert.deepEqual(reply.errors, [{ code: -32603, message: 'handler failed before response', data: undefined }]); + const failures = failureTraces(traces); + assert.equal(failures.length, 1); + assert.equal(failures[0].event, event); + assert.equal(failures[0].responseAttempted, false); + }); +} + +test('notification rejection is traced without inventing a response', async () => { + const { framework, connection, traces } = harness(); + framework.channelRegistry = { async handleChanged() { throw new Error('notification failed'); } }; + connection.emit('channels-changed', {}); + await settle(); + const failures = failureTraces(traces); + assert.equal(failures.length, 1); + assert.equal(failures[0].requestId, undefined); + assert.match(failures[0].error, /notification failed/); +}); + +test('a synchronous handler throw is contained and answered', async () => { + const { framework, connection, traces } = harness(); + framework.channelRegistry = { descriptorsForServer() { throw new Error('list failed'); } }; + const reply = responder(); + assert.doesNotThrow(() => connection.emit('channels-list', {}, reply)); + await settle(); + assert.equal(reply.errors.length, 1); + assert.equal(failureTraces(traces).length, 1); +}); + +test('a throwing success response is not retried as an error response', async () => { + const { framework, connection, traces } = harness(); + framework.channelRegistry = { descriptorsForServer() { return []; } }; + const reply = responder(); + let writes = 0; + reply.respond = () => { writes++; throw new Error('response transport failed'); }; + assert.doesNotThrow(() => connection.emit('channels-list', {}, reply)); + await settle(); + assert.equal(writes, 1); + assert.equal(reply.errors.length, 0); + assert.equal(failureTraces(traces)[0].responseAttempted, true); +}); + +test('a failed error response is traced without an escaping rejection', async () => { + const { framework, connection, traces } = harness(); + framework.channelRegistry = { async handleRegister() { throw new Error('original failure'); } }; + const reply = responder(); + let writes = 0; + reply.respondError = () => { writes++; throw new Error('error response transport failed'); }; + connection.emit('channels-register', {}, reply); + await settle(); + assert.equal(writes, 1); + const failures = failureTraces(traces); + assert.equal(failures.length, 1); + assert.match(failures[0].error, /original failure/); + assert.match(failures[0].responseError, /error response transport failed/); +}); + +test('inference responders retain request IDs and successful handling stays synchronous', async () => { + const { framework, connection, traces } = harness(); + framework.inferenceRouter = { + handleInferenceRequest(_server: string, _params: unknown, reply: any) { + assert.equal(reply.requestId, 17); + reply.respond({ accepted: true }); + }, + }; + const reply = responder(); + connection.emit('inference-request', {}, reply); + assert.deepEqual(reply.results, [{ accepted: true }]); + await settle(); + assert.equal(reply.errors.length, 0); + assert.equal(failureTraces(traces).length, 0); +}); + +test('host command response-write failure is traced without a second reply', async () => { + const { framework, connection, traces } = harness(); + framework.handleHostCommand = async () => ({ done: true }); + const reply = responder(); + reply.respond = () => { throw new Error('host response failed'); }; + connection.emit('host-command', {}, reply); + await settle(); + assert.equal(reply.errors.length, 0); + assert.match(failureTraces(traces)[0].error, /host response failed/); +}); From 1a4fbd95dc6983e0cf38b99703a5d88a0a87f337 Mon Sep 17 00:00:00 2001 From: Felix-299 Date: Fri, 2 Oct 2026 01:26:20 -0400 Subject: [PATCH 02/29] fix(mcpl): enforce a single response attempt inside handlers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wren’s independent review exercised the real InferenceRouter: it catches a failed result write and tries an error response internally, before the outer listener wrapper sees the rejection. Enforce the response-attempt guard in both tracked responder methods, preserve the original transport exception, and report it even when a handler swallows it. The wrapper consumes a subsequent throw without forwarding another response. Add the real inference-handler regression, a swallowed-write regression, and an already-sent error-response case. Against ebfc315 the expanded focused suite is 14 pass / 2 fail; with this correction it is 16 pass / 0 fail. Rebuild succeeds; updated full-suite and independent re-review follow. Co-Authored-By: GPT-6 Astra --- src/framework.ts | 37 +++++++++---- test/mcpl-request-handler-errors.test.ts | 66 ++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 11 deletions(-) diff --git a/src/framework.ts b/src/framework.ts index 9109ba93d..615bd4189 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -13557,20 +13557,30 @@ export class AgentFramework { ): void => { connection.on(event, (params: T, responder?: Responder) => { let responseAttempted = false; + let writeFailure: { error: unknown } | undefined; + const writeResponse = (write: () => void): void => { + if (responseAttempted) { + // Inner handler catches can try to answer a failed result with + // an error. Preserve the first failure and refuse the second write. + throw writeFailure ? writeFailure.error : new Error('MCPL response already attempted'); + } + // A throwing transport write may have partially sent the reply. + responseAttempted = true; + try { + write(); + } catch (error) { + writeFailure = { error }; + throw error; + } + }; const tracked = responder && { ...responder, - respond: (result: unknown) => { - // A throwing transport write may have partially sent the reply. - // Mark before writing so recovery never sends a second response. - responseAttempted = true; - responder.respond(result); - }, - respondError: (code: number, message: string, data?: unknown) => { - responseAttempted = true; - responder.respondError(code, message, data); - }, + respond: (result: unknown) => writeResponse(() => responder.respond(result)), + respondError: (code: number, message: string, data?: unknown) => + writeResponse(() => responder.respondError(code, message, data)), }; const failed = (error: unknown): void => { + if (writeFailure) error = writeFailure.error; const message = error instanceof Error ? error.message : String(error); const attemptedBeforeFailure = responseAttempted; let responseError: string | undefined; @@ -13594,8 +13604,13 @@ export class AgentFramework { try { // Invoke immediately: control-plane responses must retain their // ACK-before-reconcile ordering and data-plane barrier semantics. + const succeeded = (): void => { + // A handler may swallow a transport failure without rethrowing. + if (writeFailure) failed(writeFailure.error); + }; const result = handler(params, tracked); - if (result) void result.catch(failed); + if (result) void result.then(succeeded, failed); + else succeeded(); } catch (error) { failed(error); } diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index c6c4f84ec..2a414791f 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -4,6 +4,7 @@ import { EventEmitter } from 'node:events'; import { AgentFramework } from '../src/framework.js'; import { ChannelRegistry } from '../src/mcpl/channel-registry.js'; +import { InferenceRouter } from '../src/mcpl/inference-router.js'; import { CapabilityGrant, ALL_CAPABILITY_PATHS } from '../src/mcpl/capability-grant.js'; function harness() { @@ -162,6 +163,71 @@ test('inference responders retain request IDs and successful handling stays sync assert.equal(failureTraces(traces).length, 0); }); +test('real inference handler cannot turn a failed result write into a second response', async () => { + const { framework, connection, traces } = harness(); + framework.inferenceRouter = new InferenceRouter( + { + async complete() { + return { + content: [{ type: 'text', text: 'done' }], + stopReason: 'end_turn', + usage: { inputTokens: 1, outputTokens: 1 }, + }; + }, + } as never, + { isInHook: false } as never, + { validateInbound() {} } as never, + null, + (event) => traces.push(event), + ); + const reply = responder(); + let resultWrites = 0; + reply.respond = () => { resultWrites++; throw new Error('partially written result'); }; + connection.emit('inference-request', { + featureSet: 'test', messages: [{ role: 'user', content: 'hello' }], + }, reply); + await settle(); + assert.equal(resultWrites, 1); + assert.equal(reply.errors.length, 0, 'the inner inference catch must not send another reply'); + const failures = failureTraces(traces); + assert.equal(failures.length, 1); + assert.match(failures[0].error, /partially written result/); + assert.equal(failures[0].responseAttempted, true); +}); + +test('a swallowed response-write failure still produces a diagnostic', async () => { + const { framework, connection, traces } = harness(); + framework.channelRegistry = { + async handleRegister(_server: string, _params: unknown, reply: any) { + try { reply.respond({}); } catch { /* Simulate a handler swallowing a transport error. */ } + }, + }; + const reply = responder(); + reply.respond = () => { throw new Error('swallowed write failure'); }; + connection.emit('channels-register', {}, reply); + await settle(); + assert.equal(reply.errors.length, 0); + assert.equal(failureTraces(traces).length, 1); + assert.match(failureTraces(traces)[0].error, /swallowed write failure/); +}); + +test('an error response followed by a handler rejection is not answered again', async () => { + const { framework, connection, traces } = harness(); + framework.channelRegistry = { + async handleRegister(_server: string, _params: unknown, reply: any) { + reply.respondError(-32602, 'bad params'); + throw new Error('after error response'); + }, + }; + const reply = responder(); + connection.emit('channels-register', {}, reply); + await settle(); + assert.equal(reply.errors.length, 1); + assert.equal(failureTraces(traces).length, 1); + assert.match(failureTraces(traces)[0].error, /after error response/); + assert.equal(failureTraces(traces)[0].responseAttempted, true); +}); + test('host command response-write failure is traced without a second reply', async () => { const { framework, connection, traces } = harness(); framework.handleHostCommand = async () => ({ done: true }); From c5b1217845e439e575e3c51efe9cfaac71bd0a2d Mon Sep 17 00:00:00 2001 From: Felix-299 Date: Fri, 2 Oct 2026 02:43:29 -0400 Subject: [PATCH 03/29] fix(mcpl): distinguish response encoding failures from writes Upstream review of PR #208 identified that unserializable command progress (BigInt or circular data) fails before the transport sees bytes. Treating that as a potentially partial write prevented an internal-error response and left the request unanswered. Wrap JSON encoding failures in McplResponseSerializationError at the real connection response boundary, without retrying or pre-encoding the value. The request guard releases its attempt flag only for that explicit pre-write class, allowing one encodable error reply. Generic transport exceptions remain outcome-unknown and single-attempt. Preserve encoding failures even when swallowed by handlers, and report failures to stderr as well as traces for headless deployments. Real connection-routing regressions exercise BigInt, circular and throwing-toJSON results, invalid error data, swallowed encoding failures, and transport failures after encoding. A headless diagnostic test covers post-response failures without listeners. The expanded focused suite is 17 pass / 6 fail on 1a4fbd9, then 23 pass / 0 fail after the correction. Build passes after adding the public test-transport constructor; full suite and independent review are underway. Co-Authored-By: GPT-6 Astra --- CHANGELOG.md | 2 +- src/framework.ts | 20 ++-- src/mcpl/errors.ts | 8 ++ src/mcpl/server-connection.ts | 15 ++- test/mcpl-request-handler-errors.test.ts | 115 ++++++++++++++++++++++- 5 files changed, 147 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 49668fc0f..fcb866ca7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,7 @@ Releases up to and including 0.7.3 predate this file; for their contents see ### Fixed -- MCPL request and notification handlers contain synchronous throws and async rejections (#186), including channel reconciliation failures after an acknowledgement. Failures emit `mcpl:request-handler-error`; requests receive JSON-RPC `-32603` only when no response write has been attempted. Failed error-response writes are included in the trace, and notifications receive no response. +- MCPL request and notification handlers contain synchronous throws and async rejections (#186), including channel reconciliation failures after an acknowledgement. Failures emit `mcpl:request-handler-error`; requests receive JSON-RPC `-32603` only when no response write has been attempted. Pre-write JSON encoding failures can receive an internal-error reply; possibly partial transport writes remain single-attempt. Failed error-response writes are included in the trace, failures also reach stderr for headless hosts, and notifications receive no response. ## 0.19.0 — 2026-09-28 diff --git a/src/framework.ts b/src/framework.ts index 615bd4189..e52f08e58 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -385,6 +385,7 @@ import { EventGate, formatShadowWarning } from './gate/event-gate.js'; import { UsageTracker, type PersistedUsageState } from './usage/usage-tracker.js'; import type { SessionUsageSnapshot, UsageUpdatedEvent } from './usage/types.js'; import type { McplServerConnection } from './mcpl/server-connection.js'; +import { McplResponseSerializationError } from './mcpl/errors.js'; import type { McplServerConfig, McplHostCapabilities, @@ -13557,19 +13558,22 @@ export class AgentFramework { ): void => { connection.on(event, (params: T, responder?: Responder) => { let responseAttempted = false; - let writeFailure: { error: unknown } | undefined; + let responseFailure: { error: unknown; responseAttempted: boolean } | undefined; const writeResponse = (write: () => void): void => { if (responseAttempted) { // Inner handler catches can try to answer a failed result with // an error. Preserve the first failure and refuse the second write. - throw writeFailure ? writeFailure.error : new Error('MCPL response already attempted'); + throw responseFailure ? responseFailure.error : new Error('MCPL response already attempted'); } // A throwing transport write may have partially sent the reply. responseAttempted = true; try { write(); } catch (error) { - writeFailure = { error }; + // Only the encoder can establish that no write was attempted. + // Transport exceptions stay outcome-unknown and cannot be retried. + if (error instanceof McplResponseSerializationError) responseAttempted = false; + responseFailure = { error, responseAttempted }; throw error; } }; @@ -13580,9 +13584,9 @@ export class AgentFramework { writeResponse(() => responder.respondError(code, message, data)), }; const failed = (error: unknown): void => { - if (writeFailure) error = writeFailure.error; + if (responseFailure) error = responseFailure.error; const message = error instanceof Error ? error.message : String(error); - const attemptedBeforeFailure = responseAttempted; + const attemptedBeforeFailure = responseFailure?.responseAttempted ?? responseAttempted; let responseError: string | undefined; if (tracked && !responseAttempted) { try { @@ -13591,6 +13595,8 @@ export class AgentFramework { responseError = sendError instanceof Error ? sendError.message : String(sendError); } } + // Keep headless hosts diagnosable even without a trace subscriber. + console.error(`[mcpl] ${connection.id} ${event} handler failed:`, error); this.emitTrace({ type: 'mcpl:request-handler-error', serverId: connection.id, @@ -13605,8 +13611,8 @@ export class AgentFramework { // Invoke immediately: control-plane responses must retain their // ACK-before-reconcile ordering and data-plane barrier semantics. const succeeded = (): void => { - // A handler may swallow a transport failure without rethrowing. - if (writeFailure) failed(writeFailure.error); + // A handler may swallow an encoding/transport failure without rethrowing. + if (responseFailure) failed(responseFailure.error); }; const result = handler(params, tracked); if (result) void result.then(succeeded, failed); diff --git a/src/mcpl/errors.ts b/src/mcpl/errors.ts index 7bbb90297..b6b01bad8 100644 --- a/src/mcpl/errors.ts +++ b/src/mcpl/errors.ts @@ -40,6 +40,14 @@ export const CHANNEL_OPEN_FAILED = -32024; // Error Factories // ============================================================================ +/** Response encoding failed before the transport was given any bytes. */ +export class McplResponseSerializationError extends Error { + constructor(cause: unknown) { + super('MCPL response could not be serialized', { cause }); + this.name = 'McplResponseSerializationError'; + } +} + /** * Create a "feature set not enabled" error. * Returned when a server sends a message tagged with a feature set diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index f29fea22d..fb2ce88d6 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -14,7 +14,7 @@ import { EventEmitter } from 'node:events'; import { openTransport, type McplTransport, type TransportCloseInfo } from './transport.js'; import { maskNegotiatedCapabilities } from './capability-mask.js'; import { CapabilityGrant, expandAdvertisementShorthand } from './capability-grant.js'; -import { CAPABILITY_DISABLED } from './errors.js'; +import { CAPABILITY_DISABLED, McplResponseSerializationError } from './errors.js'; import type { McplServerConfig, @@ -1222,13 +1222,22 @@ export class McplServerConnection extends EventEmitter { } } + /** Keep definite pre-write encoding failures distinct from transport errors. */ + private serializeResponse(response: JsonRpcResponse): string { + try { + return JSON.stringify(response); + } catch (cause) { + throw new McplResponseSerializationError(cause); + } + } + /** * Send a successful JSON-RPC response back to the server. */ private sendResponse(id: string | number, result: unknown): void { if (this.closed) return; const response: JsonRpcResponse = { jsonrpc: '2.0', id, result }; - this.transport?.writeLine(JSON.stringify(response)); + this.transport?.writeLine(this.serializeResponse(response)); } /** @@ -1241,7 +1250,7 @@ export class McplServerConnection extends EventEmitter { id, error: { code, message, data }, }; - this.transport?.writeLine(JSON.stringify(response)); + this.transport?.writeLine(this.serializeResponse(response)); } // ========================================================================== diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index 2a414791f..6f9ec2fc0 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -5,11 +5,12 @@ import { EventEmitter } from 'node:events'; import { AgentFramework } from '../src/framework.js'; import { ChannelRegistry } from '../src/mcpl/channel-registry.js'; import { InferenceRouter } from '../src/mcpl/inference-router.js'; +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'; -function harness() { +function harness(connection = Object.assign(new EventEmitter(), { id: 'srv' })) { const traces: any[] = []; - const connection = Object.assign(new EventEmitter(), { id: 'srv' }); const framework = Object.create(AgentFramework.prototype) as any; framework.traceListeners = [(event: unknown) => traces.push(event)]; framework.wireMcplEvents(connection); @@ -238,3 +239,113 @@ test('host command response-write failure is traced without a second reply', asy assert.equal(reply.errors.length, 0); assert.match(failureTraces(traces)[0].error, /host response failed/); }); + +class ResponseTransport extends McplTransport { + constructor() { super(); } + readonly kind = 'stdio' as const; + readonly lines: string[] = []; + failWrite = false; + writeLine(line: string): void { + this.lines.push(line); + if (this.failWrite) throw new TypeError('transport failed after a partial write'); + } + async close(): Promise {} +} + +function wireHarness() { + const transport = new ResponseTransport(); + // Use real connection routing and response encoding without launching a server. + const connection = new (McplServerConnection as any)('srv', null, transport) as McplServerConnection; + (connection as any).allowHostCommands = true; + connection.establishGrant(new CapabilityGrant(new Set(ALL_CAPABILITY_PATHS), [])); + const h = harness(connection); + connection.ready(); + const request = (method = 'host/command') => transport.emit('line', JSON.stringify({ + jsonrpc: '2.0', id: 17, method, params: { command: 'maintain' }, + })); + return { ...h, transport, request }; +} + +for (const kind of ['bigint', 'circular', 'throwing toJSON'] as const) { + test('host-command ' + kind + ' serialization failure receives one internal-error response', async () => { + const { framework, traces, transport, request } = wireHarness(); + let calls = 0; + const circular: any = {}; + circular.self = circular; + const progress = kind === 'bigint' ? { count: 1n } : kind === 'circular' ? circular : { + toJSON() { calls++; throw new Error('snapshot serialization failed'); }, + }; + framework.handleHostCommand = async () => ({ progress }); + request(); + await settle(); + assert.equal(transport.lines.length, 1, 'encoding failed before any result bytes were written'); + const reply = JSON.parse(transport.lines[0]!); + assert.equal(reply.id, 17); + assert.equal(reply.error.code, -32603); + assert.equal(reply.result, undefined); + const failures = failureTraces(traces); + assert.equal(failures.length, 1); + assert.equal(failures[0].responseAttempted, false); + if (kind === 'throwing toJSON') assert.equal(calls, 1, 'results are encoded only once'); + }); +} + +test('unserializable error data also permits one encodable error response', async () => { + const { framework, traces, transport, request } = wireHarness(); + framework.channelRegistry = { + async handleRegister(_server: string, _params: unknown, reply: any) { + reply.respondError(-32602, 'bad input', { count: 1n }); + }, + }; + request('channels/register'); + await settle(); + assert.equal(transport.lines.length, 1); + assert.equal(JSON.parse(transport.lines[0]!).error.code, -32603); + assert.equal(failureTraces(traces)[0].responseAttempted, false); +}); + +test('a swallowed pre-write encoding failure still receives a fallback response', async () => { + const { framework, traces, transport, request } = wireHarness(); + framework.channelRegistry = { + async handleRegister(_server: string, _params: unknown, reply: any) { + try { reply.respond({ count: 1n }); } catch { /* handler swallowed the error */ } + }, + }; + request('channels/register'); + await settle(); + assert.equal(transport.lines.length, 1); + assert.equal(JSON.parse(transport.lines[0]!).error.code, -32603); + assert.equal(failureTraces(traces)[0].responseAttempted, false); +}); + +test('a real transport write failure remains single-attempt after serialization', async () => { + const { framework, traces, transport, request } = wireHarness(); + framework.handleHostCommand = async () => ({ maintained: true }); + transport.failWrite = true; + request(); + await settle(); + assert.equal(transport.lines.length, 1, 'a possibly partial write must not get a second reply'); + assert.deepEqual(JSON.parse(transport.lines[0]!).result, { maintained: true }); + assert.equal(failureTraces(traces)[0].responseAttempted, true); +}); + +test('post-response failures reach stderr when no trace subscriber is attached', async (t) => { + const { framework, connection } = harness(); + framework.traceListeners = []; + framework.channelRegistry = { + async handleRegister(_server: string, _params: unknown, reply: any) { + reply.respond({ registered: [] }); + throw new Error('headless reconciliation failed'); + }, + }; + const errors: unknown[][] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { errors.push(args); }); + const reply = responder(); + connection.emit('channels-register', {}, reply); + await settle(); + assert.equal(reply.results.length, 1); + assert.equal(reply.errors.length, 0); + assert.equal(errors.length, 1); + assert.match(String(errors[0]?.[0]), /srv.*channels-register/); + assert.match(String(errors[0]?.[1]), /headless reconciliation failed/); +}); From b31b7fc3040a914a6c42784b5046519881b079d6 Mon Sep 17 00:00:00 2001 From: Felix-299 Date: Fri, 2 Oct 2026 14:38:57 -0400 Subject: [PATCH 04/29] fix(mcpl): contain malformed inbound lines and sync dispatch faults MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Weft’s firsthand review of PR #208 found an adjacent pre-existing host-kill: JSON null and primitive values reached property access outside JSON.parse’s catch. The initialize handshake had the same null-access gap. A synchronous raw notification listener could also escape, including when buffered events were released later. Require object messages before accessing fields in both readers. Route post-parse dispatch through a containment boundary, and use the same safe event dispatch for live input and both ready flushes. Diagnostics reach stderr and registered connection error listeners without creating an unhandled error event when no listener is attached. Restrict method dispatch to own string keys rather than inherited object properties. Existing async handler containment and single-response semantics are unchanged. A separate native host process uses a real stdio child to emit malformed lines during handshake/after connect, or trigger a throwing notification listener, then proves valid notifications and a request/response still work. Direct cases cover all primitive shapes plus live/data/control-plane exception containment and framework traces. Expanded regression suite is 24 pass / 10 fail before, 34 pass / 0 fail after; build and diff checks pass. Full validation and independent review follow. Move this PR’s release note into changelog.d/208-inbound-handler-containment.fixed.md as requested by coordinating reviewer Iris in Commons #20024 and by the maintainer’s concurrent-PR convention feedback. Preserve every other changelog entry. Co-Authored-By: GPT-6 Astra --- CHANGELOG.md | 4 - .../208-inbound-handler-containment.fixed.md | 1 + src/mcpl/server-connection.ts | 134 +++++++++++------- test/fixtures/mcpl-inbound-host-probe.mjs | 48 +++++++ test/mcpl-request-handler-errors.test.ts | 50 +++++++ 5 files changed, 183 insertions(+), 54 deletions(-) create mode 100644 changelog.d/208-inbound-handler-containment.fixed.md create mode 100644 test/fixtures/mcpl-inbound-host-probe.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index fcb866ca7..6b9a30b4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,10 +12,6 @@ Releases up to and including 0.7.3 predate this file; for their contents see ## Unreleased -### Fixed - -- MCPL request and notification handlers contain synchronous throws and async rejections (#186), including channel reconciliation failures after an acknowledgement. Failures emit `mcpl:request-handler-error`; requests receive JSON-RPC `-32603` only when no response write has been attempted. Pre-write JSON encoding failures can receive an internal-error reply; possibly partial transport writes remain single-attempt. Failed error-response writes are included in the trace, failures also reach stderr for headless hosts, and notifications receive no response. - ## 0.19.0 — 2026-09-28 ### Added diff --git a/changelog.d/208-inbound-handler-containment.fixed.md b/changelog.d/208-inbound-handler-containment.fixed.md new file mode 100644 index 000000000..c37e96a8c --- /dev/null +++ b/changelog.d/208-inbound-handler-containment.fixed.md @@ -0,0 +1 @@ +- MCPL request and notification handlers contain synchronous throws and async rejections (#186), including channel reconciliation failures after an acknowledgement. Failures emit `mcpl:request-handler-error`; requests receive JSON-RPC `-32603` only when no response write has been attempted. Pre-write JSON encoding failures can receive an internal-error reply; possibly partial transport writes remain single-attempt. Failed error-response writes are included in the trace, failures also reach stderr for headless hosts, and notifications receive no response. Non-object JSON is ignored before handshake/dispatch property access; synchronous inbound dispatch failures are reported without interrupting later messages, including buffered plane flushes. diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index fb2ce88d6..2d81de9dc 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -292,7 +292,7 @@ export class McplServerConnection extends EventEmitter { // Re-enter the gate for every buffered item. A control handler (notably // tools/list_changed) may install a newer data-plane barrier while this // snapshot is being flushed; later data items must observe that pause. - this.emit(event, ...args); + this.dispatchInboundEvent(event, ...args); } } finally { this.flushDepth--; @@ -317,7 +317,7 @@ export class McplServerConnection extends EventEmitter { // the old direct flush bypassed positive-grant enforcement entirely // (PR #79 review blocker 3). Data-plane items re-buffer in order via // the same call, since dataPlaneReady is still false. - this.emit(item.event, ...item.args); + this.dispatchInboundEvent(item.event, ...item.args); } } finally { this.flushDepth--; @@ -533,6 +533,7 @@ export class McplServerConnection extends EventEmitter { } catch { return; // Ignore non-JSON lines (e.g. logback output from Java servers) } + if (msg === null || typeof msg !== 'object' || Array.isArray(msg)) return; if (msg.id !== initId) return; cleanup(); if (msg.error) { @@ -1113,61 +1114,94 @@ export class McplServerConnection extends EventEmitter { return; } - // 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; + // JSON-RPC messages must be objects. Ignore unrelated stdout/log values + // before property access, just as malformed JSON lines are ignored. + if (msg === null || typeof msg !== 'object' || Array.isArray(msg)) return; + try { + this.dispatchMessage(msg); + } catch (error) { + this.reportInboundFailure(error); } + }); + } - // It is an inbound request or notification from the server - const request = msg as JsonRpcRequest; - const eventName = McplServerConnection.METHOD_TO_EVENT[request.method]; - - // §7 is removed in 0.5.0: scope/elevate is no longer a method. Answer - // rather than hang (§6.6 — a method that will never be answered MUST - // return an error). - // featureSets/changed is REMOVED in 0.5.0 (§6.7 — it carried a - // server-authored change payload, the self-attestation defect; the - // need is met by §17's host-diffed manifest fetch). It was still - // 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)'); - } else { - console.error(`[mcpl] ${this.id}: discarded featureSets/changed — removed in 0.5.0 (§6.7/§17)`); - } - return; + /** 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; + } + + // It is an inbound request or notification from the server + const request = msg as JsonRpcRequest; + const eventName = typeof request.method === 'string' && 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 + // rather than hang (§6.6 — a method that will never be answered MUST + // return an error). + // featureSets/changed is REMOVED in 0.5.0 (§6.7 — it carried a + // server-authored change payload, the self-attestation defect; the + // need is met by §17's host-diffed manifest fetch). It was still + // 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)'); + } else { + console.error(`[mcpl] ${this.id}: discarded featureSets/changed — removed in 0.5.0 (§6.7/§17)`); } + return; + } - if (request.method === McplMethod.ScopeElevate) { - if (request.id != null) { - this.sendErrorResponse(request.id, -32601, 'scope/elevate removed in MCPL 0.5.0 (SPEC §7)'); - } - return; + if (request.method === McplMethod.ScopeElevate) { + if (request.id != null) { + this.sendErrorResponse(request.id, -32601, 'scope/elevate removed in MCPL 0.5.0 (SPEC §7)'); } + return; + } - // Positive-grant enforcement (§5.4, §14.1) happens at ADMISSION, in - // the emit() override below — the one choke-point that live routing - // and staged-buffer flushes share. See the comment there for why - // rejecting here, at line receipt, was wrong for staged connections. - - if (eventName) { - // Emit the typed event with params and (for requests) a respond callback - if (request.id != null) { - // Server expects a response — provide a respond helper - this.emit(eventName, request.params, { - id: request.id, - respond: (result: unknown) => this.sendResponse(request.id!, result), - respondError: (code: number, message: string, data?: unknown) => - this.sendErrorResponse(request.id!, code, message, data), - }); - } else { - // Notification — no response expected - this.emit(eventName, request.params); - } + // Positive-grant enforcement (§5.4, §14.1) happens at ADMISSION, in + // the emit() override below — the one choke-point that live routing + // and staged-buffer flushes share. See the comment there for why + // rejecting here, at line receipt, was wrong for staged connections. + + if (eventName) { + // Emit the typed event with params and (for requests) a respond callback + if (request.id != null) { + // Server expects a response — provide a respond helper + this.dispatchInboundEvent(eventName, request.params, { + id: request.id, + respond: (result: unknown) => this.sendResponse(request.id!, result), + respondError: (code: number, message: string, data?: unknown) => + this.sendErrorResponse(request.id!, code, message, data), + }); + } else { + // Notification — no response expected + this.dispatchInboundEvent(eventName, request.params); } - }); + } + } + + /** Live delivery and delayed plane flushes share the same containment boundary. */ + private dispatchInboundEvent(event: string, ...args: unknown[]): void { + try { + this.emit(event, ...args); + } catch (error) { + this.reportInboundFailure(error); + } + } + + private reportInboundFailure(cause: unknown): void { + const error = new Error(`MCPL server "${this.id}" inbound dispatch failed`, { cause }); + console.error(error.message, cause); + // EventEmitter's unhandled 'error' behavior would defeat containment before + // framework wiring, so stderr remains the fallback when no listener exists. + if (this.listenerCount('error') > 0) { + try { this.emit('error', error); } + catch (reportError) { console.error('MCPL error listener failed:', reportError); } + } } /** diff --git a/test/fixtures/mcpl-inbound-host-probe.mjs b/test/fixtures/mcpl-inbound-host-probe.mjs new file mode 100644 index 000000000..888e39248 --- /dev/null +++ b/test/fixtures/mcpl-inbound-host-probe.mjs @@ -0,0 +1,48 @@ +// Runs as a separate host process: an uncaught inbound exception fails the test. +import assert from 'node:assert/strict'; +import { McplServerConnection } from '../../src/mcpl/server-connection.js'; + +const mode = process.argv[2]; +const server = String.raw` +let input = ''; +const values = ['null', '42', '"hi"', 'true', '[]']; +const send = (value) => process.stdout.write(JSON.stringify(value) + '\n'); +process.stdin.on('data', (chunk) => { + input += chunk; + let end; + while ((end = input.indexOf('\n')) >= 0) { + const line = input.slice(0, end); input = input.slice(end + 1); + const request = JSON.parse(line); + if (request.method === 'initialize') { + if (process.argv[1] === 'handshake') { + for (const value of values) process.stdout.write(value + '\n'); + } + send({jsonrpc:'2.0', id:request.id, result:{capabilities:{}}}); + } else if (request.method === 'tools/list') { + if (process.argv[1] === 'connected') { + for (const value of values) process.stdout.write(value + '\n'); + } + send({jsonrpc:'2.0', method:'notifications/tools/list_changed', params:{sequence:1}}); + send({jsonrpc:'2.0', method:'notifications/tools/list_changed', params:{sequence:2}}); + send({jsonrpc:'2.0', id:request.id, result:{tools:[]}}); + } + } +}); +`; +const connection = await McplServerConnection.connect({ + id: 'probe', command: process.execPath, args: ['-e', server, mode], requestTimeoutMs: 1000, +}, { version: '0.5' }); +try { + const received = []; + connection.on('tools-list-changed', (params) => { + if (mode === 'throwing-listener' && params.sequence === 1) throw new Error('injected sync notification failure'); + received.push(params.sequence); + }); + // Deliberately no connection error subscriber: diagnostics must not throw. + connection.ready(); + assert.deepEqual(await connection.sendToolsList(), { tools: [] }); + assert.deepEqual(received, mode === 'throwing-listener' ? [2] : [1, 2]); + console.log('continued'); +} finally { + await connection.close(); +} diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index 6f9ec2fc0..d6f76a81c 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -1,6 +1,8 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; import { EventEmitter } from 'node:events'; +import { spawnSync } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; import { AgentFramework } from '../src/framework.js'; import { ChannelRegistry } from '../src/mcpl/channel-registry.js'; @@ -349,3 +351,51 @@ test('post-response failures reach stderr when no trace subscriber is attached', assert.match(String(errors[0]?.[0]), /srv.*channels-register/); assert.match(String(errors[0]?.[1]), /headless reconciliation failed/); }); + +for (const mode of ['handshake', 'connected', 'throwing-listener']) { + test('separate stdio host survives inbound ' + mode + ' faults and processes later messages', () => { + const probe = spawnSync(process.execPath, [ + '--import', 'tsx', fileURLToPath(new URL('./fixtures/mcpl-inbound-host-probe.mjs', import.meta.url)), mode, + ], { encoding: 'utf8', timeout: 5000 }); + assert.equal(probe.error, undefined); + assert.equal(probe.status, 0, probe.stderr); + assert.match(probe.stdout, /continued/); + }); +} + +for (const value of [null, 42, 'hi', true, []]) { + test('non-object inbound ' + JSON.stringify(value) + ' leaves the next notification deliverable', () => { + const { connection, transport } = wireHarness(); + let received = false; + connection.removeAllListeners('tools-list-changed'); + connection.on('tools-list-changed', () => { received = true; }); + assert.doesNotThrow(() => transport.emit('line', JSON.stringify(value))); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', method: 'notifications/tools/list_changed' })); + assert.equal(received, true); + }); +} + +for (const release of ['live', 'ready', 'readyControlPlane'] as const) { + test('synchronous notification failure is contained during ' + release + ' dispatch and traced', (t) => { + const transport = new ResponseTransport(); + const connection = new (McplServerConnection as any)('srv', null, transport) as McplServerConnection; + const { traces } = harness(connection); + const errors: unknown[][] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { errors.push(args); }); + const seen: number[] = []; + connection.removeAllListeners('tools-list-changed'); + connection.on('tools-list-changed', (params: { sequence: number }) => { + if (params.sequence === 1) throw new Error('raw notification failed'); + seen.push(params.sequence); + }); + if (release === 'live') connection.ready(); + const send = (sequence: number) => transport.emit('line', JSON.stringify({ + jsonrpc: '2.0', method: 'notifications/tools/list_changed', params: { sequence }, + })); + assert.doesNotThrow(() => { send(1); send(2); }); + if (release !== 'live') assert.doesNotThrow(() => connection[release]()); + assert.deepEqual(seen, [2]); + assert.equal(traces.filter((e) => e.type === 'mcpl:server-error').length, 1); + assert.equal(errors.length, 1); + }); +} From 1ad55766211d8563595cfae4ae29a0f323cd2d61 Mon Sep 17 00:00:00 2001 From: Ada-1017 Date: Fri, 9 Oct 2026 19:11:38 -0400 Subject: [PATCH 05/29] test(mcpl): pin the containment pieces no test reached Removal checks on the head merged with main 02adf27 (VPS, Node 24.21.0; dist/test/mcpl-request-handler-errors.test.js) left five pieces of this PR that could be removed with every test still passing. Each now has a test that fails without it: - the connected reader's non-object guard (server-connection.ts, before dispatchMessage): without it, `null`/42/"hi"/true reach `'id' in msg`, throw, and are reported as inbound dispatch failures (console.error and an 'error' emit) instead of being ignored like a non-JSON line. The dispatch catch masked this for host survival, so the old tests passed. New: four "non-object inbound X is ignored like a non-JSON line, with no failure report" tests (no console.error, no 'error' event). `[]` is left out: it doesn't throw in `in`, and the existing control already covers it. - the try/catch around dispatchMessage in live routing: listener faults inside emit are already caught by dispatchInboundEvent, so only throws from dispatchMessage's own work reach this catch, such as handleResponse's 'orphaned-response' emit. New: a throwing orphaned-response listener is contained (no throw out of the transport's 'line' event), reported as one mcpl:server-error, and the next notification still arrives. - the method-name lookup guard (typeof string + Object.hasOwn): without it an array-valued method that stringifies to a known method name (["notifications/tools/list_changed"]) is dispatched. New test asserts it isn't. The hasOwn half alone has no observable effect here: a prototype-named method dispatches to no listener either way. - the onRequest wrapping of tools-observe and model-info: no test made either handler fail. New: a throwing toolObserveFilter setter is answered with one -32603 and traced; a throwing model-info error write is traced with responseAttempted: true, no second write, nothing escapes emit. With the new tests, each of those five removals fails at least one test (M2: 4, M3: 1, M5: 1, tools-observe: 1, model-info: 1). The file is 42/42 on the merged tree. --- test/mcpl-request-handler-errors.test.ts | 59 ++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index d6f76a81c..fb57f3247 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -399,3 +399,62 @@ for (const release of ['live', 'ready', 'readyControlPlane'] as const) { assert.equal(errors.length, 1); }); } + +for (const value of [null, 42, 'hi', true]) { + test('non-object inbound ' + JSON.stringify(value) + ' is ignored like a non-JSON line, with no failure report', (t) => { + const { connection, transport } = wireHarness(); + const errors: unknown[][] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { errors.push(args); }); + const reported: unknown[] = []; + connection.on('error', (error: unknown) => { reported.push(error); }); + transport.emit('line', JSON.stringify(value)); + assert.deepEqual(errors, []); + assert.deepEqual(reported, []); + }); +} + +test('a throwing orphaned-response listener is contained, and later messages still arrive', (t) => { + const { connection, transport, traces } = wireHarness(); + t.mock.method(console, 'error', () => {}); + connection.prependListener('orphaned-response', () => { throw new Error('orphan listener failed'); }); + let received = false; + connection.removeAllListeners('tools-list-changed'); + connection.on('tools-list-changed', () => { received = true; }); + // A response to no pending request, carrying state, is surfaced as orphaned. + assert.doesNotThrow(() => transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 999, result: { state: {} } }))); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', method: 'notifications/tools/list_changed' })); + assert.equal(received, true); + 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', () => { + const { connection, transport } = wireHarness(); + let received = false; + connection.removeAllListeners('tools-list-changed'); + connection.on('tools-list-changed', () => { received = true; }); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', method: ['notifications/tools/list_changed'] })); + assert.equal(received, false); +}); + +test('tools-observe: a synchronous failure is answered with one internal error', async () => { + const { connection, traces } = harness(); + Object.defineProperty(connection, 'toolObserveFilter', { set() { throw new Error('filter store failed'); } }); + const reply = responder(); + assert.doesNotThrow(() => connection.emit('tools-observe', { rules: [] }, reply)); + await settle(); + assert.deepEqual(reply.errors, [{ code: -32603, message: 'filter store failed', data: undefined }]); + assert.equal(failureTraces(traces).length, 1); +}); + +test('model-info: a throwing error write is traced without escaping or a second write', async () => { + const { connection, traces } = harness(); + const reply = responder(); + let writes = 0; + reply.respondError = () => { writes++; throw new Error('response transport failed'); }; + assert.doesNotThrow(() => connection.emit('model-info', {}, reply)); + await settle(); + assert.equal(writes, 1); + const failures = failureTraces(traces); + assert.equal(failures.length, 1); + assert.equal(failures[0].responseAttempted, true); +}); From 03f043c7c4e24e6379e45fa0b38618cd9808ee45 Mon Sep 17 00:00:00 2001 From: Ada-1017 Date: Fri, 9 Oct 2026 19:33:01 -0400 Subject: [PATCH 06/29] test(mcpl): pin the Object.hasOwn half of the method lookup Basil-1007's review of 1ad5576 (room-203 #65148) found that the hasOwn half does have an effect on its own: while a connection's planes are paused, a method named after an Object.prototype member (constructor, __proto__, toString) resolves to a prototype value without it, and the event is buffered, taking a slot under the 1,000-event cap before being flushed to no listener. His probe: bufferedEvents.length 0 at the head, 3 with only `&& Object.hasOwn(...)` removed. New test: before ready(), send constructor, __proto__, toString and an unknown method; assert nothing was buffered (white-box read of bufferedEvents, as the file already reads allowHostCommands). Removal checks (VPS, Node 24.21.0, the file alone): with only the hasOwn clause removed, this test fails (42/43); with the whole string-key lookup removed, it and the array-method test fail (41/43). Against main's src the file is 2 pass / 41 fail of 43; the two passes are the existing controls. --- test/mcpl-request-handler-errors.test.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index fb57f3247..18b8858c6 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -458,3 +458,14 @@ test('model-info: a throwing error write is traced without escaping or a second assert.equal(failures.length, 1); assert.equal(failures[0].responseAttempted, true); }); + +test('a method named after an Object.prototype member is not buffered while the planes are paused', () => { + const transport = new ResponseTransport(); + const connection = new (McplServerConnection as any)('srv', null, transport) as McplServerConnection; + connection.establishGrant(new CapabilityGrant(new Set(ALL_CAPABILITY_PATHS), [])); + // Before ready(), inbound events wait in the buffer; an unknown method must not take a slot. + for (const method of ['constructor', '__proto__', 'toString', 'no/such-method']) { + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', method })); + } + assert.equal((connection as any).bufferedEvents.length, 0); +}); From c4f1f1a9afd04b41979930ac7df5bf6ca2453544 Mon Sep 17 00:00:00 2001 From: Ada-1017 Date: Sun, 11 Oct 2026 16:47:01 -0400 Subject: [PATCH 07/29] fix(mcpl): report a handler's own failure, never let a report end the host, and answer what nothing handles A review of this change found the containment it adds still had ways to lose a failure or end the host, and a few inbound paths outside it. The handler wrapper (now AgentFramework.containedInboundHandler, so it can be tested directly): - A failure report names the handler's own error when it threw or rejected. A reply write that failed before it is reported beside it (trace field responseFailure), instead of replacing it: a handler that swallowed an encoding failure and then failed with 'db down' used to report only the encoding failure. - A reply the handler tries after one was already attempted is refused without throwing, and reported (refusedResponse: { kind: 'result' }, or { kind: 'error', code, message } with the message bounded). So channels/register's early acknowledgement followed by a failed store append, whose catch answers again and rethrows, reports the append's error, not 'MCPL response already attempted'. A handler that answers twice and resolves is reported too. - responseAttempted in the trace is what the handler's own writes did, read when the failure is reported, not a snapshot from the first failed write. - Nothing in the report can throw: errors are described by describeCause, which handles a null-prototype reason, a throwing toPrimitive or a throwing message getter, and the report's body is guarded. On the async path it runs as the rejection handler, where an exception was an unhandled rejection, which ends the host. The trace is emitted before the stderr line: it's built from strings alone, while printing hands the raw value to util.inspect, which a value with a throwing inspect method makes throw. - The -32603 reply carries the standard message, "Internal error", not the handler's own text, which can name internal paths and errors and goes to a server that may be a third party's. Eight of the nine handlers answered nothing before this change. The detail stays in the trace and stderr. - The stderr line also prints a failed internal-error reply, so a headless host sees it. - mcpl/manifestChanged and notifications/tools/list_changed go through the same containment, so their failures emit mcpl:request-handler-error too. The Discord awareness accounting a tools-list refresh waits on stays on its own path: its failure is global, and is reported for every connection as each is recycled. The connection: - A request (it has an id) whose method isn't a string, or isn't one this host handles, is answered -32601 "Method not found", with a stderr line, as the function's own comment says a request that will never be answered must be. So is a request whose event nothing on the host listens for (checked in emit, after the capability gate, so a gated event still gets the gate's -32002 first). A notification nobody handles is ignored, as JSON-RPC asks. - A dispatch failure's error, which the framework traces as mcpl:server-error, names the event or method, the request id and the cause, bounded and with control characters replaced. - A server's live stderr lines go through the same containment as buffered ones, so a stderr listener that throws is reported, not thrown into the transport. - StdioTransport listens for 'error' on the child's stdin. A write to a child that has just exited fails there with EPIPE, and unlistened that would end the host. It's reported as the transport's error, or logged when nothing listens; the exit closes the transport. - McplResponseSerializationError's message says why encoding failed (a BigInt, a circular structure, a throwing toJSON). The changelog fragment is rewritten to match: which handlers are contained, what each trace field holds, when -32603 and -32601 are sent and with what message, and that a reply to a connection that has already closed is dropped without a trace, as before. Tests (mcpl-request-handler-errors, 43 to 57): - a swallowed encoding failure followed by the handler's own error; - an answer tried after the reply, refused, with the handler's error reported; and a handler that answers twice and resolves; - a rejection with a null-prototype reason, and one that neither String() nor util.inspect can print: each reported in the trace, answered, with no unhandled rejection; - -32601 for an unknown method, for non-string methods, and for an event nothing listens for, and nothing for an unknown notification; - a dispatch failure's trace naming the event, the request and the cause; a throwing 'error' listener caught and logged with the next message still delivered; a throwing live stderr listener contained; - a failed manifest refresh reported as a handler failure; - a failed internal-error reply on stderr; - a stdin EPIPE with and without a transport listener; - a synchronous handler that swallows an encoding failure, reported and answered once; - existing tests now assert the reply's code and message, the acknowledgement written before reconciliation's first await settles, and why encoding failed; the two negative routing tests gain positive controls in the same setup; the stdio probe waits up to 120 s for its child (request timeout 60 s) instead of 5 s and 1 s. Red checks on dist, each alone (15): reporting the write failure over the handler's error, a throwing refusal, a resolved double answer left unreported, describeCause without its fallback, no -32601, no failure context, the handler's text on the wire, an uncontained manifest listener, no reply failure on stderr, no stdin listener, sync success unchecked, an encoding error that says nothing, live stderr emitted directly, printing before the trace, and no answer for an unheard request: each fails the test aimed at it. Local: tsc clean; mcpl-request-handler-errors 57/57 on Node 24.19.0. --- .../208-inbound-handler-containment.fixed.md | 10 +- src/framework.ts | 214 ++++++++----- src/mcpl/errors.ts | 26 +- src/mcpl/server-connection.ts | 76 +++-- src/mcpl/transport.ts | 7 + src/types/trace.ts | 18 +- test/fixtures/mcpl-inbound-host-probe.mjs | 2 +- test/mcpl-request-handler-errors.test.ts | 285 +++++++++++++++++- 8 files changed, 531 insertions(+), 107 deletions(-) diff --git a/changelog.d/208-inbound-handler-containment.fixed.md b/changelog.d/208-inbound-handler-containment.fixed.md index c37e96a8c..c6ba2feb7 100644 --- a/changelog.d/208-inbound-handler-containment.fixed.md +++ b/changelog.d/208-inbound-handler-containment.fixed.md @@ -1 +1,9 @@ -- MCPL request and notification handlers contain synchronous throws and async rejections (#186), including channel reconciliation failures after an acknowledgement. Failures emit `mcpl:request-handler-error`; requests receive JSON-RPC `-32603` only when no response write has been attempted. Pre-write JSON encoding failures can receive an internal-error reply; possibly partial transport writes remain single-attempt. Failed error-response writes are included in the trace, failures also reach stderr for headless hosts, and notifications receive no response. Non-object JSON is ignored before handshake/dispatch property access; synchronous inbound dispatch failures are reported without interrupting later messages, including buffered plane flushes. +- MCPL inbound handlers, for requests and for notifications (`mcpl/manifestChanged` and `notifications/tools/list_changed` included), contain synchronous throws and async rejections (#186), including channel reconciliation failures after an acknowledgement. Nothing a server sends, and nothing a handler throws or rejects with, ends the host: a rejection reason that can't be printed is reported as one that can. + - **Each failure emits `mcpl:request-handler-error` and a stderr line.** Its `error` is the handler's own error. A reply write that failed before it is `responseFailure`, and a reply the handler tried to send after one was already attempted is `refusedResponse` (a result, or an error with its code and message): refused, never written. `responseError` is the internal-error reply's own failure, which the stderr line prints too. A failure of the Discord awareness accounting that a tools-list refresh waits on is global: it's reported as `mcpl:server-error` for every connection, as each is recycled. + - **A request gets JSON-RPC `-32603` "Internal error"** when its handler fails before any reply bytes may have gone out. That's when it attempted no reply, or its reply failed while encoding. A transport write that threw may have sent part of a reply, so it isn't followed by another. The reply carries the standard message, not the handler's own error text, which stays in the trace and stderr. A notification gets no reply. + - **A request whose method isn't a string,** or isn't one the host handles, or whose event nothing on the host listens for, is answered `-32601` "Method not found", with a stderr line. A notification nobody handles is ignored. + - **Non-object JSON** is ignored before property access, in the handshake reader and in routing, as a non-JSON line already is. + - **A synchronous failure while dispatching an inbound message**, buffered plane flushes and a server's stderr lines included, is reported without interrupting later messages. Its `mcpl:server-error` names the event or method, the request id, and the cause. + - **A failed write to a stdio server's stdin** (a child that has just exited) is reported instead of ending the host. + - **A response that can't be encoded** says why in its error (a BigInt, a circular structure, a throwing `toJSON`). + - **A reply to a connection that has already closed** is dropped without a trace, on either transport, as before. diff --git a/src/framework.ts b/src/framework.ts index 0d28e5ab7..163edac0d 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -387,7 +387,7 @@ import { EventGate, formatShadowWarning } from './gate/event-gate.js'; import { UsageTracker, type PersistedUsageState } from './usage/usage-tracker.js'; import type { SessionUsageSnapshot, UsageUpdatedEvent } from './usage/types.js'; import type { McplServerConnection } from './mcpl/server-connection.js'; -import { McplResponseSerializationError } from './mcpl/errors.js'; +import { McplResponseSerializationError, boundedForLog, describeCause } from './mcpl/errors.js'; import type { McplServerConfig, McplHostCapabilities, @@ -470,6 +470,13 @@ interface DeferredWrite { } 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; + respond: (result: unknown) => void; + respondError: (code: number, message: string, data?: unknown) => void; +}; + /** Stamp a deferred write's durable id into the metadata it is stored with * (boot recovery dedups replays by it). Idempotent for an already-stamped * message. */ @@ -13690,88 +13697,144 @@ export class AgentFramework { } /** - * Wire event listeners on an MCPL server connection. - * Push events and inference requests are deferred to Steps 6/7. + * An inbound MCPL handler, contained: EventEmitter doesn't consume the + * promise a listener returns, so this keeps each handler's throw or + * rejection at this boundary, including failures after channels/register or + * channels/changed has acknowledged the request. It's invoked at once, so + * control-plane replies keep their acknowledge-before-reconcile order. */ - private wireMcplEvents(connection: McplServerConnection): void { - type Responder = { - id: string | number; - respond: (result: unknown) => void; - respondError: (code: number, message: string, data?: unknown) => void; - }; - - // EventEmitter does not consume promises returned by listeners. Keep - // each RPC handler's rejection at this boundary, including failures after - // channels/register or channels/changed has acknowledged the request. - const onRequest = ( - event: string, - handler: (params: T, responder?: Responder) => void | Promise, - ): void => { - connection.on(event, (params: T, responder?: Responder) => { - let responseAttempted = false; - let responseFailure: { error: unknown; responseAttempted: boolean } | undefined; - const writeResponse = (write: () => void): void => { - if (responseAttempted) { - // Inner handler catches can try to answer a failed result with - // an error. Preserve the first failure and refuse the second write. - throw responseFailure ? responseFailure.error : new Error('MCPL response already attempted'); - } - // A throwing transport write may have partially sent the reply. - responseAttempted = true; - try { - write(); - } catch (error) { - // Only the encoder can establish that no write was attempted. - // Transport exceptions stay outcome-unknown and cannot be retried. - if (error instanceof McplResponseSerializationError) responseAttempted = false; - responseFailure = { error, responseAttempted }; - throw error; + private containedInboundHandler( + serverId: string, + event: string, + handler: (params: T, responder?: InboundResponder) => void | Promise, + ): (params: T, responder?: InboundResponder) => void { + return (params: T, responder?: InboundResponder) => { + // Whether a reply's bytes may have reached the transport. Only an + // encoding failure proves none did; a transport exception may have + // written part of one, so it can't be retried. + let responseAttempted = false; + // The first reply write that threw, if any, which a handler may have + // swallowed. + let writeFailure: unknown; + let writeFailed = false; + // A write the handler tried after a reply was attempted. It's refused + // without throwing, so a handler that answers again in its own catch + // and then rethrows reports its own error; what it tried to send is + // kept for the report. + let refused: { kind: 'result' } | { kind: 'error'; code: number; message: string } | undefined; + const writeResponse = ( + write: () => void, + refusedAs: { kind: 'result' } | { kind: 'error'; code: number; message: string }, + ): void => { + if (responseAttempted) { + refused ??= refusedAs; + return; + } + responseAttempted = true; + try { + write(); + } catch (error) { + if (error instanceof McplResponseSerializationError) responseAttempted = false; + if (!writeFailed) { + writeFailed = true; + writeFailure = error; } - }; - const tracked = responder && { - ...responder, - respond: (result: unknown) => writeResponse(() => responder.respond(result)), - respondError: (code: number, message: string, data?: unknown) => - writeResponse(() => responder.respondError(code, message, data)), - }; - const failed = (error: unknown): void => { - if (responseFailure) error = responseFailure.error; - const message = error instanceof Error ? error.message : String(error); - const attemptedBeforeFailure = responseFailure?.responseAttempted ?? responseAttempted; + throw error; + } + }; + const tracked = responder && { + ...responder, + respond: (result: unknown) => writeResponse(() => responder.respond(result), { kind: 'result' }), + respondError: (code: number, message: string, data?: unknown) => + writeResponse(() => responder.respondError(code, message, data), { + kind: 'error', code, message: boundedForLog(typeof message === 'string' ? message : describeCause(message), 500), + }), + }; + // Report a failure: the handler's own error when it threw or + // rejected, else the reply write it swallowed, else the reply it + // tried to send twice. Nothing in here may throw: on the async path + // it runs as a rejection handler, and an exception there would be an + // unhandled rejection, which ends the host. + const report = (handlerError: { error: unknown } | undefined): void => { + try { + const primary = handlerError + ? handlerError.error + : writeFailed ? writeFailure : new Error('the handler tried to answer a second time after its reply'); + const message = describeCause(primary); + // What the handler's own writes did, before any internal-error + // reply sent here. + const handlerResponseAttempted = responseAttempted; let responseError: string | undefined; if (tracked && !responseAttempted) { + // The server gets the standard message, not the handler's own + // text, which may name internal paths or errors; the trace and + // stderr carry the detail. try { - tracked.respondError(-32603, message); + tracked.respondError(-32603, 'Internal error'); } catch (sendError) { - responseError = sendError instanceof Error ? sendError.message : String(sendError); + responseError = describeCause(sendError); } } - // Keep headless hosts diagnosable even without a trace subscriber. - console.error(`[mcpl] ${connection.id} ${event} handler failed:`, error); + const responseFailure = writeFailed && handlerError && handlerError.error !== writeFailure + ? describeCause(writeFailure) : undefined; + // The trace first: it's built from strings alone, while printing + // hands the raw thrown value to util.inspect, which an exotic value + // (a Proxy whose traps throw) can make throw. this.emitTrace({ type: 'mcpl:request-handler-error', - serverId: connection.id, + serverId, event, requestId: responder?.id, - responseAttempted: attemptedBeforeFailure, + responseAttempted: handlerResponseAttempted, error: message, + ...(responseFailure === undefined ? {} : { responseFailure }), + ...(refused === undefined ? {} : { refusedResponse: refused }), ...(responseError === undefined ? {} : { responseError }), }); - }; - try { - // Invoke immediately: control-plane responses must retain their - // ACK-before-reconcile ordering and data-plane barrier semantics. - const succeeded = (): void => { - // A handler may swallow an encoding/transport failure without rethrowing. - if (responseFailure) failed(responseFailure.error); - }; - const result = handler(params, tracked); - if (result) void result.then(succeeded, failed); - else succeeded(); - } catch (error) { - failed(error); + // Keep headless hosts diagnosable even without a trace subscriber. + console.error( + `[mcpl] ${serverId} ${event} handler failed:`, primary, + ...(responseFailure === undefined ? [] : ['| its reply write had failed:', writeFailure]), + ...(refused === undefined ? [] : ['| it then tried to answer again:', refused]), + ...(responseError === undefined ? [] : ['| the internal-error reply failed too:', responseError]), + ); + } catch (reportError) { + try { console.error(`[mcpl] ${serverId} ${event} handler failed, and reporting it failed too:`, reportError); } + catch { /* stderr itself is gone */ } } - }); + }; + const failed = (error: unknown): void => report({ error }); + // A handler may swallow a write failure, or answer twice, and still + // resolve. + const succeeded = (): void => { + if (writeFailed || refused) report(undefined); + }; + try { + // Invoke immediately: control-plane responses must retain their + // ACK-before-reconcile ordering and data-plane barrier semantics. + const result = handler(params, tracked); + if (result) void Promise.resolve(result).then(succeeded, failed); + else succeeded(); + } catch (error) { + failed(error); + } + }; + } + + /** + * Wire event listeners on an MCPL server connection. + * Push events and inference requests are deferred to Steps 6/7. + */ + private wireMcplEvents(connection: McplServerConnection): void { + type Responder = InboundResponder; + + // Every inbound handler, request or notification, is contained + // (containedInboundHandler). + const onRequest = ( + event: string, + handler: (params: T, responder?: Responder) => void | Promise, + ): void => { + connection.on(event, this.containedInboundHandler(connection.id, event, handler)); }; // Forward subprocess stderr lines as trace events so consumers (conhost, @@ -13845,11 +13908,10 @@ export class AgentFramework { // §17.3: a manifest-change announcement. Rate-limited re-fetch → diff // → §6.7 consequences → one receipt. The announcement itself carries no // payload and no authority; everything below acts on FETCHED content. - connection.on('manifest-changed', (params: { revision?: string; domains?: string[] }) => { - void this.handleManifestChanged(connection, params).catch((err) => { - console.error(`[mcpl] ${connection.id} manifest refresh failed:`, err instanceof Error ? err.message : err); - }); - }); + // A notification, contained and reported like every inbound handler: a + // refresh that fails emits mcpl:request-handler-error. + onRequest('manifest-changed', (params: { revision?: string; domains?: string[] }) => + this.handleManifestChanged(connection, params)); onRequest('channels-list', ( _params: unknown, @@ -13915,8 +13977,12 @@ export class AgentFramework { responder.respond(result); }); - // Handle dynamic tool list changes (notifications/tools/list_changed) - connection.on('tools-list-changed', () => { + // Handle dynamic tool list changes (notifications/tools/list_changed). + // Contained like every inbound handler: a synchronous failure here emits + // mcpl:request-handler-error. A failure of the awareness accounting its + // barrier waits on is global, and failMcplDataPlaneGate reports it for + // every connection (mcpl:server-error) as it recycles them. + onRequest('tools-list-changed', () => { // Pause before starting any async refresh. This listener runs // synchronously in the transport line callback, so a following inbound // data event cannot pass before the new barrier exists. diff --git a/src/mcpl/errors.ts b/src/mcpl/errors.ts index b6b01bad8..bd6afd08a 100644 --- a/src/mcpl/errors.ts +++ b/src/mcpl/errors.ts @@ -40,10 +40,32 @@ export const CHANNEL_OPEN_FAILED = -32024; // Error Factories // ============================================================================ -/** Response encoding failed before the transport was given any bytes. */ +/** The text of anything thrown or rejected. Never throws, whatever it is: a + * null-prototype object, or a message getter or toString that throws. */ +export function describeCause(cause: unknown): string { + try { + return cause instanceof Error ? String(cause.message) : String(cause); + } catch { + try { + return Object.prototype.toString.call(cause); + } catch { + return 'an error that could not be printed'; + } + } +} + +/** Text a server or a handler chose, made fit for one log or trace line: + * control characters replaced, and cut to `max` code points. */ +export function boundedForLog(text: string, max = 200): string { + const points = [...text.replace(/[\u0000-\u001f\u007f-\u009f]/g, '?')]; + return points.length > max ? `${points.slice(0, max).join('')}…` : points.join(''); +} + +/** Response encoding failed before the transport was given any bytes. Its + * message says why (a BigInt, a circular structure, a throwing toJSON). */ export class McplResponseSerializationError extends Error { constructor(cause: unknown) { - super('MCPL response could not be serialized', { cause }); + super(`MCPL response could not be serialized: ${boundedForLog(describeCause(cause))}`, { cause }); this.name = 'McplResponseSerializationError'; } } diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index 2d81de9dc..69a0f5761 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -14,7 +14,7 @@ import { EventEmitter } from 'node:events'; import { openTransport, type McplTransport, type TransportCloseInfo } from './transport.js'; import { maskNegotiatedCapabilities } from './capability-mask.js'; import { CapabilityGrant, expandAdvertisementShorthand } from './capability-grant.js'; -import { CAPABILITY_DISABLED, McplResponseSerializationError } from './errors.js'; +import { CAPABILITY_DISABLED, McplResponseSerializationError, boundedForLog, describeCause } from './errors.js'; import type { McplServerConfig, @@ -80,6 +80,13 @@ 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 "${boundedForLog(method)}"` + : `a method of type ${method === null ? 'null' : typeof method}`; +} + /** * McplServerConnection manages a single JSON-RPC 2.0 connection to an * MCPL server over stdio. Use the static `connect()` factory to create @@ -379,6 +386,14 @@ export class McplServerConnection extends EventEmitter { } return true; } + // §6.6: a request that will never be answered MUST get an error. A + // request whose event nothing on this host listens for would never be. + const request = args[1] as { respondError?: (code: number, message: string, data?: unknown) => void } | undefined; + if (typeof request?.respondError === 'function' && this.listenerCount(event) === 0) { + request.respondError(-32601, 'Method not found'); + console.error(`[mcpl] ${this.id}: answered -32601 to ${name}, which nothing on this host handles`); + return true; + } return super.emit(event, ...args); } // Buffer cap (issue #122): a long data-plane pause (quiesce window) on a @@ -1098,7 +1113,9 @@ export class McplServerConnection extends EventEmitter { this.setupLifecycle(transport); // Surface transport diagnostics (stdio child stderr) as 'stderr' events, // preserving the `{ line }` payload existing consumers expect. - transport.on('stderr', (line: string) => this.emit('stderr', { line })); + // Through the same containment boundary as every inbound event, so a + // stderr listener that throws is reported, not thrown into the transport. + transport.on('stderr', (line: string) => this.dispatchInboundEvent('stderr', { line })); } /** @@ -1120,7 +1137,11 @@ export class McplServerConnection extends EventEmitter { try { this.dispatchMessage(msg); } catch (error) { - this.reportInboundFailure(error); + const request = msg as { method?: unknown; id?: unknown }; + this.reportInboundFailure(error, { + ...(typeof request.method === 'string' ? { event: request.method } : {}), + ...(typeof request.id === 'string' || typeof request.id === 'number' ? { requestId: request.id } : {}), + }); } }); } @@ -1167,20 +1188,30 @@ export class McplServerConnection extends EventEmitter { // and staged-buffer flushes share. See the comment there for why // rejecting here, at line receipt, was wrong for staged connections. - if (eventName) { - // Emit the typed event with params and (for requests) a respond callback + 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) { - // Server expects a response — provide a respond helper - this.dispatchInboundEvent(eventName, request.params, { - id: request.id, - respond: (result: unknown) => this.sendResponse(request.id!, result), - respondError: (code: number, message: string, data?: unknown) => - this.sendErrorResponse(request.id!, code, message, data), - }); - } else { - // Notification — no response expected - this.dispatchInboundEvent(eventName, request.params); + 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`); } + return; + } + + // Emit the typed event with params and (for requests) a respond callback + if (request.id != null) { + // Server expects a response — provide a respond helper + this.dispatchInboundEvent(eventName, request.params, { + id: request.id, + respond: (result: unknown) => this.sendResponse(request.id!, result), + respondError: (code: number, message: string, data?: unknown) => + this.sendErrorResponse(request.id!, code, message, data), + }); + } else { + // Notification — no response expected + this.dispatchInboundEvent(eventName, request.params); } } @@ -1189,12 +1220,21 @@ export class McplServerConnection extends EventEmitter { try { this.emit(event, ...args); } catch (error) { - this.reportInboundFailure(error); + const requestId = (args[1] as { id?: unknown } | undefined)?.id; + this.reportInboundFailure(error, { + event, + ...(typeof requestId === 'string' || typeof requestId === 'number' ? { requestId } : {}), + }); } } - private reportInboundFailure(cause: unknown): void { - const error = new Error(`MCPL server "${this.id}" inbound dispatch failed`, { cause }); + /** Report a failure the containment boundary caught, naming the event (or + * method) and request it was dispatching, and what failed. The message is + * what the framework's 'error' listener traces. */ + private reportInboundFailure(cause: unknown, context: { event?: string; requestId?: string | number } = {}): void { + const where = context.event === undefined ? '' : ` for ${boundedForLog(context.event)}` + + (context.requestId === undefined ? '' : ` (request ${boundedForLog(String(context.requestId))})`); + const error = new Error(`MCPL server "${this.id}" inbound dispatch failed${where}: ${boundedForLog(describeCause(cause))}`, { cause }); console.error(error.message, cause); // EventEmitter's unhandled 'error' behavior would defeat containment before // framework wiring, so stderr remains the fallback when no listener exists. diff --git a/src/mcpl/transport.ts b/src/mcpl/transport.ts index 2fc8a523e..40275d118 100644 --- a/src/mcpl/transport.ts +++ b/src/mcpl/transport.ts @@ -156,6 +156,13 @@ export class StdioTransport extends McplTransport { }); this.child.on('error', (err) => this.emit('error', err)); + // A write to a child that has just exited fails asynchronously on its + // stdin (EPIPE). Unlistened, that 'error' would end the host; the exit + // itself closes the transport, so the write's failure is only reported. + this.child.stdin?.on('error', (err) => { + if (this.listenerCount('error') > 0) this.emit('error', err); + else console.error(`[mcpl] a write to the server's stdin failed: ${err.message}`); + }); this.child.on('exit', (code, signal) => this.markClosed({ code, signal, reason: 'child process exited' }), ); diff --git a/src/types/trace.ts b/src/types/trace.ts index 47baeada2..2bef7a57b 100644 --- a/src/types/trace.ts +++ b/src/types/trace.ts @@ -438,10 +438,24 @@ export type TraceEvent = event: string; /** Absent for notifications. */ requestId?: string | number; - /** True if a response write was attempted before the handler failed. */ + /** Whether the handler's own reply may have reached the server: true + * once it attempted one, unless the attempt failed while encoding, + * before any bytes were written. When it's false and a responder was + * given, the host answered -32603 "Internal error" itself, unless + * `responseError` says that reply failed. */ responseAttempted: boolean; + /** The handler's own error when it threw or rejected; otherwise the + * reply write that failed and that it swallowed, or that it tried to + * answer a second time. */ error: string; - /** Failure while trying to send an internal-error response. */ + /** A reply write that failed, when the handler then failed with an + * error of its own (which is `error`). */ + responseFailure?: string; + /** What the handler tried to send after a reply was already attempted: + * refused, never written. A result, or an error reply with its code + * and (bounded) message. */ + refusedResponse?: { kind: 'result' } | { kind: 'error'; code: number; message: string }; + /** Failure while trying to send the internal-error response. */ responseError?: string; }) | (TraceEventBase & { diff --git a/test/fixtures/mcpl-inbound-host-probe.mjs b/test/fixtures/mcpl-inbound-host-probe.mjs index 888e39248..9906d7615 100644 --- a/test/fixtures/mcpl-inbound-host-probe.mjs +++ b/test/fixtures/mcpl-inbound-host-probe.mjs @@ -30,7 +30,7 @@ process.stdin.on('data', (chunk) => { }); `; const connection = await McplServerConnection.connect({ - id: 'probe', command: process.execPath, args: ['-e', server, mode], requestTimeoutMs: 1000, + id: 'probe', command: process.execPath, args: ['-e', server, mode], requestTimeoutMs: 60_000, }, { version: '0.5' }); try { const received = []; diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index 18b8858c6..004dd0c3d 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -3,13 +3,15 @@ import assert from 'node:assert/strict'; import { EventEmitter } from 'node:events'; import { spawnSync } from 'node:child_process'; import { fileURLToPath } from 'node:url'; +import { inspect } from 'node:util'; import { AgentFramework } from '../src/framework.js'; import { ChannelRegistry } from '../src/mcpl/channel-registry.js'; import { InferenceRouter } from '../src/mcpl/inference-router.js'; import { McplServerConnection } from '../src/mcpl/server-connection.js'; -import { McplTransport } from '../src/mcpl/transport.js'; +import { McplTransport, StdioTransport } from '../src/mcpl/transport.js'; import { CapabilityGrant, ALL_CAPABILITY_PATHS } from '../src/mcpl/capability-grant.js'; +import { McplResponseSerializationError } from '../src/mcpl/errors.js'; function harness(connection = Object.assign(new EventEmitter(), { id: 'srv' })) { const traces: any[] = []; @@ -59,9 +61,11 @@ for (const [event, field] of [['channels-register', 'channels'], ['channels-chan framework.channelRegistry = registry; const reply = responder(); connection.emit(event, { [field]: [{ id: 'room', type: 'test', direction: 'bidirectional' }] }, reply); + // Synchronously, before reconciliation's first await settles. + assert.equal(reply.results.length, 1, 'ACK precedes reconciliation'); await settle(); - assert.equal(reply.results.length, 1, 'ACK still precedes reconciliation'); + assert.equal(reply.results.length, 1, 'and nothing else is answered'); assert.equal(reply.errors.length, 0, 'an accepted registration must not get a second response'); const failures = failureTraces(traces); assert.equal(failures.length, 1); @@ -93,11 +97,13 @@ for (const [event, component, method] of [ await settle(); assert.equal(reply.results.length, 0); - assert.deepEqual(reply.errors, [{ code: -32603, message: 'handler failed before response', data: undefined }]); + // The server gets the standard message; the handler's own text stays home. + assert.deepEqual(reply.errors, [{ code: -32603, message: 'Internal error', data: undefined }]); const failures = failureTraces(traces); assert.equal(failures.length, 1); assert.equal(failures[0].event, event); assert.equal(failures[0].responseAttempted, false); + assert.match(failures[0].error, /handler failed before response/); }); } @@ -118,8 +124,9 @@ test('a synchronous handler throw is contained and answered', async () => { const reply = responder(); assert.doesNotThrow(() => connection.emit('channels-list', {}, reply)); await settle(); - assert.equal(reply.errors.length, 1); + assert.deepEqual(reply.errors, [{ code: -32603, message: 'Internal error', data: undefined }]); assert.equal(failureTraces(traces).length, 1); + assert.match(failureTraces(traces)[0].error, /list failed/); }); test('a throwing success response is not retried as an error response', async () => { @@ -285,9 +292,14 @@ for (const kind of ['bigint', 'circular', 'throwing toJSON'] as const) { assert.equal(reply.id, 17); assert.equal(reply.error.code, -32603); assert.equal(reply.result, undefined); + assert.equal(reply.error.message, 'Internal error'); const failures = failureTraces(traces); assert.equal(failures.length, 1); assert.equal(failures[0].responseAttempted, false); + // The encoding failure says why. + assert.match(failures[0].error, kind === 'bigint' ? /could not be serialized: .*BigInt/i + : kind === 'circular' ? /could not be serialized: .*circular/i + : /could not be serialized: snapshot serialization failed/); if (kind === 'throwing toJSON') assert.equal(calls, 1, 'results are encoded only once'); }); } @@ -356,7 +368,7 @@ for (const mode of ['handshake', 'connected', 'throwing-listener']) { test('separate stdio host survives inbound ' + mode + ' faults and processes later messages', () => { const probe = spawnSync(process.execPath, [ '--import', 'tsx', fileURLToPath(new URL('./fixtures/mcpl-inbound-host-probe.mjs', import.meta.url)), mode, - ], { encoding: 'utf8', timeout: 5000 }); + ], { encoding: 'utf8', timeout: 120_000 }); assert.equal(probe.error, undefined); assert.equal(probe.status, 0, probe.stderr); assert.match(probe.stdout, /continued/); @@ -434,6 +446,9 @@ test('a method that is not a string is not dispatched, even if it stringifies to connection.on('tools-list-changed', () => { received = true; }); transport.emit('line', JSON.stringify({ jsonrpc: '2.0', method: ['notifications/tools/list_changed'] })); assert.equal(received, false); + // The control: the same method as a string is dispatched in this setup. + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', method: 'notifications/tools/list_changed' })); + assert.equal(received, true); }); test('tools-observe: a synchronous failure is answered with one internal error', async () => { @@ -442,21 +457,26 @@ test('tools-observe: a synchronous failure is answered with one internal error', const reply = responder(); assert.doesNotThrow(() => connection.emit('tools-observe', { rules: [] }, reply)); await settle(); - assert.deepEqual(reply.errors, [{ code: -32603, message: 'filter store failed', data: undefined }]); + assert.deepEqual(reply.errors, [{ code: -32603, message: 'Internal error', data: undefined }]); assert.equal(failureTraces(traces).length, 1); + assert.match(failureTraces(traces)[0].error, /filter store failed/); }); test('model-info: a throwing error write is traced without escaping or a second write', async () => { const { connection, traces } = harness(); const reply = responder(); - let writes = 0; - reply.respondError = () => { writes++; throw new Error('response transport failed'); }; + const writes: Array<{ code: number; message: string }> = []; + reply.respondError = (code: number, message: string) => { writes.push({ code, message }); throw new Error('response transport failed'); }; assert.doesNotThrow(() => connection.emit('model-info', {}, reply)); await settle(); - assert.equal(writes, 1); + assert.equal(writes.length, 1); + assert.equal(writes[0]!.code, -32601); + assert.match(writes[0]!.message, /model\/info unavailable/); const failures = failureTraces(traces); assert.equal(failures.length, 1); assert.equal(failures[0].responseAttempted, true); + assert.match(failures[0].error, /response transport failed/); + assert.equal(failures[0].responseFailure, undefined, 'the write failure is the error itself, not repeated'); }); test('a method named after an Object.prototype member is not buffered while the planes are paused', () => { @@ -468,4 +488,251 @@ test('a method named after an Object.prototype member is not buffered while the transport.emit('line', JSON.stringify({ jsonrpc: '2.0', method })); } assert.equal((connection as any).bufferedEvents.length, 0); + // The control: a known notification does wait in the buffer here. + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', method: 'notifications/tools/list_changed' })); + assert.equal((connection as any).bufferedEvents.length, 1); +}); + +test('a handler that swallows an encoding failure and then fails reports its own error, and the failed write beside it', async () => { + const { framework, traces, transport, request } = wireHarness(); + framework.channelRegistry = { + async handleRegister(_server: string, _params: unknown, reply: any) { + try { reply.respond({ count: 1n }); } catch { /* swallowed */ } + throw new Error('db down'); + }, + }; + request('channels/register'); + await settle(); + assert.equal(transport.lines.length, 1, 'no bytes of the result went out, so one internal-error reply does'); + assert.deepEqual(JSON.parse(transport.lines[0]!).error, { code: -32603, message: 'Internal error' }); + const failures = failureTraces(traces); + assert.equal(failures.length, 1); + assert.match(failures[0].error, /db down/); + assert.match(failures[0].responseFailure, /could not be serialized: .*BigInt/i); + assert.equal(failures[0].responseAttempted, false); +}); + +test('an answer tried after the reply is refused without throwing, so the handler reports its own error', async () => { + const { framework, connection, traces } = harness(); + framework.channelRegistry = { + async handleRegister(_server: string, _params: unknown, reply: any) { + reply.respond({ registered: [] }); + try { + await Promise.resolve(); + throw new Error('store append failed'); + } catch (error) { + reply.respondError(-32603, 'could not record the registration'); + throw error; + } + }, + }; + const reply = responder(); + connection.emit('channels-register', {}, reply); + await settle(); + assert.equal(reply.results.length, 1); + assert.equal(reply.errors.length, 0, 'the second answer is never written'); + const failures = failureTraces(traces); + assert.equal(failures.length, 1); + assert.match(failures[0].error, /store append failed/); + assert.deepEqual(failures[0].refusedResponse, { kind: 'error', code: -32603, message: 'could not record the registration' }); + assert.equal(failures[0].responseAttempted, true); +}); + +test('a handler that answers twice and resolves is still reported', async () => { + const { framework, connection, traces } = harness(); + framework.channelRegistry = { + async handleRegister(_server: string, _params: unknown, reply: any) { + reply.respond({ registered: [] }); + reply.respondError(-32602, 'second thoughts'); + }, + }; + const reply = responder(); + connection.emit('channels-register', {}, reply); + await settle(); + assert.equal(reply.results.length, 1); + assert.equal(reply.errors.length, 0); + const failures = failureTraces(traces); + assert.equal(failures.length, 1); + assert.match(failures[0].error, /second time/); + assert.deepEqual(failures[0].refusedResponse, { kind: 'error', code: -32602, message: 'second thoughts' }); +}); + +test('a rejection whose reason cannot be printed is reported and answered, with no unhandled rejection', async () => { + const { framework, connection, traces } = harness(); + const unhandled: unknown[] = []; + const onUnhandled = (reason: unknown) => { unhandled.push(reason); }; + process.on('unhandledRejection', onUnhandled); + try { + framework.channelRegistry = { + async handleRegister() { throw Object.create(null); }, + }; + const reply = responder(); + connection.emit('channels-register', {}, reply); + await settle(); + await settle(); + assert.deepEqual(unhandled, []); + assert.deepEqual(reply.errors, [{ code: -32603, message: 'Internal error', data: undefined }]); + const failures = failureTraces(traces); + assert.equal(failures.length, 1); + assert.equal(typeof failures[0].error, 'string'); + } finally { + process.off('unhandledRejection', onUnhandled); + } +}); + +test('a request for a method nothing handles is answered -32601; such a notification is ignored', (t) => { + const { transport } = wireHarness(); + t.mock.method(console, 'error', () => {}); + 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', method: 'no/such-notification' })); + const replies = transport.lines.map((line) => JSON.parse(line)); + 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'], + ]); +}); + +test('a dispatch failure names the event, the request and what failed', (t) => { + const { connection, transport, traces } = wireHarness(); + t.mock.method(console, 'error', () => {}); + connection.prependListener('channels-list', () => { throw new Error('listener exploded'); }); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 41, method: 'channels/list' })); + const errors = traces.filter((e) => e.type === 'mcpl:server-error'); + assert.equal(errors.length, 1); + assert.match(errors[0].error, /inbound dispatch failed for channels-list \(request 41\): listener exploded/); +}); + +test('a manifest refresh that fails is reported as a handler failure', async (t) => { + const { framework, connection, traces } = harness(); + t.mock.method(console, 'error', () => {}); + framework.handleManifestChanged = async () => { throw new Error('manifest fetch failed'); }; + connection.emit('manifest-changed', { revision: 'r2' }); + await settle(); + const failures = failureTraces(traces); + assert.equal(failures.length, 1); + assert.equal(failures[0].event, 'manifest-changed'); + assert.equal(failures[0].requestId, undefined); + assert.match(failures[0].error, /manifest fetch failed/); +}); + +test('a failed internal-error reply reaches stderr too', async (t) => { + const { framework, connection } = harness(); + framework.traceListeners = []; + framework.channelRegistry = { async handleRegister() { throw new Error('register failed'); } }; + const lines: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }); + const reply = responder(); + reply.respondError = () => { throw new Error('internal-error reply could not be written'); }; + connection.emit('channels-register', {}, reply); + await settle(); + assert.equal(lines.length, 1); + assert.match(lines[0]!, /channels-register handler failed: .*register failed/); + assert.match(lines[0]!, /the internal-error reply failed too: internal-error reply could not be written/); +}); + +test("an 'error' listener that throws is caught and logged, and the next message still arrives", (t) => { + const { connection, transport } = wireHarness(); + const logged: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { logged.push(String(args[0])); }); + connection.removeAllListeners('error'); + connection.on('error', () => { throw new Error('error listener exploded'); }); + connection.prependListener('channels-list', () => { throw new Error('listener exploded'); }); + let received = false; + connection.removeAllListeners('tools-list-changed'); + connection.on('tools-list-changed', () => { received = true; }); + assert.doesNotThrow(() => transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 51, method: 'channels/list' }))); + assert.ok(logged.some((line) => /MCPL error listener failed/.test(line)), logged.join('\n')); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', method: 'notifications/tools/list_changed' })); + assert.equal(received, true); +}); + +test("a failed write to a stdio server's stdin is reported, never thrown", (t) => { + const child = Object.assign(new EventEmitter(), { stdin: new EventEmitter(), stderr: new EventEmitter(), killed: false, exitCode: null }); + const rl = new EventEmitter(); + const transport = new (StdioTransport as any)(child, rl) as StdioTransport; + const logged: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { logged.push(String(args[0])); }); + const epipe = Object.assign(new Error('write EPIPE'), { code: 'EPIPE' }); + // With nobody listening on the transport, it's logged. + assert.doesNotThrow(() => child.stdin.emit('error', epipe)); + assert.match(logged[0] ?? '', /stdin failed: write EPIPE/); + // With a listener, it's the transport's error. + const reported: unknown[] = []; + transport.on('error', (error) => reported.push(error)); + child.stdin.emit('error', epipe); + assert.deepEqual(reported, [epipe]); +}); + +test('a synchronous handler that swallows an encoding failure is reported and answered once', async () => { + const { framework, traces } = harness(); + const listener = framework.containedInboundHandler('srv', 'test-sync', (_params: unknown, reply: any) => { + try { reply.respond({ count: 1n }); } catch { /* swallowed, synchronously */ } + }); + const writes: unknown[] = []; + listener({}, { + id: 61, + // Encode as the connection does: an encoding failure is typed, and + // proves no bytes went out. + respond(result: unknown) { + try { JSON.stringify(result); } catch (cause) { throw new McplResponseSerializationError(cause); } + writes.push(result); + }, + respondError(code: number, message: string) { writes.push({ code, message }); }, + }); + await settle(); + assert.deepEqual(writes, [{ code: -32603, message: 'Internal error' }]); + const failures = failureTraces(traces); + assert.equal(failures.length, 1); + assert.equal(failures[0].event, 'test-sync'); + assert.match(failures[0].error, /BigInt/); +}); + +test("a stderr line whose listener throws is contained like any inbound event", (t) => { + const { connection, transport, traces } = wireHarness(); + t.mock.method(console, 'error', () => {}); + connection.prependListener('stderr', () => { throw new Error('stderr listener exploded'); }); + assert.doesNotThrow(() => transport.emit('stderr', 'a line from the server')); + const errors = traces.filter((e) => e.type === 'mcpl:server-error'); + assert.equal(errors.length, 1); + assert.match(errors[0].error, /inbound dispatch failed for stderr: stderr listener exploded/); +}); + +test('a rejection no printer can inspect still reaches the trace first', async (t) => { + const { framework, connection, traces } = harness(); + // Print as console.error does: every argument goes through util.inspect. + t.mock.method(console, 'error', (...args: unknown[]) => { for (const arg of args) inspect(arg); }); + const unhandled: unknown[] = []; + const onUnhandled = (reason: unknown) => { unhandled.push(reason); }; + process.on('unhandledRejection', onUnhandled); + try { + // String() throws on it, and so does printing it. + const hostile = { + [inspect.custom]() { throw new Error('inspect refused'); }, + [Symbol.toPrimitive]() { throw new Error('toPrimitive refused'); }, + }; + framework.channelRegistry = { async handleRegister() { throw hostile; } }; + const reply = responder(); + connection.emit('channels-register', {}, reply); + await settle(); + await settle(); + assert.deepEqual(unhandled, []); + assert.deepEqual(reply.errors, [{ code: -32603, message: 'Internal error', data: undefined }]); + const failures = failureTraces(traces); + assert.equal(failures.length, 1); + assert.equal(typeof failures[0].error, 'string'); + } finally { + process.off('unhandledRejection', onUnhandled); + } +}); + +test('a request for an event nothing on the host listens for is answered -32601', (t) => { + const { connection, transport } = wireHarness(); + t.mock.method(console, 'error', () => {}); + connection.removeAllListeners('channels-list'); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 71, method: 'channels/list' })); + const replies = transport.lines.map((line) => JSON.parse(line)); + assert.deepEqual(replies.map((r) => [r.id, r.error?.code, r.error?.message]), [[71, -32601, 'Method not found']]); }); From be01bd4b28674eaefb2e783bc654c5c20b5e6089 Mon Sep 17 00:00:00 2001 From: Ada-1017 Date: Sun, 11 Oct 2026 17:03:16 -0400 Subject: [PATCH 08/29] fix(mcpl): a failed report still names the failure; log text replaces format and separator characters too Two follow-ups from a review of the previous commit: - When printing a failure report fails (the raw thrown value can't be inspected), the fallback stderr line now names the failure and why reporting it failed, as strings from describeCause, instead of printing the raw value again, so a headless host's stderr still says which failure it was. The trace has already been emitted by then. - boundedForLog replaces every \p{Cc} and \p{Cf} character and U+2028 and U+2029, not only C0 and C1 controls: a server-chosen method, request id or cause can carry bidi and other format characters, or a line or paragraph separator, which some log readers reorder or break lines on. Tests (mcpl-request-handler-errors, 57 to 59): a value printing can't handle still leaves a stderr line naming the failure; a dispatch failure's text has control, bidi and separator characters replaced. Red checks on dist, each alone: printing the raw value in the fallback, and replacing controls only, each fail the test aimed at it. Local: tsc clean; mcpl-request-handler-errors 59/59 on Node 24.19.0. --- src/framework.ts | 15 ++++++++----- src/mcpl/errors.ts | 6 ++++-- test/mcpl-request-handler-errors.test.ts | 27 ++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 7 deletions(-) diff --git a/src/framework.ts b/src/framework.ts index 163edac0d..5611571c4 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -13756,10 +13756,10 @@ export class AgentFramework { // it runs as a rejection handler, and an exception there would be an // unhandled rejection, which ends the host. const report = (handlerError: { error: unknown } | undefined): void => { + const primary = handlerError + ? handlerError.error + : writeFailed ? writeFailure : new Error('the handler tried to answer a second time after its reply'); try { - const primary = handlerError - ? handlerError.error - : writeFailed ? writeFailure : new Error('the handler tried to answer a second time after its reply'); const message = describeCause(primary); // What the handler's own writes did, before any internal-error // reply sent here. @@ -13799,8 +13799,13 @@ export class AgentFramework { ...(responseError === undefined ? [] : ['| the internal-error reply failed too:', responseError]), ); } catch (reportError) { - try { console.error(`[mcpl] ${serverId} ${event} handler failed, and reporting it failed too:`, reportError); } - catch { /* stderr itself is gone */ } + // Strings only: the raw values may be what failed to print. + try { + console.error( + `[mcpl] ${serverId} ${event} handler failed: ${describeCause(primary)}` + + ` (and reporting it failed too: ${describeCause(reportError)})`, + ); + } catch { /* stderr itself is gone */ } } }; const failed = (error: unknown): void => report({ error }); diff --git a/src/mcpl/errors.ts b/src/mcpl/errors.ts index bd6afd08a..9fcab5c99 100644 --- a/src/mcpl/errors.ts +++ b/src/mcpl/errors.ts @@ -55,9 +55,11 @@ export function describeCause(cause: unknown): string { } /** Text a server or a handler chose, made fit for one log or trace line: - * control characters replaced, and cut to `max` code points. */ + * control and format characters replaced (C0 and C1 controls, bidi and other + * \p{Cf} characters, and the line and paragraph separators some log readers + * break lines on), and cut to `max` code points. */ export function boundedForLog(text: string, max = 200): string { - const points = [...text.replace(/[\u0000-\u001f\u007f-\u009f]/g, '?')]; + const points = [...text.replace(/[\p{Cc}\p{Cf}\u2028\u2029]/gu, '?')]; return points.length > max ? `${points.slice(0, max).join('')}…` : points.join(''); } diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index 004dd0c3d..d21a4fdbc 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -736,3 +736,30 @@ test('a request for an event nothing on the host listens for is answered -32601' const replies = transport.lines.map((line) => JSON.parse(line)); assert.deepEqual(replies.map((r) => [r.id, r.error?.code, r.error?.message]), [[71, -32601, 'Method not found']]); }); + +test('a value printing cannot handle still leaves a stderr line naming the failure', async (t) => { + const { framework, connection } = harness(); + framework.traceListeners = []; + const lines: string[] = []; + // Print as console.error does, and keep what it printed. + t.mock.method(console, 'error', (...args: unknown[]) => { lines.push(args.map((a) => typeof a === 'string' ? a : inspect(a)).join(' ')); }); + const hostile = { + [inspect.custom]() { throw new Error('inspect refused'); }, + toString() { return 'the hostile failure'; }, + }; + framework.channelRegistry = { async handleRegister() { throw hostile; } }; + connection.emit('channels-register', {}, responder()); + await settle(); + assert.equal(lines.length, 1); + assert.match(lines[0]!, /channels-register handler failed: the hostile failure \(and reporting it failed too: inspect refused\)/); +}); + +test('log text from a server has control, format and separator characters replaced', (t) => { + const { connection, transport, traces } = wireHarness(); + t.mock.method(console, 'error', () => {}); + connection.prependListener('channels-list', () => { throw new Error('a\u2028b\u202ec\u0007d'); }); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 81, method: 'channels/list' })); + const errors = traces.filter((e) => e.type === 'mcpl:server-error'); + assert.equal(errors.length, 1); + assert.match(errors[0].error, /: a\?b\?c\?d$/); +}); From f7652ed333450ecc6d617ed0b6624627d66d9f5f Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Thu, 15 Oct 2026 09:18:43 -0400 Subject: [PATCH 09/29] fix(mcpl): #208's log lines print a server's and a handler's text through log-value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Main's log-value.ts is the house's rule for text a server or a handler chose in a log line: logValue for a field, logDetail for the free text that ends the line. It escapes where #208's boundedForLog replaced characters with '?', so two different texts can't print alike, and it cuts a long value with its full length. - containedInboundHandler's stderr line is one string: the handler's error, a reply write it had swallowed, what it tried to answer again with and a failed internal-error reply, each through logDetail. It printed the raw thrown values, which util.inspect expands without bound, stack included, and which an exotic value could make throw; the fallback line kept for a report that fails anyway uses logDetail too. - The connection's lines: an unknown method (describeMethod) and a request nothing on the host handles name the method through logValue; an inbound dispatch failure logs its event and request id through logValue and its cause through logDetail, instead of printing the raw cause; an 'error' listener that throws is logged through logDetail. - The stdio transport's line for a failed stdin write, when nothing listens, prints the error through logDetail. boundedForLog stays where log-value doesn't serve: text that goes into an error's message or a trace field, read as data by a trace's or an error listener's reader, so it is bounded rather than escaped. Those are the inbound dispatch failure's error, McplResponseSerializationError's message and the trace's refusedResponse. Its doc now says so. Tests: #208's and #263's assertions of these lines take the new form, and the long-error case in stop-with-work-in-flight again checks that its log line is bounded (…(5000)). Co-Authored-By: Claude Opus 5.5 --- src/framework.ts | 22 +++++++++++----------- src/mcpl/errors.ts | 12 ++++++++---- src/mcpl/server-connection.ts | 19 +++++++++++-------- src/mcpl/transport.ts | 3 ++- test/mcpl-request-handler-errors.test.ts | 16 +++++++++------- test/stop-with-work-in-flight.test.ts | 15 +++++++++------ 6 files changed, 50 insertions(+), 37 deletions(-) diff --git a/src/framework.ts b/src/framework.ts index 18fdd99bc..34d4ca9b2 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -74,7 +74,7 @@ import { REFUSAL_REACTIONS, REFUSAL_REACTION_FALLBACK, REFUSAL_REACTION_BASELINE import { Agent } from './agent.js'; import { ModuleRegistry, checkedToolResult, isStateExistsError, storableToolResult, thrownResult } from './module-registry.js'; import { McplServerRegistry } from './mcpl/server-registry.js'; -import { logValue } from './mcpl/log-value.js'; +import { logDetail, logValue } from './mcpl/log-value.js'; import { FeatureSetManager } from './mcpl/feature-set-manager.js'; import { computeGrant, CapabilityGrant, expandAdvertisementShorthand } from './mcpl/capability-grant.js'; import { maskNegotiatedCapabilities } from './mcpl/capability-mask.js'; @@ -17539,9 +17539,6 @@ export class AgentFramework { } const responseFailure = writeFailed && handlerError && handlerError.error !== writeFailure ? describeCause(writeFailure) : undefined; - // The trace first: it's built from strings alone, while printing - // hands the raw thrown value to util.inspect, which an exotic value - // (a Proxy whose traps throw) can make throw. this.emitTrace({ type: 'mcpl:request-handler-error', serverId, @@ -17554,18 +17551,21 @@ export class AgentFramework { ...(responseError === undefined ? {} : { responseError }), }); // Keep headless hosts diagnosable even without a trace subscriber. + // The handler's text and the server's go through log-value, so a + // line stays one line of bounded, escaped text. + const tried = refused === undefined ? undefined + : refused.kind === 'result' ? 'a result' : `error ${refused.code}: ${refused.message}`; console.error( - `[mcpl] ${serverId} ${event} handler failed:`, primary, - ...(responseFailure === undefined ? [] : ['| its reply write had failed:', writeFailure]), - ...(refused === undefined ? [] : ['| it then tried to answer again:', refused]), - ...(responseError === undefined ? [] : ['| the internal-error reply failed too:', responseError]), + `[mcpl] ${serverId} ${event} handler failed: ${logDetail(message)}` + + (responseFailure === undefined ? '' : ` | its reply write had failed: ${logDetail(responseFailure)}`) + + (tried === undefined ? '' : ` | it then tried to answer again with ${logDetail(tried)}`) + + (responseError === undefined ? '' : ` | the internal-error reply failed too: ${logDetail(responseError)}`), ); } catch (reportError) { - // Strings only: the raw values may be what failed to print. try { console.error( - `[mcpl] ${serverId} ${event} handler failed: ${describeCause(primary)}` + - ` (and reporting it failed too: ${describeCause(reportError)})`, + `[mcpl] ${serverId} ${event} handler failed: ${logDetail(describeCause(primary))}` + + ` (and reporting it failed too: ${logDetail(describeCause(reportError))})`, ); } catch { /* stderr itself is gone */ } } diff --git a/src/mcpl/errors.ts b/src/mcpl/errors.ts index 9fcab5c99..941fa1b97 100644 --- a/src/mcpl/errors.ts +++ b/src/mcpl/errors.ts @@ -54,10 +54,14 @@ export function describeCause(cause: unknown): string { } } -/** Text a server or a handler chose, made fit for one log or trace line: - * control and format characters replaced (C0 and C1 controls, bidi and other - * \p{Cf} characters, and the line and paragraph separators some log readers - * break lines on), and cut to `max` code points. */ +/** Text a server or a handler chose, made fit for an error's message or a + * trace field: control and format characters replaced (C0 and C1 controls, + * bidi and other \p{Cf} characters, and the line and paragraph separators + * some log readers break lines on), and cut to `max` code points. Those are + * read as data, by a trace's or an error listener's reader, so the text is + * bounded rather than escaped. A log line prints text a server or a handler + * chose through log-value instead (logValue for a field, logDetail for the + * free text that ends the line). */ export function boundedForLog(text: string, max = 200): string { const points = [...text.replace(/[\p{Cc}\p{Cf}\u2028\u2029]/gu, '?')]; return points.length > max ? `${points.slice(0, max).join('')}…` : points.join(''); diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index 1997fc072..57a0c4213 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -15,7 +15,7 @@ import { openTransport, type McplTransport, type TransportCloseInfo } from './tr import { maskNegotiatedCapabilities } from './capability-mask.js'; import { CapabilityGrant, expandAdvertisementShorthand } from './capability-grant.js'; import { CAPABILITY_DISABLED, McplResponseSerializationError, boundedForLog, describeCause } from './errors.js'; -import { logValue } from './log-value.js'; +import { logDetail, logValue } from './log-value.js'; import type { McplServerConfig, @@ -106,7 +106,7 @@ interface RequestOptions { /** A request's method for a log line, whatever the server sent there. */ function describeMethod(method: unknown): string { return typeof method === 'string' - ? `method "${boundedForLog(method)}"` + ? `method ${logValue(method)}` : `a method of type ${method === null ? 'null' : typeof method}`; } @@ -420,7 +420,7 @@ export class McplServerConnection extends EventEmitter { const request = args[1] as { respondError?: (code: number, message: string, data?: unknown) => void } | undefined; if (typeof request?.respondError === 'function' && this.listenerCount(event) === 0) { request.respondError(-32601, 'Method not found'); - console.error(`[mcpl] ${this.id}: answered -32601 to ${name}, which nothing on this host handles`); + console.error(`[mcpl] ${this.id}: answered -32601 to ${logValue(name)}, which nothing on this host handles`); return true; } return super.emit(event, ...args); @@ -1322,15 +1322,18 @@ export class McplServerConnection extends EventEmitter { * method) and request it was dispatching, and what failed. The message is * what the framework's 'error' listener traces. */ private reportInboundFailure(cause: unknown, context: { event?: string; requestId?: string | number } = {}): void { - const where = context.event === undefined ? '' : ` for ${boundedForLog(context.event)}` + - (context.requestId === undefined ? '' : ` (request ${boundedForLog(String(context.requestId))})`); - const error = new Error(`MCPL server "${this.id}" inbound dispatch failed${where}: ${boundedForLog(describeCause(cause))}`, { cause }); - console.error(error.message, cause); + const where = (show: (text: string) => string): string => (context.event === undefined ? '' : ` for ${show(context.event)}`) + + (context.requestId === undefined ? '' : ` (request ${show(String(context.requestId))})`); + // The error's message is what the 'error' listener traces, as text a + // reader parses, so it is bounded rather than escaped (boundedForLog); + // the log line prints the same through log-value. + const error = new Error(`MCPL server "${this.id}" inbound dispatch failed${where(boundedForLog)}: ${boundedForLog(describeCause(cause))}`, { cause }); + console.error(`[mcpl] ${this.id}: inbound dispatch failed${where(logValue)}: ${logDetail(describeCause(cause))}`); // EventEmitter's unhandled 'error' behavior would defeat containment before // framework wiring, so stderr remains the fallback when no listener exists. if (this.listenerCount('error') > 0) { try { this.emit('error', error); } - catch (reportError) { console.error('MCPL error listener failed:', reportError); } + catch (reportError) { console.error(`[mcpl] ${this.id}: an 'error' listener failed: ${logDetail(describeCause(reportError))}`); } } } diff --git a/src/mcpl/transport.ts b/src/mcpl/transport.ts index b8dea24dd..db10ef183 100644 --- a/src/mcpl/transport.ts +++ b/src/mcpl/transport.ts @@ -22,6 +22,7 @@ import { EventEmitter } from 'node:events'; import WebSocket from 'ws'; import type { McplServerConfig } from './types.js'; +import { logDetail } from './log-value.js'; /** Why/how a transport closed. `code`/`signal` are populated for stdio child * exits; `reason` carries a WebSocket close reason or an error message. */ @@ -193,7 +194,7 @@ export class StdioTransport extends McplTransport { // itself closes the transport, so the write's failure is only reported. this.child.stdin?.on('error', (err) => { if (this.listenerCount('error') > 0) this.emit('error', err); - else console.error(`[mcpl] a write to the server's stdin failed: ${err.message}`); + else console.error(`[mcpl] a write to the server's stdin failed: ${logDetail(err.message)}`); }); // A read error on the child's stdout or stderr reaches the transport's // 'error' listener if it has one, and is never thrown. The stdout diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index 4873eb80f..f38111ca7 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -362,8 +362,9 @@ test('post-response failures reach stderr when no trace subscriber is attached', assert.equal(reply.results.length, 1); assert.equal(reply.errors.length, 0); assert.equal(errors.length, 1); - assert.match(String(errors[0]?.[0]), /srv.*channels-register/); - assert.match(String(errors[0]?.[1]), /headless reconciliation failed/); + // One line, its text through log-value. + assert.equal(errors[0]?.length, 1); + assert.match(String(errors[0]?.[0]), /^\[mcpl\] srv channels-register handler failed: "headless reconciliation failed"$/); }); for (const mode of ['handshake', 'connected', 'throwing-listener']) { @@ -631,8 +632,8 @@ test('a failed internal-error reply reaches stderr too', async (t) => { connection.emit('channels-register', {}, reply); await settle(); assert.equal(lines.length, 1); - assert.match(lines[0]!, /channels-register handler failed: .*register failed/); - assert.match(lines[0]!, /the internal-error reply failed too: internal-error reply could not be written/); + assert.match(lines[0]!, /channels-register handler failed: "register failed"/); + assert.match(lines[0]!, /the internal-error reply failed too: "internal-error reply could not be written"/); }); test("an 'error' listener that throws is caught and logged, and the next message still arrives", (t) => { @@ -646,7 +647,7 @@ test("an 'error' listener that throws is caught and logged, and the next message connection.removeAllListeners('tools-list-changed'); connection.on('tools-list-changed', () => { received = true; }); assert.doesNotThrow(() => transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 51, method: 'channels/list' }))); - assert.ok(logged.some((line) => /MCPL error listener failed/.test(line)), logged.join('\n')); + assert.ok(logged.some((line) => line.includes(`an 'error' listener failed: "error listener exploded"`)), logged.join('\n')); transport.emit('line', JSON.stringify({ jsonrpc: '2.0', method: 'notifications/tools/list_changed' })); assert.equal(received, true); }); @@ -660,7 +661,7 @@ test("a failed write to a stdio server's stdin is reported, never thrown", (t) = const epipe = Object.assign(new Error('write EPIPE'), { code: 'EPIPE' }); // With nobody listening on the transport, it's logged. assert.doesNotThrow(() => child.stdin.emit('error', epipe)); - assert.match(logged[0] ?? '', /stdin failed: write EPIPE/); + assert.match(logged[0] ?? '', /stdin failed: "write EPIPE"/); // With a listener, it's the transport's error. const reported: unknown[] = []; transport.on('error', (error) => reported.push(error)); @@ -753,7 +754,8 @@ test('a value printing cannot handle still leaves a stderr line naming the failu connection.emit('channels-register', {}, responder()); await settle(); assert.equal(lines.length, 1); - assert.match(lines[0]!, /channels-register handler failed: the hostile failure \(and reporting it failed too: inspect refused\)/); + // The line is built from strings through log-value, so nothing inspects the value. + assert.match(lines[0]!, /channels-register handler failed: "the hostile failure"$/); }); test('log text from a server has control, format and separator characters replaced', (t) => { diff --git a/test/stop-with-work-in-flight.test.ts b/test/stop-with-work-in-flight.test.ts index 7da6a5f37..454946809 100644 --- a/test/stop-with-work-in-flight.test.ts +++ b/test/stop-with-work-in-flight.test.ts @@ -624,7 +624,7 @@ describe('stop() with work still in flight', () => { await answered(push); assert.deepEqual(push.answers, [{ error: { code: -32000, message: 'the host stopped while handling this request' } }]); - assert.ok(logged.some((line) => line.includes('[mcpl] srv push-event handler failed: Error: Queue is closed')), logged.join('\n')); + assert.ok(logged.some((line) => line.includes('[mcpl] srv push-event handler failed: "Queue is closed"')), logged.join('\n')); assert.deepEqual(rejections, []); }); @@ -642,8 +642,8 @@ describe('stop() with work still in flight', () => { await until(() => logged.some((line) => line.includes('the handler broke on e4')), "the notification's log line"); assert.deepEqual(push.answers, [{ error: { code: -32603, message: 'Internal error' } }]); - assert.ok(logged.some((line) => line.includes('[mcpl] srv push-event handler failed: Error: the handler broke on e3')), logged.join('\n')); - assert.ok(logged.some((line) => line.includes('[mcpl] srv push-event handler failed: Error: the handler broke on e4')), logged.join('\n')); + assert.ok(logged.some((line) => line.includes('[mcpl] srv push-event handler failed: "the handler broke on e3"')), logged.join('\n')); + assert.ok(logged.some((line) => line.includes('[mcpl] srv push-event handler failed: "the handler broke on e4"')), logged.join('\n')); assert.deepEqual(rejections, []); } finally { await framework.stop(); @@ -696,7 +696,7 @@ describe('stop() with work still in flight', () => { } }); - it("a long error is answered with the standard message, not the handler's text", async () => { + it("a long error is answered with the standard message, and logged bounded", async () => { const { framework, connection } = await wiredFramework(); try { (framework as unknown as { pushHandler: unknown }).pushHandler = { @@ -706,6 +706,9 @@ describe('stop() with work still in flight', () => { connection.emit('push-event', { featureSet: 'chat', eventId: 'e9', payload: { content: [{ type: 'text', text: 'x' }] } }, push); await answered(push); assert.deepEqual(push.answers, [{ error: { code: -32603, message: 'Internal error' } }]); + const line = logged.find((l) => l.includes('push-event handler failed')) ?? ''; + assert.ok(line.length < 600, `logged ${line.length} characters`); + assert.ok(line.endsWith('…(5000)'), line.slice(-40)); } finally { await framework.stop(); } @@ -726,7 +729,7 @@ describe('stop() with work still in flight', () => { await until(() => logged.some((line) => line.includes('the trace emit broke')), 'the log line'); assert.deepEqual(push.answers, [{ result: { accepted: true } }]); - assert.ok(logged.some((line) => line.includes('[mcpl] srv push-event handler failed: Error: the trace emit broke')), logged.join('\n')); + assert.ok(logged.some((line) => line.includes('[mcpl] srv push-event handler failed: "the trace emit broke"')), logged.join('\n')); assert.deepEqual(rejections, []); } finally { await framework.stop(); @@ -808,7 +811,7 @@ describe('stop() with work still in flight', () => { for (const [i, [event, method]] of requests.entries()) { assert.deepEqual(answers[i]!.answers, [{ error: { code: -32603, message: 'Internal error' } }], method); - assert.ok(logged.some((line) => line.includes(`[mcpl] srv ${event} handler failed: Error: the handler broke`)), `${method}:\n${logged.join('\n')}`); + assert.ok(logged.some((line) => line.includes(`[mcpl] srv ${event} handler failed: "the handler broke"`)), `${method}:\n${logged.join('\n')}`); } assert.deepEqual(rejections, []); } finally { From d45f230c3b844de3d1d5f8f1b5ae565962e10c66 Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Thu, 15 Oct 2026 09:19:52 -0400 Subject: [PATCH 10/29] fix(mcpl): a failed internal-error reply is reported once, not also as the handler's reply write containedInboundHandler's report read the reply write that failed, for responseFailure, after it had sent the internal-error reply. That reply goes through the same tracked write, so when it failed, its failure was recorded as the handler's own reply write too: the trace carried it as both responseFailure and responseError, and the stderr line said "its reply write had failed" beside "the internal-error reply failed too". responseFailure is now read before the internal-error reply is sent. Test: the failed internal-error reply's line no longer says "its reply write had failed". It fails without the fix. Co-Authored-By: Claude Opus 5.5 --- src/framework.ts | 6 ++++-- test/mcpl-request-handler-errors.test.ts | 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/framework.ts b/src/framework.ts index 34d4ca9b2..f32843f8d 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -17518,6 +17518,10 @@ export class AgentFramework { // What the handler's own writes did, before any internal-error // reply sent here. const handlerResponseAttempted = responseAttempted; + // A reply write the handler's own attempt failed, read before the + // internal-error reply below, whose own failure is responseError. + const responseFailure = writeFailed && handlerError && handlerError.error !== writeFailure + ? describeCause(writeFailure) : undefined; let responseError: string | undefined; if (tracked && !responseAttempted) { // The server gets the standard message, not the handler's own @@ -17537,8 +17541,6 @@ export class AgentFramework { responseError = describeCause(sendError); } } - const responseFailure = writeFailed && handlerError && handlerError.error !== writeFailure - ? describeCause(writeFailure) : undefined; this.emitTrace({ type: 'mcpl:request-handler-error', serverId, diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index f38111ca7..ad57b613e 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -634,6 +634,8 @@ test('a failed internal-error reply reaches stderr too', async (t) => { assert.equal(lines.length, 1); assert.match(lines[0]!, /channels-register handler failed: "register failed"/); assert.match(lines[0]!, /the internal-error reply failed too: "internal-error reply could not be written"/); + // Its failure is the internal-error reply's, not a reply write of the handler's own. + assert.doesNotMatch(lines[0]!, /its reply write had failed/); }); test("an 'error' listener that throws is caught and logged, and the next message still arrives", (t) => { From b41fbf4c2594ebd7014c24731d07802ddb717487 Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Thu, 15 Oct 2026 09:22:50 -0400 Subject: [PATCH 11/29] fix(mcpl): a handler failure's trace fields are bounded, and its log line prints each text raw MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mcpl:request-handler-error trace carried the handler's error, a failed reply write and a failed internal-error reply as describeCause gave them, whole. main's #263 had bounded a handler's text wherever it went, and the merge's wrapper is now where that text goes. The trace's copies of those three, and of a refused answer's message, are bounded with boundedForLog at 500 code points, since a trace field is read as data. The stderr line prints each of the four texts raw through log-value, which escapes and cuts it there. A refused answer's message, a failed reply write and a failed internal-error reply had reached the line as bounded copies, so logDetail escaped text whose controls were already replaced with '?', and two different texts could print alike. A refused answer now keeps its raw message until the report, and only the trace's copy is bounded. Tests: a handler that throws a 5,000-character error gets a trace error of 500 characters and '…', and one log line under 600 characters ending in '…(5000)'. A handler whose reply write fails with a control character in its text, and which then answers again and fails, gets a log line with each of the three texts escaped (\u0007) and trace copies with '?'; a failed internal-error reply likewise. Co-Authored-By: Claude Opus 5.5 --- src/framework.ts | 14 ++++--- test/mcpl-request-handler-errors.test.ts | 53 ++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 5 deletions(-) diff --git a/src/framework.ts b/src/framework.ts index f32843f8d..9d1783fc8 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -17499,7 +17499,7 @@ export class AgentFramework { ...(respondError ? { respondError: (code: number, message: string, data?: unknown) => writeResponse(() => respondError(code, message, data), { - kind: 'error', code, message: boundedForLog(typeof message === 'string' ? message : describeCause(message), 500), + kind: 'error', code, message: typeof message === 'string' ? message : describeCause(message), }), } : {}), }) as InboundResponder | undefined; @@ -17547,10 +17547,14 @@ export class AgentFramework { event, requestId: responder?.id, responseAttempted: handlerResponseAttempted, - error: message, - ...(responseFailure === undefined ? {} : { responseFailure }), - ...(refused === undefined ? {} : { refusedResponse: refused }), - ...(responseError === undefined ? {} : { responseError }), + // The trace's copies are bounded, as data; the log line below + // prints each text raw through log-value, which escapes it. + error: boundedForLog(message, 500), + ...(responseFailure === undefined ? {} : { responseFailure: boundedForLog(responseFailure, 500) }), + ...(refused === undefined ? {} : { + refusedResponse: refused.kind === 'error' ? { ...refused, message: boundedForLog(refused.message, 500) } : refused, + }), + ...(responseError === undefined ? {} : { responseError: boundedForLog(responseError, 500) }), }); // Keep headless hosts diagnosable even without a trace subscriber. // The handler's text and the server's go through log-value, so a diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index ad57b613e..0c51497c7 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -742,6 +742,59 @@ test('a request for an event nothing on the host listens for is answered -32601' assert.deepEqual(replies.map((r) => [r.id, r.error?.code, r.error?.message]), [[71, -32601, 'Method not found']]); }); +test("a long handler error is bounded in its trace, and its log line is cut with its length", async (t) => { + const { framework, connection, traces } = harness(); + const lines: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }); + framework.channelRegistry = { async handleRegister() { throw new Error('x'.repeat(5000)); } }; + connection.emit('channels-register', {}, responder()); + await settle(); + const failure = traces.find((e) => e.type === 'mcpl:request-handler-error'); + assert.equal(failure?.error, `${'x'.repeat(500)}…`); + assert.equal(lines.length, 1); + assert.ok(lines[0]!.length < 600, `logged ${lines[0]!.length} characters`); + assert.ok(lines[0]!.endsWith('…(5000)'), lines[0]!.slice(-40)); +}); + +test("the log line prints a failed reply write and a refused answer raw, and only the trace's copies are bounded", async (t) => { + const { framework, connection, traces } = harness(); + const lines: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }); + framework.channelRegistry = { + async handleRegister(_server: string, _params: unknown, reply: any) { + try { reply.respond({ registered: [] }); } catch { /* swallowed */ } + reply.respondError(-32602, 'second\u0007answer'); + throw new Error('handler\u0007failed'); + }, + }; + const reply = responder(); + reply.respond = () => { throw new Error('write\u0007broke'); }; + connection.emit('channels-register', {}, reply); + await settle(); + const failure = failureTraces(traces)[0]; + assert.equal(failure.error, 'handler?failed'); + assert.equal(failure.responseFailure, 'write?broke'); + assert.deepEqual(failure.refusedResponse, { kind: 'error', code: -32602, message: 'second?answer' }); + assert.equal(lines.length, 1); + assert.ok(lines[0]!.includes('handler failed: "handler\\u0007failed"'), lines[0]); + assert.ok(lines[0]!.includes('its reply write had failed: "write\\u0007broke"'), lines[0]); + assert.ok(lines[0]!.includes('it then tried to answer again with "error -32602: second\\u0007answer"'), lines[0]); +}); + +test("the log line prints a failed internal-error reply raw, and only the trace's copy is bounded", async (t) => { + const { framework, connection, traces } = harness(); + const lines: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }); + framework.channelRegistry = { async handleRegister() { throw new Error('original failure'); } }; + const reply = responder(); + reply.respondError = () => { throw new Error('send\u0007failed'); }; + connection.emit('channels-register', {}, reply); + await settle(); + assert.equal(failureTraces(traces)[0].responseError, 'send?failed'); + assert.equal(lines.length, 1); + assert.ok(lines[0]!.includes('the internal-error reply failed too: "send\\u0007failed"'), lines[0]); +}); + test('a value printing cannot handle still leaves a stderr line naming the failure', async (t) => { const { framework, connection } = harness(); framework.traceListeners = []; From cc010f9ad9fadd64318f82f9fc0e44ca3dced8f3 Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Thu, 15 Oct 2026 22:03:44 -0400 Subject: [PATCH 12/29] test(mcpl): the -32601 and dispatch-failure log lines are asserted The description lists the log lines this PR adds, and three of them were printed by code no test read: the -32601 answer to a request for a method the host doesn't handle, the one for an event nothing on the host listens for, and an inbound dispatch failure's line. Their tests now capture console.error and assert each line whole, as log-value prints it: the method or event as a field, and a dispatch failure's cause quoted. No source change. Co-Authored-By: Claude Opus 5.5 --- test/mcpl-request-handler-errors.test.ts | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index 0c51497c7..ac1dd7fd6 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -585,7 +585,8 @@ test('a rejection whose reason cannot be printed is reported and answered, with test('a request for a method nothing handles is answered -32601; such a notification is ignored', (t) => { const { transport } = wireHarness(); - t.mock.method(console, 'error', () => {}); + 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 })); @@ -596,16 +597,25 @@ test('a request for a method nothing handles is answered -32601; such a notifica [32, -32601, 'Method not found'], [33, -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", + ]); }); test('a dispatch failure names the event, the request and what failed', (t) => { const { connection, transport, traces } = wireHarness(); - t.mock.method(console, 'error', () => {}); + const logged: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }); connection.prependListener('channels-list', () => { throw new Error('listener exploded'); }); transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 41, method: 'channels/list' })); const errors = traces.filter((e) => e.type === 'mcpl:server-error'); assert.equal(errors.length, 1); assert.match(errors[0].error, /inbound dispatch failed for channels-list \(request 41\): listener exploded/); + // The stderr line says the same, through log-value. + assert.ok(logged.includes('[mcpl] srv: inbound dispatch failed for channels-list (request 41): "listener exploded"'), logged.join('\n')); }); test('a manifest refresh that fails is reported as a handler failure', async (t) => { @@ -735,11 +745,13 @@ test('a rejection no printer can inspect still reaches the trace first', async ( test('a request for an event nothing on the host listens for is answered -32601', (t) => { const { connection, transport } = wireHarness(); - t.mock.method(console, 'error', () => {}); + const logged: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }); connection.removeAllListeners('channels-list'); transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 71, method: 'channels/list' })); const replies = transport.lines.map((line) => JSON.parse(line)); assert.deepEqual(replies.map((r) => [r.id, r.error?.code, r.error?.message]), [[71, -32601, 'Method not found']]); + assert.deepEqual(logged, ['[mcpl] srv: answered -32601 to channels-list, which nothing on this host handles']); }); test("a long handler error is bounded in its trace, and its log line is cut with its length", async (t) => { From 7a8c963256194cfab6cab13a7b1dc09bd5a47316 Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Thu, 15 Oct 2026 22:58:26 -0400 Subject: [PATCH 13/29] fix(mcpl): the request forms of manifestChanged and tools/list_changed are answered (R1-2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mcpl/manifestChanged (SPEC §17.3) and notifications/tools/list_changed are notifications. Sent with an id, dispatchMessage passed them a responder that their framework handlers ignore, and the wrapper answers -32603 only when a handler fails. So a server that sent either as a request got no answer when the refresh succeeded, and an error when it failed, though the framework acted on it either way. The maintainer's review measured both: a request-form tools/list_changed got no reply, and a request-form manifestChanged got -32603 because its handler failed in the probe's harness. Now emit() answers either request form with an empty result as it's admitted, after the -32601 check for an event nothing listens for, and passes it on as the notification is, with no responder. The answer doesn't depend on what the refresh does, and it's written before any request the refresh sends. It's decided at admission, the one place live routing and buffer flushes share, rather than as the line arrives in dispatchMessage. A connection's first lines after its handshake are held in the transport until the connection subscribes, and their flush runs before the framework wires its listeners, so a request form sent right after initialize meets no listener in dispatchMessage. It's buffered with its responder until the planes open, and only admission sees it with the framework's listener in place. Deciding as the line arrives would also have answered a request held during a staged startup before the startup succeeded, which emit()'s comment says must not be observable to the server. A held request form is answered only when it's let through. A connection with nothing listening for the event has nothing that takes it, so it's answered -32601 as before. Only the framework's wiring, which always listens for both, gets the empty result. Tests, with id 0: on the framework's wiring, both methods are answered {} once, the tools/list_changed listener runs after the answer is written and with no responder, and a failed manifest refresh is traced and answers nothing more. A request form that arrives before the framework wires the connection is held, and answered {} when its plane opens. On a connection with no listener, both are answered -32601. Co-Authored-By: Claude Opus 5.5 --- src/mcpl/server-connection.ts | 17 ++++++- test/mcpl-request-handler-errors.test.ts | 58 ++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index 57a0c4213..a440381b6 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -417,12 +417,27 @@ export class McplServerConnection extends EventEmitter { } // §6.6: a request that will never be answered MUST get an error. A // request whose event nothing on this host listens for would never be. - const request = args[1] as { respondError?: (code: number, message: string, data?: unknown) => void } | undefined; + const request = args[1] as { + respond?: (result: unknown) => void; + respondError?: (code: number, message: string, data?: unknown) => void; + } | undefined; if (typeof request?.respondError === 'function' && this.listenerCount(event) === 0) { request.respondError(-32601, 'Method not found'); console.error(`[mcpl] ${this.id}: answered -32601 to ${logValue(name)}, which nothing on this host handles`); return true; } + // mcpl/manifestChanged (§17.3) and tools/list_changed are notifications. + // Sent as a request, either is answered with an empty result as it's + // admitted, and passed on as the notification is, with no responder: + // the host acts on it the same way, so the answer doesn't depend on what + // the refresh does, and it's written before any request the refresh + // sends. This is where live routing and buffer flushes meet, so a request + // held while its plane is closed is answered only when it's let through, + // never by a startup that fails first. + if ((name === 'manifest-changed' || name === 'tools-list-changed') && typeof request?.respond === 'function') { + request.respond({}); + return super.emit(event, args[0]); + } return super.emit(event, ...args); } // Buffer cap (issue #122): a long data-plane pause (quiesce window) on a diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index ac1dd7fd6..727cf5f8a 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -631,6 +631,64 @@ test('a manifest refresh that fails is reported as a handler failure', async (t) assert.match(failures[0].error, /manifest fetch failed/); }); +test('mcpl/manifestChanged and tools/list_changed sent as requests are answered at once with an empty result, whatever the refresh does', async (t) => { + const { framework, connection, transport, traces } = wireHarness(); + t.mock.method(console, 'error', () => {}); + framework.handleManifestChanged = async () => { throw new Error('manifest fetch failed'); }; + const seen: Array<{ answered: number; responder: unknown }> = []; + connection.removeAllListeners('tools-list-changed'); + connection.on('tools-list-changed', (_params: unknown, responder?: unknown) => { + seen.push({ answered: transport.lines.length, responder }); + }); + // An id of 0 makes a request like any other id. + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 0, method: 'notifications/tools/list_changed' })); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 0, method: 'mcpl/manifestChanged', params: { revision: 'r2' } })); + await settle(); + assert.deepEqual(transport.lines.map((line) => JSON.parse(line)), [ + { jsonrpc: '2.0', id: 0, result: {} }, + { jsonrpc: '2.0', id: 0, result: {} }, + ]); + // The handler runs as for the notification, with no responder, once the answer is written. + assert.deepEqual(seen, [{ answered: 1, responder: undefined }]); + // The failed manifest refresh is reported, and answers nothing more. + const failures = failureTraces(traces); + assert.equal(failures.length, 1); + assert.equal(failures[0].event, 'manifest-changed'); + assert.equal(failures[0].requestId, undefined); +}); + +test('a request form that arrives before the framework wires the connection is answered when its plane opens', async (t) => { + t.mock.method(console, 'error', () => {}); + const transport = new ResponseTransport(); + const connection = new (McplServerConnection as any)('srv', null, transport) as McplServerConnection; + connection.establishGrant(new CapabilityGrant(new Set(ALL_CAPABILITY_PATHS), [])); + // Nothing on the host listens yet, and both planes are closed, so it's held. + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 0, method: 'notifications/tools/list_changed' })); + assert.deepEqual(transport.lines, []); + harness(connection); + const seen: unknown[] = []; + connection.removeAllListeners('tools-list-changed'); + connection.on('tools-list-changed', (_params: unknown, responder?: unknown) => { seen.push(responder); }); + connection.ready(); + await settle(); + assert.deepEqual(transport.lines.map((line) => JSON.parse(line)), [{ jsonrpc: '2.0', id: 0, result: {} }]); + assert.deepEqual(seen, [undefined]); +}); + +test('with nothing listening for them, their request forms are still answered -32601', (t) => { + 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(); + t.mock.method(console, 'error', () => {}); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 0, method: 'notifications/tools/list_changed' })); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 0, method: 'mcpl/manifestChanged' })); + assert.deepEqual(transport.lines.map((line) => JSON.parse(line)), [ + { jsonrpc: '2.0', id: 0, error: { code: -32601, message: 'Method not found' } }, + { jsonrpc: '2.0', id: 0, error: { code: -32601, message: 'Method not found' } }, + ]); +}); + test('a failed internal-error reply reaches stderr too', async (t) => { const { framework, connection } = harness(); framework.traceListeners = []; From 87bdf5cb1cae6088b56fcf3da16f267e87c75bec Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Thu, 15 Oct 2026 22:58:42 -0400 Subject: [PATCH 14/29] fix(mcpl): the handler-failure trace keeps only a string or number request id (R1-3) containedInboundHandler put the responder's id into the mcpl:request-handler-error trace unchecked, though the trace's type says requestId is a string or a number, and the connection's own reports (dispatchInboundEvent, reportInboundFailure) keep only those. The maintainer's review sent a channels/register with the id {"nested":[1]} whose handler threw, and the trace carried that object. A long string id wasn't bounded either, unlike every text beside it. The trace now keeps a number id as it is, a string id bounded with boundedForLog(id, 500) like the trace's other texts, and leaves any other id undefined. The key stays on the trace, as before. Validating ids once in dispatchMessage would refuse requests that are answered today, which is a protocol change of its own, so it isn't done here. Test: three failing channels-register calls, with an object id, a 2,000-character string id and the id 0, trace undefined, the string's first 500 characters and an ellipsis, and 0. Co-Authored-By: Claude Opus 5.5 --- src/framework.ts | 6 +++++- test/mcpl-request-handler-errors.test.ts | 11 +++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/framework.ts b/src/framework.ts index 9d1783fc8..b13bc5e67 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -17545,7 +17545,11 @@ export class AgentFramework { type: 'mcpl:request-handler-error', serverId, event, - requestId: responder?.id, + // The trace's type is string | number. Only those are kept, as + // the connection's own reports keep them, and a string is + // bounded like the texts beside it. + requestId: typeof responder?.id === 'number' ? responder.id + : typeof responder?.id === 'string' ? boundedForLog(responder.id, 500) : undefined, responseAttempted: handlerResponseAttempted, // The trace's copies are bounded, as data; the log line below // prints each text raw through log-value, which escapes it. diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index 727cf5f8a..22a379eff 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -689,6 +689,17 @@ test('with nothing listening for them, their request forms are still answered -3 ]); }); +test('the failure trace keeps a request id only when it is a string or a number, and bounds a long string', async (t) => { + const { framework, connection, traces } = harness(); + t.mock.method(console, 'error', () => {}); + framework.channelRegistry = { async handleRegister() { throw new Error('register failed'); } }; + for (const id of [{ nested: [1] }, 'x'.repeat(2000), 0]) { + connection.emit('channels-register', {}, { ...responder(), id }); + } + await settle(); + assert.deepEqual(failureTraces(traces).map((trace) => trace.requestId), [undefined, `${'x'.repeat(500)}…`, 0]); +}); + test('a failed internal-error reply reaches stderr too', async (t) => { const { framework, connection } = harness(); framework.traceListeners = []; From e368e4cd0b3152d64b65a468f72c14c84015909b Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Thu, 15 Oct 2026 23:00:51 -0400 Subject: [PATCH 15/29] docs(changelog): #208's note describes the change from 0.22.x (R1-1) The maintainer's review found the note describing the code as it was before 0.22.0. Since 0.22.0, six request handlers (push/event, inference/request, channels/register, channels/changed, channels/incoming and host/command) have answered their failures: -32603 with the handler's message, or -32000 with "the host stopped while handling this request" and the handler's message while the host stopped. The note read as if containing those handlers, channel reconciliation after an acknowledgement included, were new. The note now says what changes from 0.22.x: - the five handlers 0.22.x left uncontained (channels/list, tools/observe, model/info and the two notifications) are contained, and the six go through the same boundary; - the -32603 reply says "Internal error" where it carried the handler's text, and the -32000 reply while stopping no longer carries it; - the new trace, the -32601 answers, the request forms of the two notifications (R1-2), the non-object guard (#217), the dispatch boundary and the encoding error's text. Its rule is scoped to connections the framework wires and to throws and rejections, which is what the boundary covers. The stdin bullet contradicted main's unreleased #344 fragment, which says an unlistened stdin error is dropped, where this branch logs it. It's folded into that fragment: a stdin write failure is logged when nothing listens, and a stdout or stderr read error is still dropped then. #208's note no longer has it. Co-Authored-By: Claude Opus 5.5 --- changelog.d/208-inbound-handler-containment.fixed.md | 10 +++++----- changelog.d/mcpl-transport-close.fixed.md | 9 +++++---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/changelog.d/208-inbound-handler-containment.fixed.md b/changelog.d/208-inbound-handler-containment.fixed.md index c6ba2feb7..3c5955ac4 100644 --- a/changelog.d/208-inbound-handler-containment.fixed.md +++ b/changelog.d/208-inbound-handler-containment.fixed.md @@ -1,9 +1,9 @@ -- MCPL inbound handlers, for requests and for notifications (`mcpl/manifestChanged` and `notifications/tools/list_changed` included), contain synchronous throws and async rejections (#186), including channel reconciliation failures after an acknowledgement. Nothing a server sends, and nothing a handler throws or rejects with, ends the host: a rejection reason that can't be printed is reported as one that can. - - **Each failure emits `mcpl:request-handler-error` and a stderr line.** Its `error` is the handler's own error. A reply write that failed before it is `responseFailure`, and a reply the handler tried to send after one was already attempted is `refusedResponse` (a result, or an error with its code and message): refused, never written. `responseError` is the internal-error reply's own failure, which the stderr line prints too. A failure of the Discord awareness accounting that a tools-list refresh waits on is global: it's reported as `mcpl:server-error` for every connection, as each is recycled. - - **A request gets JSON-RPC `-32603` "Internal error"** when its handler fails before any reply bytes may have gone out. That's when it attempted no reply, or its reply failed while encoding. A transport write that threw may have sent part of a reply, so it isn't followed by another. The reply carries the standard message, not the handler's own error text, which stays in the trace and stderr. A notification gets no reply. +- MCPL's inbound handlers are contained at one boundary, the framework's wiring, for every request and notification it handles (#186). The `channels/list`, `tools/observe` and `model/info` requests, and the `mcpl/manifestChanged` and `notifications/tools/list_changed` notifications, are newly contained: a throw or rejection of theirs is reported as the other handlers' are. The six requests whose handler failures 0.22.0 already answered (`push/event`, `inference/request`, `channels/register`, `channels/changed`, `channels/incoming` and `host/command`) go through the same boundary now. On a connection the framework wires, no handler's throw or rejection ends the host, and a rejection reason that can't be printed is reported as one that can. + - **A request whose handler fails is answered `-32603` "Internal error",** where 0.22.x answered with the handler's own message, which can name internal paths and errors and goes to a server that may be a third party's. While the host is stopping, it's answered `-32000` "the host stopped while handling this request", without the handler's message that 0.22.x added after those words. The handler's error stays in the trace and on stderr. A request is answered this way when its handler fails before any reply bytes may have gone out: it attempted no reply, or its reply failed while encoding. A transport write that threw may have sent part of a reply, so it isn't followed by another. A notification gets no reply. + - **Each failure emits a new trace, `mcpl:request-handler-error`, and a stderr line.** Its `error` is the handler's own error. A reply write that failed before it is `responseFailure`, and a reply the handler tried to send after one was already attempted is `refusedResponse` (a result, or an error with its code and message): refused, never written. `responseError` is the internal-error reply's own failure, which the stderr line prints too. `requestId` is the request's id when that is a string or a number. A failure of the Discord awareness accounting that a tools-list refresh waits on is global: it's reported as `mcpl:server-error` for every connection, as each is recycled. - **A request whose method isn't a string,** or isn't one the host handles, or whose event nothing on the host listens for, is answered `-32601` "Method not found", with a stderr line. A notification nobody handles is ignored. - - **Non-object JSON** is ignored before property access, in the handshake reader and in routing, as a non-JSON line already is. + - **`mcpl/manifestChanged` and `notifications/tools/list_changed` sent as requests,** with an id, are answered with an empty result when the connection admits them, if something on the host listens for them, as the framework's wiring always does, and are handled as the notification is. + - **Non-object JSON** (`null`, `42`, `"hi"`, `true`) is ignored before property access, in the handshake reader and in routing, as a non-JSON line already is. One such line from a server used to end the host (#217). - **A synchronous failure while dispatching an inbound message**, buffered plane flushes and a server's stderr lines included, is reported without interrupting later messages. Its `mcpl:server-error` names the event or method, the request id, and the cause. - - **A failed write to a stdio server's stdin** (a child that has just exited) is reported instead of ending the host. - **A response that can't be encoded** says why in its error (a BigInt, a circular structure, a throwing `toJSON`). - **A reply to a connection that has already closed** is dropped without a trace, on either transport, as before. diff --git a/changelog.d/mcpl-transport-close.fixed.md b/changelog.d/mcpl-transport-close.fixed.md index de908fdd4..a01323f07 100644 --- a/changelog.d/mcpl-transport-close.fixed.md +++ b/changelog.d/mcpl-transport-close.fixed.md @@ -11,7 +11,8 @@ settled until the connection takes it, and one that a failed handshake is closing. A malformed frame from a WebSocket server is such an error. - A failed write to a stdio child's stdin, such as EPIPE from a child that - has gone, reaches the transport's `'error'` listener if it has one and is - otherwise dropped, since the child's exit is reported anyway. It was thrown - as an uncaught exception. So was a read error on the child's stdout or - stderr, which is now handled the same way. + has gone, reaches the transport's `'error'` listener if it has one, and is + logged when nothing listens. A read error on the child's stdout or stderr + reaches the listener the same way, and is dropped when nothing listens, + since the child's exit is reported anyway. Each was thrown as an uncaught + exception. From 129de6b0150454d972cb1b82fe445f52756a3709 Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Fri, 16 Oct 2026 01:16:56 -0400 Subject: [PATCH 16/29] fix(mcpl): an initialize error whose message has no string form refuses the connect The handshake reader built an initialize error's message with a template literal, after cleanup() had removed its listeners and cleared its timeout. A server's error `message` can be any JSON value. One with no primitive form, such as {"toString":1,"valueOf":1}, made the template throw: the TypeError escaped the 'line' listener as an uncaught exception, which ends a host that doesn't catch them, and with the listeners and the timeout gone, nothing was left to settle connect(). The message is now built with describeCause, which gives the template's own text for every value the template can convert, and the object's tag for one it can't. So the rejection always runs, and such a server is refused at once with `initialize error: [object Object]`. #208 already keeps non-object JSON from throwing in this reader (#217), and this is the same kind of fault: a server's line throwing out of the handshake reader. A separate host process, like #208's inbound probe, connects to a server that sends that answer, and asserts the refusal within 10 s and that the process survives. The changelog fragment gains a bullet for it. Co-Authored-By: Claude Opus 5.5 --- .../208-inbound-handler-containment.fixed.md | 1 + src/mcpl/server-connection.ts | 7 ++++- test/fixtures/mcpl-initialize-error-probe.mjs | 30 +++++++++++++++++++ test/mcpl-request-handler-errors.test.ts | 9 ++++++ 4 files changed, 46 insertions(+), 1 deletion(-) create mode 100644 test/fixtures/mcpl-initialize-error-probe.mjs diff --git a/changelog.d/208-inbound-handler-containment.fixed.md b/changelog.d/208-inbound-handler-containment.fixed.md index 3c5955ac4..fc4888af9 100644 --- a/changelog.d/208-inbound-handler-containment.fixed.md +++ b/changelog.d/208-inbound-handler-containment.fixed.md @@ -4,6 +4,7 @@ - **A request whose method isn't a string,** or isn't one the host handles, or whose event nothing on the host listens for, is answered `-32601` "Method not found", with a stderr line. A notification nobody handles is ignored. - **`mcpl/manifestChanged` and `notifications/tools/list_changed` sent as requests,** with an id, are answered with an empty result when the connection admits them, if something on the host listens for them, as the framework's wiring always does, and are handled as the notification is. - **Non-object JSON** (`null`, `42`, `"hi"`, `true`) is ignored before property access, in the handshake reader and in routing, as a non-JSON line already is. One such line from a server used to end the host (#217). + - **An `initialize` error whose `message` has no string form,** such as `{"toString":1}`, refuses the connect at once, with `initialize error: [object Object]`. 0.22.x converted that message after the handshake had stopped listening and cleared its timeout, so the conversion's throw escaped as an uncaught exception, which ends a host that doesn't catch them, and the connect never settled. - **A synchronous failure while dispatching an inbound message**, buffered plane flushes and a server's stderr lines included, is reported without interrupting later messages. Its `mcpl:server-error` names the event or method, the request id, and the cause. - **A response that can't be encoded** says why in its error (a BigInt, a circular structure, a throwing `toJSON`). - **A reply to a connection that has already closed** is dropped without a trace, on either transport, as before. diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index a440381b6..1964c1cc1 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -606,7 +606,12 @@ export class McplServerConnection extends EventEmitter { if (msg.id !== initId) return; cleanup(); if (msg.error) { - reject(new Error(`MCPL server "${config.id}" initialize error: ${msg.error.message}`)); + // describeCause rather than the template's own conversion: the + // server's `message` can be any JSON value, and one that has no + // primitive form, such as {"toString":1}, would throw out of this + // listener after cleanup() has cleared the timeout, leaving the + // handshake to wait forever. + reject(new Error(`MCPL server "${config.id}" initialize error: ${describeCause(msg.error.message)}`)); return; } const result = msg.result as Record | undefined; diff --git a/test/fixtures/mcpl-initialize-error-probe.mjs b/test/fixtures/mcpl-initialize-error-probe.mjs new file mode 100644 index 000000000..c6ab6d134 --- /dev/null +++ b/test/fixtures/mcpl-initialize-error-probe.mjs @@ -0,0 +1,30 @@ +// Runs as a separate host process: an uncaught exception ends it, as it would end a host. +import assert from 'node:assert/strict'; +import { McplServerConnection } from '../../src/mcpl/server-connection.js'; + +// A server that answers initialize with an error whose message is an object +// with no primitive form, so a template literal can't convert it. +const server = String.raw` +let input = ''; +process.stdin.on('data', (chunk) => { + input += chunk; + let end; + while ((end = input.indexOf('\n')) >= 0) { + const request = JSON.parse(input.slice(0, end)); + input = input.slice(end + 1); + if (request.method === 'initialize') { + process.stdout.write(JSON.stringify({jsonrpc:'2.0', id:request.id, error:{code:-32603, message:{toString:1, valueOf:1}}}) + '\n'); + } + } +}); +`; +let timer; +const outcome = await Promise.race([ + McplServerConnection.connect({ id: 'probe', command: process.execPath, args: ['-e', server] }, { version: '0.5' }) + .then(() => 'connected', (error) => error), + new Promise((resolve) => { timer = setTimeout(resolve, 10_000, 'still pending after 10 s'); }), +]); +clearTimeout(timer); +assert.ok(outcome instanceof Error, String(outcome)); +assert.equal(outcome.message, 'MCPL server "probe" initialize error: [object Object]'); +console.log('refused'); diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index 22a379eff..fab2e8239 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -378,6 +378,15 @@ for (const mode of ['handshake', 'connected', 'throwing-listener']) { }); } +test('a separate stdio host whose server answers initialize with an error message no template can convert is refused at once, and survives', () => { + const probe = spawnSync(process.execPath, [ + '--import', 'tsx', fileURLToPath(new URL('./fixtures/mcpl-initialize-error-probe.mjs', import.meta.url)), + ], { encoding: 'utf8', timeout: 120_000 }); + assert.equal(probe.error, undefined); + assert.equal(probe.status, 0, probe.stderr); + assert.match(probe.stdout, /refused/); +}); + for (const value of [null, 42, 'hi', true, []]) { test('non-object inbound ' + JSON.stringify(value) + ' leaves the next notification deliverable', () => { const { connection, transport } = wireHarness(); From f26a45043e39c18876522bfea08a314db440f16b Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Fri, 16 Oct 2026 15:51:22 -0400 Subject: [PATCH 17/29] fix(mcpl): the R1 round's texts say what the code does; an initialize error's message is bounded The internal review of this round found texts that said more than the code, and tests that left behaviour the texts rely on unpinned. The one code change is the initialize error's message: it is the server's text in an error's message, so it is now bounded with boundedForLog(describeCause(...)), as the inbound-dispatch error beside it is and as errors.ts says. A short plain message reads as before; a line break becomes '?', so it can no longer pose as a host log line. Texts: - mcpl/manifestChanged isn't newly contained. A manifest fetch that fails is still caught and logged inside handleManifestChanged, as in 0.22.x, and the previous manifest stands (SPEC 17.5); anything else its handler throws or rejects with is now traced, where 0.22.x only logged it. So four are newly contained. This also corrects 7a8c9632's message, which said a failed manifest refresh is traced, and the wiring's comment, which said the same. - 7a8c9632's message also said its tests run on the framework's wiring. They run a listener in place of the framework's tools-list-changed one, which takes no arguments, so a request form reaches the real one exactly as the notification does. - The trace's requestId is a number as it is, or a string bounded as the trace's other texts are, and absent for a notification, for an id that is neither, and for the two request forms, which are answered as they're admitted and then handled as the notification is. Its responseAttempted doc names the -32000 a stopping host answers. - The note says 0.22.x answered the six #263 handlers' failures with the handler's message, that a request form's answer waits while its plane is paused, and no longer that an initialize error is refused "at once": the handshake awaits the transport's close before it rethrows, and a WebSocket's close resolves on the socket's. - The #344 fragment no longer says a read error is dropped "since the child's exit is reported anyway": a read error doesn't end the child. - Test titles no longer claim "at once" for the request forms or the initialize probe (its 10 s race is shorter than the handshake's 30 s timeout), or "a real transport" for a fake whose write throws. Tests: - A failed channels/register is answered -32603 with its own id, a string id with control and format characters and an id of true; its trace bounds the string id and leaves out the other. - A manifest fetch that fails is only logged, with no handler-failure trace. - A request-form mcpl/manifestChanged held while the data plane is paused, the control plane open, is answered and its params passed on when the data plane opens. - At the buffer cap of 1,000 held events, a held request form is kept and answered when its plane opens while the oldest notifications are dropped, with the cap's and the flush's log lines. - The request forms' params reach their listeners. - An initialize error's message with a line break and more than 200 code points is refused with it bounded. Co-Authored-By: Claude Opus 5.5 --- .../208-inbound-handler-containment.fixed.md | 10 +- changelog.d/mcpl-transport-close.fixed.md | 5 +- src/framework.ts | 6 +- src/mcpl/server-connection.ts | 5 +- src/types/trace.ts | 11 +- test/mcpl-request-handler-errors.test.ts | 127 ++++++++++++++++-- 6 files changed, 138 insertions(+), 26 deletions(-) diff --git a/changelog.d/208-inbound-handler-containment.fixed.md b/changelog.d/208-inbound-handler-containment.fixed.md index fc4888af9..e0e801b36 100644 --- a/changelog.d/208-inbound-handler-containment.fixed.md +++ b/changelog.d/208-inbound-handler-containment.fixed.md @@ -1,10 +1,10 @@ -- MCPL's inbound handlers are contained at one boundary, the framework's wiring, for every request and notification it handles (#186). The `channels/list`, `tools/observe` and `model/info` requests, and the `mcpl/manifestChanged` and `notifications/tools/list_changed` notifications, are newly contained: a throw or rejection of theirs is reported as the other handlers' are. The six requests whose handler failures 0.22.0 already answered (`push/event`, `inference/request`, `channels/register`, `channels/changed`, `channels/incoming` and `host/command`) go through the same boundary now. On a connection the framework wires, no handler's throw or rejection ends the host, and a rejection reason that can't be printed is reported as one that can. - - **A request whose handler fails is answered `-32603` "Internal error",** where 0.22.x answered with the handler's own message, which can name internal paths and errors and goes to a server that may be a third party's. While the host is stopping, it's answered `-32000` "the host stopped while handling this request", without the handler's message that 0.22.x added after those words. The handler's error stays in the trace and on stderr. A request is answered this way when its handler fails before any reply bytes may have gone out: it attempted no reply, or its reply failed while encoding. A transport write that threw may have sent part of a reply, so it isn't followed by another. A notification gets no reply. - - **Each failure emits a new trace, `mcpl:request-handler-error`, and a stderr line.** Its `error` is the handler's own error. A reply write that failed before it is `responseFailure`, and a reply the handler tried to send after one was already attempted is `refusedResponse` (a result, or an error with its code and message): refused, never written. `responseError` is the internal-error reply's own failure, which the stderr line prints too. `requestId` is the request's id when that is a string or a number. A failure of the Discord awareness accounting that a tools-list refresh waits on is global: it's reported as `mcpl:server-error` for every connection, as each is recycled. +- MCPL's inbound handlers are contained at one boundary, the framework's wiring, for every request and notification it handles (#186). The `channels/list`, `tools/observe` and `model/info` requests and the `notifications/tools/list_changed` notification are newly contained: a throw or rejection of theirs is reported as the other handlers' are. `mcpl/manifestChanged` goes through the same boundary: a manifest fetch that fails is still caught and logged inside its handler, as in 0.22.x, and anything else its handler throws or rejects with is now reported as the others' are, where 0.22.x only logged it. The six requests whose handler failures 0.22.0 already answered (`push/event`, `inference/request`, `channels/register`, `channels/changed`, `channels/incoming` and `host/command`) go through the same boundary now. On a connection the framework wires, no handler's throw or rejection ends the host, and a rejection reason that can't be printed is reported as one that can. + - **A request whose handler fails is answered `-32603` "Internal error",** where 0.22.x answered those six with the handler's own message, which can name internal paths and errors and goes to a server that may be a third party's. While the host is stopping, it's answered `-32000` "the host stopped while handling this request", without the handler's message that 0.22.x added after those words. The handler's error stays in the trace and on stderr. A request is answered this way when its handler fails before any reply bytes may have gone out: it attempted no reply, or its reply failed while encoding. A transport write that threw may have sent part of a reply, so it isn't followed by another. A notification gets no reply. + - **Each failure emits a new trace, `mcpl:request-handler-error`, and a stderr line.** Its `error` is the handler's own error. A reply write that failed before it is `responseFailure`, and a reply the handler tried to send after one was already attempted is `refusedResponse` (a result, or an error with its code and message): refused, never written. `responseError` is the internal-error reply's own failure, which the stderr line prints too. `requestId` is the request's id when that is a number, and a string id bounded as the trace's other texts are. A request form of `mcpl/manifestChanged` or `notifications/tools/list_changed` has none, since it's answered as it's admitted and then handled as the notification is. A failure of the Discord awareness accounting that a tools-list refresh waits on is global: it's reported as `mcpl:server-error` for every connection, as each is recycled. - **A request whose method isn't a string,** or isn't one the host handles, or whose event nothing on the host listens for, is answered `-32601` "Method not found", with a stderr line. A notification nobody handles is ignored. - - **`mcpl/manifestChanged` and `notifications/tools/list_changed` sent as requests,** with an id, are answered with an empty result when the connection admits them, if something on the host listens for them, as the framework's wiring always does, and are handled as the notification is. + - **`mcpl/manifestChanged` and `notifications/tools/list_changed` sent as requests,** with an id, are answered with an empty result when the connection admits them, which waits while their plane is paused, if something on the host listens for them, as the framework's wiring always does, and are handled as the notification is. - **Non-object JSON** (`null`, `42`, `"hi"`, `true`) is ignored before property access, in the handshake reader and in routing, as a non-JSON line already is. One such line from a server used to end the host (#217). - - **An `initialize` error whose `message` has no string form,** such as `{"toString":1}`, refuses the connect at once, with `initialize error: [object Object]`. 0.22.x converted that message after the handshake had stopped listening and cleared its timeout, so the conversion's throw escaped as an uncaught exception, which ends a host that doesn't catch them, and the connect never settled. + - **An `initialize` error whose `message` has no string form,** such as `{"toString":1}`, refuses the connect, with `initialize error: [object Object]`. 0.22.x converted that message after the handshake had stopped listening and cleared its timeout, so the conversion's throw escaped as an uncaught exception, which ends a host that doesn't catch them, and the connect never settled. An `initialize` error's message is now bounded too, as the framework's other errors bound a server's text: control, format and line-separator characters become `?`, and it's cut at 200 code points. - **A synchronous failure while dispatching an inbound message**, buffered plane flushes and a server's stderr lines included, is reported without interrupting later messages. Its `mcpl:server-error` names the event or method, the request id, and the cause. - **A response that can't be encoded** says why in its error (a BigInt, a circular structure, a throwing `toJSON`). - **A reply to a connection that has already closed** is dropped without a trace, on either transport, as before. diff --git a/changelog.d/mcpl-transport-close.fixed.md b/changelog.d/mcpl-transport-close.fixed.md index a01323f07..f9a534e8d 100644 --- a/changelog.d/mcpl-transport-close.fixed.md +++ b/changelog.d/mcpl-transport-close.fixed.md @@ -13,6 +13,5 @@ - A failed write to a stdio child's stdin, such as EPIPE from a child that has gone, reaches the transport's `'error'` listener if it has one, and is logged when nothing listens. A read error on the child's stdout or stderr - reaches the listener the same way, and is dropped when nothing listens, - since the child's exit is reported anyway. Each was thrown as an uncaught - exception. + reaches the listener the same way, and is dropped when nothing listens. + Each was thrown as an uncaught exception. diff --git a/src/framework.ts b/src/framework.ts index b13bc5e67..7b20afd7f 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -17710,8 +17710,10 @@ export class AgentFramework { // §17.3: a manifest-change announcement. Rate-limited re-fetch → diff // → §6.7 consequences → one receipt. The announcement itself carries no // payload and no authority; everything below acts on FETCHED content. - // A notification, contained and reported like every inbound handler: a - // refresh that fails emits mcpl:request-handler-error. + // A notification, contained like every inbound handler. A manifest fetch + // that fails is caught and logged inside handleManifestChanged, and the + // previous manifest stands (§17.5); anything else it throws or rejects + // with emits mcpl:request-handler-error. onRequest('manifest-changed', (params: { revision?: string; domains?: string[] }) => this.handleManifestChanged(connection, params)); diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index 1964c1cc1..beef2ef66 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -610,8 +610,9 @@ export class McplServerConnection extends EventEmitter { // server's `message` can be any JSON value, and one that has no // primitive form, such as {"toString":1}, would throw out of this // listener after cleanup() has cleared the timeout, leaving the - // handshake to wait forever. - reject(new Error(`MCPL server "${config.id}" initialize error: ${describeCause(msg.error.message)}`)); + // handshake to wait forever. It's the server's text in an error's + // message, so it's bounded as errors.ts says. + reject(new Error(`MCPL server "${config.id}" initialize error: ${boundedForLog(describeCause(msg.error.message))}`)); return; } const result = msg.result as Record | undefined; diff --git a/src/types/trace.ts b/src/types/trace.ts index 5747747c4..8f595f07d 100644 --- a/src/types/trace.ts +++ b/src/types/trace.ts @@ -565,13 +565,18 @@ export type TraceEvent = serverId: string; /** Connection event name, e.g. channels-register or push-event. */ event: string; - /** Absent for notifications. */ + /** The request's id when it is a number, or a string id bounded as the + * other texts here are: control, format and line-separator characters + * replaced, and cut at 500 code points. Absent for a notification, for + * a request whose id is neither, and for a request form of + * mcpl/manifestChanged or tools/list_changed, which is answered as + * it's admitted and then handled as the notification is. */ requestId?: string | number; /** Whether the handler's own reply may have reached the server: true * once it attempted one, unless the attempt failed while encoding, * before any bytes were written. When it's false and a responder was - * given, the host answered -32603 "Internal error" itself, unless - * `responseError` says that reply failed. */ + * given, the host answered -32603 "Internal error" itself, or -32000 + * while it stops, unless `responseError` says that reply failed. */ responseAttempted: boolean; /** The handler's own error when it threw or rejected; otherwise the * reply write that failed and that it swallowed, or that it tried to diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index fab2e8239..97f0eeebb 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -334,7 +334,7 @@ test('a swallowed pre-write encoding failure still receives a fallback response' assert.equal(failureTraces(traces)[0].responseAttempted, false); }); -test('a real transport write failure remains single-attempt after serialization', async () => { +test('a transport write that throws remains single-attempt after serialization', async () => { const { framework, traces, transport, request } = wireHarness(); framework.handleHostCommand = async () => ({ maintained: true }); transport.failWrite = true; @@ -378,7 +378,7 @@ for (const mode of ['handshake', 'connected', 'throwing-listener']) { }); } -test('a separate stdio host whose server answers initialize with an error message no template can convert is refused at once, and survives', () => { +test('a separate stdio host whose server answers initialize with an error message no template can convert is refused well before the handshake times out, and survives', () => { const probe = spawnSync(process.execPath, [ '--import', 'tsx', fileURLToPath(new URL('./fixtures/mcpl-initialize-error-probe.mjs', import.meta.url)), ], { encoding: 'utf8', timeout: 120_000 }); @@ -387,6 +387,28 @@ test('a separate stdio host whose server answers initialize with an error messag assert.match(probe.stdout, /refused/); }); +test("an initialize error's message is bounded, as a server's text in the framework's errors is", { timeout: 30_000 }, async () => { + // Its message has a line break and runs past 200 code points. + const server = String.raw` +let input = ''; +process.stdin.on('data', (chunk) => { + input += chunk; + let end; + while ((end = input.indexOf('\n')) >= 0) { + const request = JSON.parse(input.slice(0, end)); + input = input.slice(end + 1); + if (request.method === 'initialize') { + process.stdout.write(JSON.stringify({jsonrpc:'2.0', id:request.id, error:{code:-32603, message:'first line\nsecond' + 'x'.repeat(300)}}) + '\n'); + } + } +}); +`; + await assert.rejects( + McplServerConnection.connect({ id: 'srv', command: process.execPath, args: ['-e', server] }, { version: '0.5' }), + { message: `MCPL server "srv" initialize error: first line?second${'x'.repeat(183)}…` }, + ); +}); + for (const value of [null, 42, 'hi', true, []]) { test('non-object inbound ' + JSON.stringify(value) + ' leaves the next notification deliverable', () => { const { connection, transport } = wireHarness(); @@ -627,7 +649,7 @@ test('a dispatch failure names the event, the request and what failed', (t) => { assert.ok(logged.includes('[mcpl] srv: inbound dispatch failed for channels-list (request 41): "listener exploded"'), logged.join('\n')); }); -test('a manifest refresh that fails is reported as a handler failure', async (t) => { +test('a manifest-changed handler that throws is reported as a handler failure', async (t) => { const { framework, connection, traces } = harness(); t.mock.method(console, 'error', () => {}); framework.handleManifestChanged = async () => { throw new Error('manifest fetch failed'); }; @@ -640,25 +662,47 @@ test('a manifest refresh that fails is reported as a handler failure', async (t) assert.match(failures[0].error, /manifest fetch failed/); }); -test('mcpl/manifestChanged and tools/list_changed sent as requests are answered at once with an empty result, whatever the refresh does', async (t) => { +test('a manifest fetch that fails is only logged, as in 0.22.x: the previous manifest stands, and no handler failure is reported', async (t) => { + const { framework, connection, traces } = harness(); + const errors = t.mock.method(console, 'error', () => {}); + framework.manifestRefreshState = new Map(); + framework.mcplServerConfigs = new Map([['srv', { id: 'srv' }]]); + (connection as any).sendManifestRequest = async () => { throw new Error('the server is gone'); }; + connection.emit('manifest-changed', { revision: 'r2' }); + await settle(); + assert.deepEqual(failureTraces(traces), []); + assert.deepEqual(errors.mock.calls.map((call) => String(call.arguments[0])), [ + '[mcpl] srv: mcpl/manifest fetch failed — previous manifest stands (§17.5): the server is gone', + ]); +}); + +test('mcpl/manifestChanged and tools/list_changed sent as requests are answered with an empty result as they are admitted, whatever their handlers do', async (t) => { const { framework, connection, transport, traces } = wireHarness(); t.mock.method(console, 'error', () => {}); - framework.handleManifestChanged = async () => { throw new Error('manifest fetch failed'); }; - const seen: Array<{ answered: number; responder: unknown }> = []; + const manifestParams: unknown[] = []; + framework.handleManifestChanged = async (_connection: unknown, params: unknown) => { + manifestParams.push(params); + throw new Error('manifest fetch failed'); + }; + const seen: Array<{ answered: number; params: unknown; responder: unknown }> = []; + // A listener in place of the framework's tools-list-changed one, to see + // when it runs and with what. connection.removeAllListeners('tools-list-changed'); - connection.on('tools-list-changed', (_params: unknown, responder?: unknown) => { - seen.push({ answered: transport.lines.length, responder }); + connection.on('tools-list-changed', (params: unknown, responder?: unknown) => { + seen.push({ answered: transport.lines.length, params, responder }); }); // An id of 0 makes a request like any other id. - transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 0, method: 'notifications/tools/list_changed' })); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 0, method: 'notifications/tools/list_changed', params: { sequence: 4 } })); transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 0, method: 'mcpl/manifestChanged', params: { revision: 'r2' } })); await settle(); assert.deepEqual(transport.lines.map((line) => JSON.parse(line)), [ { jsonrpc: '2.0', id: 0, result: {} }, { jsonrpc: '2.0', id: 0, result: {} }, ]); - // The handler runs as for the notification, with no responder, once the answer is written. - assert.deepEqual(seen, [{ answered: 1, responder: undefined }]); + // Each is handled as its notification is, with its params and no + // responder, once its answer is written. + assert.deepEqual(seen, [{ answered: 1, params: { sequence: 4 }, responder: undefined }]); + assert.deepEqual(manifestParams, [{ revision: 'r2' }]); // The failed manifest refresh is reported, and answers nothing more. const failures = failureTraces(traces); assert.equal(failures.length, 1); @@ -684,6 +728,52 @@ test('a request form that arrives before the framework wires the connection is a assert.deepEqual(seen, [undefined]); }); +function stagedHarness() { + const transport = new ResponseTransport(); + const connection = new (McplServerConnection as any)('srv', null, transport) as McplServerConnection; + connection.establishGrant(new CapabilityGrant(new Set(ALL_CAPABILITY_PATHS), [])); + const { framework } = harness(connection); + const refreshed: Array<{ revision?: string }> = []; + framework.handleManifestChanged = async (_connection: unknown, params: { revision?: string }) => { refreshed.push(params); }; + // The control plane open and the data plane paused, as in a quiesce. + connection.readyControlPlane(); + return { connection, transport, refreshed }; +} + +test('a request-form mcpl/manifestChanged held while the data plane is paused is answered, and its params passed on, when the data plane opens', async (t) => { + t.mock.method(console, 'error', () => {}); + const { connection, transport, refreshed } = stagedHarness(); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 3, method: 'mcpl/manifestChanged', params: { revision: 'r2' } })); + await settle(); + assert.deepEqual(transport.lines, [], 'held: mcpl/manifestChanged is a data-plane event'); + assert.deepEqual(refreshed, []); + connection.ready(); + await settle(); + assert.deepEqual(transport.lines.map((line) => JSON.parse(line)), [{ jsonrpc: '2.0', id: 3, result: {} }]); + assert.deepEqual(refreshed, [{ revision: 'r2' }]); +}); + +test('a request form held at the buffer cap is kept, and answered when its plane opens, while the oldest notifications are dropped', async (t) => { + const errors = t.mock.method(console, 'error', () => {}); + const { connection, transport, refreshed } = stagedHarness(); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 5, method: 'mcpl/manifestChanged', params: { revision: 'held' } })); + // 1,100 notifications pass the cap of 1,000 held events by 101. + for (let i = 0; i < 1_100; i++) { + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', method: 'mcpl/manifestChanged', params: { revision: `n${i}` } })); + } + connection.ready(); + await settle(); + assert.deepEqual(transport.lines.map((line) => JSON.parse(line)), [{ jsonrpc: '2.0', id: 5, result: {} }]); + assert.equal(refreshed.length, 1_000); + assert.deepEqual(refreshed.slice(0, 2), [{ revision: 'held' }, { revision: 'n101' }]); + assert.deepEqual(refreshed.at(-1), { revision: 'n1099' }); + assert.deepEqual(errors.mock.calls.map((call) => String(call.arguments[0])), [ + '[mcpl] srv: buffered-event cap (1000) — dropped 1 responder-less event(s) while the plane is paused', + '[mcpl] srv: buffered-event cap (1000) — dropped 101 responder-less event(s) while the plane is paused', + '[mcpl] srv: flushing 1000 buffered event(s) synchronously (long pause backlog; 101 dropped at cap)', + ]); +}); + test('with nothing listening for them, their request forms are still answered -32601', (t) => { const transport = new ResponseTransport(); const connection = new (McplServerConnection as any)('srv', null, transport) as McplServerConnection; @@ -698,6 +788,21 @@ test('with nothing listening for them, their request forms are still answered -3 ]); }); +test('a failed request is answered -32603 with its own id, while its trace bounds a string id and leaves out one that is neither a string nor a number', async (t) => { + 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]) { + 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]], + ); + assert.deepEqual(failureTraces(traces).map((trace) => trace.requestId), ['a?b?c?', undefined]); +}); + test('the failure trace keeps a request id only when it is a string or a number, and bounds a long string', async (t) => { const { framework, connection, traces } = harness(); t.mock.method(console, 'error', () => {}); From 3c7efa29b3f356f6364d69b94937b0cd5c013523 Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Fri, 16 Oct 2026 17:31:50 -0400 Subject: [PATCH 18/29] docs(changelog): the six handlers' reply text is a Changed entry 0.22.0's CHANGELOG told server authors that a failing push/event, inference/request, channels/register, channels/changed, channels/incoming or host/command handler is answered with its message: -32603 with the handler's message on a live host, and -32000 with it in parentheses after "the host stopped while handling this request" while the host stops (v0.22.0 and v0.22.1 refuseServerItem). #208 answers those six with the standard wording alone, "Internal error" or the stop wording, and keeps the handler's text in the mcpl:request-handler-error trace and the stderr line. That changes a released, documented reply text a server can see, so it moves from the Fixed fragment to its own Changed fragment, which opens with whom it reaches (MCPL server authors) and says where the text is now. The Fixed fragment keeps the reply as the fix it is for the three requests whose handler failures 0.22.x didn't answer (channels/list, tools/observe and model/info), with a pointer to Changed for the six. The -32000 half was already in the fragment (since e368e4cd); this commit only moves it. No source or test changes. Co-Authored-By: Claude Opus 5.5 --- changelog.d/208-handler-failure-replies.changed.md | 1 + changelog.d/208-inbound-handler-containment.fixed.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 changelog.d/208-handler-failure-replies.changed.md diff --git a/changelog.d/208-handler-failure-replies.changed.md b/changelog.d/208-handler-failure-replies.changed.md new file mode 100644 index 000000000..ea5332c49 --- /dev/null +++ b/changelog.d/208-handler-failure-replies.changed.md @@ -0,0 +1 @@ +- **MCPL server authors:** when the host's handler for a server's request fails, the server's answer no longer carries the handler's own message (#208). 0.22.x answered a failed `push/event`, `inference/request`, `channels/register`, `channels/changed`, `channels/incoming` or `host/command` with `-32603` and the handler's message, or, once the host was stopping, with `-32000` and "the host stopped while handling this request" followed by the handler's message in parentheses. These requests now get `-32603` "Internal error", or, while the host is stopping, `-32000` "the host stopped while handling this request" alone. A handler's message can name the host's internal paths and errors, and the server may be a third party's. A server that shows or parses the handler's text from these replies no longer receives it: the host writes it to stderr and to its `mcpl:request-handler-error` trace instead. diff --git a/changelog.d/208-inbound-handler-containment.fixed.md b/changelog.d/208-inbound-handler-containment.fixed.md index e0e801b36..276e6f10d 100644 --- a/changelog.d/208-inbound-handler-containment.fixed.md +++ b/changelog.d/208-inbound-handler-containment.fixed.md @@ -1,5 +1,5 @@ - MCPL's inbound handlers are contained at one boundary, the framework's wiring, for every request and notification it handles (#186). The `channels/list`, `tools/observe` and `model/info` requests and the `notifications/tools/list_changed` notification are newly contained: a throw or rejection of theirs is reported as the other handlers' are. `mcpl/manifestChanged` goes through the same boundary: a manifest fetch that fails is still caught and logged inside its handler, as in 0.22.x, and anything else its handler throws or rejects with is now reported as the others' are, where 0.22.x only logged it. The six requests whose handler failures 0.22.0 already answered (`push/event`, `inference/request`, `channels/register`, `channels/changed`, `channels/incoming` and `host/command`) go through the same boundary now. On a connection the framework wires, no handler's throw or rejection ends the host, and a rejection reason that can't be printed is reported as one that can. - - **A request whose handler fails is answered `-32603` "Internal error",** where 0.22.x answered those six with the handler's own message, which can name internal paths and errors and goes to a server that may be a third party's. While the host is stopping, it's answered `-32000` "the host stopped while handling this request", without the handler's message that 0.22.x added after those words. The handler's error stays in the trace and on stderr. A request is answered this way when its handler fails before any reply bytes may have gone out: it attempted no reply, or its reply failed while encoding. A transport write that threw may have sent part of a reply, so it isn't followed by another. A notification gets no reply. + - **A request whose handler fails is answered `-32603` "Internal error",** or, while the host is stopping, `-32000` "the host stopped while handling this request". The handler's error stays in the trace and on stderr. For the six that 0.22.x answered with the handler's own message, that's a change, described under Changed. A request is answered this way when its handler fails before any reply bytes may have gone out: it attempted no reply, or its reply failed while encoding. A transport write that threw may have sent part of a reply, so it isn't followed by another. A notification gets no reply. - **Each failure emits a new trace, `mcpl:request-handler-error`, and a stderr line.** Its `error` is the handler's own error. A reply write that failed before it is `responseFailure`, and a reply the handler tried to send after one was already attempted is `refusedResponse` (a result, or an error with its code and message): refused, never written. `responseError` is the internal-error reply's own failure, which the stderr line prints too. `requestId` is the request's id when that is a number, and a string id bounded as the trace's other texts are. A request form of `mcpl/manifestChanged` or `notifications/tools/list_changed` has none, since it's answered as it's admitted and then handled as the notification is. A failure of the Discord awareness accounting that a tools-list refresh waits on is global: it's reported as `mcpl:server-error` for every connection, as each is recycled. - **A request whose method isn't a string,** or isn't one the host handles, or whose event nothing on the host listens for, is answered `-32601` "Method not found", with a stderr line. A notification nobody handles is ignored. - **`mcpl/manifestChanged` and `notifications/tools/list_changed` sent as requests,** with an id, are answered with an empty result when the connection admits them, which waits while their plane is paused, if something on the host listens for them, as the framework's wiring always does, and are handled as the notification is. From 8051b2452f91ff33662b0d4065323e1c9fbf573b Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Sat, 17 Oct 2026 01:44:05 -0400 Subject: [PATCH 19/29] fix(mcpl): an error answer settles its request, and manifest re-runs are contained MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The internal review of b838f09e raised 30 issues; the issue numbers below are its. The repository owner decided I1, I2, I4, I6, I7, I9 and I10. Code: - An error answer to a request the host sent (I4, I17): handleResponse built its Error from response.error.code and .message with a template, after the request had left pendingRequests and its timer was cleared. A code or message with no primitive form, such as {"toString":1}, threw there. On main that's in the transport's line listener with no catch; on this branch the routing catch contains it, and the request then never settled, so a manifest fetch stayed in flight and every later announcement on that connection only queued. Both go through describeCause now, and the message, the server's text, through boundedForLog, as an initialize error's is. That also bounds what the fetch-failure log line prints. - Manifest re-runs (I2): the refresh handleManifestChanged schedules for itself, after the fetch floor or behind a fetch in flight, was started with .catch(() => {}), as in 0.22.x, so a failure other than its fetch's was neither traced nor logged. Both re-runs now go through rerunManifestRefresh, which contains them with containedInboundHandler as manifest-changed with no responder, so only what the fetch's own catch takes stays unreported. Text: - I1, I3: the Changed entry covers a handler that throws or rejects before it has answered; a handler that answers a failure itself (inference/request when the inference fails, push/event when a coalesced push fails) still sends its own code and message. - I2: the Fixed fragment says 0.22.x logged a failure from an announcement's own run and dropped it from a scheduled re-run. - I4: the Fixed fragment has a bullet for an error answer, and one for the bound on a server's text in both errors. - I11: the trace type's header names the resolved cases, and the fragment says what `error` is for a handler that resolved. - I18: errors.ts says boundedForLog cuts after `max` code points, with "…" marking the cut. - I19, I20: responseAttempted's doc says the host's reply needs a responder with respondError, and that a closed connection drops it. - I22: every connection gets its server-error trace, and then each is recycled unless the host is stopping. - I27: the initialize comment names both consequences of the old conversion's throw. - I28: trace.ts names notifications/tools/list_changed. Tests (test/mcpl-request-handler-errors.test.ts): - I2: a refresh deferred by the fetch floor, and one queued behind a fetch in flight, each report a failure other than their fetch's (mock timers). - I4: a manifest fetch answered with an error no template can convert is logged, leaving no refresh in flight and no dispatch failure; a server's error message is bounded in the error a request rejects with. - I14: the "previous manifest stands" test checks that the grant, the feature sets and the manifest state are untouched. - I24: a request-form notifications/tools/list_changed is answered at once while the data plane is paused, since the control plane carries it. Not changed here: I6 (a manifestChanged with no params), I7 (a peer's close reason in a handshake error) and I10 (the buffer cap's drop count) are on main already, and go to one issue and a small PR on main. I5, I9 and I23 aren't real, I8 and I25 are known, and I12, I13, I15, I16, I26, I29 and I30 aren't worth a change, each with its reason in the review's verdicts. I21 is the description's. Co-Authored-By: Claude Opus 5.5 --- .../208-handler-failure-replies.changed.md | 2 +- .../208-inbound-handler-containment.fixed.md | 8 +- src/framework.ts | 19 ++- src/mcpl/errors.ts | 10 +- src/mcpl/server-connection.ts | 15 ++- src/types/trace.ts | 17 ++- test/mcpl-request-handler-errors.test.ts | 116 ++++++++++++++++++ 7 files changed, 165 insertions(+), 22 deletions(-) diff --git a/changelog.d/208-handler-failure-replies.changed.md b/changelog.d/208-handler-failure-replies.changed.md index ea5332c49..e4b5c6159 100644 --- a/changelog.d/208-handler-failure-replies.changed.md +++ b/changelog.d/208-handler-failure-replies.changed.md @@ -1 +1 @@ -- **MCPL server authors:** when the host's handler for a server's request fails, the server's answer no longer carries the handler's own message (#208). 0.22.x answered a failed `push/event`, `inference/request`, `channels/register`, `channels/changed`, `channels/incoming` or `host/command` with `-32603` and the handler's message, or, once the host was stopping, with `-32000` and "the host stopped while handling this request" followed by the handler's message in parentheses. These requests now get `-32603` "Internal error", or, while the host is stopping, `-32000` "the host stopped while handling this request" alone. A handler's message can name the host's internal paths and errors, and the server may be a third party's. A server that shows or parses the handler's text from these replies no longer receives it: the host writes it to stderr and to its `mcpl:request-handler-error` trace instead. +- **MCPL server authors:** when the host's handler for a server's request throws or rejects before it has answered, the server's answer no longer carries the handler's own message (#208). 0.22.x answered such a failure of a `push/event`, `inference/request`, `channels/register`, `channels/changed`, `channels/incoming` or `host/command` handler with `-32603` and the handler's message, or, once the host was stopping, with `-32000` and "the host stopped while handling this request" followed by the handler's message in parentheses. These requests now get `-32603` "Internal error", or, while the host is stopping, `-32000` "the host stopped while handling this request" alone. A handler's message can name the host's internal paths and errors, and the server may be a third party's. A server that shows or parses the handler's text from these replies no longer receives it: the host writes it to stderr and to its `mcpl:request-handler-error` trace instead. A handler that answers a failure itself still sends its own code and message, as before: `inference/request` when the inference fails, and `push/event` when a coalesced push fails. diff --git a/changelog.d/208-inbound-handler-containment.fixed.md b/changelog.d/208-inbound-handler-containment.fixed.md index 276e6f10d..87d2d2cc9 100644 --- a/changelog.d/208-inbound-handler-containment.fixed.md +++ b/changelog.d/208-inbound-handler-containment.fixed.md @@ -1,10 +1,12 @@ -- MCPL's inbound handlers are contained at one boundary, the framework's wiring, for every request and notification it handles (#186). The `channels/list`, `tools/observe` and `model/info` requests and the `notifications/tools/list_changed` notification are newly contained: a throw or rejection of theirs is reported as the other handlers' are. `mcpl/manifestChanged` goes through the same boundary: a manifest fetch that fails is still caught and logged inside its handler, as in 0.22.x, and anything else its handler throws or rejects with is now reported as the others' are, where 0.22.x only logged it. The six requests whose handler failures 0.22.0 already answered (`push/event`, `inference/request`, `channels/register`, `channels/changed`, `channels/incoming` and `host/command`) go through the same boundary now. On a connection the framework wires, no handler's throw or rejection ends the host, and a rejection reason that can't be printed is reported as one that can. +- MCPL's inbound handlers are contained at one boundary, the framework's wiring, for every request and notification it handles (#186). The `channels/list`, `tools/observe` and `model/info` requests and the `notifications/tools/list_changed` notification are newly contained: a throw or rejection of theirs is reported as the other handlers' are. `mcpl/manifestChanged` goes through the same boundary, and so does a refresh its handler schedules for later, after the fetch floor or behind a fetch in flight: a manifest fetch that fails is still caught and logged inside the handler, as in 0.22.x, and anything else the handler throws or rejects with is now reported as the others' are. 0.22.x logged that from an announcement's own run, and dropped it from a scheduled re-run. The six requests whose handler failures 0.22.0 already answered (`push/event`, `inference/request`, `channels/register`, `channels/changed`, `channels/incoming` and `host/command`) go through the same boundary now. On a connection the framework wires, no handler's throw or rejection ends the host, and a rejection reason that can't be printed is reported as one that can. - **A request whose handler fails is answered `-32603` "Internal error",** or, while the host is stopping, `-32000` "the host stopped while handling this request". The handler's error stays in the trace and on stderr. For the six that 0.22.x answered with the handler's own message, that's a change, described under Changed. A request is answered this way when its handler fails before any reply bytes may have gone out: it attempted no reply, or its reply failed while encoding. A transport write that threw may have sent part of a reply, so it isn't followed by another. A notification gets no reply. - - **Each failure emits a new trace, `mcpl:request-handler-error`, and a stderr line.** Its `error` is the handler's own error. A reply write that failed before it is `responseFailure`, and a reply the handler tried to send after one was already attempted is `refusedResponse` (a result, or an error with its code and message): refused, never written. `responseError` is the internal-error reply's own failure, which the stderr line prints too. `requestId` is the request's id when that is a number, and a string id bounded as the trace's other texts are. A request form of `mcpl/manifestChanged` or `notifications/tools/list_changed` has none, since it's answered as it's admitted and then handled as the notification is. A failure of the Discord awareness accounting that a tools-list refresh waits on is global: it's reported as `mcpl:server-error` for every connection, as each is recycled. + - **Each failure emits a new trace, `mcpl:request-handler-error`, and a stderr line.** Its `error` is the handler's own error, or, for a handler that resolved, the reply write it swallowed, or a note that it tried to answer a second time. A reply write that failed before it is `responseFailure`, and a reply the handler tried to send after one was already attempted is `refusedResponse` (a result, or an error with its code and message): refused, never written. `responseError` is the internal-error reply's own failure, which the stderr line prints too. `requestId` is the request's id when that is a number, and a string id bounded as the trace's other texts are. A request form of `mcpl/manifestChanged` or `notifications/tools/list_changed` has none, since it's answered as it's admitted and then handled as the notification is. A failure of the Discord awareness accounting that a tools-list refresh waits on is global: it's reported as `mcpl:server-error` for every connection, and then each connection is recycled, unless the host is stopping. - **A request whose method isn't a string,** or isn't one the host handles, or whose event nothing on the host listens for, is answered `-32601` "Method not found", with a stderr line. A notification nobody handles is ignored. - **`mcpl/manifestChanged` and `notifications/tools/list_changed` sent as requests,** with an id, are answered with an empty result when the connection admits them, which waits while their plane is paused, if something on the host listens for them, as the framework's wiring always does, and are handled as the notification is. - **Non-object JSON** (`null`, `42`, `"hi"`, `true`) is ignored before property access, in the handshake reader and in routing, as a non-JSON line already is. One such line from a server used to end the host (#217). - - **An `initialize` error whose `message` has no string form,** such as `{"toString":1}`, refuses the connect, with `initialize error: [object Object]`. 0.22.x converted that message after the handshake had stopped listening and cleared its timeout, so the conversion's throw escaped as an uncaught exception, which ends a host that doesn't catch them, and the connect never settled. An `initialize` error's message is now bounded too, as the framework's other errors bound a server's text: control, format and line-separator characters become `?`, and it's cut at 200 code points. + - **An `initialize` error whose `message` has no string form,** such as `{"toString":1}`, refuses the connect, with `initialize error: [object Object]`. 0.22.x converted that message after the handshake had stopped listening and cleared its timeout, so the conversion's throw escaped as an uncaught exception, which ends a host that doesn't catch them, and the connect never settled. + - **An error answer to a request the host sent,** such as its manifest fetch, settles that request whatever its `code` and `message` are. 0.22.x's conversion of them threw out of the transport's line listener in the same way. + - **A server's text in these errors is bounded:** in an `initialize` error's message and in an error answer's, control, format and line-separator characters become `?`, and it's cut at 200 code points. - **A synchronous failure while dispatching an inbound message**, buffered plane flushes and a server's stderr lines included, is reported without interrupting later messages. Its `mcpl:server-error` names the event or method, the request id, and the cause. - **A response that can't be encoded** says why in its error (a BigInt, a circular structure, a throwing `toJSON`). - **A reply to a connection that has already closed** is dropped without a trace, on either transport, as before. diff --git a/src/framework.ts b/src/framework.ts index eeacec5ea..3d6d0e665 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -16877,7 +16877,7 @@ export class AgentFramework { const cur = this.manifestRefreshState.get(connection.id); if (cur?.queued && !cur.inFlight) { cur.queued = false; - void this.handleManifestChanged(connection, announce).catch(() => {}); + this.rerunManifestRefresh(connection, announce); } }, AgentFramework.MANIFEST_FETCH_FLOOR_MS - sinceLast).unref?.(); } @@ -17022,12 +17022,24 @@ export class AgentFramework { st.lastCompletedAt = Date.now(); if (st.queued) { st.queued = false; - setTimeout(() => { void this.handleManifestChanged(connection, announce).catch(() => {}); }, + setTimeout(() => { this.rerunManifestRefresh(connection, announce); }, AgentFramework.MANIFEST_FETCH_FLOOR_MS).unref?.(); } } } + /** A refresh handleManifestChanged schedules for itself, after the fetch + * floor or behind a fetch in flight. It's contained as the announcement's + * own run is, so a failure other than its fetch's, which the fetch's own + * catch logs, emits mcpl:request-handler-error and a stderr line. */ + private rerunManifestRefresh( + connection: McplServerConnection, + announce: { revision?: string; domains?: string[] }, + ): void { + this.containedInboundHandler(connection.id, 'manifest-changed', + (params: { revision?: string; domains?: string[] }) => this.handleManifestChanged(connection, params))(announce); + } + /** * (Re-)establish a server's host-side MCPL registration: feature-set state * (plus the featureSets/update sent to the server), scope patterns, and @@ -17715,7 +17727,8 @@ export class AgentFramework { // A notification, contained like every inbound handler. A manifest fetch // that fails is caught and logged inside handleManifestChanged, and the // previous manifest stands (§17.5); anything else it throws or rejects - // with emits mcpl:request-handler-error. + // with emits mcpl:request-handler-error, in this run or in a re-run it + // schedules (rerunManifestRefresh). onRequest('manifest-changed', (params: { revision?: string; domains?: string[] }) => this.handleManifestChanged(connection, params)); diff --git a/src/mcpl/errors.ts b/src/mcpl/errors.ts index 941fa1b97..0d672d7b5 100644 --- a/src/mcpl/errors.ts +++ b/src/mcpl/errors.ts @@ -57,11 +57,11 @@ export function describeCause(cause: unknown): string { /** Text a server or a handler chose, made fit for an error's message or a * trace field: control and format characters replaced (C0 and C1 controls, * bidi and other \p{Cf} characters, and the line and paragraph separators - * some log readers break lines on), and cut to `max` code points. Those are - * read as data, by a trace's or an error listener's reader, so the text is - * bounded rather than escaped. A log line prints text a server or a handler - * chose through log-value instead (logValue for a field, logDetail for the - * free text that ends the line). */ + * some log readers break lines on), and cut after `max` code points, with + * "…" marking the cut. Those are read as data, by a trace's or an error + * listener's reader, so the text is bounded rather than escaped. A log line + * prints text a server or a handler chose through log-value instead + * (logValue for a field, logDetail for the free text that ends the line). */ export function boundedForLog(text: string, max = 200): string { const points = [...text.replace(/[\p{Cc}\p{Cf}\u2028\u2029]/gu, '?')]; return points.length > max ? `${points.slice(0, max).join('')}…` : points.join(''); diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index a01b43f41..2a3141383 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -643,9 +643,10 @@ export class McplServerConnection extends EventEmitter { // describeCause rather than the template's own conversion: the // server's `message` can be any JSON value, and one that has no // primitive form, such as {"toString":1}, would throw out of this - // listener after cleanup() has cleared the timeout, leaving the - // handshake to wait forever. It's the server's text in an error's - // message, so it's bounded as errors.ts says. + // listener after cleanup() has cleared the timeout: an uncaught + // exception, which ends a host that doesn't catch them, and a + // handshake left to wait forever. It's the server's text in an + // error's message, so it's bounded as errors.ts says. reject(new Error(`MCPL server "${config.id}" initialize error: ${boundedForLog(describeCause(msg.error.message))}`)); return; } @@ -1438,8 +1439,14 @@ export class McplServerConnection extends EventEmitter { if (pending.timer) clearTimeout(pending.timer); if (response.error) { + // describeCause rather than the template's own conversion, as for an + // initialize error: the server's code and message can be any JSON + // value, and one with no primitive form, such as {"toString":1}, would + // throw here, after the request has left pendingRequests and its timer + // is cleared, so it would never settle. The message is the server's + // text, so it's bounded as errors.ts says. pending.reject( - new Error(`MCPL server "${this.id}" returned error for ${pending.method}: [${response.error.code}] ${response.error.message}`), + new Error(`MCPL server "${this.id}" returned error for ${pending.method}: [${describeCause(response.error.code)}] ${boundedForLog(describeCause(response.error.message))}`), ); } else { pending.resolve(response.result); diff --git a/src/types/trace.ts b/src/types/trace.ts index 21aaf59b9..948a16ea4 100644 --- a/src/types/trace.ts +++ b/src/types/trace.ts @@ -571,7 +571,9 @@ export type TraceEvent = hint: string; }) | (TraceEventBase & { - /** An inbound RPC request/notification handler threw or rejected. */ + /** An inbound RPC request/notification handler failed: it threw or + * rejected, or it resolved after a reply write of its own failed or + * after it tried to answer a second time. */ type: 'mcpl:request-handler-error'; serverId: string; /** Connection event name, e.g. channels-register or push-event. */ @@ -580,14 +582,17 @@ export type TraceEvent = * other texts here are: control, format and line-separator characters * replaced, and cut at 500 code points. Absent for a notification, for * a request whose id is neither, and for a request form of - * mcpl/manifestChanged or tools/list_changed, which is answered as - * it's admitted and then handled as the notification is. */ + * mcpl/manifestChanged or notifications/tools/list_changed, which is + * answered as it's admitted and then handled as the notification is. */ requestId?: string | number; /** Whether the handler's own reply may have reached the server: true * once it attempted one, unless the attempt failed while encoding, - * before any bytes were written. When it's false and a responder was - * given, the host answered -32603 "Internal error" itself, or -32000 - * while it stops, unless `responseError` says that reply failed. */ + * before any bytes were written. When it's false and the responder + * can send an error reply (it has respondError), the host sent -32603 + * "Internal error" itself, or -32000 while it stops, unless + * `responseError` says that reply failed. A connection that has + * already closed drops that reply, as it drops every reply, with no + * `responseError`. */ responseAttempted: boolean; /** The handler's own error when it threw or rejected; otherwise the * reply write that failed and that it swallowed, or that it tried to diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index 6d0c17fa2..8fd15b01a 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -667,6 +667,16 @@ test('a manifest fetch that fails is only logged, as in 0.22.x: the previous man const errors = t.mock.method(console, 'error', () => {}); framework.manifestRefreshState = new Map(); framework.mcplServerConfigs = new Map([['srv', { id: 'srv' }]]); + // What a refresh changes: the grant, the feature sets, and the manifest it + // adopts with its tracking. + const changed: string[] = []; + const previous = { + capabilities: { version: '0.5' }, + manifestState: { lastValidatedRevision: 'r1', lastFetchedAt: 1 }, + droppedCapabilities: new Set(), + }; + Object.assign(connection, previous, { establishGrant() { changed.push('grant'); } }); + framework.featureSetManager = { initializeServer() { changed.push('feature sets'); } }; (connection as any).sendManifestRequest = async () => { throw new Error('the server is gone'); }; connection.emit('manifest-changed', { revision: 'r2' }); await settle(); @@ -674,6 +684,101 @@ test('a manifest fetch that fails is only logged, as in 0.22.x: the previous man assert.deepEqual(errors.mock.calls.map((call) => String(call.arguments[0])), [ '[mcpl] srv: mcpl/manifest fetch failed — previous manifest stands (§17.5): the server is gone', ]); + assert.deepEqual(changed, []); + assert.equal((connection as any).capabilities, previous.capabilities); + assert.deepEqual((connection as any).manifestState, { lastValidatedRevision: 'r1', lastFetchedAt: 1 }); + assert.equal((connection as any).droppedCapabilities, previous.droppedCapabilities); +}); + +const manifestFetchFloorMs = (AgentFramework as any).MANIFEST_FETCH_FLOOR_MS as number; + +test('a manifest refresh deferred by the fetch floor reports a failure other than its fetch\'s, as the announcement\'s own run does', async (t) => { + t.mock.timers.enable({ apis: ['setTimeout', 'Date'], now: 100_000 }); + const { framework, connection, traces } = harness(); + const lines: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }); + // The last refresh completed just now, so this announcement waits out the floor. + framework.manifestRefreshState = new Map([['srv', { inFlight: false, queued: false, lastCompletedAt: 100_000 }]]); + framework.mcplServerConfigs = { get() { throw new Error('the deferred refresh failed before its fetch'); } }; + connection.emit('manifest-changed', { revision: 'r2' }); + await settle(); + assert.deepEqual(failureTraces(traces), [], 'nothing has run yet'); + t.mock.timers.tick(manifestFetchFloorMs); + await settle(); + const failures = failureTraces(traces); + assert.equal(failures.length, 1); + assert.equal(failures[0].event, 'manifest-changed'); + assert.equal(failures[0].requestId, undefined); + assert.match(failures[0].error, /the deferred refresh failed before its fetch/); + assert.ok(lines.includes('[mcpl] srv manifest-changed handler failed: "the deferred refresh failed before its fetch"'), lines.join('\n')); +}); + +test('a manifest refresh queued behind a fetch in flight reports a failure other than its fetch\'s, after the first run\'s failed fetch is only logged', async (t) => { + t.mock.timers.enable({ apis: ['setTimeout', 'Date'], now: 100_000 }); + const { framework, connection, traces } = harness(); + const lines: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }); + framework.manifestRefreshState = new Map(); + let lookups = 0; + framework.mcplServerConfigs = { + get() { + lookups++; + if (lookups > 1) throw new Error('the queued refresh failed before its fetch'); + return { id: 'srv' }; + }, + }; + let failFetch!: (error: Error) => void; + (connection as any).sendManifestRequest = () => new Promise((_resolve, reject) => { failFetch = reject; }); + connection.emit('manifest-changed', { revision: 'r2' }); + // This one arrives while the first fetch is in flight, so it queues a re-run. + connection.emit('manifest-changed', { revision: 'r3' }); + failFetch(new Error('the server is gone')); + await settle(); + assert.deepEqual(failureTraces(traces), [], 'a failed fetch is only logged'); + assert.deepEqual(lines, ['[mcpl] srv: mcpl/manifest fetch failed — previous manifest stands (§17.5): the server is gone']); + t.mock.timers.tick(manifestFetchFloorMs); + await settle(); + const failures = failureTraces(traces); + assert.equal(failures.length, 1); + assert.equal(failures[0].event, 'manifest-changed'); + assert.match(failures[0].error, /the queued refresh failed before its fetch/); + assert.equal(lookups, 2); + assert.ok(lines.includes('[mcpl] srv manifest-changed handler failed: "the queued refresh failed before its fetch"'), lines.join('\n')); +}); + +test('a server\'s error answer that no template can convert settles the host\'s request: a manifest fetch answered so is logged and leaves no refresh in flight', async (t) => { + const transport = new ResponseTransport(); + const connection = new (McplServerConnection as any)('srv', null, transport) as McplServerConnection; + connection.establishGrant(new CapabilityGrant(new Set(ALL_CAPABILITY_PATHS), [])); + const { framework, traces } = harness(connection); + connection.ready(); + const lines: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }); + framework.manifestRefreshState = new Map(); + framework.mcplServerConfigs = new Map([['srv', { id: 'srv' }]]); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', method: 'mcpl/manifestChanged', params: { revision: 'r2' } })); + const fetch = JSON.parse(transport.lines.at(-1)!); + assert.equal(fetch.method, 'mcpl/manifest'); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: fetch.id, error: { code: { toString: 1 }, message: { toString: 1 } } })); + await settle(); + assert.deepEqual(lines, [ + '[mcpl] srv: mcpl/manifest fetch failed — previous manifest stands (§17.5): MCPL server "srv" returned error for mcpl/manifest: [[object Object]] [object Object]', + ]); + assert.deepEqual(traces.filter((e) => e.type === 'mcpl:server-error' || e.type === 'mcpl:request-handler-error'), []); + assert.equal(framework.manifestRefreshState.get('srv').inFlight, false); +}); + +test('a server\'s error message in the error a host request rejects with is bounded: control, format and line characters replaced, and cut at 200 code points', 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.ready(); + const fetched = connection.sendManifestRequest(); + const fetch = JSON.parse(transport.lines.at(-1)!); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: fetch.id, error: { code: -32000, message: `line one\nline two\u202e${'x'.repeat(300)}` } })); + await assert.rejects(fetched, { + message: `MCPL server "srv" returned error for mcpl/manifest: [-32000] line one?line two?${'x'.repeat(182)}…`, + }); }); test('mcpl/manifestChanged and tools/list_changed sent as requests are answered with an empty result as they are admitted, whatever their handlers do', async (t) => { @@ -753,6 +858,17 @@ test('a request-form mcpl/manifestChanged held while the data plane is paused is assert.deepEqual(refreshed, [{ revision: 'r2' }]); }); +test('a request-form notifications/tools/list_changed is answered at once while the data plane is paused, since the control plane carries it', (t) => { + t.mock.method(console, 'error', () => {}); + const { connection, transport } = stagedHarness(); + const seen: unknown[] = []; + connection.removeAllListeners('tools-list-changed'); + connection.on('tools-list-changed', (params: unknown, responder?: unknown) => { seen.push({ params, responder }); }); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: 4, method: 'notifications/tools/list_changed', params: { sequence: 9 } })); + assert.deepEqual(transport.lines.map((line) => JSON.parse(line)), [{ jsonrpc: '2.0', id: 4, result: {} }]); + assert.deepEqual(seen, [{ params: { sequence: 9 }, responder: undefined }]); +}); + test('a request form held at the buffer cap is kept, and answered when its plane opens, while the oldest notifications are dropped', async (t) => { const errors = t.mock.method(console, 'error', () => {}); const { connection, transport, refreshed } = stagedHarness(); From d9560abd6868a92e9a6483e90a2ab3b06812b35e Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Sat, 17 Oct 2026 02:11:30 -0400 Subject: [PATCH 20/29] test(mcpl): pin what a handler that answers its own failure sends The Changed entry now says that a handler which answers a failure itself still sends its own code and message, as 0.22.x did: inference/request when the inference fails, and push/event when a coalesced push fails. The wrapper never sees those failures, since the inference router and the push handler catch them. Two assertions pin that sentence: - a real InferenceRouter whose completion throws answers -32603 with the thrown message, and no mcpl:request-handler-error is emitted; - the parity test's coalesced push, whose coalescer throws 'coalescer_broke', is answered with that message, beside the -32603 it already asserted. Tests only. Co-Authored-By: Claude Opus 5.5 --- test/mcpl-coalesced-content-parity.test.ts | 2 ++ test/mcpl-request-handler-errors.test.ts | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/test/mcpl-coalesced-content-parity.test.ts b/test/mcpl-coalesced-content-parity.test.ts index 4b8612b51..0c54823b6 100644 --- a/test/mcpl-coalesced-content-parity.test.ts +++ b/test/mcpl-coalesced-content-parity.test.ts @@ -145,6 +145,8 @@ test('#266: a coalescer failure is traced as the host\'s (coalesce-failed), as t assert.deepEqual(channel.result.results[0], { messageId: 'fine', accepted: false, reason: 'coalescer_broke' }, 'the wire calls it a failure'); const push = await f.send('push/event', f.params('p1', 'fine_push', { initial: true })); assert.equal(push.error?.code, -32603, 'the wire calls it a failure'); + // The push handler answers a failure itself, with its own message, as in 0.22.x. + assert.equal(push.error?.message, 'coalescer_broke'); assert.deepEqual( rejected.map(({ type, reason, detail }) => ({ type, reason, detail })), [ diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index 8fd15b01a..9c1c29c79 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -207,6 +207,25 @@ test('real inference handler cannot turn a failed result write into a second res assert.equal(failures[0].responseAttempted, true); }); +test('a real inference handler whose inference fails answers -32603 with the failure\'s own message, as in 0.22.x, and no handler failure is reported', async () => { + const { framework, connection, traces } = harness(); + framework.inferenceRouter = new InferenceRouter( + { async complete() { throw new Error('the provider refused the request'); } } as never, + { isInHook: false } as never, + { validateInbound() {} } as never, + null, + (event) => traces.push(event), + ); + const reply = responder(); + connection.emit('inference-request', { + featureSet: 'test', messages: [{ role: 'user', content: 'hello' }], + }, reply); + await settle(); + // The router answers the failure itself, so the wrapper sees none. + assert.deepEqual(reply.errors, [{ code: -32603, message: 'the provider refused the request', data: undefined }]); + assert.deepEqual(failureTraces(traces), []); +}); + test('a swallowed response-write failure still produces a diagnostic', async () => { const { framework, connection, traces } = harness(); framework.channelRegistry = { From 01ed39bb601a5712aceade27ced1a48c6ab56c51 Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Sat, 17 Oct 2026 02:33:51 -0400 Subject: [PATCH 21/29] fix(mcpl): a reply counts once its write returns; drop the encoding-failure class A first-principles pass over this PR (each part: what it buys, what it costs to keep, whether something simpler does the same) found that the encoding-failure distinction tells apart a case neither transport produces, and the repository owner cut it: - containedInboundHandler told a reply that failed while encoding (McplResponseSerializationError, after which the internal-error reply could follow) from a transport write that threw (which might have sent bytes, so nothing followed). Neither transport's write throws after writing. StdioTransport.writeLine is a stream write, which reports a closed pipe asynchronously; a write to a destroyed stream, an ended one or an exited child's stdin returns false on Node 24 and never throws. WebSocketTransport.writeLine sends only while the socket is open, and ws's send then ends in a socket write. - So main's rule comes back: a reply counts once its write returns, and a write that throws has sent nothing. The comment on responseAttempted names the transport assumption, as answerOnce's did on main. - McplResponseSerializationError and serializeResponse go; sendResponse and sendErrorResponse write JSON.stringify(response) as main does. An encoding failure's error is JSON.stringify's own ("Do not know how to serialize a BigInt", "Converting circular structure to JSON", or what a throwing toJSON threw), without the "MCPL response could not be serialized:" prefix. - The trace's responseAttempted doc says what it now means: whether the handler wrote a reply. - The Fixed fragment's sentence about a transport write that threw, its bullet about an encoding failure's message, and its last bullet, which described no change (a reply to a closed connection dropped as before), go. The report's fallback stays: emitTrace logs a trace listener's throw through console.error, which inspects the raw value, so a listener throwing a value whose inspection throws would otherwise throw out of the report and, on the async path, end the host. Tests: the encoding-failure tests assert JSON.stringify's messages; the test of a transport that throws after writing goes with the case, and so do two fake-responder tests that the wire harness's encoding-failure tests cover through the real connection; the real inference router whose result write throws answers with its own error, its only reply; a channels/list answer whose write throws gets one internal-error reply; model-info's error write that throws is followed by one internal-error attempt, which throws too, and neither escapes; the log line's test writes once after a failed write, so its third answer is the refused one; and stop-with-work-in-flight's throwing responder throws without sending, as the connection's would, so its two tests expect the internal-error or stop answer to be tried once more, and no rejection. Co-Authored-By: Claude Opus 5.5 --- .../208-inbound-handler-containment.fixed.md | 4 +- src/framework.ts | 17 +-- src/mcpl/errors.ts | 9 -- src/mcpl/server-connection.ts | 15 +-- src/types/trace.ts | 6 +- test/mcpl-request-handler-errors.test.ts | 109 +++++++----------- test/stop-with-work-in-flight.test.ts | 38 +++--- 7 files changed, 84 insertions(+), 114 deletions(-) diff --git a/changelog.d/208-inbound-handler-containment.fixed.md b/changelog.d/208-inbound-handler-containment.fixed.md index 87d2d2cc9..e14519159 100644 --- a/changelog.d/208-inbound-handler-containment.fixed.md +++ b/changelog.d/208-inbound-handler-containment.fixed.md @@ -1,5 +1,5 @@ - MCPL's inbound handlers are contained at one boundary, the framework's wiring, for every request and notification it handles (#186). The `channels/list`, `tools/observe` and `model/info` requests and the `notifications/tools/list_changed` notification are newly contained: a throw or rejection of theirs is reported as the other handlers' are. `mcpl/manifestChanged` goes through the same boundary, and so does a refresh its handler schedules for later, after the fetch floor or behind a fetch in flight: a manifest fetch that fails is still caught and logged inside the handler, as in 0.22.x, and anything else the handler throws or rejects with is now reported as the others' are. 0.22.x logged that from an announcement's own run, and dropped it from a scheduled re-run. The six requests whose handler failures 0.22.0 already answered (`push/event`, `inference/request`, `channels/register`, `channels/changed`, `channels/incoming` and `host/command`) go through the same boundary now. On a connection the framework wires, no handler's throw or rejection ends the host, and a rejection reason that can't be printed is reported as one that can. - - **A request whose handler fails is answered `-32603` "Internal error",** or, while the host is stopping, `-32000` "the host stopped while handling this request". The handler's error stays in the trace and on stderr. For the six that 0.22.x answered with the handler's own message, that's a change, described under Changed. A request is answered this way when its handler fails before any reply bytes may have gone out: it attempted no reply, or its reply failed while encoding. A transport write that threw may have sent part of a reply, so it isn't followed by another. A notification gets no reply. + - **A request whose handler fails is answered `-32603` "Internal error",** or, while the host is stopping, `-32000` "the host stopped while handling this request". The handler's error stays in the trace and on stderr. For the six that 0.22.x answered with the handler's own message, that's a change, described under Changed. A request is answered this way when its handler fails without having written a reply, which includes a reply whose write threw, such as one that can't be encoded. A notification gets no reply. - **Each failure emits a new trace, `mcpl:request-handler-error`, and a stderr line.** Its `error` is the handler's own error, or, for a handler that resolved, the reply write it swallowed, or a note that it tried to answer a second time. A reply write that failed before it is `responseFailure`, and a reply the handler tried to send after one was already attempted is `refusedResponse` (a result, or an error with its code and message): refused, never written. `responseError` is the internal-error reply's own failure, which the stderr line prints too. `requestId` is the request's id when that is a number, and a string id bounded as the trace's other texts are. A request form of `mcpl/manifestChanged` or `notifications/tools/list_changed` has none, since it's answered as it's admitted and then handled as the notification is. A failure of the Discord awareness accounting that a tools-list refresh waits on is global: it's reported as `mcpl:server-error` for every connection, and then each connection is recycled, unless the host is stopping. - **A request whose method isn't a string,** or isn't one the host handles, or whose event nothing on the host listens for, is answered `-32601` "Method not found", with a stderr line. A notification nobody handles is ignored. - **`mcpl/manifestChanged` and `notifications/tools/list_changed` sent as requests,** with an id, are answered with an empty result when the connection admits them, which waits while their plane is paused, if something on the host listens for them, as the framework's wiring always does, and are handled as the notification is. @@ -8,5 +8,3 @@ - **An error answer to a request the host sent,** such as its manifest fetch, settles that request whatever its `code` and `message` are. 0.22.x's conversion of them threw out of the transport's line listener in the same way. - **A server's text in these errors is bounded:** in an `initialize` error's message and in an error answer's, control, format and line-separator characters become `?`, and it's cut at 200 code points. - **A synchronous failure while dispatching an inbound message**, buffered plane flushes and a server's stderr lines included, is reported without interrupting later messages. Its `mcpl:server-error` names the event or method, the request id, and the cause. - - **A response that can't be encoded** says why in its error (a BigInt, a circular structure, a throwing `toJSON`). - - **A reply to a connection that has already closed** is dropped without a trace, on either transport, as before. diff --git a/src/framework.ts b/src/framework.ts index 3d6d0e665..38de4dba8 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -404,7 +404,7 @@ import { EventGate, formatShadowWarning } from './gate/event-gate.js'; import { UsageTracker, type PersistedUsageState } from './usage/usage-tracker.js'; import type { SessionUsageSnapshot, UsageUpdatedEvent } from './usage/types.js'; import { stderrTailOf, withStderr, type McplConnectionCloseInfo, type McplServerConnection } from './mcpl/server-connection.js'; -import { McplResponseSerializationError, boundedForLog, describeCause } from './mcpl/errors.js'; +import { boundedForLog, describeCause } from './mcpl/errors.js'; import { isWebSocketTransport } from './mcpl/transport.js'; import type { McplServerConfig, @@ -17464,15 +17464,19 @@ export class AgentFramework { handler: (params: T, responder?: InboundResponder) => void | Promise, ): (params: T, responder?: InboundResponder) => void { return (params: T, responder?: InboundResponder) => { - // Whether a reply's bytes may have reached the transport. Only an - // encoding failure proves none did; a transport exception may have - // written part of one, so it can't be retried. + // Whether the handler wrote a reply. A reply counts once its write + // returns: a write that throws has sent nothing, since the connection + // encodes a reply before it writes it, and neither transport's write + // throws after writing (a stream write reports a failure + // asynchronously, and a WebSocket is written only while it's open). A + // transport whose write could throw after sending bytes would break + // this: the internal-error reply would follow part of a reply. let responseAttempted = false; // The first reply write that threw, if any, which a handler may have // swallowed. let writeFailure: unknown; let writeFailed = false; - // A write the handler tried after a reply was attempted. It's refused + // A write the handler tried after a reply was written. It's refused // without throwing, so a handler that answers again in its own catch // and then rethrows reports its own error; what it tried to send is // kept for the report. @@ -17491,17 +17495,16 @@ export class AgentFramework { } return; } - responseAttempted = true; try { write(); } catch (error) { - if (error instanceof McplResponseSerializationError) responseAttempted = false; if (!writeFailed) { writeFailed = true; writeFailure = error; } throw error; } + responseAttempted = true; }; // Only the methods the responder has are wrapped, so a handler that // checks for respondError still finds a responder without one, such diff --git a/src/mcpl/errors.ts b/src/mcpl/errors.ts index 0d672d7b5..fefca380f 100644 --- a/src/mcpl/errors.ts +++ b/src/mcpl/errors.ts @@ -67,15 +67,6 @@ export function boundedForLog(text: string, max = 200): string { return points.length > max ? `${points.slice(0, max).join('')}…` : points.join(''); } -/** Response encoding failed before the transport was given any bytes. Its - * message says why (a BigInt, a circular structure, a throwing toJSON). */ -export class McplResponseSerializationError extends Error { - constructor(cause: unknown) { - super(`MCPL response could not be serialized: ${boundedForLog(describeCause(cause))}`, { cause }); - this.name = 'McplResponseSerializationError'; - } -} - /** * Create a "feature set not enabled" error. * Returned when a server sends a message tagged with a feature set diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index 2a3141383..e74851843 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -14,7 +14,7 @@ import { EventEmitter } from 'node:events'; import { openTransport, type McplTransport, type TransportCloseInfo } from './transport.js'; import { maskNegotiatedCapabilities } from './capability-mask.js'; import { CapabilityGrant, expandAdvertisementShorthand } from './capability-grant.js'; -import { CAPABILITY_DISABLED, McplResponseSerializationError, boundedForLog, describeCause } from './errors.js'; +import { CAPABILITY_DISABLED, boundedForLog, describeCause } from './errors.js'; import { logDetail, logValue } from './log-value.js'; import type { @@ -1453,22 +1453,13 @@ export class McplServerConnection extends EventEmitter { } } - /** Keep definite pre-write encoding failures distinct from transport errors. */ - private serializeResponse(response: JsonRpcResponse): string { - try { - return JSON.stringify(response); - } catch (cause) { - throw new McplResponseSerializationError(cause); - } - } - /** * Send a successful JSON-RPC response back to the server. */ private sendResponse(id: string | number, result: unknown): void { if (this.closed) return; const response: JsonRpcResponse = { jsonrpc: '2.0', id, result }; - this.transport?.writeLine(this.serializeResponse(response)); + this.transport?.writeLine(JSON.stringify(response)); } /** @@ -1481,7 +1472,7 @@ export class McplServerConnection extends EventEmitter { id, error: { code, message, data }, }; - this.transport?.writeLine(this.serializeResponse(response)); + this.transport?.writeLine(JSON.stringify(response)); } // ========================================================================== diff --git a/src/types/trace.ts b/src/types/trace.ts index 948a16ea4..0f2597d44 100644 --- a/src/types/trace.ts +++ b/src/types/trace.ts @@ -585,9 +585,9 @@ export type TraceEvent = * mcpl/manifestChanged or notifications/tools/list_changed, which is * answered as it's admitted and then handled as the notification is. */ requestId?: string | number; - /** Whether the handler's own reply may have reached the server: true - * once it attempted one, unless the attempt failed while encoding, - * before any bytes were written. When it's false and the responder + /** Whether the handler wrote a reply: true once a reply's write + * returned. A write that throws, as one that can't be encoded does, + * sends nothing. When it's false and the responder * can send an error reply (it has respondError), the host sent -32603 * "Internal error" itself, or -32000 while it stops, unless * `responseError` says that reply failed. A connection that has diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index 9c1c29c79..c31f68458 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -11,7 +11,6 @@ import { InferenceRouter } from '../src/mcpl/inference-router.js'; 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 { McplResponseSerializationError } from '../src/mcpl/errors.js'; function harness(connection = Object.assign(new EventEmitter(), { id: 'srv' })) { const traces: any[] = []; @@ -131,17 +130,17 @@ test('a synchronous handler throw is contained and answered', async () => { assert.match(failureTraces(traces)[0].error, /list failed/); }); -test('a throwing success response is not retried as an error response', async () => { +test('a success response whose write throws has sent nothing, so the request gets one internal-error reply', async () => { const { framework, connection, traces } = harness(); framework.channelRegistry = { descriptorsForServer() { return []; } }; const reply = responder(); - let writes = 0; - reply.respond = () => { writes++; throw new Error('response transport failed'); }; + let tries = 0; + reply.respond = () => { tries++; throw new Error('the result could not be encoded'); }; assert.doesNotThrow(() => connection.emit('channels-list', {}, reply)); await settle(); - assert.equal(writes, 1); - assert.equal(reply.errors.length, 0); - assert.equal(failureTraces(traces)[0].responseAttempted, true); + assert.equal(tries, 1); + assert.deepEqual(reply.errors, [{ code: -32603, message: 'Internal error', data: undefined }]); + assert.equal(failureTraces(traces)[0].responseAttempted, false); }); test('a failed error response is traced without an escaping rejection', async () => { @@ -175,7 +174,7 @@ test('inference responders retain request IDs and successful handling stays sync assert.equal(failureTraces(traces).length, 0); }); -test('real inference handler cannot turn a failed result write into a second response', async () => { +test('a real inference handler whose result write throws answers with its own error, its only reply, and the failed write is reported', async () => { const { framework, connection, traces } = harness(); framework.inferenceRouter = new InferenceRouter( { @@ -194,16 +193,19 @@ test('real inference handler cannot turn a failed result write into a second res ); const reply = responder(); let resultWrites = 0; - reply.respond = () => { resultWrites++; throw new Error('partially written result'); }; + // A write that throws has sent nothing, as when a result can't be encoded. + reply.respond = () => { resultWrites++; throw new Error('the result could not be encoded'); }; connection.emit('inference-request', { featureSet: 'test', messages: [{ role: 'user', content: 'hello' }], }, reply); await settle(); assert.equal(resultWrites, 1); - assert.equal(reply.errors.length, 0, 'the inner inference catch must not send another reply'); + // The router's own catch answers, as in 0.22.x, and the wrapper sends no + // internal-error reply after it. + assert.deepEqual(reply.errors, [{ code: -32603, message: 'the result could not be encoded', data: undefined }]); const failures = failureTraces(traces); assert.equal(failures.length, 1); - assert.match(failures[0].error, /partially written result/); + assert.match(failures[0].error, /the result could not be encoded/); assert.equal(failures[0].responseAttempted, true); }); @@ -226,22 +228,6 @@ test('a real inference handler whose inference fails answers -32603 with the fai assert.deepEqual(failureTraces(traces), []); }); -test('a swallowed response-write failure still produces a diagnostic', async () => { - const { framework, connection, traces } = harness(); - framework.channelRegistry = { - async handleRegister(_server: string, _params: unknown, reply: any) { - try { reply.respond({}); } catch { /* Simulate a handler swallowing a transport error. */ } - }, - }; - const reply = responder(); - reply.respond = () => { throw new Error('swallowed write failure'); }; - connection.emit('channels-register', {}, reply); - await settle(); - assert.equal(reply.errors.length, 0); - assert.equal(failureTraces(traces).length, 1); - assert.match(failureTraces(traces)[0].error, /swallowed write failure/); -}); - test('an error response followed by a handler rejection is not answered again', async () => { const { framework, connection, traces } = harness(); framework.channelRegistry = { @@ -259,25 +245,12 @@ test('an error response followed by a handler rejection is not answered again', assert.equal(failureTraces(traces)[0].responseAttempted, true); }); -test('host command response-write failure is traced without a second reply', async () => { - const { framework, connection, traces } = harness(); - framework.handleHostCommand = async () => ({ done: true }); - const reply = responder(); - reply.respond = () => { throw new Error('host response failed'); }; - connection.emit('host-command', {}, reply); - await settle(); - assert.equal(reply.errors.length, 0); - assert.match(failureTraces(traces)[0].error, /host response failed/); -}); - class ResponseTransport extends McplTransport { constructor() { super(); } readonly kind = 'stdio' as const; readonly lines: string[] = []; - failWrite = false; writeLine(line: string): void { this.lines.push(line); - if (this.failWrite) throw new TypeError('transport failed after a partial write'); } async close(): Promise {} } @@ -317,10 +290,10 @@ for (const kind of ['bigint', 'circular', 'throwing toJSON'] as const) { const failures = failureTraces(traces); assert.equal(failures.length, 1); assert.equal(failures[0].responseAttempted, false); - // The encoding failure says why. - assert.match(failures[0].error, kind === 'bigint' ? /could not be serialized: .*BigInt/i - : kind === 'circular' ? /could not be serialized: .*circular/i - : /could not be serialized: snapshot serialization failed/); + // JSON.stringify's own message says why. + assert.match(failures[0].error, kind === 'bigint' ? /BigInt/ + : kind === 'circular' ? /circular structure/ + : /snapshot serialization failed/); if (kind === 'throwing toJSON') assert.equal(calls, 1, 'results are encoded only once'); }); } @@ -351,17 +324,7 @@ test('a swallowed pre-write encoding failure still receives a fallback response' assert.equal(transport.lines.length, 1); assert.equal(JSON.parse(transport.lines[0]!).error.code, -32603); assert.equal(failureTraces(traces)[0].responseAttempted, false); -}); - -test('a transport write that throws remains single-attempt after serialization', async () => { - const { framework, traces, transport, request } = wireHarness(); - framework.handleHostCommand = async () => ({ maintained: true }); - transport.failWrite = true; - request(); - await settle(); - assert.equal(transport.lines.length, 1, 'a possibly partial write must not get a second reply'); - assert.deepEqual(JSON.parse(transport.lines[0]!).result, { maintained: true }); - assert.equal(failureTraces(traces)[0].responseAttempted, true); + assert.match(failureTraces(traces)[0].error, /BigInt/); }); test('post-response failures reach stderr when no trace subscriber is attached', async (t) => { @@ -515,21 +478,23 @@ test('tools-observe: a synchronous failure is answered with one internal error', assert.match(failureTraces(traces)[0].error, /filter store failed/); }); -test('model-info: a throwing error write is traced without escaping or a second write', async () => { +test('model-info: an error write that throws has sent nothing, so one internal-error reply is tried, and neither throw escapes', async () => { const { connection, traces } = harness(); const reply = responder(); - const writes: Array<{ code: number; message: string }> = []; - reply.respondError = (code: number, message: string) => { writes.push({ code, message }); throw new Error('response transport failed'); }; + const tried: Array<{ code: number; message: string }> = []; + reply.respondError = (code: number, message: string) => { tried.push({ code, message }); throw new Error('response transport failed'); }; assert.doesNotThrow(() => connection.emit('model-info', {}, reply)); await settle(); - assert.equal(writes.length, 1); - assert.equal(writes[0]!.code, -32601); - assert.match(writes[0]!.message, /model\/info unavailable/); + assert.equal(tried.length, 2); + assert.equal(tried[0]!.code, -32601); + assert.match(tried[0]!.message, /model\/info unavailable/); + assert.deepEqual(tried[1], { code: -32603, message: 'Internal error' }); const failures = failureTraces(traces); assert.equal(failures.length, 1); - assert.equal(failures[0].responseAttempted, true); + assert.equal(failures[0].responseAttempted, false); assert.match(failures[0].error, /response transport failed/); assert.equal(failures[0].responseFailure, undefined, 'the write failure is the error itself, not repeated'); + assert.match(failures[0].responseError, /response transport failed/); }); test('a method named after an Object.prototype member is not buffered while the planes are paused', () => { @@ -561,7 +526,7 @@ test('a handler that swallows an encoding failure and then fails reports its own const failures = failureTraces(traces); assert.equal(failures.length, 1); assert.match(failures[0].error, /db down/); - assert.match(failures[0].responseFailure, /could not be serialized: .*BigInt/i); + assert.match(failures[0].responseFailure, /BigInt/); assert.equal(failures[0].responseAttempted, false); }); @@ -990,10 +955,10 @@ test('a synchronous handler that swallows an encoding failure is reported and an const writes: unknown[] = []; listener({}, { id: 61, - // Encode as the connection does: an encoding failure is typed, and - // proves no bytes went out. + // Encode before writing, as the connection does, so a reply that can't + // be encoded throws with nothing sent. respond(result: unknown) { - try { JSON.stringify(result); } catch (cause) { throw new McplResponseSerializationError(cause); } + JSON.stringify(result); writes.push(result); }, respondError(code: number, message: string) { writes.push({ code, message }); }, @@ -1075,16 +1040,26 @@ test("the log line prints a failed reply write and a refused answer raw, and onl t.mock.method(console, 'error', (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }); framework.channelRegistry = { async handleRegister(_server: string, _params: unknown, reply: any) { + // The first write throws, sending nothing; the second is written; the + // third is refused. try { reply.respond({ registered: [] }); } catch { /* swallowed */ } + reply.respond({ registered: [] }); reply.respondError(-32602, 'second\u0007answer'); throw new Error('handler\u0007failed'); }, }; const reply = responder(); - reply.respond = () => { throw new Error('write\u0007broke'); }; + const respond = reply.respond; + let first = true; + reply.respond = (result: unknown) => { + if (first) { first = false; throw new Error('write\u0007broke'); } + respond(result); + }; connection.emit('channels-register', {}, reply); await settle(); + assert.equal(reply.errors.length, 0, 'the written answer stands: no internal-error reply follows it'); const failure = failureTraces(traces)[0]; + assert.equal(failure.responseAttempted, true); assert.equal(failure.error, 'handler?failed'); assert.equal(failure.responseFailure, 'write?broke'); assert.deepEqual(failure.refusedResponse, { kind: 'error', code: -32602, message: 'second?answer' }); diff --git a/test/stop-with-work-in-flight.test.ts b/test/stop-with-work-in-flight.test.ts index 454946809..f89a94840 100644 --- a/test/stop-with-work-in-flight.test.ts +++ b/test/stop-with-work-in-flight.test.ts @@ -42,7 +42,6 @@ import type { ToolResult, } from '../src/index.js'; import { AgentFramework } from '../src/index.js'; -import { McplResponseSerializationError } from '../src/mcpl/errors.js'; import { MockMembrane, createMockResponse } from './helpers/mock-membrane.js'; /** Resolve once `cond` holds, polling each turn; fail, naming `what`, after `ms`. */ @@ -523,26 +522,28 @@ describe('stop() with work still in flight', () => { ] as const; /** - * A server's responder, recording each answer. - * - throws: respondError records its answer, then throws (the connection is going). + * A server's responder, recording each answer it sends, and in `tried` + * each error answer asked of it. + * - throws: respondError throws, sending nothing (the connection is going). * - serializes: respond serializes the result before it records it, as the - * connection's sendResponse does, so a result JSON can't hold throws - * McplResponseSerializationError, which proves nothing was sent. + * connection's sendResponse does, so a result JSON can't hold throws, + * with nothing sent. */ function responder(opts: { throws?: boolean; serializes?: boolean } = {}) { const answers: Array<{ result?: unknown; error?: { code: number; message: string } }> = []; + const tried: Array<{ code: number; message: string }> = []; return { id: 7, answers, + tried, respond: (result: unknown) => { - if (opts.serializes) { - try { JSON.stringify(result); } catch (cause) { throw new McplResponseSerializationError(cause); } - } + if (opts.serializes) JSON.stringify(result); answers.push({ result }); }, respondError: (code: number, message: string) => { - answers.push({ error: { code, message } }); + tried.push({ code, message }); if (opts.throws) throw new Error('the connection is gone'); + answers.push({ error: { code, message } }); }, }; } @@ -827,9 +828,16 @@ describe('stop() with work still in flight', () => { fw.pushCoalescer = { isRendering: () => true, suspend: () => {}, flush: () => {} }; const inference = responder({ throws: true }); connection.emit('inference-request', { messages: [] }, inference); - await answered(inference); + // A write that throws sent nothing, so the request is answered once + // more, as any handler's unanswered failure is; that throws too. + await until(() => inference.tried.length >= 2, 'the internal-error answer after the refusal failed'); + await turn(); - assert.deepEqual(inference.answers, [{ error: { code: -32600, message: 'Cannot request inference while push/render is in progress' } }]); + assert.deepEqual(inference.tried, [ + { code: -32600, message: 'Cannot request inference while push/render is in progress' }, + { code: -32603, message: 'Internal error' }, + ]); + assert.deepEqual(inference.answers, []); assert.deepEqual(rejections, []); fw.pushCoalescer = coalescer; } finally { @@ -842,9 +850,13 @@ describe('stop() with work still in flight', () => { await framework.stop(); const push = responder({ throws: true }); connection.emit('push-event', { featureSet: 'chat', eventId: 'e11', payload: { content: [{ type: 'text', text: 'x' }] } }, push); - await answered(push); + // The refusal's write threw, sending nothing, so the request is answered + // once more with the stop wording a failed handler gets; that throws too. + await until(() => push.tried.length >= 2, 'the stop answer after the refusal failed'); + await turn(); - assert.deepEqual(push.answers, [stopping]); + assert.deepEqual(push.tried, [stopping.error, { code: -32000, message: 'the host stopped while handling this request' }]); + assert.deepEqual(push.answers, []); assert.ok(logged.some((line) => line.includes('push/event from srv refused: the host is stopping')), logged.join('\n')); assert.deepEqual(rejections, []); }); From 5c04e14dfa5bdf5567e6787db3a994972af15e2c Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Sat, 17 Oct 2026 04:41:32 -0400 Subject: [PATCH 22/29] test(mcpl): pin the report's fallback, for a trace listener whose throw can't be logged The report of a handler's failure keeps an outer catch whose fallback prints one line, " handler failed: ... (and reporting it failed too: ...)". The report's own texts can't throw, but emitTrace logs a trace listener's throw with console.error('Trace listener error:', error), which inspects the raw value, so a listener that throws a value whose inspection throws makes emitTrace throw, and the report with it. Without the fallback that throw would leave the report, and on the asynchronous path become an unhandled rejection: a way to end the host that this PR's report would itself add. No test made the fallback run. The new test registers a second trace listener that throws such a value, with console.error mocked to inspect its non-string arguments as the real one does, and a handler whose own error also can't go in a template. It asserts no unhandled rejection, the request's one "Internal error" reply, the trace reaching the listener before the throwing one, and exactly one fallback line naming the handler's error and the report's, converted rather than printed raw. Co-Authored-By: Claude Opus 5.5 --- test/mcpl-request-handler-errors.test.ts | 37 ++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index c31f68458..8099af74e 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -1009,6 +1009,43 @@ test('a rejection no printer can inspect still reaches the trace first', async ( } }); +test("a trace listener that throws a value no printer can inspect leaves the report's fallback line, and nothing escapes", async (t) => { + const { framework, connection, traces } = harness(); + // Print as console.error does: an argument that isn't a string goes + // through util.inspect, so logging the listener's throw throws too. + const logged: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { + logged.push(args.map((arg) => (typeof arg === 'string' ? arg : inspect(arg))).join(' ')); + }); + const unhandled: unknown[] = []; + const onUnhandled = (reason: unknown) => { unhandled.push(reason); }; + process.on('unhandledRejection', onUnhandled); + const hostile = { [inspect.custom]() { throw new Error('inspect refused'); } }; + const off = framework.onTrace(() => { throw hostile; }); + // The handler's own error can't be put in a template either, so the + // fallback line must convert it as the report does. + const primary = Object.assign(new Error('register failed'), { + [Symbol.toPrimitive]() { throw new Error('toPrimitive refused'); }, + }); + try { + framework.channelRegistry = { async handleRegister() { throw primary; } }; + const reply = responder(); + connection.emit('channels-register', {}, reply); + await settle(); + await settle(); + assert.deepEqual(unhandled, []); + assert.deepEqual(reply.errors, [{ code: -32603, message: 'Internal error', data: undefined }]); + assert.equal(failureTraces(traces).length, 1, 'the listener before the throwing one still has the trace'); + const fallback = logged.filter((line) => line.includes('(and reporting it failed too:')); + assert.equal(fallback.length, 1, logged.join('\n')); + assert.match(fallback[0], /^\[mcpl\] srv channels-register handler failed: .*register failed.* \(and reporting it failed too: .*inspect refused/); + assert.ok(!fallback[0].includes('toPrimitive refused'), fallback[0]); + } finally { + off(); + process.off('unhandledRejection', onUnhandled); + } +}); + test('a request for an event nothing on the host listens for is answered -32601', (t) => { const { connection, transport } = wireHarness(); const logged: string[] = []; From f6675c4a56344beaec962ac0323cc7a6cc8924ba Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Sat, 17 Oct 2026 06:25:51 -0400 Subject: [PATCH 23/29] fix(mcpl): a trace listener's error can't stop emitTrace, so the report needs no fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The internal review of b838f09e..5c04e14d raised 18 issues; the issue numbers below are its. The repository owner decided I2. Code: - emitTrace (I2): its catch logged a listener's throw with console.error('Trace listener error:', error), which inspects the value. A value whose inspection throws, such as one with a throwing util.inspect.custom, made that call throw too, and the throw left the loop: the listeners after it missed the event, and the code that emitted the trace got the throw. The catch now prints the value's describeCause text when inspecting it throws. Every trace event goes through this catch; only a listener error whose inspection throws is handled differently. - The handler-failure report's fallback catch is gone, and with it the fallback stderr line, which printed less than the report's own line (I9). It guarded only that throw out of emitTrace: the report's other calls are describeCause, which never throws, string bounds and escapes, and the internal-error reply's send, which is caught. - An error answer's code (I1) goes through boundedForLog, as its message does: a server can send a string code, and the fetch-failure log line prints the request's error message as it is. Text: - I1: the Fixed fragment says an error answer's code and message are bounded. - I7: responseAttempted's doc says that on a connection that had already closed, a reply write that returned dropped the reply. - I10: rerunManifestRefresh's doc separates the fetch's own failure, logged by its catch, from the others, which the containment reports. - I14: "cut after N code points, with … marking the cut", as errors.ts says, in the trace doc, the Fixed fragment and a test's name. - I17: describeCause's doc says it converts a value a server sent too. - A Fixed bullet for emitTrace's catch. Tests: - I2: a listener after one that throws a value no printer can inspect still gets the trace; that error is logged as text, the handler's failure line follows, one "Internal error" reply goes out, and nothing rejects unhandled. It replaces 5c04e14d's test of the fallback. - I1: a string code with a line break and over 200 code points is bounded in the error a host request rejects with. - I6: a throw from the wired tools/list_changed handler is contained, traced and logged; no test made that handler throw before. Co-Authored-By: Claude Opus 5.5 --- .../208-inbound-handler-containment.fixed.md | 3 +- src/framework.ts | 137 +++++++++--------- src/mcpl/errors.ts | 3 +- src/mcpl/server-connection.ts | 7 +- src/types/trace.ts | 17 ++- test/mcpl-request-handler-errors.test.ts | 48 ++++-- 6 files changed, 125 insertions(+), 90 deletions(-) diff --git a/changelog.d/208-inbound-handler-containment.fixed.md b/changelog.d/208-inbound-handler-containment.fixed.md index e14519159..de26a2792 100644 --- a/changelog.d/208-inbound-handler-containment.fixed.md +++ b/changelog.d/208-inbound-handler-containment.fixed.md @@ -6,5 +6,6 @@ - **Non-object JSON** (`null`, `42`, `"hi"`, `true`) is ignored before property access, in the handshake reader and in routing, as a non-JSON line already is. One such line from a server used to end the host (#217). - **An `initialize` error whose `message` has no string form,** such as `{"toString":1}`, refuses the connect, with `initialize error: [object Object]`. 0.22.x converted that message after the handshake had stopped listening and cleared its timeout, so the conversion's throw escaped as an uncaught exception, which ends a host that doesn't catch them, and the connect never settled. - **An error answer to a request the host sent,** such as its manifest fetch, settles that request whatever its `code` and `message` are. 0.22.x's conversion of them threw out of the transport's line listener in the same way. - - **A server's text in these errors is bounded:** in an `initialize` error's message and in an error answer's, control, format and line-separator characters become `?`, and it's cut at 200 code points. + - **A server's text in these errors is bounded:** in an `initialize` error's message and in an error answer's code and message, control, format and line-separator characters become `?`, and each is cut after 200 code points, with `…` marking the cut. - **A synchronous failure while dispatching an inbound message**, buffered plane flushes and a server's stderr lines included, is reported without interrupting later messages. Its `mcpl:server-error` names the event or method, the request id, and the cause. +- A trace listener that throws a value `console.error` can't print, such as one whose `util.inspect.custom` throws, no longer keeps the listeners after it from getting the event, or throws out at the code that emitted it: its error is printed as text instead. diff --git a/src/framework.ts b/src/framework.ts index 38de4dba8..e7465e52d 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -15582,7 +15582,16 @@ export class AgentFramework { try { listener(traceEvent); } catch (error) { - console.error('Trace listener error:', error); + // console.error inspects the value, and inspecting can throw (a + // throwing util.inspect.custom, say). That throw would leave this + // loop, so the listeners after this one would miss the event and the + // code that emitted it would get the throw. describeCause never + // throws, and a template string isn't inspected. + try { + console.error('Trace listener error:', error); + } catch { + console.error(`Trace listener error: ${describeCause(error)}`); + } } } } @@ -17030,8 +17039,9 @@ export class AgentFramework { /** A refresh handleManifestChanged schedules for itself, after the fetch * floor or behind a fetch in flight. It's contained as the announcement's - * own run is, so a failure other than its fetch's, which the fetch's own - * catch logs, emits mcpl:request-handler-error and a stderr line. */ + * own run is. Its fetch's own failure is logged by the fetch's catch, as + * in 0.22.x; any other failure emits mcpl:request-handler-error and a + * stderr line through the containment. */ private rerunManifestRefresh( connection: McplServerConnection, announce: { revision?: string; domains?: string[] }, @@ -17524,78 +17534,71 @@ export class AgentFramework { // rejected, else the reply write it swallowed, else the reply it // tried to send twice. Nothing in here may throw: on the async path // it runs as a rejection handler, and an exception there would be an - // unhandled rejection, which ends the host. + // unhandled rejection, which ends the host. So every text is converted + // with describeCause, the internal-error reply's send is caught, and + // emitTrace's own catch can't throw. const report = (handlerError: { error: unknown } | undefined): void => { const primary = handlerError ? handlerError.error : writeFailed ? writeFailure : new Error('the handler tried to answer a second time after its reply'); reported = true; - try { - const message = describeCause(primary); - // What the handler's own writes did, before any internal-error - // reply sent here. - const handlerResponseAttempted = responseAttempted; - // A reply write the handler's own attempt failed, read before the - // internal-error reply below, whose own failure is responseError. - const responseFailure = writeFailed && handlerError && handlerError.error !== writeFailure - ? describeCause(writeFailure) : undefined; - let responseError: string | undefined; - if (tracked && !responseAttempted) { - // The server gets the standard message, not the handler's own - // text, which may name internal paths or errors; the trace and - // stderr carry the detail. Once the host is stopping (its queue - // closed), it gets -32000 with the stop wording alone, the code a - // request turned away at stop gets, so it can send it again to - // the host's next run. Optional chaining: prototype-built - // harnesses leave the queue unset. - const stopping = !!this.queue?.isClosed; - try { - tracked.respondError?.( - stopping ? -32000 : -32603, - stopping ? 'the host stopped while handling this request' : 'Internal error', - ); - } catch (sendError) { - responseError = describeCause(sendError); - } - } - this.emitTrace({ - type: 'mcpl:request-handler-error', - serverId, - event, - // The trace's type is string | number. Only those are kept, as - // the connection's own reports keep them, and a string is - // bounded like the texts beside it. - requestId: typeof responder?.id === 'number' ? responder.id - : typeof responder?.id === 'string' ? boundedForLog(responder.id, 500) : undefined, - responseAttempted: handlerResponseAttempted, - // The trace's copies are bounded, as data; the log line below - // prints each text raw through log-value, which escapes it. - error: boundedForLog(message, 500), - ...(responseFailure === undefined ? {} : { responseFailure: boundedForLog(responseFailure, 500) }), - ...(refused === undefined ? {} : { - refusedResponse: refused.kind === 'error' ? { ...refused, message: boundedForLog(refused.message, 500) } : refused, - }), - ...(responseError === undefined ? {} : { responseError: boundedForLog(responseError, 500) }), - }); - // Keep headless hosts diagnosable even without a trace subscriber. - // The handler's text and the server's go through log-value, so a - // line stays one line of bounded, escaped text. - const tried = refused === undefined ? undefined - : refused.kind === 'result' ? 'a result' : `error ${refused.code}: ${refused.message}`; - console.error( - `[mcpl] ${serverId} ${event} handler failed: ${logDetail(message)}` + - (responseFailure === undefined ? '' : ` | its reply write had failed: ${logDetail(responseFailure)}`) + - (tried === undefined ? '' : ` | it then tried to answer again with ${logDetail(tried)}`) + - (responseError === undefined ? '' : ` | the internal-error reply failed too: ${logDetail(responseError)}`), - ); - } catch (reportError) { + const message = describeCause(primary); + // What the handler's own writes did, before any internal-error + // reply sent here. + const handlerResponseAttempted = responseAttempted; + // A reply write the handler's own attempt failed, read before the + // internal-error reply below, whose own failure is responseError. + const responseFailure = writeFailed && handlerError && handlerError.error !== writeFailure + ? describeCause(writeFailure) : undefined; + let responseError: string | undefined; + if (tracked && !responseAttempted) { + // The server gets the standard message, not the handler's own + // text, which may name internal paths or errors; the trace and + // stderr carry the detail. Once the host is stopping (its queue + // closed), it gets -32000 with the stop wording alone, the code a + // request turned away at stop gets, so it can send it again to + // the host's next run. Optional chaining: prototype-built + // harnesses leave the queue unset. + const stopping = !!this.queue?.isClosed; try { - console.error( - `[mcpl] ${serverId} ${event} handler failed: ${logDetail(describeCause(primary))}` + - ` (and reporting it failed too: ${logDetail(describeCause(reportError))})`, + tracked.respondError?.( + stopping ? -32000 : -32603, + stopping ? 'the host stopped while handling this request' : 'Internal error', ); - } catch { /* stderr itself is gone */ } + } catch (sendError) { + responseError = describeCause(sendError); + } } + this.emitTrace({ + type: 'mcpl:request-handler-error', + serverId, + event, + // The trace's type is string | number. Only those are kept, as + // the connection's own reports keep them, and a string is + // bounded like the texts beside it. + requestId: typeof responder?.id === 'number' ? responder.id + : typeof responder?.id === 'string' ? boundedForLog(responder.id, 500) : undefined, + responseAttempted: handlerResponseAttempted, + // The trace's copies are bounded, as data; the log line below + // prints each text raw through log-value, which escapes it. + error: boundedForLog(message, 500), + ...(responseFailure === undefined ? {} : { responseFailure: boundedForLog(responseFailure, 500) }), + ...(refused === undefined ? {} : { + refusedResponse: refused.kind === 'error' ? { ...refused, message: boundedForLog(refused.message, 500) } : refused, + }), + ...(responseError === undefined ? {} : { responseError: boundedForLog(responseError, 500) }), + }); + // Keep headless hosts diagnosable even without a trace subscriber. + // The handler's text and the server's go through log-value, so a + // line stays one line of bounded, escaped text. + const tried = refused === undefined ? undefined + : refused.kind === 'result' ? 'a result' : `error ${refused.code}: ${refused.message}`; + console.error( + `[mcpl] ${serverId} ${event} handler failed: ${logDetail(message)}` + + (responseFailure === undefined ? '' : ` | its reply write had failed: ${logDetail(responseFailure)}`) + + (tried === undefined ? '' : ` | it then tried to answer again with ${logDetail(tried)}`) + + (responseError === undefined ? '' : ` | the internal-error reply failed too: ${logDetail(responseError)}`), + ); }; const failed = (error: unknown): void => report({ error }); // A handler may swallow a write failure, or answer twice, and still diff --git a/src/mcpl/errors.ts b/src/mcpl/errors.ts index fefca380f..651c2cb92 100644 --- a/src/mcpl/errors.ts +++ b/src/mcpl/errors.ts @@ -40,7 +40,8 @@ export const CHANNEL_OPEN_FAILED = -32024; // Error Factories // ============================================================================ -/** The text of anything thrown or rejected. Never throws, whatever it is: a +/** The text of any value: anything thrown or rejected, or a value a server + * sent, such as an error answer's code. Never throws, whatever it is: a * null-prototype object, or a message getter or toString that throws. */ export function describeCause(cause: unknown): string { try { diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index e74851843..e2a925a35 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -1443,10 +1443,11 @@ export class McplServerConnection extends EventEmitter { // initialize error: the server's code and message can be any JSON // value, and one with no primitive form, such as {"toString":1}, would // throw here, after the request has left pendingRequests and its timer - // is cleared, so it would never settle. The message is the server's - // text, so it's bounded as errors.ts says. + // is cleared, so it would never settle. Both are the server's text, a + // code as much as a message, since a server can send a string, so both + // are bounded as errors.ts says. pending.reject( - new Error(`MCPL server "${this.id}" returned error for ${pending.method}: [${describeCause(response.error.code)}] ${boundedForLog(describeCause(response.error.message))}`), + new Error(`MCPL server "${this.id}" returned error for ${pending.method}: [${boundedForLog(describeCause(response.error.code))}] ${boundedForLog(describeCause(response.error.message))}`), ); } else { pending.resolve(response.result); diff --git a/src/types/trace.ts b/src/types/trace.ts index 0f2597d44..ce2dd7876 100644 --- a/src/types/trace.ts +++ b/src/types/trace.ts @@ -580,14 +580,17 @@ export type TraceEvent = event: string; /** The request's id when it is a number, or a string id bounded as the * other texts here are: control, format and line-separator characters - * replaced, and cut at 500 code points. Absent for a notification, for - * a request whose id is neither, and for a request form of - * mcpl/manifestChanged or notifications/tools/list_changed, which is - * answered as it's admitted and then handled as the notification is. */ + * replaced, and cut after 500 code points, with "…" marking the cut. + * Absent for a notification, for a request whose id is neither, and + * for a request form of mcpl/manifestChanged or + * notifications/tools/list_changed, which is answered as it's admitted + * and then handled as the notification is. */ requestId?: string | number; - /** Whether the handler wrote a reply: true once a reply's write - * returned. A write that throws, as one that can't be encoded does, - * sends nothing. When it's false and the responder + /** Whether a reply write of the handler's returned: true once one + * did. On a connection that had already closed, that write dropped + * the reply, as the connection then drops every reply, so nothing + * reached the server. A write that throws, as one that can't be + * encoded does, sends nothing. When it's false and the responder * can send an error reply (it has respondError), the host sent -32603 * "Internal error" itself, or -32000 while it stops, unless * `responseError` says that reply failed. A connection that has diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index 8099af74e..9a3caeaf1 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -752,7 +752,7 @@ test('a server\'s error answer that no template can convert settles the host\'s assert.equal(framework.manifestRefreshState.get('srv').inFlight, false); }); -test('a server\'s error message in the error a host request rejects with is bounded: control, format and line characters replaced, and cut at 200 code points', async () => { +test('a server\'s error code and message in the error a host request rejects with are bounded: control, format and line characters replaced, and each cut after 200 code points', 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), [])); @@ -763,6 +763,28 @@ test('a server\'s error message in the error a host request rejects with is boun await assert.rejects(fetched, { message: `MCPL server "srv" returned error for mcpl/manifest: [-32000] line one?line two?${'x'.repeat(182)}…`, }); + // JSON-RPC's code is an integer, but a server can send a string, and it's + // the server's text as much as the message is. + const again = connection.sendManifestRequest(); + const second = JSON.parse(transport.lines.at(-1)!); + transport.emit('line', JSON.stringify({ jsonrpc: '2.0', id: second.id, error: { code: `E\nx\u202e${'y'.repeat(300)}`, message: 'refused' } })); + await assert.rejects(again, { + message: `MCPL server "srv" returned error for mcpl/manifest: [E?x?${'y'.repeat(196)}…] refused`, + }); +}); + +test('a throw from the wired tools/list_changed handler is contained, traced and logged as the other handlers\' are', (t) => { + const { framework, connection, traces } = harness(); + const lines: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }); + framework.handleToolsListChanged = () => { throw new Error('the tool list refresh failed'); }; + connection.emit('tools-list-changed'); + const failures = failureTraces(traces); + assert.equal(failures.length, 1); + assert.equal(failures[0].event, 'tools-list-changed'); + assert.equal(failures[0].requestId, undefined); + assert.equal(failures[0].error, 'the tool list refresh failed'); + assert.deepEqual(lines, ['[mcpl] srv tools-list-changed handler failed: "the tool list refresh failed"']); }); test('mcpl/manifestChanged and tools/list_changed sent as requests are answered with an empty result as they are admitted, whatever their handlers do', async (t) => { @@ -1009,10 +1031,10 @@ test('a rejection no printer can inspect still reaches the trace first', async ( } }); -test("a trace listener that throws a value no printer can inspect leaves the report's fallback line, and nothing escapes", async (t) => { +test("a trace listener that throws a value no printer can inspect is logged as text, the listeners after it still get the trace, and nothing escapes", async (t) => { const { framework, connection, traces } = harness(); // Print as console.error does: an argument that isn't a string goes - // through util.inspect, so logging the listener's throw throws too. + // through util.inspect, so logging the listener's throw raw throws too. const logged: string[] = []; t.mock.method(console, 'error', (...args: unknown[]) => { logged.push(args.map((arg) => (typeof arg === 'string' ? arg : inspect(arg))).join(' ')); @@ -1021,9 +1043,11 @@ test("a trace listener that throws a value no printer can inspect leaves the rep const onUnhandled = (reason: unknown) => { unhandled.push(reason); }; process.on('unhandledRejection', onUnhandled); const hostile = { [inspect.custom]() { throw new Error('inspect refused'); } }; - const off = framework.onTrace(() => { throw hostile; }); + const offHostile = framework.onTrace(() => { throw hostile; }); + const after: any[] = []; + const offAfter = framework.onTrace((event: unknown) => after.push(event)); // The handler's own error can't be put in a template either, so the - // fallback line must convert it as the report does. + // report must convert it. const primary = Object.assign(new Error('register failed'), { [Symbol.toPrimitive]() { throw new Error('toPrimitive refused'); }, }); @@ -1035,13 +1059,15 @@ test("a trace listener that throws a value no printer can inspect leaves the rep await settle(); assert.deepEqual(unhandled, []); assert.deepEqual(reply.errors, [{ code: -32603, message: 'Internal error', data: undefined }]); - assert.equal(failureTraces(traces).length, 1, 'the listener before the throwing one still has the trace'); - const fallback = logged.filter((line) => line.includes('(and reporting it failed too:')); - assert.equal(fallback.length, 1, logged.join('\n')); - assert.match(fallback[0], /^\[mcpl\] srv channels-register handler failed: .*register failed.* \(and reporting it failed too: .*inspect refused/); - assert.ok(!fallback[0].includes('toPrimitive refused'), fallback[0]); + assert.equal(failureTraces(traces).length, 1, 'the listener before the throwing one has the trace'); + assert.equal(failureTraces(after).length, 1, 'the listener after the throwing one has it too'); + assert.deepEqual(logged, [ + 'Trace listener error: [object Object]', + '[mcpl] srv channels-register handler failed: "register failed"', + ]); } finally { - off(); + offHostile(); + offAfter(); process.off('unhandledRejection', onUnhandled); } }); From 22b593fbbdbcea7665a6e81a40729b3cccb61162 Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Sat, 17 Oct 2026 06:43:49 -0400 Subject: [PATCH 24/29] test(mcpl): the failed-generation test's framework has its trace listeners The test of an inbound handler waiting on a generation that fails builds its framework with Object.create, so it had no traceListeners. Each request it refuses is reported as a handler failure through emitTrace, whose loop then threw on the missing list: f6675c4a's head failed it on Node 20 and 24, since the report's fallback, which had caught that throw, is gone. A constructed framework always has the list (a class field), so the test now sets it, as the handler-error tests' harness does. Co-Authored-By: Claude Opus 5.5 --- test/mcpl-awareness-barrier.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/mcpl-awareness-barrier.test.ts b/test/mcpl-awareness-barrier.test.ts index 6530bee6a..a8a4822e1 100644 --- a/test/mcpl-awareness-barrier.test.ts +++ b/test/mcpl-awareness-barrier.test.ts @@ -888,6 +888,10 @@ test('an inbound handler waiting on a generation that fails refuses its request // its marks were not all attempted: nothing that waited behind it may run, // and each request is answered with the failure rather than left waiting. const framework = Object.create(AgentFramework.prototype) as any; + // Each refused request is reported as a handler failure, through + // emitTrace, which iterates the trace listeners a constructed framework + // always has (#318's registrations, which onTrace makes). + framework.traceListeners = []; const failing = deferred(); framework.discordAwarenessBarrier = { generation: 1, requiresBarrier: true, promise: failing.promise }; const handled: string[] = []; From 224d9aa916ed180d5219ddee4e91ac173969c31b Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Sat, 17 Oct 2026 08:02:23 -0400 Subject: [PATCH 25/29] fix(mcpl): the report converts a second answer's code; the tools/list_changed gate completes after a throw The internal review of 5c04e14d..22b593fb raised 8 issues; the issue numbers below are its. - I6: the report's log line printed the code of an answer the handler tried after its first through a template, while its message was converted. A code with no prototype, or a Symbol, made the report throw, which on the async path is an unhandled rejection that ends the host; the report's removed fallback caught that throw too. The code now goes through describeCause, as the report's comment says every value it puts in text does. A number prints as before. - I2: the wired tools/list_changed handler attached its gate's completion after handleToolsListChanged returned, so a synchronous throw there, which the handler's containment now reports where main's ended the host, left a barrier the data planes wait for installed, and its rejection unhandled. The completion is attached in a finally: the gate is completed when its barrier settles, or failed with it, and the success path's order is unchanged. - I8: f6675c4a's message says the fallback guarded only a throw out of emitTrace's catch. It also caught I6's throw, and, in prototype-built harnesses, the loop over a missing listener list (22b593fb). Tests: - I1: the trace listener's hostile value has no prototype, so a template throws on it too, and only describeCause's text keeps emitTrace's fallback line from throwing. - I6: a second answer whose code has no prototype is printed with describeCause's text, and nothing escapes. - I2: after the handler's throw, the gate completes when a barrier the data planes wait for settles, and is failed when it rejects, with the rejection handled. Co-Authored-By: Claude Opus 5.5 --- src/framework.ts | 31 ++++++----- test/mcpl-request-handler-errors.test.ts | 69 +++++++++++++++++++++++- 2 files changed, 87 insertions(+), 13 deletions(-) diff --git a/src/framework.ts b/src/framework.ts index e7465e52d..2b9a52b40 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -17534,9 +17534,10 @@ export class AgentFramework { // rejected, else the reply write it swallowed, else the reply it // tried to send twice. Nothing in here may throw: on the async path // it runs as a rejection handler, and an exception there would be an - // unhandled rejection, which ends the host. So every text is converted - // with describeCause, the internal-error reply's send is caught, and - // emitTrace's own catch can't throw. + // unhandled rejection, which ends the host. So every value it puts in + // text, the code of an answer the handler tried again included, is + // converted with describeCause, the internal-error reply's send is + // caught, and emitTrace's own catch can't throw. const report = (handlerError: { error: unknown } | undefined): void => { const primary = handlerError ? handlerError.error @@ -17592,7 +17593,7 @@ export class AgentFramework { // The handler's text and the server's go through log-value, so a // line stays one line of bounded, escaped text. const tried = refused === undefined ? undefined - : refused.kind === 'result' ? 'a result' : `error ${refused.code}: ${refused.message}`; + : refused.kind === 'result' ? 'a result' : `error ${describeCause(refused.code)}: ${refused.message}`; console.error( `[mcpl] ${serverId} ${event} handler failed: ${logDetail(message)}` + (responseFailure === undefined ? '' : ` | its reply write had failed: ${logDetail(responseFailure)}`) + @@ -17821,14 +17822,20 @@ export class AgentFramework { // data event cannot pass before the new barrier exists. const awarenessBarrier = this.installMcplDataPlaneGate(); this.releaseMcplDataPlaneGate(awarenessBarrier); - this.handleToolsListChanged(connection.id); - void awarenessBarrier.promise.then(() => { - this.completeMcplDataPlaneGate(awarenessBarrier); - }).catch((error) => this.failMcplDataPlaneGate( - awarenessBarrier, - 'tools-list reconciliation', - error, - )); + try { + this.handleToolsListChanged(connection.id); + } finally { + // A throw from the refresh is contained like the handler's; the gate + // is still completed when its barrier settles, or failed with it, as + // on the way through. + void awarenessBarrier.promise.then(() => { + this.completeMcplDataPlaneGate(awarenessBarrier); + }).catch((error) => this.failMcplDataPlaneGate( + awarenessBarrier, + 'tools-list reconciliation', + error, + )); + } }); // Re-establish full server registration on reconnect. The 'close' handler diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index 9a3caeaf1..adfcf2a76 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -787,6 +787,40 @@ test('a throw from the wired tools/list_changed handler is contained, traced and assert.deepEqual(lines, ['[mcpl] srv tools-list-changed handler failed: "the tool list refresh failed"']); }); +test('after a throw from the wired tools/list_changed handler, its data-plane gate is still completed when its barrier settles, or failed with it', async (t) => { + const { framework, connection } = harness(); + t.mock.method(console, 'error', () => {}); + const unhandled: unknown[] = []; + const onUnhandled = (reason: unknown) => { unhandled.push(reason); }; + process.on('unhandledRejection', onUnhandled); + try { + framework.handleToolsListChanged = () => { throw new Error('the tool list refresh failed'); }; + const failed: unknown[][] = []; + framework.failMcplDataPlaneGate = async (barrier: unknown, what: string, error: unknown) => { failed.push([barrier, what, error]); }; + // A barrier the data planes wait for, which settles: the gate completes. + framework.beginDiscordAwarenessBarrier = () => ({ + requiresBarrier: true, + promise: Promise.resolve({ status: 'delivered', delivered: 0, failed: 0 }), + }); + connection.emit('tools-list-changed'); + const settling = framework.discordAwarenessBarrier; + assert.ok(settling, 'the gate is installed until its barrier settles'); + await settle(); + assert.equal(framework.discordAwarenessBarrier, null, 'the gate completed'); + // One that fails: the gate is failed with it, and its rejection is handled. + const accounting = new Error('the awareness accounting failed'); + framework.beginDiscordAwarenessBarrier = () => ({ requiresBarrier: true, promise: Promise.reject(accounting) }); + connection.emit('tools-list-changed'); + const failing = framework.discordAwarenessBarrier; + await settle(); + await settle(); + assert.deepEqual(failed, [[failing, 'tools-list reconciliation', accounting]]); + assert.deepEqual(unhandled, []); + } finally { + process.off('unhandledRejection', onUnhandled); + } +}); + test('mcpl/manifestChanged and tools/list_changed sent as requests are answered with an empty result as they are admitted, whatever their handlers do', async (t) => { const { framework, connection, transport, traces } = wireHarness(); t.mock.method(console, 'error', () => {}); @@ -1042,7 +1076,9 @@ test("a trace listener that throws a value no printer can inspect is logged as t const unhandled: unknown[] = []; const onUnhandled = (reason: unknown) => { unhandled.push(reason); }; process.on('unhandledRejection', onUnhandled); - const hostile = { [inspect.custom]() { throw new Error('inspect refused'); } }; + // With no prototype, a template can't convert it either: only + // describeCause's text keeps the fallback line from throwing. + const hostile = Object.assign(Object.create(null), { [inspect.custom]() { throw new Error('inspect refused'); } }); const offHostile = framework.onTrace(() => { throw hostile; }); const after: any[] = []; const offAfter = framework.onTrace((event: unknown) => after.push(event)); @@ -1132,6 +1168,37 @@ test("the log line prints a failed reply write and a refused answer raw, and onl assert.ok(lines[0]!.includes('it then tried to answer again with "error -32602: second\\u0007answer"'), lines[0]); }); +test('an answer the handler tried after its first, with a code no template can convert, is printed with describeCause\'s text for the code, and nothing escapes', async (t) => { + const { framework, connection, traces } = harness(); + const lines: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }); + const unhandled: unknown[] = []; + const onUnhandled = (reason: unknown) => { unhandled.push(reason); }; + process.on('unhandledRejection', onUnhandled); + // A code with no prototype: a template throws on it. + const code = Object.create(null) as number; + try { + framework.channelRegistry = { + async handleRegister(_server: string, _params: unknown, reply: any) { + reply.respond({ registered: [] }); + reply.respondError(code, 'again'); + throw new Error('register failed'); + }, + }; + const reply = responder(); + connection.emit('channels-register', {}, reply); + await settle(); + await settle(); + assert.deepEqual(unhandled, []); + assert.equal(failureTraces(traces).length, 1); + assert.equal(lines.length, 1); + assert.ok(lines[0]!.includes('handler failed: "register failed"'), lines[0]); + assert.ok(lines[0]!.includes('it then tried to answer again with "error [object Object]: again"'), lines[0]); + } finally { + process.off('unhandledRejection', onUnhandled); + } +}); + test("the log line prints a failed internal-error reply raw, and only the trace's copy is bounded", async (t) => { const { framework, connection, traces } = harness(); const lines: string[] = []; From b8c1ca4ce1ef91348793c490d37ab564c8f5b2da Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Sat, 17 Oct 2026 08:12:40 -0400 Subject: [PATCH 26/29] fix(mcpl): a backstop logs a throw out of the async report on a line of its own On the async path the handler-failure report runs as a promise handler, where a throw would be an unhandled rejection, which ends the host. The report converts and catches what it knows can throw, but an internal review has twice found a throw it didn't know of: emitTrace's catch (f6675c4a) and a second answer's code (224d9aa9), each fixed where it arose. This path exists to contain odd values from servers, so the repository owner asked for a backstop: a catch on the async report that logs describeCause of what it caught on a fixed line, [mcpl] : the handler failure's report threw: The report's comment now says an exception there leaves the report unfinished for the backstop to log. On the synchronous path a throw out of the report still reaches the connection's own containment. Test: with emitTrace stubbed to throw, a rejecting handler's request is still answered -32603, the backstop's line is the one line logged, and nothing escapes as an unhandled rejection. Co-Authored-By: Claude Opus 5.5 --- src/framework.ts | 22 +++++++++++++++------- test/mcpl-request-handler-errors.test.ts | 23 +++++++++++++++++++++++ 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/src/framework.ts b/src/framework.ts index 2b9a52b40..a0e9b706a 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -17533,11 +17533,11 @@ export class AgentFramework { // Report a failure: the handler's own error when it threw or // rejected, else the reply write it swallowed, else the reply it // tried to send twice. Nothing in here may throw: on the async path - // it runs as a rejection handler, and an exception there would be an - // unhandled rejection, which ends the host. So every value it puts in - // text, the code of an answer the handler tried again included, is - // converted with describeCause, the internal-error reply's send is - // caught, and emitTrace's own catch can't throw. + // it runs as a rejection handler, where an exception would leave the + // report unfinished for the backstop below to log. So every value it + // puts in text, the code of an answer the handler tried again + // included, is converted with describeCause, the internal-error + // reply's send is caught, and emitTrace's own catch can't throw. const report = (handlerError: { error: unknown } | undefined): void => { const primary = handlerError ? handlerError.error @@ -17611,8 +17611,16 @@ export class AgentFramework { // Invoke immediately: control-plane responses must retain their // ACK-before-reconcile ordering and data-plane barrier semantics. const result = handler(params, tracked); - if (result) void Promise.resolve(result).then(succeeded, failed); - else succeeded(); + if (result) { + // On this path the report runs as a rejection handler, where a + // throw would be an unhandled rejection, which ends the host. The + // report converts and catches what it knows can throw; this + // backstop catches anything else, a defect in the report itself, + // and logs it on a line of its own. + void Promise.resolve(result).then(succeeded, failed).catch((reportFailure: unknown) => { + console.error(`[mcpl] ${serverId} ${event}: the handler failure's report threw: ${describeCause(reportFailure)}`); + }); + } else succeeded(); } catch (error) { failed(error); } diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index adfcf2a76..326f407d5 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -1199,6 +1199,29 @@ test('an answer the handler tried after its first, with a code no template can c } }); +test('a throw out of the report on the async path is caught by its backstop and logged on a line of its own, and nothing escapes', async (t) => { + const { framework, connection } = harness(); + const lines: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }); + const unhandled: unknown[] = []; + const onUnhandled = (reason: unknown) => { unhandled.push(reason); }; + process.on('unhandledRejection', onUnhandled); + // A defect in the report itself: its trace throws. + framework.emitTrace = () => { throw new Error('the trace failed'); }; + try { + framework.channelRegistry = { async handleRegister() { throw new Error('register failed'); } }; + const reply = responder(); + connection.emit('channels-register', {}, reply); + await settle(); + await settle(); + assert.deepEqual(unhandled, []); + assert.deepEqual(reply.errors, [{ code: -32603, message: 'Internal error', data: undefined }], 'the reply is sent before the trace'); + assert.deepEqual(lines, ["[mcpl] srv channels-register: the handler failure's report threw: the trace failed"]); + } finally { + process.off('unhandledRejection', onUnhandled); + } +}); + test("the log line prints a failed internal-error reply raw, and only the trace's copy is bounded", async (t) => { const { framework, connection, traces } = harness(); const lines: string[] = []; From 4773c6373bbf6c563fc6192d6f0e6048ad94637a Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Sat, 17 Oct 2026 08:25:14 -0400 Subject: [PATCH 27/29] fix(mcpl): the backstop's line is printed through log-value and can't throw; so is emitTrace's fallback line From Esther-2584's read of 8051b245..b8c1ca4c, with a probe for each, and the repository owner's decision to take both: - The backstop's own console.error wasn't caught. A console that throws (an embedder's replacement, say) made it an unhandled rejection, the one step nothing else catches. It's now inside its own try/catch. - The backstop printed the caught text raw, so a text with a line break printed a second line that reads as another record. It now prints logDetail(describeCause(...)), one escaped and bounded line, as the report's own line prints its texts. emitTrace's fallback line, for a trace listener's throw that can't be inspected, prints the same way. Tests: the backstop's test throws a text with a line break and a forged "[mcpl] srv" record, and expects one quoted line; a backstop whose own log line throws leaves nothing unhandled, and the request is still answered; the hostile trace listener's line is quoted. Co-Authored-By: Claude Opus 5.5 --- src/framework.ts | 13 +++++++---- test/mcpl-request-handler-errors.test.ts | 29 ++++++++++++++++++++---- 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/src/framework.ts b/src/framework.ts index a0e9b706a..fa6119042 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -15586,11 +15586,12 @@ export class AgentFramework { // throwing util.inspect.custom, say). That throw would leave this // loop, so the listeners after this one would miss the event and the // code that emitted it would get the throw. describeCause never - // throws, and a template string isn't inspected. + // throws, a template string isn't inspected, and log-value prints + // the text as one escaped, bounded line. try { console.error('Trace listener error:', error); } catch { - console.error(`Trace listener error: ${describeCause(error)}`); + console.error(`Trace listener error: ${logDetail(describeCause(error))}`); } } } @@ -17616,9 +17617,13 @@ export class AgentFramework { // throw would be an unhandled rejection, which ends the host. The // report converts and catches what it knows can throw; this // backstop catches anything else, a defect in the report itself, - // and logs it on a line of its own. + // and logs it on a line of its own, printed through log-value as + // the report's line is. Nothing catches the backstop, so a failure + // of its own log is swallowed. void Promise.resolve(result).then(succeeded, failed).catch((reportFailure: unknown) => { - console.error(`[mcpl] ${serverId} ${event}: the handler failure's report threw: ${describeCause(reportFailure)}`); + try { + console.error(`[mcpl] ${serverId} ${event}: the handler failure's report threw: ${logDetail(describeCause(reportFailure))}`); + } catch { /* nothing is left to log it to */ } }); } else succeeded(); } catch (error) { diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index 326f407d5..12a3f1d1d 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -1098,7 +1098,7 @@ test("a trace listener that throws a value no printer can inspect is logged as t assert.equal(failureTraces(traces).length, 1, 'the listener before the throwing one has the trace'); assert.equal(failureTraces(after).length, 1, 'the listener after the throwing one has it too'); assert.deepEqual(logged, [ - 'Trace listener error: [object Object]', + 'Trace listener error: "[object Object]"', '[mcpl] srv channels-register handler failed: "register failed"', ]); } finally { @@ -1206,8 +1206,9 @@ test('a throw out of the report on the async path is caught by its backstop and const unhandled: unknown[] = []; const onUnhandled = (reason: unknown) => { unhandled.push(reason); }; process.on('unhandledRejection', onUnhandled); - // A defect in the report itself: its trace throws. - framework.emitTrace = () => { throw new Error('the trace failed'); }; + // A defect in the report itself: its trace throws, with a text that + // would print a second line raw. + framework.emitTrace = () => { throw new Error('the trace failed\n[mcpl] srv forged'); }; try { framework.channelRegistry = { async handleRegister() { throw new Error('register failed'); } }; const reply = responder(); @@ -1216,7 +1217,27 @@ test('a throw out of the report on the async path is caught by its backstop and await settle(); assert.deepEqual(unhandled, []); assert.deepEqual(reply.errors, [{ code: -32603, message: 'Internal error', data: undefined }], 'the reply is sent before the trace'); - assert.deepEqual(lines, ["[mcpl] srv channels-register: the handler failure's report threw: the trace failed"]); + assert.deepEqual(lines, ["[mcpl] srv channels-register: the handler failure's report threw: \"the trace failed\\n[mcpl] srv forged\""]); + } finally { + process.off('unhandledRejection', onUnhandled); + } +}); + +test('a backstop whose own log line throws leaves nothing unhandled', async (t) => { + const { framework, connection } = harness(); + t.mock.method(console, 'error', () => { throw new Error('the console is gone'); }); + const unhandled: unknown[] = []; + const onUnhandled = (reason: unknown) => { unhandled.push(reason); }; + process.on('unhandledRejection', onUnhandled); + framework.emitTrace = () => { throw new Error('the trace failed'); }; + try { + framework.channelRegistry = { async handleRegister() { throw new Error('register failed'); } }; + const reply = responder(); + connection.emit('channels-register', {}, reply); + await settle(); + await settle(); + assert.deepEqual(unhandled, []); + assert.deepEqual(reply.errors, [{ code: -32603, message: 'Internal error', data: undefined }]); } finally { process.off('unhandledRejection', onUnhandled); } From 30e6965023ba0ffae9cd229bd44a9c29478d743b Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Sat, 17 Oct 2026 11:44:58 -0400 Subject: [PATCH 28/29] test(mcpl): the result-write test's fake inference returns the shared helper's response The test "a real inference handler whose result write throws answers with its own error..." gave InferenceRouter a fake membrane whose complete() returned an object of this file's own, with content, stopReason and usage only, cast as never. main's router reads only those three of a response (src/mcpl/inference-router.ts:174-185 on main), so the test passes there. #380's router also reads response.details.stop and response.toolCalls (src/mcpl/inference-router.ts:89 and :95 at its head 647dd612), and with #380 merged as well the test fails on Node 20 and 24: the result write it counts never happens (resultWrites 0, expected 1). The fake now returns createMockResponse() from test/helpers/mock-membrane, the shared helper other tests' fakes use. Its response carries toolCalls and details, and #380 adds to that helper the details.stop its router reads. So the test passes on main, and with #380 merged as well. Co-Authored-By: Claude Opus 5.5 --- test/mcpl-request-handler-errors.test.ts | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index 12a3f1d1d..adfbdd081 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -11,6 +11,7 @@ import { InferenceRouter } from '../src/mcpl/inference-router.js'; 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 { createMockResponse } from './helpers/mock-membrane.js'; function harness(connection = Object.assign(new EventEmitter(), { id: 'srv' })) { const traces: any[] = []; @@ -177,15 +178,9 @@ test('inference responders retain request IDs and successful handling stays sync test('a real inference handler whose result write throws answers with its own error, its only reply, and the failed write is reported', async () => { const { framework, connection, traces } = harness(); framework.inferenceRouter = new InferenceRouter( - { - async complete() { - return { - content: [{ type: 'text', text: 'done' }], - stopReason: 'end_turn', - usage: { inputTokens: 1, outputTokens: 1 }, - }; - }, - } as never, + // The shared helper's response, so a change that has the router read more + // of a response gives this test that field along with every other test. + { async complete() { return createMockResponse([{ type: 'text', text: 'done' }]); } } as never, { isInHook: false } as never, { validateInbound() {} } as never, null, From ef682d04ba16eae098cb0eff5a7d32119164a1e0 Mon Sep 17 00:00:00 2001 From: Kestrel-2409 Date: Sat, 17 Oct 2026 22:49:57 -0400 Subject: [PATCH 29/29] docs(changelog): leave the awareness gate's failure out of the Fixed entry (R2-2) The entry said a failed Discord awareness accounting that a tools-list refresh waits on is reported for every connection, which then recycles. That is main's behaviour, which this branch doesn't change, and a changelog says what changed. The code comment that explains it stays. Co-Authored-By: Claude Opus 5.5 --- changelog.d/208-inbound-handler-containment.fixed.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/208-inbound-handler-containment.fixed.md b/changelog.d/208-inbound-handler-containment.fixed.md index de26a2792..ad2ec697c 100644 --- a/changelog.d/208-inbound-handler-containment.fixed.md +++ b/changelog.d/208-inbound-handler-containment.fixed.md @@ -1,6 +1,6 @@ - MCPL's inbound handlers are contained at one boundary, the framework's wiring, for every request and notification it handles (#186). The `channels/list`, `tools/observe` and `model/info` requests and the `notifications/tools/list_changed` notification are newly contained: a throw or rejection of theirs is reported as the other handlers' are. `mcpl/manifestChanged` goes through the same boundary, and so does a refresh its handler schedules for later, after the fetch floor or behind a fetch in flight: a manifest fetch that fails is still caught and logged inside the handler, as in 0.22.x, and anything else the handler throws or rejects with is now reported as the others' are. 0.22.x logged that from an announcement's own run, and dropped it from a scheduled re-run. The six requests whose handler failures 0.22.0 already answered (`push/event`, `inference/request`, `channels/register`, `channels/changed`, `channels/incoming` and `host/command`) go through the same boundary now. On a connection the framework wires, no handler's throw or rejection ends the host, and a rejection reason that can't be printed is reported as one that can. - **A request whose handler fails is answered `-32603` "Internal error",** or, while the host is stopping, `-32000` "the host stopped while handling this request". The handler's error stays in the trace and on stderr. For the six that 0.22.x answered with the handler's own message, that's a change, described under Changed. A request is answered this way when its handler fails without having written a reply, which includes a reply whose write threw, such as one that can't be encoded. A notification gets no reply. - - **Each failure emits a new trace, `mcpl:request-handler-error`, and a stderr line.** Its `error` is the handler's own error, or, for a handler that resolved, the reply write it swallowed, or a note that it tried to answer a second time. A reply write that failed before it is `responseFailure`, and a reply the handler tried to send after one was already attempted is `refusedResponse` (a result, or an error with its code and message): refused, never written. `responseError` is the internal-error reply's own failure, which the stderr line prints too. `requestId` is the request's id when that is a number, and a string id bounded as the trace's other texts are. A request form of `mcpl/manifestChanged` or `notifications/tools/list_changed` has none, since it's answered as it's admitted and then handled as the notification is. A failure of the Discord awareness accounting that a tools-list refresh waits on is global: it's reported as `mcpl:server-error` for every connection, and then each connection is recycled, unless the host is stopping. + - **Each failure emits a new trace, `mcpl:request-handler-error`, and a stderr line.** Its `error` is the handler's own error, or, for a handler that resolved, the reply write it swallowed, or a note that it tried to answer a second time. A reply write that failed before it is `responseFailure`, and a reply the handler tried to send after one was already attempted is `refusedResponse` (a result, or an error with its code and message): refused, never written. `responseError` is the internal-error reply's own failure, which the stderr line prints too. `requestId` is the request's id when that is a number, and a string id bounded as the trace's other texts are. A request form of `mcpl/manifestChanged` or `notifications/tools/list_changed` has none, since it's answered as it's admitted and then handled as the notification is. - **A request whose method isn't a string,** or isn't one the host handles, or whose event nothing on the host listens for, is answered `-32601` "Method not found", with a stderr line. A notification nobody handles is ignored. - **`mcpl/manifestChanged` and `notifications/tools/list_changed` sent as requests,** with an id, are answered with an empty result when the connection admits them, which waits while their plane is paused, if something on the host listens for them, as the framework's wiring always does, and are handled as the notification is. - **Non-object JSON** (`null`, `42`, `"hi"`, `true`) is ignored before property access, in the handshake reader and in routing, as a non-JSON line already is. One such line from a server used to end the host (#217).