From 73fb941ef40895922e9ebdf2688ffd637144ee50 Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sun, 18 Oct 2026 03:35:29 -0400 Subject: [PATCH 01/11] fix(mcpl): a failed MCPL request, the framework's refusals of an MCPL tool call, and the blocks the host can't show say what happened in the agent's words (W4a) The first of W4's three parts, rebuilt on main 07563022 (Ada-1017, room-203 #139500, #139890). Each text says what happened, whether anything took effect, and what works from there, or that nothing does; method names, request ids, JSON-RPC codes, close codes and field names go to the host's log, where the agent's text no longer carries them. A failed MCPL request (server-connection.ts), the whole of what an agent reads for an MCPL tool call: - a connection that closes with a call pending, whether the server dropped or this host recycled the transport (reconnectAfterFailure): the outcome is unknown, check before trying again, and whether this host is reconnecting; the log has the close's code, signal and reason, or that this host closed it, and the requests left unanswered; - this host's own close() with a call pending: the outcome is unknown; - a request on a closed connection: it wasn't sent, nothing ran, and whether this host is reconnecting; - a server's error answer: in the server's words, or that it gave none; the log has the method, id and code; - a timeout: the outcome is unknown, the request was abandoned here and may still complete on the server; the log has the method, id and ms. The framework's MCPL tool calls (framework.ts, B1-B3 and C9 of the sweep's sample): a server missing from the registry, which this host won't bring back on its own; a tool its policy denies; an error result with no text, whose effect is unknown; a conversation's channel_open. Blocks the host can't show (references.ts, convert-block.ts, visible-content.ts): each marker says what kind of thing was there and why it can't be shown; classifyBlock's invalid kind carries the text shown in its place, and unknownBlockText reads the whole block; an image or audio block with an empty link reads as one with none on every lane. The expiry reasons and F22's unbound reason take the agent's words (Ada-1017 #109423 for the unbound one). Rebased onto main d6593a304, which carries anima-research/agent-framework#425. Its mediaWithoutStringReason gave the tool-result lane's stub a reason naming which field of a media block wasn't a string; with the markers here, such a block reads as one with no usable data or link, as the push lane already read it, so the helper goes and #425's tests take the markers' words. INLINE_WITHHELD_TEXT, which #425 reworded, takes this change's words. Rebased onto main 0d19f7c89, after train J, two of whose pull requests met this change (Ada-1017 agreed the resolution, room-203 #140908): - anima-research/agent-framework#208 bounds a server's error code and message in the error a request rejects with. Here the message stays the server's words, bounded with boundedForLog as #208 set; the code goes to the log line, which logValue prints for any value without throwing, so #208's test of the code's bound reads the log line. - anima-research/agent-framework#490 makes a server's error result a failure on each path, through mcplToolFailure. Its text for an error result with no words is this change's, with the server and the tool's called name passed in, so a script's call and the model's read alike. --- src/framework.ts | 58 ++++-- src/mcpl/convert-block.ts | 18 +- src/mcpl/references.ts | 78 ++++--- src/mcpl/server-connection.ts | 92 ++++++--- src/mcpl/visible-content.ts | 3 +- test/conversation-routing.test.ts | 2 +- test/mcpl-agent-words.test.ts | 224 +++++++++++++++++++++ test/mcpl-awareness-barrier.test.ts | 4 +- test/mcpl-coalesced-content-parity.test.ts | 47 +++-- test/mcpl-iserror-failure.test.ts | 17 +- test/mcpl-push-convert.test.ts | 2 +- test/mcpl-reference-fetcher.test.ts | 4 +- test/mcpl-reference-fields.test.ts | 4 +- test/mcpl-request-handler-errors.test.ts | 13 +- test/mcpl-request-timeout.test.ts | 21 +- test/mcpl-tool-lifecycle-e2e.test.ts | 2 +- test/mcpl-tool-resolution.test.ts | 10 +- test/mcpl-transport-close.test.ts | 4 +- test/mcpl-ws-transport.test.ts | 3 +- test/save-recent-image.test.ts | 4 +- test/tool-result-reference-uri.test.ts | 22 +- 21 files changed, 488 insertions(+), 144 deletions(-) create mode 100644 test/mcpl-agent-words.test.ts diff --git a/src/framework.ts b/src/framework.ts index ae7f1231e..6cbcaddec 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -139,6 +139,23 @@ import { stampThinkingTokenEstimates } from './thinking-token-stamp.js'; /** Detect a supported image media type from magic bytes (the model API * rejects mislabeled media types, so trust bytes over extensions). * Returns undefined for non-image content. */ +/** An MCPL tool call refused because its server isn't in the registry: one + * closed for good, one removed, or one never connected, which this host + * doesn't bring back on its own. A server that's reconnecting stays + * registered, and its closed connection answers the call instead. */ +function mcplServerGoneText(serverId: string, toolName: string): string { + return `The "${serverId}" server that provides ${toolName} isn't connected, and this host won't reconnect to it on its own, ` + + "so the call didn't run. If you need it, ask whoever runs this host."; +} + +/** An MCPL tool call its server's tool policy denies. The policy hides the + * tool from the list it's offered in, so this is a call to a tool the agent + * wasn't offered. */ +function mcplToolDeniedText(serverId: string, toolName: string): string { + return `${toolName} isn't allowed by this host's policy for the "${serverId}" server, so the call didn't run, ` + + "and calling it again won't change that. If you need it, ask whoever runs this host."; +} + function sniffImageMediaType(data: Buffer): string | undefined { if (data.length >= 4 && data[0] === 0x89 && data[1] === 0x50 && data[2] === 0x4e && data[3] === 0x47) { return 'image/png'; @@ -312,13 +329,20 @@ function mcplResultText(content: McpToolResultContent[] | undefined): string { /** * An MCPL tools/call result the server marked `isError`, as a failure: its - * text is the error, or `Tool call failed` when it has none, and its other - * blocks aren't kept. The model's dispatch and an in-process caller - * (`executeToolCall`, `ModuleContext.callTool`) both take a server's error - * from here, so it reads the same on each path. + * text is the error, and with none, the error says the server gave no words + * and that whether the call did anything is unknown. Its other blocks aren't + * kept. The model's dispatch and an in-process caller (`executeToolCall`, + * `ModuleContext.callTool`) both take a server's error from here, so it reads + * the same on each path. `callName` is the tool's name as it was called. */ -function mcplToolFailure(content: McpToolResultContent[] | undefined): ToolResult { - return { success: false, error: mcplResultText(content) || 'Tool call failed', isError: true }; +function mcplToolFailure(content: McpToolResultContent[] | undefined, serverId: string, callName: string): ToolResult { + return { + success: false, + error: mcplResultText(content) || + `The "${serverId}" server reported an error for ${callName} without saying what went wrong. ` + + 'Whether it did anything is unknown: check before calling it again.', + isError: true, + }; } /** @@ -15712,22 +15736,18 @@ export class AgentFramework { } const server = this.mcplServerRegistry.getServer(serverId); if (!server) { - return { success: false, error: `MCPL server ${serverId} not found`, isError: true }; + return { success: false, error: mcplServerGoneText(serverId, call.name), isError: true }; } const toolName = call.name.slice(prefix.length + 2); // Strip prefix + '--' if (!isToolAllowed(toolName, this.mcplServerConfigs.get(serverId))) { - return { - success: false, - error: `Tool '${call.name}' is not permitted by this server's tool policy.`, - isError: true, - }; + return { success: false, error: mcplToolDeniedText(serverId, call.name), isError: true }; } try { const result = await server.sendToolsCall(toolName, call.input as Record); // A server's error is a failure here too, as the model's dispatch // reads it. A success is the server's own blocks, for the caller to // use in process. - if (result.isError) return mcplToolFailure(result.content); + if (result.isError) return mcplToolFailure(result.content, serverId, call.name); return { success: true, data: result.content, @@ -20408,7 +20428,7 @@ export class AgentFramework { toolName: call.name, agentName, moduleName: `mcpl:${serverId}`, - result: { success: false, error: `MCPL server not found: ${serverId}`, isError: true }, + result: { success: false, error: mcplServerGoneText(serverId, call.name), isError: true }, }); return; } @@ -20423,11 +20443,7 @@ export class AgentFramework { toolName: call.name, agentName, moduleName: `mcpl:${serverId}`, - result: { - success: false, - error: `Tool '${call.name}' is not permitted by this server's tool policy.`, - isError: true, - }, + result: { success: false, error: mcplToolDeniedText(serverId, call.name), isError: true }, }); return; } @@ -20505,7 +20521,7 @@ export class AgentFramework { // string for backward compatibility with callers that expect data to // be string-ish. const converted: ToolResult = result.isError - ? mcplToolFailure(result.content) + ? mcplToolFailure(result.content, serverId, call.name) : { success: true, data: result.content?.some((c) => c.type !== 'text') @@ -20638,7 +20654,7 @@ export class AgentFramework { }; if (call.name === 'channel_open') { - reject(`This conversation is bound to channel ${home}; conversation agents cannot open channels.`); + reject(`This conversation is bound to channel "${home}", so it can't open other channels, and nothing was opened. Reply in "${home}".`); return; } if (call.name === 'channel_publish' || call.name === 'channel_close') { diff --git a/src/mcpl/convert-block.ts b/src/mcpl/convert-block.ts index 0c3b68076..71679e2c5 100644 --- a/src/mcpl/convert-block.ts +++ b/src/mcpl/convert-block.ts @@ -5,7 +5,16 @@ */ import type { ContentBlock } from '@animalabs/membrane'; -import { INLINE_WITHHELD_TEXT, isInlineContradiction, referenceStubOrNull } from './references.js'; +import { + AUDIO_NO_DATA_TEXT, + AUDIO_NO_MEDIA_TYPE_TEXT, + IMAGE_NO_DATA_TEXT, + IMAGE_NO_MEDIA_TYPE_TEXT, + INLINE_WITHHELD_TEXT, + isInlineContradiction, + referenceStubOrNull, + unknownBlockText, +} from './references.js'; import { NON_STRING_TEXT_PLACEHOLDER } from './visible-content.js'; import type { McplContentBlock } from './types.js'; @@ -48,7 +57,7 @@ export function convertBlock(block: McplContentBlock, provenance: string): Conte source: { type: 'url', url: block.uri }, } as ContentBlock; } - return { type: 'text', text: isFilledString(block.data) ? '[Image: no media type]' : '[Image: no data]' }; + return { type: 'text', text: isFilledString(block.data) ? IMAGE_NO_MEDIA_TYPE_TEXT : IMAGE_NO_DATA_TEXT }; case 'audio': if (isInlineContradiction(block)) { @@ -67,7 +76,7 @@ export function convertBlock(block: McplContentBlock, provenance: string): Conte source: { type: 'base64', data: block.data, mediaType: block.mimeType }, } as ContentBlock; } - return { type: 'text', text: isFilledString(block.data) ? '[Audio: no media type]' : '[Audio: no data]' }; + return { type: 'text', text: isFilledString(block.data) ? AUDIO_NO_MEDIA_TYPE_TEXT : AUDIO_NO_DATA_TEXT }; case 'resource': // RFC-005: reference blocks become bounded stubs — never raw URIs @@ -79,8 +88,7 @@ export function convertBlock(block: McplContentBlock, provenance: string): Conte // and propagated `undefined` into ContentBlock[]. Fail visibly. A type // that isn't a string is no type, and is never converted to one: an // object can throw when it is. - const type: unknown = (block as { type?: unknown }).type; - return { type: 'text', text: `[unrecognized content block: ${typeof type === 'string' ? type : 'untyped'}]` }; + return { type: 'text', text: unknownBlockText(block) }; } } } diff --git a/src/mcpl/references.ts b/src/mcpl/references.ts index 04476cbe9..59d417286 100644 --- a/src/mcpl/references.ts +++ b/src/mcpl/references.ts @@ -26,6 +26,8 @@ * code-execution materialization hook possible. */ +import { NON_STRING_TEXT_PLACEHOLDER } from './visible-content.js'; + export const REFERENCE_LIMITS = { uri: 4096, mimeType: 255, name: 255, expiresAt: 64 } as const; export const DIGEST_PATTERN = /^sha256:[A-Za-z0-9_-]{43}$/; const MAX_SAFE = 9007199254740991; // 2^53 - 1 @@ -53,7 +55,8 @@ export type BlockClassification = | { kind: 'text' } | { kind: 'inline'; contradiction: boolean } | { kind: 'reference'; testimony: ReferenceTestimony } - | { kind: 'invalid'; reason: string }; + /** `shown` is what the model-visible lanes put in the block's place. */ + | { kind: 'invalid'; shown: string }; function isRecord(x: unknown): x is Record { return typeof x === 'object' && x !== null && !Array.isArray(x); @@ -80,13 +83,13 @@ function takeString( export function classifyBlock(block: unknown): BlockClassification { if (!isRecord(block) || typeof block.type !== 'string') { - return { kind: 'invalid', reason: 'not a content block' }; + return { kind: 'invalid', shown: unknownBlockText(block) }; } switch (block.type) { case 'text': return typeof block.text === 'string' ? { kind: 'text' } - : { kind: 'invalid', reason: 'text block without string text' }; + : { kind: 'invalid', shown: NON_STRING_TEXT_PLACEHOLDER }; case 'image': case 'audio': { const hasData = typeof block.data === 'string'; @@ -96,30 +99,23 @@ export function classifyBlock(block: unknown): BlockClassification { // the data) is the vector-2 contradiction: fail closed, withhold. return { kind: 'inline', contradiction: block.disposition !== undefined || hasUri }; } - if (hasUri) return parseReferenceFields(block); - return { kind: 'invalid', reason: `${block.type} block ${mediaWithoutStringReason(block)}` }; + // An empty uri is no link, as the push lane's converter reads it. + if (hasUri && block.uri !== '') return parseReferenceFields(block); + return { kind: 'invalid', shown: block.type === 'image' ? IMAGE_NO_DATA_TEXT : AUDIO_NO_DATA_TEXT }; } case 'resource': return parseReferenceFields(block); default: - return { kind: 'invalid', reason: `unknown block type: ${block.type}` }; + return { kind: 'invalid', shown: unknownBlockText(block) }; } } -/** What a media block with neither string data nor a string uri has instead: - * nothing, or a field that isn't a string. */ -function mediaWithoutStringReason(block: Record): string { - const notString = (['data', 'uri'] as const).filter((f) => block[f] !== undefined && block[f] !== null); - if (notString.length === 0) return 'with neither data nor uri'; - return notString.length === 2 ? "whose data and uri aren't strings" : `whose ${notString[0]} isn't a string`; -} - function parseReferenceFields(block: Record): BlockClassification { if (typeof block.uri !== 'string' || block.uri.length === 0) { - return { kind: 'invalid', reason: 'reference without string uri' }; + return { kind: 'invalid', shown: REFERENCE_NO_LINK_TEXT }; } if (block.uri.length > REFERENCE_LIMITS.uri) { - return { kind: 'invalid', reason: 'uri exceeds schema maximum' }; + return { kind: 'invalid', shown: REFERENCE_LINK_TOO_LONG_TEXT }; } const out = { rejected: [] as string[], truncated: [] as string[] }; const t: ReferenceTestimony = { uri: block.uri, rejectedFields: out.rejected, truncatedFields: out.truncated }; @@ -152,7 +148,36 @@ export function isInlineContradiction(block: unknown): boolean { } export const INLINE_WITHHELD_TEXT = - '[inline content withheld: inline data that also claims a uri or a bulk disposition]'; + '[Content not shown: its inline data arrived marked as a reference too, so this host withheld it.]'; + +/* + * What the model-visible lanes (push, channel, context injection and tool + * results) put in place of a block they can't show as itself. Each says what + * kind of thing was there and why it can't be seen, in the host's words, so + * the agent doesn't read the content as complete; there's nothing for the + * agent to do about any of them. The text marker is NON_STRING_TEXT_PLACEHOLDER + * (visible-content.ts). + */ +export const IMAGE_NO_MEDIA_TYPE_TEXT = '[Image not shown: its data arrived without a media type.]'; +export const IMAGE_NO_DATA_TEXT = '[Image not shown: it arrived with no usable data or link.]'; +export const AUDIO_NO_MEDIA_TYPE_TEXT = '[Audio not shown: its data arrived without a media type.]'; +/** Audio shows only as inline data, so a link alone isn't enough either. */ +export const AUDIO_NO_DATA_TEXT = '[Audio not shown: it arrived with no usable inline data.]'; +export const REFERENCE_NO_LINK_TEXT = '[Reference not shown: it arrived without a link.]'; +export const REFERENCE_LINK_TOO_LONG_TEXT = '[Reference not shown: its link is longer than this host accepts.]'; +const UNREADABLE_BLOCK_TEXT = "[Content not shown: a part arrived in a form this host can't read.]"; + +/** A block of a type the host doesn't show, of no type at all, or no block. + * The type is the sender's word, so it's shown as a label is: bounded, with + * its controls removed. One that isn't a string is no type, and is never + * converted to one. */ +export function unknownBlockText(block: unknown): string { + if (!isRecord(block)) return UNREADABLE_BLOCK_TEXT; + const label = typeof block.type === 'string' ? sanitizeLabel(block.type, STUB_FIELD_CHARS) : ''; + return label + ? `[Content not shown: this host can't display "${label}" content.]` + : '[Content not shown: a part arrived with no type.]'; +} /** * Why a reference counts as expired, or null when it doesn't: its expiresAt @@ -177,17 +202,18 @@ export function isReferenceExpired(t: Pick `${pending.method} (id=${id})`).join(', '); + return `${this.pendingRequests.size} request(s) unanswered: ${list}`; + } + /** Close the connection: disable reconnect, kill the child process, and clean up resources. */ async close(): Promise { // Disable reconnect before closing — explicit close means stop retrying @@ -997,10 +1004,20 @@ export class McplServerConnection extends EventEmitter { this.closeIncomplete = true; this.resetPolicyForTransportBoundary(); - // Reject all pending requests - for (const [id, pending] of this.pendingRequests) { + // Reject all pending requests. Each message is the whole of what an + // agent reads for a tools/call, and reads after a channel tool's own + // words for the requests those make, so it names no method; the log line + // does. It doesn't say whether this host reconnects: a restart closes the + // connection and then connects again. + if (this.pendingRequests.size > 0) { + console.error(`MCPL server "${this.id}": this host closed its connection with ${this.unansweredRequests()}`); + } + for (const pending of this.pendingRequests.values()) { if (pending.timer) clearTimeout(pending.timer); - pending.reject(new Error(`Connection to MCPL server "${this.id}" closed while awaiting response for ${pending.method} (id=${id})`)); + pending.reject(new Error( + `The "${this.id}" server didn't answer before this host closed its connection, so whether the request took effect is unknown. ` + + 'Check its effect before trying again.', + )); } this.pendingRequests.clear(); this.orphanedRequests.clear(); @@ -1176,7 +1193,12 @@ export class McplServerConnection extends EventEmitter { options: RequestOptions = {}, ): Promise { if (this.closed) { - return Promise.reject(new Error(`Cannot send request: connection to "${this.id}" is closed`)); + return Promise.reject(new Error( + `The request wasn't sent: the connection to the "${this.id}" server is closed, so nothing ran. ` + + (this.willReconnect + ? "This host is reconnecting to it; try again once it's back." + : "This host won't reconnect to it on its own."), + )); } const id = this.nextRequestId++; @@ -1197,13 +1219,17 @@ export class McplServerConnection extends EventEmitter { this.orphanedRequests.set(id, { method, ...(method === 'push/render' ? { renderParams: { featureSet: params.featureSet, key: params.key, eventId: params.eventId } } : {}) }); if (this.orphanedRequests.size > 4096) this.orphanedRequests.delete(this.orphanedRequests.keys().next().value!); } + // The method and id are the operator's, so they go to the log. + console.error( + `MCPL server "${this.id}" did not respond to ${method} (id=${id}) within ${timeoutMs}ms; ` + + 'the request was abandoned here, not cancelled', + ); + const seconds = timeoutMs / 1000; reject(new Error( - `MCPL server "${this.id}" did not respond to ${method} (id=${id}) ` + - `within ${timeoutMs}ms — the server may be hung. The response ` + - `outcome is unknown: the request was abandoned locally but was not ` + - `cancelled, so the tool may still have completed server-side; verify ` + - `state before retrying; a blind retry of a stateful/side-effecting ` + - `tool may duplicate it.`, + `The "${this.id}" server didn't answer within ${seconds} second${seconds === 1 ? '' : 's'}, ` + + 'so whether the request took effect is unknown: ' + + 'it was abandoned here, not cancelled, and may still complete on the server. ' + + 'Check its effect before trying again, since repeating something that changes state may do it twice.', )); }, timeoutMs); // Don't hold the event loop open for the watchdog alone. @@ -1478,16 +1504,19 @@ 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. 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}: [${boundedForLog(describeCause(response.error.code))}] ${boundedForLog(describeCause(response.error.message))}`), + // The server's own words, after a word on whose they are, bounded as + // errors.ts says, since they're the server's text. Only a string is + // words: a message of any other JSON value reads as none. The method, + // id and JSON-RPC code are the operator's, so they go to the log, which + // prints any value a server can send without throwing (logValue). + const said = typeof response.error.message === 'string' ? boundedForLog(response.error.message.trim()) : ''; + console.error( + `MCPL server "${this.id}" answered ${pending.method} (id=${response.id}) with error ${logValue(response.error.code)}: ` + + logValue(response.error.message), ); + pending.reject(new Error(said + ? `The "${this.id}" server answered with an error: ${said}` + : `The "${this.id}" server answered with an error without saying what went wrong.`)); } else { pending.resolve(response.result); } @@ -1534,13 +1563,27 @@ export class McplServerConnection extends EventEmitter { if (!this.closed) { this.closed = true; this.resetPolicyForTransportBoundary(); + // A teardown the host asked for via reconnectAfterFailure() arrives + // here too; it's reported as such below. + const byHost = this.hostCloseRequested; + this.hostCloseRequested = false; - // Reject all pending requests - for (const [id, pending] of this.pendingRequests) { + // Reject all pending requests. The words hold whether the server + // dropped or this host closed the transport to recover; how it closed + // is the operator's, so it goes to the log, not into the messages. + if (this.pendingRequests.size > 0) { + const how = byHost + ? 'closed by this host' + : `code=${info.code ?? 'n/a'}, signal=${info.signal ?? 'n/a'}, reason=${closeReasonText(info)}`; + console.error(`MCPL server "${this.id}" disconnected (${how}) with ${this.unansweredRequests()}`); + } + for (const pending of this.pendingRequests.values()) { if (pending.timer) clearTimeout(pending.timer); pending.reject( new Error( - `MCPL server "${this.id}" disconnected unexpectedly (code=${info.code ?? 'n/a'}, signal=${info.signal ?? 'n/a'}, reason=${closeReasonText(info)}) while awaiting ${pending.method} (id=${id})`, + `The connection to the "${this.id}" server closed before it answered, so whether the request took effect is unknown. ` + + 'Check its effect before trying again; ' + + (this.reconnectEnabled ? 'this host is reconnecting to it.' : "this host won't reconnect to it on its own."), ), ); } @@ -1549,10 +1592,7 @@ export class McplServerConnection extends EventEmitter { // Forward the transport's close info so listeners can tell a server // that refused the credential (WebSocket 4001) from a crash, and act - // before the reconnect below redials. A teardown the host asked for - // via reconnectAfterFailure() arrives here too; report it as such. - const byHost = this.hostCloseRequested; - this.hostCloseRequested = false; + // before the reconnect below redials. const closeInfo: McplConnectionCloseInfo = { transport: transport.kind, code: info.code ?? null, diff --git a/src/mcpl/visible-content.ts b/src/mcpl/visible-content.ts index 0f6ba545c..ed63e349b 100644 --- a/src/mcpl/visible-content.ts +++ b/src/mcpl/visible-content.ts @@ -36,8 +36,9 @@ export function isVisiblyEmptyContent(content: readonly unknown[] | null | undef * block whose text isn't a string, which only a nonconformant sender writes, * so the agent learns that a block was there. It is the host's word, not the * sender's, so it never makes content visible (isVisiblyEmptyConversion). + * The other blocks the converters can't show have theirs in references.ts. */ -export const NON_STRING_TEXT_PLACEHOLDER = "[Text not shown: the server's text block had no string text.]"; +export const NON_STRING_TEXT_PLACEHOLDER = '[Text not shown: a text part arrived without readable text.]'; /** A wire text block whose text isn't a string, which the converter shows as * NON_STRING_TEXT_PLACEHOLDER. */ diff --git a/test/conversation-routing.test.ts b/test/conversation-routing.test.ts index 50db20df3..7782dd34b 100644 --- a/test/conversation-routing.test.ts +++ b/test/conversation-routing.test.ts @@ -789,7 +789,7 @@ describe('Conversation routing', () => { fw.dispatchChannelToolCall(forkName, { id: 't2', name: 'channel_close', input: { channelId: 'slack:C8' } }); assert.equal(failures.length, 2, 'both foreign channel operations should be rejected'); - assert.ok(failures[0]!.error.includes('cannot open channels'), 'channel_open is rejected outright'); + assert.ok(failures[0]!.error.includes("so it can't open other channels, and nothing was opened"), 'channel_open is rejected outright'); assert.ok(failures[1]!.error.includes('closing slack:C8 is not allowed'), 'foreign channel_close is rejected'); await framework.stop(); diff --git a/test/mcpl-agent-words.test.ts b/test/mcpl-agent-words.test.ts new file mode 100644 index 000000000..e4f397d27 --- /dev/null +++ b/test/mcpl-agent-words.test.ts @@ -0,0 +1,224 @@ +/** + * What the MCPL lanes tell an agent, from the error-wording sweep: an MCPL + * request that fails says whether it took effect, the framework's refusals of + * an MCPL tool call say whether the call ran and what can change that, and a + * block the host can't show says what kind of thing it was and why. + * + * Run: node --import tsx --test test/mcpl-agent-words.test.ts + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; + +import { AgentFramework } from '../src/framework.js'; +import { + AUDIO_NO_DATA_TEXT, + AUDIO_NO_MEDIA_TYPE_TEXT, + IMAGE_NO_DATA_TEXT, + IMAGE_NO_MEDIA_TYPE_TEXT, + INLINE_WITHHELD_TEXT, + REFERENCE_LIMITS, + REFERENCE_LINK_TOO_LONG_TEXT, + REFERENCE_NO_LINK_TEXT, + referenceStubOrNull, +} from '../src/mcpl/references.js'; +import { convertBlock } from '../src/mcpl/convert-block.js'; +import { McplServerConnection } from '../src/mcpl/server-connection.js'; +import type { McplHostCapabilities, McplServerConfig } from '../src/mcpl/types.js'; + +// --------------------------------------------------------------------------- +// Blocks the host can't show +// --------------------------------------------------------------------------- + +test("the words that stand in for a block the host can't show, which other tests take from the constants", () => { + assert.deepEqual( + [IMAGE_NO_MEDIA_TYPE_TEXT, IMAGE_NO_DATA_TEXT, AUDIO_NO_MEDIA_TYPE_TEXT, AUDIO_NO_DATA_TEXT, REFERENCE_NO_LINK_TEXT, REFERENCE_LINK_TOO_LONG_TEXT, INLINE_WITHHELD_TEXT], + [ + '[Image not shown: its data arrived without a media type.]', + '[Image not shown: it arrived with no usable data or link.]', + '[Audio not shown: its data arrived without a media type.]', + '[Audio not shown: it arrived with no usable inline data.]', + '[Reference not shown: it arrived without a link.]', + '[Reference not shown: its link is longer than this host accepts.]', + '[Content not shown: its inline data arrived marked as a reference too, so this host withheld it.]', + ], + ); +}); + +test("a reference block that can't be read says which part is missing, and what isn't a block says it can't be read", () => { + assert.equal(referenceStubOrNull({ type: 'resource', uri: 'u'.repeat(REFERENCE_LIMITS.uri + 1) }), REFERENCE_LINK_TOO_LONG_TEXT); + assert.equal(referenceStubOrNull({ type: 'resource' }), REFERENCE_NO_LINK_TEXT); + assert.equal(referenceStubOrNull(424242), "[Content not shown: a part arrived in a form this host can't read.]"); + assert.equal(referenceStubOrNull({ type: 'hologram\u0007' }), `[Content not shown: this host can't display "hologram" content.]`); + assert.equal(referenceStubOrNull({ type: '\u0007' }), '[Content not shown: a part arrived with no type.]'); + // An empty link is no link: the tool-result lane says what the push lane's + // converter says for the same block. + for (const [block, text] of [[{ type: 'image', uri: '' }, IMAGE_NO_DATA_TEXT], [{ type: 'audio', uri: '' }, AUDIO_NO_DATA_TEXT]] as const) { + assert.equal(referenceStubOrNull(block), text); + assert.deepEqual(convertBlock(block as never, 'test'), { type: 'text', text }); + } +}); + +// --------------------------------------------------------------------------- +// An MCPL request that fails +// --------------------------------------------------------------------------- + +// Stdio server: answers initialize and tools/list; tools/call "refuse" gets a +// JSON-RPC error, "mute" one with an empty message, "exit" ends the process +// with the call pending, and "hang" is never answered. +const SERVER = ` +let buf = ''; +process.stdin.on('data', (c) => { + buf += c.toString('utf8'); + let i; + while ((i = buf.indexOf('\\n')) >= 0) { + const line = buf.slice(0, i); buf = buf.slice(i + 1); + if (!line.trim()) continue; + let m; try { m = JSON.parse(line); } catch { continue; } + const send = (o) => process.stdout.write(JSON.stringify({ jsonrpc: '2.0', id: m.id, ...o }) + '\\n'); + if (m.method === 'initialize') send({ result: { capabilities: {} } }); + else if (m.method === 'tools/list') send({ result: { tools: [] } }); + else if (m.method === 'tools/call' && m.params.name === 'refuse') send({ error: { code: -32000, message: 'Missing Access' } }); + else if (m.method === 'tools/call' && m.params.name === 'mute') send({ error: { code: -32000, message: ' ' } }); + else if (m.method === 'tools/call' && m.params.name === 'exit') process.exit(3); + } +}); +setInterval(() => {}, 1 << 30); +`; + +const HOST_CAPS: McplHostCapabilities = { version: '0.4', pushEvents: true, contextHooks: { beforeInference: true }, featureSets: true }; +const config = (overrides: Partial = {}): McplServerConfig => + ({ id: 'words', command: process.execPath, args: ['-e', SERVER], ...overrides }); + +/** Whether some console.error line matches. */ +const logged = (calls: ReadonlyArray<{ arguments: unknown[] }>, line: RegExp): boolean => + calls.some((call) => line.test(String(call.arguments[0]))); + +test("a server's error answer is shown as the server's, and its method, id and code go to the log", async (t) => { + const errors = t.mock.method(console, 'error', () => {}); + const connection = await McplServerConnection.connect(config(), HOST_CAPS); + t.after(() => connection.close()); + connection.ready(); + await assert.rejects(connection.sendToolsCall('refuse', {}), + { message: 'The "words" server answered with an error: Missing Access' }); + assert.ok(logged(errors.mock.calls, /^MCPL server "words" answered tools\/call \(id=\d+\) with error -32000: /), 'the operator log has the answer'); + // An answer with no words says so, rather than ending on a colon. + await assert.rejects(connection.sendToolsCall('mute', {}), + { message: 'The "words" server answered with an error without saying what went wrong.' }); +}); + +test("a server that drops with a call pending says the outcome is unknown, and whether it's coming back", async (t) => { + const errors = t.mock.method(console, 'error', () => {}); + const once = await McplServerConnection.connect(config(), HOST_CAPS); + t.after(() => once.close()); + once.ready(); + await assert.rejects(once.sendToolsCall('exit', {}), { + message: 'The connection to the "words" server closed before it answered, so whether the request took effect is unknown. ' + + "Check its effect before trying again; this host won't reconnect to it on its own.", + }); + // The close's details are the operator's: they go to the log. + assert.ok(logged(errors.mock.calls, /^MCPL server "words" disconnected \(code=.*\) with 1 request\(s\) unanswered: tools\/call \(id=\d+\)$/), + 'the operator log has the close'); + await assert.rejects(once.sendToolsCall('refuse', {}), { + message: `The request wasn't sent: the connection to the "words" server is closed, so nothing ran. This host won't reconnect to it on its own.`, + }); + + const again = await McplServerConnection.connectWithReconnect(config({ reconnect: true, reconnectIntervalMs: 600_000 }), HOST_CAPS); + t.after(() => again.close()); + again.ready(); + await assert.rejects(again.sendToolsCall('exit', {}), { + message: 'The connection to the "words" server closed before it answered, so whether the request took effect is unknown. ' + + 'Check its effect before trying again; this host is reconnecting to it.', + }); + await assert.rejects(again.sendToolsCall('refuse', {}), { + message: `The request wasn't sent: the connection to the "words" server is closed, so nothing ran. This host is reconnecting to it; try again once it's back.`, + }); +}); + +test('a call pending when this host recycles the connection reads as a closed connection, and the log says the host closed it', async (t) => { + const errors = t.mock.method(console, 'error', () => {}); + const connection = await McplServerConnection.connectWithReconnect(config({ reconnect: true, reconnectIntervalMs: 600_000 }), HOST_CAPS); + t.after(() => connection.close()); + connection.ready(); + const pending = assert.rejects(connection.sendToolsCall('hang', {}), { + message: 'The connection to the "words" server closed before it answered, so whether the request took effect is unknown. ' + + 'Check its effect before trying again; this host is reconnecting to it.', + }); + await connection.reconnectAfterFailure(); + await pending; + assert.ok(logged(errors.mock.calls, /^MCPL server "words" disconnected \(closed by this host\) with 1 request\(s\) unanswered: tools\/call \(id=\d+\)$/), + 'the operator log says the host closed it'); +}); + +test('a call pending when this host closes the connection says the outcome is unknown, and the log names it', async (t) => { + const errors = t.mock.method(console, 'error', () => {}); + const connection = await McplServerConnection.connect(config(), HOST_CAPS); + connection.ready(); + // No word on reconnecting: a restart closes the connection, then connects again. + const pending = assert.rejects(connection.sendToolsCall('hang', {}), { + message: `The "words" server didn't answer before this host closed its connection, so whether the request took effect is unknown. ` + + 'Check its effect before trying again.', + }); + await connection.close(); + await pending; + assert.ok(logged(errors.mock.calls, /^MCPL server "words": this host closed its connection with 1 request\(s\) unanswered: tools\/call \(id=\d+\)$/), + 'the operator log has the request'); +}); + +// --------------------------------------------------------------------------- +// The framework's MCPL tool calls, and a conversation's channel guard +// --------------------------------------------------------------------------- + +type ToolResultEvent = { type: string; result?: { success: boolean; error?: string; isError?: boolean } }; + +/** A framework with only what dispatching an MCPL tool call reads. */ +function frameworkWith(server: unknown, config: Record = {}) { + const events: ToolResultEvent[] = []; + let resolve!: () => void; + const delivered = new Promise((r) => { resolve = r; }); + const framework = Object.create(AgentFramework.prototype) as Record & { + dispatchMcplToolCall(agentName: string, call: { id: string; name: string; input: unknown }, serverId: string, prefix: string): void; + executeMcplToolCall(call: { id: string; name: string; input: unknown }, serverId: string, prefix: string): Promise<{ error?: string }>; + dispatchChannelToolCall(agentName: string, call: { id: string; name: string; input: unknown }): void; + }; + framework.mcplServerRegistry = { getServer: () => server }; + framework.mcplServerConfigs = new Map([['words', { id: 'words', ...config }]]); + framework.emitTrace = () => {}; + framework.pushEvent = (event: ToolResultEvent) => { events.push(event); if (event.type === 'tool-result') resolve(); }; + return { framework, events, delivered }; +} + +test("an MCPL tool call refused before it runs says it didn't run, and whether anything can change that", async () => { + const gone = frameworkWith(null); + gone.framework.dispatchMcplToolCall('agent', { id: 'c1', name: 'words--lookup', input: {} }, 'words', 'words'); + // A server missing from the registry was closed for good, removed, or never connected. + assert.equal(gone.events[0]?.result?.error, `The "words" server that provides words--lookup isn't connected, ` + + "and this host won't reconnect to it on its own, so the call didn't run. If you need it, ask whoever runs this host."); + assert.equal((await gone.framework.executeMcplToolCall({ id: 'c2', name: 'words--lookup', input: {} }, 'words', 'words')).error, + gone.events[0]?.result?.error, "a script's call reads the same"); + + const denied = frameworkWith({}, { disabledTools: ['lookup'] }); + denied.framework.dispatchMcplToolCall('agent', { id: 'c3', name: 'words--lookup', input: {} }, 'words', 'words'); + const deniedText = `words--lookup isn't allowed by this host's policy for the "words" server, so the call didn't run, ` + + "and calling it again won't change that. If you need it, ask whoever runs this host."; + assert.equal(denied.events[0]?.result?.error, deniedText); + assert.equal((await denied.framework.executeMcplToolCall({ id: 'c4', name: 'words--lookup', input: {} }, 'words', 'words')).error, deniedText); +}); + +test("an MCPL tool's error result with no words says so, and that its effect is unknown", async () => { + const silent = frameworkWith({ sendToolsCall: async () => ({ isError: true, content: [] }) }); + silent.framework.dispatchMcplToolCall('agent', { id: 'c1', name: 'words--lookup', input: {} }, 'words', 'words'); + await silent.delivered; + const result = silent.events.find((event) => event.type === 'tool-result')!.result!; + assert.equal(result.isError, true); + assert.equal(result.error, `The "words" server reported an error for words--lookup without saying what went wrong. ` + + 'Whether it did anything is unknown: check before calling it again.'); +}); + +test("a conversation bound to its channel is told it can't open another, and where to reply", () => { + const { framework, events } = frameworkWith(null); + framework.conversationAgentHomes = new Map([['fork', 'home-1']]); + framework.toolLifecycleEmitter = undefined; + framework.dispatchChannelToolCall('fork', { id: 'c1', name: 'channel_open', input: { channelId: 'elsewhere' } }); + assert.equal(events[0]?.result?.error, + `This conversation is bound to channel "home-1", so it can't open other channels, and nothing was opened. Reply in "home-1".`); +}); diff --git a/test/mcpl-awareness-barrier.test.ts b/test/mcpl-awareness-barrier.test.ts index 23b6350d5..52c657152 100644 --- a/test/mcpl-awareness-barrier.test.ts +++ b/test/mcpl-awareness-barrier.test.ts @@ -264,8 +264,8 @@ test('mandatory awareness deadline survives requestTimeoutMs: 0 and durably rele const entry = outbox.batches()[0]!.refs[0]!; assert.equal(entry.deliveryStatus, 'pending'); assert.equal(entry.attempts, 1); - assert.match(entry.lastError ?? '', /did not respond to tools\/call/); - assert.match(entry.lastError ?? '', /outcome is unknown/i); + assert.match(entry.lastError ?? '', /didn't answer within/); + assert.match(entry.lastError ?? '', /took effect is unknown/i); assert.equal(records(statusPath).filter((record) => record.event === 'reaction-call').length, 1); const push = records(statusPath).find((record) => record.event === 'push-response'); assert.equal(push?.ledgerStatus, 'pending', 'failure was durable before inference release'); diff --git a/test/mcpl-coalesced-content-parity.test.ts b/test/mcpl-coalesced-content-parity.test.ts index 175de3ced..3de87d882 100644 --- a/test/mcpl-coalesced-content-parity.test.ts +++ b/test/mcpl-coalesced-content-parity.test.ts @@ -19,10 +19,20 @@ import assert from 'node:assert/strict'; import { fixture, TS } from './helpers/coalescing-fixture.js'; import { RefusalLines } from '../src/mcpl/refusal-lines.js'; import { NON_STRING_TEXT_PLACEHOLDER } from '../src/mcpl/visible-content.js'; +import { + AUDIO_NO_DATA_TEXT, + AUDIO_NO_MEDIA_TYPE_TEXT, + IMAGE_NO_DATA_TEXT, + IMAGE_NO_MEDIA_TYPE_TEXT, + REFERENCE_NO_LINK_TEXT, +} from '../src/mcpl/references.js'; import { HookOrchestrator } from '../src/mcpl/hook-orchestrator.js'; import { CapabilityGrant } from '../src/mcpl/capability-grant.js'; import { logDetail, logValue } from '../src/mcpl/log-value.js'; +/** What every lane shows for a block whose type isn't a string. */ +const NO_TYPE = '[Content not shown: a part arrived with no type.]'; + /** Base64 for an image well over 1 MiB once serialized (a Retina screenshot). */ const BIG_IMAGE = { type: 'image', data: 'A'.repeat(1_600_000), mimeType: 'image/png' }; @@ -243,13 +253,13 @@ test('a block whose type, text or source isn\'t a string converts on both lanes, // mimeType and uri also takes an array, a number and null. const odd = (marker: string) => [{ toString: 1 }, [marker], 424242, null]; const malformed: Array<{ content: unknown[]; shows: string }> = [ - { content: [{ type: { toString: 1 } }], shows: '[unrecognized content block: untyped]' }, - { content: [{ type: 7 }], shows: '[unrecognized content block: untyped]' }, - ...odd('array_data').map((data) => ({ content: [{ type: 'image', data, mimeType: 'image/png' }], shows: '[Image: no data]' })), - ...odd('array_uri').map((uri) => ({ content: [{ type: 'image', uri }], shows: '[Image: no data]' })), + { content: [{ type: { toString: 1 } }], shows: NO_TYPE }, + { content: [{ type: 7 }], shows: NO_TYPE }, + ...odd('array_data').map((data) => ({ content: [{ type: 'image', data, mimeType: 'image/png' }], shows: IMAGE_NO_DATA_TEXT })), + ...odd('array_uri').map((uri) => ({ content: [{ type: 'image', uri }], shows: IMAGE_NO_DATA_TEXT })), // Data whose media type is unusable is named as such, not as no data. - ...odd('array_mime').map((mimeType) => ({ content: [{ type: 'image', data: 'AAAA', mimeType }], shows: '[Image: no media type]' })), - ...odd('array_mime').map((mimeType) => ({ content: [{ type: 'audio', data: 'AAAA', mimeType }], shows: '[Audio: no media type]' })), + ...odd('array_mime').map((mimeType) => ({ content: [{ type: 'image', data: 'AAAA', mimeType }], shows: IMAGE_NO_MEDIA_TYPE_TEXT })), + ...odd('array_mime').map((mimeType) => ({ content: [{ type: 'audio', data: 'AAAA', mimeType }], shows: AUDIO_NO_MEDIA_TYPE_TEXT })), // A text block whose text isn't a string shows as its placeholder, so the // agent reads that a block was there; the text beside it shows as itself. ...odd('array_text').map((text, i) => ({ @@ -257,10 +267,10 @@ test('a block whose type, text or source isn\'t a string converts on both lanes, })), // Reference-shaped: a uri that isn't a non-empty string never makes a // reference, with a disposition or without. - { content: [{ type: 'resource', uri: 424242 }], shows: '[unrecognized content block: reference without string uri]' }, - { content: [{ type: 'image', uri: 424242, disposition: 'ref' }], shows: '[Image: no data]' }, - { content: [{ type: 'image', uri: '', disposition: 'ref' }], shows: '[Image: no data]' }, - { content: [{ type: 'audio', uri: { toString: 1 }, disposition: 'ref' }], shows: '[Audio: no data]' }, + { content: [{ type: 'resource', uri: 424242 }], shows: REFERENCE_NO_LINK_TEXT }, + { content: [{ type: 'image', uri: 424242, disposition: 'ref' }], shows: IMAGE_NO_DATA_TEXT }, + { content: [{ type: 'image', uri: '', disposition: 'ref' }], shows: IMAGE_NO_DATA_TEXT }, + { content: [{ type: 'audio', uri: { toString: 1 }, disposition: 'ref' }], shows: AUDIO_NO_DATA_TEXT }, ]; const messages: unknown[] = malformed.flatMap(({ content }, i) => [ { ...f.channel(`e${i}`, '', { initial: true }, 'chat', `coalesced${i}`), content }, @@ -340,7 +350,10 @@ test('content whose blocks aren\'t objects, or whose block of another type carri f.alwaysRespond(); // Only a text block shown as the non-string text placeholder counts as // nothing; any other placeholder still counts as something. - for (const [i, content, shows] of [[0, [424242], 'untyped'], [1, [{ type: 'video', text: 5 }], 'video']] as const) { + for (const [i, content, shows] of [ + [0, [424242], "[Content not shown: a part arrived in a form this host can't read.]"], + [1, [{ type: 'video', text: 5 }], `[Content not shown: this host can't display "video" content.]`], + ] as const) { const r = await f.send('channels/incoming', { messages: [ { channelId: 'chat', messageId: `plain${i}`, timestamp: TS, author: { id: 'u', name: 'User' }, content }, ] }); @@ -350,7 +363,7 @@ test('content whose blocks aren\'t objects, or whose block of another type carri assert.equal(plain.error, undefined); assert.equal(plain.result.accepted, true); await f.framework.runUntilIdle(); - assert.equal(occurrences(f.context(), JSON.stringify(`[unrecognized content block: ${shows}]`)), 2); + assert.equal(occurrences(f.context(), JSON.stringify(shows)), 2); } }); @@ -369,7 +382,7 @@ test('a context injection\'s text block whose text isn\'t a string shows as the const hooks = new HookOrchestrator({ getAllServers: () => [server] } as never, { validateInbound: () => {} } as never); const injections = await hooks.beforeInference({} as never); // The words themselves, which every other test takes from the constant. - const placeholder = "[Text not shown: the server's text block had no string text.]"; + const placeholder = '[Text not shown: a text part arrived without readable text.]'; assert.equal(NON_STRING_TEXT_PLACEHOLDER, placeholder); assert.deepEqual(injections.map((injection) => injection.content), [ [{ type: 'text', text: placeholder }, { type: 'text', text: 'injected_text' }], @@ -408,9 +421,9 @@ test('a malformed block in one server\'s injection shows as its placeholder, as const injections = await hooks.beforeInference({} as never); assert.deepEqual(injections.map((injection) => injection.content), [ [ - { type: 'text', text: '[unrecognized content block: untyped]' }, - { type: 'text', text: '[Image: no data]' }, - { type: 'text', text: '[Audio: no data]' }, + { type: 'text', text: NO_TYPE }, + { type: 'text', text: IMAGE_NO_DATA_TEXT }, + { type: 'text', text: AUDIO_NO_DATA_TEXT }, ], [{ type: 'text', text: 'good_text' }], ]); @@ -570,7 +583,7 @@ test('an inference filter reads the text conversion stores, a placeholder includ // doesn't handle, each of which converts to its placeholder; and visible // text. const content = [{ type: 'text', text: { toString: 1 } }, { type: 'video', uri: 'https://example.invalid/v.mp4' }, { type: 'text', text: 'visible_text' }]; - const shown = `${NON_STRING_TEXT_PLACEHOLDER}\n[unrecognized content block: video]\nvisible_text`; + const shown = `${NON_STRING_TEXT_PLACEHOLDER}\n[Content not shown: this host can't display "video" content.]\nvisible_text`; const r = await f.send('channels/incoming', { messages: [ { ...f.channel('e1', '', { initial: true }, 'chat', 'coalesced'), content }, { channelId: 'chat', messageId: 'plain', timestamp: TS, author: { id: 'u', name: 'User' }, content }, diff --git a/test/mcpl-iserror-failure.test.ts b/test/mcpl-iserror-failure.test.ts index 4e6e70422..c1e87a057 100644 --- a/test/mcpl-iserror-failure.test.ts +++ b/test/mcpl-iserror-failure.test.ts @@ -27,14 +27,19 @@ const ANSWERS: Record = { okEmpty: { content: [] }, }; -/** What each error answer reads as: its text, or `Tool call failed` with none. */ +/** What an error answer with no text reads as. */ +const NO_WORDS = + 'The "editor" server reported an error for mcpl--editor--lookup without saying what went wrong. ' + + 'Whether it did anything is unknown: check before calling it again.'; + +/** What each error answer reads as: its text, or NO_WORDS with none. */ const ERRORS: Record = { one: 'no such row', two: 'first\nsecond', notString: NON_STRING_TEXT_PLACEHOLDER, - noText: 'Tool call failed', - noContent: 'Tool call failed', - notList: 'Tool call failed', + noText: NO_WORDS, + noContent: NO_WORDS, + notList: NO_WORDS, blank: 'after a blank', image: 'see the image', }; @@ -82,7 +87,7 @@ const fields = (r: ToolResult) => ({ success: r.success, data: r.data, error: r. const lookup = (id: string, c: string) => ({ id, name: 'mcpl--editor--lookup', input: { case: c } }); -test('a server\'s error reaches an in-process caller as the failure the model\'s call reads, its text joined, a text that isn\'t a string as the placeholder, and none as "Tool call failed"', { timeout: 20_000 }, async (t) => { +test('a server\'s error reaches an in-process caller as the failure the model\'s call reads, its text joined, a text that isn\'t a string as the placeholder, and none as a failure whose effect is unknown', { timeout: 20_000 }, async (t) => { const { f, ctx, viaModel } = await setup(t); for (const [c, error] of Object.entries(ERRORS)) { const failure = { success: false, data: undefined, error, isError: true }; @@ -124,7 +129,7 @@ test('a server\'s error through the model\'s call is traced as tool:failed with await viaModel('d-one', 'mcpl--editor--lookup', { case: 'one' }); assert.deepEqual(terminal('d-one'), [{ type: 'tool:failed', module: 'mcpl:editor', error: 'no such row' }]); await viaModel('d-noText', 'mcpl--editor--lookup', { case: 'noText' }); - assert.deepEqual(terminal('d-noText'), [{ type: 'tool:failed', module: 'mcpl:editor', error: 'Tool call failed' }]); + assert.deepEqual(terminal('d-noText'), [{ type: 'tool:failed', module: 'mcpl:editor', error: NO_WORDS }]); await viaModel('d-ok', 'mcpl--editor--lookup', { case: 'ok' }); assert.deepEqual(terminal('d-ok'), [{ type: 'tool:completed', module: 'mcpl:editor' }]); }); diff --git a/test/mcpl-push-convert.test.ts b/test/mcpl-push-convert.test.ts index 519fb3c6e..16f4162de 100644 --- a/test/mcpl-push-convert.test.ts +++ b/test/mcpl-push-convert.test.ts @@ -45,5 +45,5 @@ test('undecorated uri-form image keeps url-source handling (behavior preserved)' test('unknown block type fails visibly, never undefined', () => { const out = t({ type: 'holo', frames: 9000 }); assert.equal(out.type, 'text'); - assert.ok(out.text!.includes('unrecognized content block')); + assert.equal(out.text, `[Content not shown: this host can't display "holo" content.]`); }); diff --git a/test/mcpl-reference-fetcher.test.ts b/test/mcpl-reference-fetcher.test.ts index e8e3f7100..955e8639d 100644 --- a/test/mcpl-reference-fetcher.test.ts +++ b/test/mcpl-reference-fetcher.test.ts @@ -149,7 +149,7 @@ test('third-party origin refused (v1 policy: dialed origin only)', async () => { test('vector 16: expired (and unparseable-expiry) references refuse locally, and take no eager slot', async () => { const reg = new ReferenceRegistry(); const f = makeFetcher({ srv: { url: ORIGIN } }); - for (const [expiresAt, why] of [['2001-01-01T00:00:00Z', /has passed/], ['not-a-date', /does not parse/]] as const) { + for (const [expiresAt, why] of [['2001-01-01T00:00:00Z', /^reference expired$/], ['not-a-date', /^reference treated as expired, since its expiry time can't be read$/]] as const) { // Small, sized and from the dialed origin: only its expiry keeps it from the eager default. const rec = makeRecord(reg, `${ORIGIN}/ok?e=${expiresAt}`, { expiresAt, sizeBytes: PAYLOAD.length }); assert.equal(f.isEagerEligible(rec), false, `${expiresAt}: the eager default would only be refused`); @@ -171,7 +171,7 @@ test('vector 16: an expiry rejected as a field refuses locally, with nothing dia assert.equal(f.isEagerEligible(rec), false, `${label}: the eager default would only be refused`); const out = await f.fetch(rec); assert.equal(out.ok, false, label); - assert.match(out.error!, /rejected as a field/, label); + assert.equal(out.error, "reference treated as expired, since its expiry time isn't in a form this host accepts", label); } assert.equal(saved.length, before, 'nothing saved'); assert.equal(lastAuth, 'untouched', 'the origin was never dialed'); diff --git a/test/mcpl-reference-fields.test.ts b/test/mcpl-reference-fields.test.ts index f5d690879..265962122 100644 --- a/test/mcpl-reference-fields.test.ts +++ b/test/mcpl-reference-fields.test.ts @@ -118,8 +118,8 @@ test('the stub leaves out a type, or a provenance, that sanitizes to nothing', ( test("an expired reference's stub says why, where a live one's advises fetch_reference", () => { // Each bound to its server, as a tool result's references are, so the expiry is what's left to say. - const cases = [['2001-01-01T00:00:00Z', /expired: its expiresAt has passed/], ['not-a-date', /treated as expired: its expiresAt does not parse/], - ['x'.repeat(REFERENCE_LIMITS.expiresAt + 1), /treated as expired: its expiresAt was rejected as a field/]] as const; + const cases = [['2001-01-01T00:00:00Z', /[\]—] expired$/], ['not-a-date', /[\]—] treated as expired, since its expiry time can't be read$/], + ['x'.repeat(REFERENCE_LIMITS.expiresAt + 1), /[\]—] treated as expired, since its expiry time isn't in a form this host accepts$/]] as const; for (const [i, [expiresAt, why]] of cases.entries()) { const { testimony } = reference('resource', { uri: `https://example.test/stub-expired-${i}`, expiresAt }); const stub = buildReferenceStub(testimony, undefined, 'srv'); diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index adfbdd081..68d0fb63f 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -741,13 +741,16 @@ test('a server\'s error answer that no template can convert settles the host\'s 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]', + `MCPL server "srv" answered mcpl/manifest (id=${fetch.id}) with error "{\\"toString\\":1}": "{\\"toString\\":1}"`, + '[mcpl] srv: mcpl/manifest fetch failed — previous manifest stands (§17.5): The "srv" server answered with an error without saying what went wrong.', ]); 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 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 () => { +test('a server\'s error message in the error a host request rejects with is bounded: control, format and line characters replaced, and cut after 200 code points; its code goes to the log line instead, as logValue prints it', async (t) => { + const lines: string[] = []; + t.mock.method(console, 'error', (...args: unknown[]) => { lines.push(args.map(String).join(' ')); }); const transport = new ResponseTransport(); const connection = new (McplServerConnection as any)('srv', null, transport) as McplServerConnection; connection.establishGrant(new CapabilityGrant(new Set(ALL_CAPABILITY_PATHS), [])); @@ -756,7 +759,7 @@ test('a server\'s error code and message in the error a host request rejects wit 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)}…`, + message: `The "srv" server answered with an error: 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. @@ -764,8 +767,10 @@ test('a server\'s error code and message in the error a host request rejects wit 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`, + message: 'The "srv" server answered with an error: refused', }); + // The code is the operator's, so it's in the log line, escaped and cut there. + assert.ok(lines.some((line) => line.startsWith(`MCPL server "srv" answered mcpl/manifest (id=${second.id}) with error "E\\nx\\u202e${'y'.repeat(252)}"…(304): refused`)), lines.join('\n')); }); test('a throw from the wired tools/list_changed handler is contained, traced and logged as the other handlers\' are', (t) => { diff --git a/test/mcpl-request-timeout.test.ts b/test/mcpl-request-timeout.test.ts index 1491ff093..26bdca371 100644 --- a/test/mcpl-request-timeout.test.ts +++ b/test/mcpl-request-timeout.test.ts @@ -72,7 +72,8 @@ afterEach(async () => { connection = null; }); -test('tools/call against a hung server rejects within the configured timeout', async () => { +test('tools/call against a hung server rejects within the configured timeout', async (t) => { + const errors = t.mock.method(console, 'error', () => {}); connection = await McplServerConnection.connect(config({ requestTimeoutMs: 250 }), HOST_CAPS); connection.ready(); @@ -80,9 +81,11 @@ test('tools/call against a hung server rejects within the configured timeout', a await assert.rejects( connection.sendToolsCall('stuck', {}), (err: Error) => { - assert.match(err.message, /did not respond to tools\/call/); - assert.match(err.message, /250ms/); - assert.match(err.message, /hung/); + // The agent's words name no method; the log keeps it and the id. + assert.match(err.message, /^The "hung" server didn't answer within 0\.25 seconds, so whether the request took effect is unknown/); + assert.ok(!err.message.includes('tools/call'), err.message); + assert.ok(errors.mock.calls.some((call) => /^MCPL server "hung" did not respond to tools\/call \(id=\d+\) within 250ms/ + .test(String(call.arguments[0]))), 'the operator log has the method and id'); return true; }, ); @@ -135,8 +138,8 @@ test('the timeout error warns the model the tool may have completed server-side' connection.sendToolsCall('stuck', {}), (err: Error) => { // The model reads this string; it must not imply the call was cancelled. - assert.match(err.message, /may still have completed server-side/); - assert.match(err.message, /verify state before retrying/); + assert.match(err.message, /it was abandoned here, not cancelled, and may still complete on the server/); + assert.match(err.message, /Check its effect before trying again/); return true; }, ); @@ -160,7 +163,7 @@ test('a timed-out request that later receives a response does not crash (orphane const orphaned: unknown[] = []; connection.on('orphaned-response', (info) => orphaned.push(info)); - await assert.rejects(connection.sendToolsCall('stuck', {}), /did not respond/); + await assert.rejects(connection.sendToolsCall('stuck', {}), /didn't answer within/); liftRequestTimeout(connection); // The next tools/list makes the stub write the late reply ahead of the // list's own, on the same stream, so by the time the list resolves the late @@ -188,7 +191,7 @@ test('a late STATEFUL orphaned response is surfaced via mcpl:orphaned-response ( const orphaned: Array<{ hadState: boolean; hadCheckpoint: boolean }> = []; connection.on('orphaned-response', (info) => orphaned.push(info)); - await assert.rejects(connection.sendToolsCall('stuck', {}), /did not respond/); + await assert.rejects(connection.sendToolsCall('stuck', {}), /didn't answer within/); liftRequestTimeout(connection); // The next tools/list releases the late reply ahead of its own answer, so it // has been handled by the time the list resolves, and the connection still @@ -215,7 +218,7 @@ test('a mandatory call deadline overrides requestTimeoutMs: 0 and surfaces its l connection.on('orphaned-response', (info) => orphaned.push(info)); await assert.rejects( connection.sendToolsCallWithDeadline('stuck', {}, 75), - /outcome is unknown.*verify state before retrying/i, + /took effect is unknown.*Check its effect before trying again/i, ); // Only the deadline could settle the call, since the server sends nothing // until the next tools/list, which releases the late reply ahead of its own. diff --git a/test/mcpl-tool-lifecycle-e2e.test.ts b/test/mcpl-tool-lifecycle-e2e.test.ts index 57940610a..956d8633e 100644 --- a/test/mcpl-tool-lifecycle-e2e.test.ts +++ b/test/mcpl-tool-lifecycle-e2e.test.ts @@ -266,7 +266,7 @@ describe('tool lifecycle end to end (RFC-007)', () => { await waitFor(() => membrane.calls.length > callsBefore && (membrane.lastStream?.receivedToolResults.length ?? 0) >= 1, 'the refused call answered'); const answer = JSON.stringify(membrane.lastStream!.receivedToolResults[0]); - assert.match(answer, /cannot open channels/, 'the guard did refuse it'); + assert.match(answer, /can't open other channels, and nothing was opened/, 'the guard did refuse it'); await new Promise((r) => setTimeout(r, 300)); const after = readLog(obsLog).filter((e) => e.event === 'lifecycle').length; assert.equal(after, before, 'a refused host-tool call produces no events'); diff --git a/test/mcpl-tool-resolution.test.ts b/test/mcpl-tool-resolution.test.ts index 765cc11bf..d366e0971 100644 --- a/test/mcpl-tool-resolution.test.ts +++ b/test/mcpl-tool-resolution.test.ts @@ -211,7 +211,8 @@ test("a call of a tool its server's policy denies is refused on every path, not const h = await standIns([{ id: 'short', prefix: 'foo', lists: ['secret', 'echo'], disabledTools: ['secret'] }]); try { for (const path of PATHS) { - assert.equal(await h.call(path, 'foo--secret'), "refused: Tool 'foo--secret' is not permitted by this server's tool policy.", `by ${path}`); + assert.equal(await h.call(path, 'foo--secret'), `refused: foo--secret isn't allowed by this host's policy for the "short" server, ` + + "so the call didn't run, and calling it again won't change that. If you need it, ask whoever runs this host.", `by ${path}`); } assert.equal(await h.call('executeToolCall', 'foo--echo'), 'short:echo', 'an allowed tool is sent'); } finally { @@ -238,10 +239,11 @@ test('a name an MCPL prefix fits goes to MCPL on every path, even with no config try { // A prefix with neither a config nor a server: refused as MCPL, never handed to a module. h.internals.mcplPrefixMap.set('ghost', 'ghost'); - for (const path of ['executeToolCall', 'ctx.callTool'] as const) { - assert.equal(await h.call(path, 'ghost--echo'), 'refused: MCPL server ghost not found', `by ${path}`); + // Every path says the same: the server isn't connected, and won't be on its own. + for (const path of ['executeToolCall', 'ctx.callTool', 'model'] as const) { + assert.equal(await h.call(path, 'ghost--echo'), `refused: The "ghost" server that provides ghost--echo isn't connected, ` + + "and this host won't reconnect to it on its own, so the call didn't run. If you need it, ask whoever runs this host.", `by ${path}`); } - assert.equal(await h.call('model', 'ghost--echo'), 'refused: MCPL server not found: ghost', 'by model'); // A server with no config is still sent the call. h.internals.mcplServerConfigs.delete('short'); for (const path of PATHS) assert.equal(await h.call(path, 'foo--echo'), 'short:echo', `by ${path}`); diff --git a/test/mcpl-transport-close.test.ts b/test/mcpl-transport-close.test.ts index 50080d5dc..b9dcdf147 100644 --- a/test/mcpl-transport-close.test.ts +++ b/test/mcpl-transport-close.test.ts @@ -339,7 +339,7 @@ test('a server that closes its stdin is ended: a request fails with the disconne const answer = connection.sendManifestRequest(30_000).then(() => 'answered', (error: Error) => error.message); assert.match( await within(answer, 'still waiting after 15 s', 15_000), - /^MCPL server "deaf-after-initialize" disconnected unexpectedly \(code=n\/a, signal=SIGTERM, reason=child process exited, and its pipe failed with write EPIPE\) while awaiting mcpl\/manifest/, + /^The connection to the "deaf-after-initialize" server closed before it answered, so whether the request took effect is unknown\. Check its effect before trying again; this host is reconnecting to it\.$/, ); assert.equal((await within(closed, null, 15_000))?.reason, 'child process exited, and its pipe failed with write EPIPE'); assert.equal(await within(reconnected.then(() => 'reconnected'), 'no reconnect within 15 s', 15_000), 'reconnected'); @@ -409,7 +409,7 @@ test("a server that stops reading its stdin without closing it stays connected, // Nothing fails the write, so only the request's own 500 ms timeout ends it. const answer = connection.sendManifestRequest(500).then(() => 'answered', (error: Error) => error.message); - assert.match(await within(answer, 'still waiting after 5 s'), /^MCPL server "stops-reading" did not respond to mcpl\/manifest \(id=\d+\) within 500ms/); + assert.match(await within(answer, 'still waiting after 5 s'), /^The "stops-reading" server didn't answer within 0\.5 seconds, so whether the request took effect is unknown/); assert.equal(closed, false, 'nothing closed the connection'); assert.equal(connection.isConnected, true, 'the connection is still up'); }); diff --git a/test/mcpl-ws-transport.test.ts b/test/mcpl-ws-transport.test.ts index 8e99befa7..b572e54a2 100644 --- a/test/mcpl-ws-transport.test.ts +++ b/test/mcpl-ws-transport.test.ts @@ -373,7 +373,8 @@ test("a server's close reason stays out of the error a pending request is reject await assert.rejects( withDeadline(conn.sendToolsCall('echo', { text: 'hello' }), 10_000, 'the call to be rejected'), (error: Error) => { - assert.match(error.message, /^MCPL server "editor" disconnected unexpectedly \(code=4001, signal=n\/a, reason=websocket closed\) while awaiting tools\/call \(id=\d+\)$/); + assert.equal(error.message, 'The connection to the "editor" server closed before it answered, so whether the request took effect is unknown. ' + + "Check its effect before trying again; this host won't reconnect to it on its own."); return true; }, ); diff --git a/test/save-recent-image.test.ts b/test/save-recent-image.test.ts index b0fc6ac05..13e5694cd 100644 --- a/test/save-recent-image.test.ts +++ b/test/save-recent-image.test.ts @@ -563,7 +563,7 @@ describe('save_recent_image provenance (issue #104)', () => { toolUseId: 'call_cam', toolName: 'mcpl--vst--camera', content: `[${kept.refId}] kept.png — image/png — from tool result — saved: files/refs/kept.png\n` - + `[${expired.refId}] expired.png — image/png — from tool result — expired: its expiresAt has passed`, + + `[${expired.refId}] expired.png — image/png — from tool result — expired`, isError: false, }] as ContentBlock[]); }, @@ -572,7 +572,7 @@ describe('save_recent_image provenance (issue #104)', () => { const refused = await waitForToolResult(h.framework, 'call_save0'); assert.strictEqual(refused.isError, true); assert.match(refused.content, new RegExp(`index 0 is a reference \\(${expired.refId}, image/png, from`)); - assert.match(refused.content, /It can't be fetched either — expired: its expiresAt has passed\./); + assert.match(refused.content, /It can't be fetched either — expired\./); assert.ok(!refused.content.includes('fetch_reference'), refused.content); const keptRefusal = await waitForToolResult(h.framework, 'call_save1'); assert.strictEqual(keptRefusal.isError, true); diff --git a/test/tool-result-reference-uri.test.ts b/test/tool-result-reference-uri.test.ts index 27f36dabe..c16402d29 100644 --- a/test/tool-result-reference-uri.test.ts +++ b/test/tool-result-reference-uri.test.ts @@ -24,7 +24,7 @@ const RESOURCE = { type: 'resource', uri: URI, mimeType: 'application/pdf', name const UNKNOWN = { type: 'zz-unknown', note: 'zz-kept' }; const STUB = /\[ref_[^\]]+\] zz\.pdf — application\/pdf — from tool result/; const BASE64 = 'aGVsbG8gd29ybGQgaGVsbG8gd29ybGQ='; -const WITHHELD = '[inline content withheld: inline data that also claims a uri or a bulk disposition]'; +const WITHHELD = '[Content not shown: its inline data arrived marked as a reference too, so this host withheld it.]'; function parsed(text: string): unknown { return JSON.parse(text); @@ -77,11 +77,11 @@ describe('data the serializer doesn\'t read as content blocks', () => { it('withholds what the reference lanes withhold, saying why', () => { const notAString = parsed(toolResultDataToHistoryString([UNKNOWN, { type: 'resource', uri: { sig: SIGNATURE } }])) as [unknown, string]; - assert.equal(notAString[1], '[unrecognized content block: reference without string uri]'); + assert.equal(notAString[1], '[Reference not shown: it arrived without a link.]'); const tooLong = parsed(toolResultDataToHistoryString([UNKNOWN, { type: 'resource', uri: `${URI}${'x'.repeat(5000)}` }])) as [unknown, string]; - assert.equal(tooLong[1], '[unrecognized content block: uri exceeds schema maximum]'); + assert.equal(tooLong[1], '[Reference not shown: its link is longer than this host accepts.]'); const objectUri = parsed(toolResultDataToHistoryString([UNKNOWN, { type: 'image', uri: { href: URI } }])) as [unknown, string]; - assert.equal(objectUri[1], "[unrecognized content block: image block whose uri isn't a string]"); + assert.equal(objectUri[1], '[Image not shown: it arrived with no usable data or link.]'); const smuggled = toolResultDataToHistoryString([UNKNOWN, { type: 'image', data: BASE64, mimeType: 'image/png', uri: URI }]); assert.ok(!smuggled.includes(SIGNATURE) && !smuggled.includes(BASE64), smuggled); assert.deepEqual(parsed(smuggled), [UNKNOWN, WITHHELD]); @@ -145,7 +145,7 @@ describe('data the serializer doesn\'t read as content blocks', () => { it("governs MCP's embedded resource, whose uri sits under resource, at any depth", () => { const embedded = { type: 'resource', resource: { uri: URI, mimeType: 'application/pdf', blob: BASE64 } }; - const line = '[unrecognized content block: reference without string uri]'; + const line = '[Reference not shown: it arrived without a link.]'; const nested = toolResultDataToHistoryString({ result: { content: [embedded] } }); assert.ok(!nested.includes(SIGNATURE) && !nested.includes(BASE64), nested); assert.deepEqual(parsed(nested), { result: { content: [line] } }); @@ -156,18 +156,18 @@ describe('data the serializer doesn\'t read as content blocks', () => { assert.equal(toolResultDataToHistoryString([{ type: 'text', text: 'zz-text' }, embedded]), `zz-text\n${line}`); }); - it('governs media given a uri that is not a string, failing closed and saying which', () => { + it('governs media given a uri that is not a string, failing closed as the push lane does', () => { const text = toolResultDataToHistoryString({ numeric: { type: 'image', uri: 42, name: 'zz-n' }, both: { type: 'audio', data: [4, 5], uri: 43 }, }); assert.ok(!text.includes('4,5'), text); assert.deepEqual(parsed(text), { - numeric: "[unrecognized content block: image block whose uri isn't a string]", - both: "[unrecognized content block: audio block whose data and uri aren't strings]", + numeric: '[Image not shown: it arrived with no usable data or link.]', + both: '[Audio not shown: it arrived with no usable inline data.]', }); - // In the lanes, a media block with neither keeps the line it had. - assert.deepEqual(classifyBlock({ type: 'image' }), { kind: 'invalid', reason: 'image block with neither data nor uri' }); + // A media block with neither reads as it does with fields that aren't strings. + assert.deepEqual(classifyBlock({ type: 'image' }), { kind: 'invalid', shown: '[Image not shown: it arrived with no usable data or link.]' }); }); it("leaves a nested reference's stub out of save_recent_image's inventory, as main left the reference", () => { @@ -337,6 +337,6 @@ describe('a script given such data', () => { } assert.ok(!text.includes(SIGNATURE), text); assert.match(text, /^zz-text\n\[ref_[^\]]+\] zz\.pdf — application\/pdf — from tool result/); - assert.ok(logged.some((line) => /\[rfc005\] script-demand fetch of ref_\S+ failed: reference unbound: no server is recorded to fetch it from/.test(line)), logged.join('\n')); + assert.ok(logged.some((line) => /\[rfc005\] script-demand fetch of ref_\S+ failed: reference not tied to any server, so there's nowhere to fetch it from/.test(line)), logged.join('\n')); }); }); From 90b1519c4d54b44d0543a0eef44a757ae55a2944 Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sun, 18 Oct 2026 04:10:27 -0400 Subject: [PATCH 02/11] fix(mcpl): the af-w4a round: remedies where this host won't reconnect, the close's details kept in every log line, and the tests the sheet asked for From af-w4a's sheet (wf-2582, verdicts recorded): - I4: where this host won't reconnect, the close text and the not-sent text end with "If you need it, ask whoever runs this host.", the remedy the framework's text for the same state gives; the close text no longer says "before trying again" there, since trying again can't help. - I14: a reconnecting server's not-sent text says "try again later", since an agent has no sign of when it's back. - I21: the close log line keeps code, signal and reason in every case, and adds "closed by this host, " when the host asked for the close, so a drop that races a recycle loses nothing. - I11: the two helpers sit above sniffImageMediaType's doc comment. - I12, I13: AUDIO_NO_DATA_TEXT's comment names the lanes it holds on; closeReasonText's comment says its words go to the log line. - Tests: the WebSocket close's log line, with no peer text (I8); a block whose type is a number (I9); a numeric error message (I10); a one-second timeout in the singular (I16); closes with nothing pending log nothing (I19); the error-result await is bounded (I25). - The changelog fragment this change lacked, mcpl-agent-words.changed.md. --- changelog.d/mcpl-agent-words.changed.md | 11 +++++ src/framework.ts | 6 +-- src/mcpl/references.ts | 4 +- src/mcpl/server-connection.ts | 33 ++++++++------- test/mcpl-agent-words.test.ts | 56 +++++++++++++++++++++---- test/mcpl-ws-transport.test.ts | 9 +++- 6 files changed, 90 insertions(+), 29 deletions(-) create mode 100644 changelog.d/mcpl-agent-words.changed.md diff --git a/changelog.d/mcpl-agent-words.changed.md b/changelog.d/mcpl-agent-words.changed.md new file mode 100644 index 000000000..e167b0f47 --- /dev/null +++ b/changelog.d/mcpl-agent-words.changed.md @@ -0,0 +1,11 @@ +- The MCPL texts an agent reads for a failed request, a refused tool call and a block the host can't show now say what happened, whether anything took effect, and what works from there, or that nothing does. Method names, request ids, JSON-RPC codes, close details and field names leave them. The ones an operator needs go to the host's log. + - **A failed MCPL request:** + - A connection that closes with a call pending reads `The connection to the "" server closed before it answered, so whether the request took effect is unknown.` It then says whether this host is reconnecting, where it read `MCPL server "" disconnected unexpectedly (code=…, signal=…, reason=…) while awaiting (id=N)`. + - The other texts change the same way: a close by this host, a request on a closed connection, a server's error answer (in the server's own words) and a timeout. + - The log gains a line for each with the details the texts dropped. For a close with requests pending, that's its code, signal and reason, whether this host closed it, and the requests left unanswered. + - **The framework's MCPL tool calls:** + - A call to a server missing from the registry, or to a tool its policy denies, says it didn't run and whether anything can change that, in the same words for the model's call and a script's. + - An error result with no text reads as one whose effect is unknown, where it read `Tool call failed`. + - A conversation's `channel_open` is told where to reply. + - **Blocks the host can't show** read as what they were and why: `[Image not shown: …]`, `[Audio not shown: …]`, `[Reference not shown: …]`, `[Content not shown: …]`, and the text marker `[Text not shown: a text part arrived without readable text.]`. They had read `[Image: no data]`, `[unrecognized content block: …]`, `[inline content withheld: …]` and `[Text not shown: the server's text block had no string text.]`. An image or audio block with an empty link reads as having no data or link on the tool-result lane too. + - **A reference's expiry and missing server** read `expired`, `treated as expired, since its expiry time can't be read`, `treated as expired, since its expiry time isn't in a form this host accepts` and `not tied to any server, so there's nowhere to fetch it from`. They had read `expired: its expiresAt has passed`, `… (fail closed)` and `unbound: no server is recorded to fetch it from`. diff --git a/src/framework.ts b/src/framework.ts index 6cbcaddec..5edb8482f 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -136,9 +136,6 @@ import { splitProseSegments, withoutRoundsProse } from './prose-segments.js'; import { cumulativeDelta } from './usage-accounting.js'; import { stampThinkingTokenEstimates } from './thinking-token-stamp.js'; -/** Detect a supported image media type from magic bytes (the model API - * rejects mislabeled media types, so trust bytes over extensions). - * Returns undefined for non-image content. */ /** An MCPL tool call refused because its server isn't in the registry: one * closed for good, one removed, or one never connected, which this host * doesn't bring back on its own. A server that's reconnecting stays @@ -156,6 +153,9 @@ function mcplToolDeniedText(serverId: string, toolName: string): string { "and calling it again won't change that. If you need it, ask whoever runs this host."; } +/** Detect a supported image media type from magic bytes (the model API + * rejects mislabeled media types, so trust bytes over extensions). + * Returns undefined for non-image content. */ function sniffImageMediaType(data: Buffer): string | undefined { if (data.length >= 4 && data[0] === 0x89 && data[1] === 0x50 && data[2] === 0x4e && data[3] === 0x47) { return 'image/png'; diff --git a/src/mcpl/references.ts b/src/mcpl/references.ts index 59d417286..92812b1a0 100644 --- a/src/mcpl/references.ts +++ b/src/mcpl/references.ts @@ -161,7 +161,9 @@ export const INLINE_WITHHELD_TEXT = export const IMAGE_NO_MEDIA_TYPE_TEXT = '[Image not shown: its data arrived without a media type.]'; export const IMAGE_NO_DATA_TEXT = '[Image not shown: it arrived with no usable data or link.]'; export const AUDIO_NO_MEDIA_TYPE_TEXT = '[Audio not shown: its data arrived without a media type.]'; -/** Audio shows only as inline data, so a link alone isn't enough either. */ +/** On the push, channel and context-injection lanes, audio shows only as + * inline data, so a plain link isn't enough either. (A tool result's audio + * with a link is a reference, as any block with one is there.) */ export const AUDIO_NO_DATA_TEXT = '[Audio not shown: it arrived with no usable inline data.]'; export const REFERENCE_NO_LINK_TEXT = '[Reference not shown: it arrived without a link.]'; export const REFERENCE_LINK_TOO_LONG_TEXT = '[Reference not shown: its link is longer than this host accepts.]'; diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index f74486a4d..53448766d 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -95,15 +95,16 @@ export function withStderr(error: unknown): string { return '. The last of its stderr:' + tail.split('\n').map((line) => `\n stderr: ${logDetail(line)}`).join(''); } -/** A transport close's reason, as an error message gives it: the transport's - * own words. A WebSocket peer's close reason is the server's text - * (`reasonFromPeer`), so an error gives the words the transport uses for a - * close without one, as `mcpl:server-closed` leaves that text out. A +/** A transport close's reason, as the host's log and the handshake's error + * give it: the transport's own words. A WebSocket peer's close reason is the + * server's text (`reasonFromPeer`), so they give the words the transport uses + * for a close without one, as `mcpl:server-closed` leaves that text out. A * WebSocket close's code, a number, says what the server meant (4001: a - * refused credential), with or without a reason, so each error gives it too: - * a pending request's error has `code=`, and the handshake's adds it for any - * WebSocket close, since a close before the handshake completes reaches no - * 'close' event. A stdio child's exit reads as before. */ + * refused credential), with or without a reason, so each gives it too: the + * log line for a close with requests pending has `code=`, and the + * handshake's error adds it for any WebSocket close, since a close before the + * handshake completes reaches no 'close' event. A stdio child's exit reads + * as before. */ function closeReasonText(info: TransportCloseInfo): string { if (info.reasonFromPeer) return 'websocket closed'; return info.reason ?? 'unknown'; @@ -1196,8 +1197,8 @@ export class McplServerConnection extends EventEmitter { return Promise.reject(new Error( `The request wasn't sent: the connection to the "${this.id}" server is closed, so nothing ran. ` + (this.willReconnect - ? "This host is reconnecting to it; try again once it's back." - : "This host won't reconnect to it on its own."), + ? 'This host is reconnecting to it; try again later.' + : "This host won't reconnect to it on its own. If you need it, ask whoever runs this host."), )); } @@ -1572,9 +1573,10 @@ export class McplServerConnection extends EventEmitter { // dropped or this host closed the transport to recover; how it closed // is the operator's, so it goes to the log, not into the messages. if (this.pendingRequests.size > 0) { - const how = byHost - ? 'closed by this host' - : `code=${info.code ?? 'n/a'}, signal=${info.signal ?? 'n/a'}, reason=${closeReasonText(info)}`; + // Whatever the host asked for, the close's own details are kept, so + // a drop that races a recycle loses nothing. + const how = `${byHost ? 'closed by this host, ' : ''}` + + `code=${info.code ?? 'n/a'}, signal=${info.signal ?? 'n/a'}, reason=${closeReasonText(info)}`; console.error(`MCPL server "${this.id}" disconnected (${how}) with ${this.unansweredRequests()}`); } for (const pending of this.pendingRequests.values()) { @@ -1582,8 +1584,9 @@ export class McplServerConnection extends EventEmitter { pending.reject( new Error( `The connection to the "${this.id}" server closed before it answered, so whether the request took effect is unknown. ` + - 'Check its effect before trying again; ' + - (this.reconnectEnabled ? 'this host is reconnecting to it.' : "this host won't reconnect to it on its own."), + (this.reconnectEnabled + ? 'Check its effect before trying again; this host is reconnecting to it.' + : "This host won't reconnect to it on its own. If you need it, ask whoever runs this host."), ), ); } diff --git a/test/mcpl-agent-words.test.ts b/test/mcpl-agent-words.test.ts index e4f397d27..c0d319c0e 100644 --- a/test/mcpl-agent-words.test.ts +++ b/test/mcpl-agent-words.test.ts @@ -50,6 +50,7 @@ test("a reference block that can't be read says which part is missing, and what assert.equal(referenceStubOrNull(424242), "[Content not shown: a part arrived in a form this host can't read.]"); assert.equal(referenceStubOrNull({ type: 'hologram\u0007' }), `[Content not shown: this host can't display "hologram" content.]`); assert.equal(referenceStubOrNull({ type: '\u0007' }), '[Content not shown: a part arrived with no type.]'); + assert.equal(referenceStubOrNull({ type: 42 }), '[Content not shown: a part arrived with no type.]'); // An empty link is no link: the tool-result lane says what the push lane's // converter says for the same block. for (const [block, text] of [[{ type: 'image', uri: '' }, IMAGE_NO_DATA_TEXT], [{ type: 'audio', uri: '' }, AUDIO_NO_DATA_TEXT]] as const) { @@ -63,8 +64,9 @@ test("a reference block that can't be read says which part is missing, and what // --------------------------------------------------------------------------- // Stdio server: answers initialize and tools/list; tools/call "refuse" gets a -// JSON-RPC error, "mute" one with an empty message, "exit" ends the process -// with the call pending, and "hang" is never answered. +// JSON-RPC error, "mute" one with an empty message, "odd" one whose message +// isn't a string, "exit" ends the process with the call pending, +// "answer-then-exit" answers and then ends it, and "hang" is never answered. const SERVER = ` let buf = ''; process.stdin.on('data', (c) => { @@ -79,6 +81,8 @@ process.stdin.on('data', (c) => { else if (m.method === 'tools/list') send({ result: { tools: [] } }); else if (m.method === 'tools/call' && m.params.name === 'refuse') send({ error: { code: -32000, message: 'Missing Access' } }); else if (m.method === 'tools/call' && m.params.name === 'mute') send({ error: { code: -32000, message: ' ' } }); + else if (m.method === 'tools/call' && m.params.name === 'odd') send({ error: { code: -32000, message: 42 } }); + else if (m.method === 'tools/call' && m.params.name === 'answer-then-exit') { send({ result: { content: [] } }); setTimeout(() => process.exit(0), 50); } else if (m.method === 'tools/call' && m.params.name === 'exit') process.exit(3); } }); @@ -89,6 +93,13 @@ const HOST_CAPS: McplHostCapabilities = { version: '0.4', pushEvents: true, cont const config = (overrides: Partial = {}): McplServerConfig => ({ id: 'words', command: process.execPath, args: ['-e', SERVER], ...overrides }); +/** `promise`, or a failure naming `what` after five seconds, so a regression fails rather than hangs. */ +async function within(promise: Promise, what: string): Promise { + let timer: ReturnType | undefined; + const late = new Promise((_, reject) => { timer = setTimeout(() => reject(new Error(`no ${what} within 5 s`)), 5000); }); + try { return await Promise.race([promise, late]); } finally { clearTimeout(timer); } +} + /** Whether some console.error line matches. */ const logged = (calls: ReadonlyArray<{ arguments: unknown[] }>, line: RegExp): boolean => calls.some((call) => line.test(String(call.arguments[0]))); @@ -104,6 +115,10 @@ test("a server's error answer is shown as the server's, and its method, id and c // An answer with no words says so, rather than ending on a colon. await assert.rejects(connection.sendToolsCall('mute', {}), { message: 'The "words" server answered with an error without saying what went wrong.' }); + // A message that isn't a string reads the same, and the log keeps what was sent. + await assert.rejects(connection.sendToolsCall('odd', {}), + { message: 'The "words" server answered with an error without saying what went wrong.' }); + assert.ok(logged(errors.mock.calls, /^MCPL server "words" answered tools\/call \(id=\d+\) with error -32000: 42$/), 'the log has the number'); }); test("a server that drops with a call pending says the outcome is unknown, and whether it's coming back", async (t) => { @@ -113,13 +128,13 @@ test("a server that drops with a call pending says the outcome is unknown, and w once.ready(); await assert.rejects(once.sendToolsCall('exit', {}), { message: 'The connection to the "words" server closed before it answered, so whether the request took effect is unknown. ' + - "Check its effect before trying again; this host won't reconnect to it on its own.", + "This host won't reconnect to it on its own. If you need it, ask whoever runs this host.", }); // The close's details are the operator's: they go to the log. assert.ok(logged(errors.mock.calls, /^MCPL server "words" disconnected \(code=.*\) with 1 request\(s\) unanswered: tools\/call \(id=\d+\)$/), 'the operator log has the close'); await assert.rejects(once.sendToolsCall('refuse', {}), { - message: `The request wasn't sent: the connection to the "words" server is closed, so nothing ran. This host won't reconnect to it on its own.`, + message: `The request wasn't sent: the connection to the "words" server is closed, so nothing ran. This host won't reconnect to it on its own. If you need it, ask whoever runs this host.`, }); const again = await McplServerConnection.connectWithReconnect(config({ reconnect: true, reconnectIntervalMs: 600_000 }), HOST_CAPS); @@ -130,7 +145,7 @@ test("a server that drops with a call pending says the outcome is unknown, and w 'Check its effect before trying again; this host is reconnecting to it.', }); await assert.rejects(again.sendToolsCall('refuse', {}), { - message: `The request wasn't sent: the connection to the "words" server is closed, so nothing ran. This host is reconnecting to it; try again once it's back.`, + message: `The request wasn't sent: the connection to the "words" server is closed, so nothing ran. This host is reconnecting to it; try again later.`, }); }); @@ -145,8 +160,33 @@ test('a call pending when this host recycles the connection reads as a closed co }); await connection.reconnectAfterFailure(); await pending; - assert.ok(logged(errors.mock.calls, /^MCPL server "words" disconnected \(closed by this host\) with 1 request\(s\) unanswered: tools\/call \(id=\d+\)$/), - 'the operator log says the host closed it'); + assert.ok(logged(errors.mock.calls, /^MCPL server "words" disconnected \(closed by this host, code=.*, signal=.*, reason=.*\) with 1 request\(s\) unanswered: tools\/call \(id=\d+\)$/), + 'the operator log says the host closed it, and keeps the close\'s details'); +}); + +test('a timeout of one second says so in the singular', async (t) => { + t.mock.method(console, 'error', () => {}); + const connection = await McplServerConnection.connect(config({ requestTimeoutMs: 1000 }), HOST_CAPS); + t.after(() => connection.close()); + connection.ready(); + await assert.rejects(connection.sendToolsCall('hang', {}), (error: Error) => { + assert.match(error.message, /^The "words" server didn't answer within 1 second, so whether the request took effect is unknown: /); + return true; + }); +}); + +test('a connection that closes with nothing pending logs nothing new, by this host or by the server', async (t) => { + const errors = t.mock.method(console, 'error', () => {}); + const byHost = await McplServerConnection.connect(config(), HOST_CAPS); + byHost.ready(); + await byHost.close(); + const dropped = await McplServerConnection.connect(config(), HOST_CAPS); + t.after(() => dropped.close()); + dropped.ready(); + const closed = new Promise((resolve) => { dropped.once('close', () => resolve()); }); + await dropped.sendToolsCall('answer-then-exit', {}); + await within(closed, 'the server to exit'); + assert.ok(!logged(errors.mock.calls, /unanswered/), errors.mock.calls.map((call) => String(call.arguments[0])).join('\n')); }); test('a call pending when this host closes the connection says the outcome is unknown, and the log names it', async (t) => { @@ -207,7 +247,7 @@ test("an MCPL tool call refused before it runs says it didn't run, and whether a test("an MCPL tool's error result with no words says so, and that its effect is unknown", async () => { const silent = frameworkWith({ sendToolsCall: async () => ({ isError: true, content: [] }) }); silent.framework.dispatchMcplToolCall('agent', { id: 'c1', name: 'words--lookup', input: {} }, 'words', 'words'); - await silent.delivered; + await within(silent.delivered, 'the tool result'); const result = silent.events.find((event) => event.type === 'tool-result')!.result!; assert.equal(result.isError, true); assert.equal(result.error, `The "words" server reported an error for words--lookup without saying what went wrong. ` + diff --git a/test/mcpl-ws-transport.test.ts b/test/mcpl-ws-transport.test.ts index b572e54a2..3c5c397cf 100644 --- a/test/mcpl-ws-transport.test.ts +++ b/test/mcpl-ws-transport.test.ts @@ -363,7 +363,8 @@ test('a server that closes during the handshake with a code and no reason gets t } }); -test("a server's close reason stays out of the error a pending request is rejected with", async () => { +test("a server's close reason stays out of the error a pending request is rejected with, and out of the log", async (t) => { + const errors = t.mock.method(console, 'error', () => {}); const wss = startClosingWsServer('tools/call', 4001, PEER_REASON); await waitListening(wss); let conn: McplServerConnection | undefined; @@ -374,10 +375,14 @@ test("a server's close reason stays out of the error a pending request is reject withDeadline(conn.sendToolsCall('echo', { text: 'hello' }), 10_000, 'the call to be rejected'), (error: Error) => { assert.equal(error.message, 'The connection to the "editor" server closed before it answered, so whether the request took effect is unknown. ' + - "Check its effect before trying again; this host won't reconnect to it on its own."); + "This host won't reconnect to it on its own. If you need it, ask whoever runs this host."); return true; }, ); + // The log has the close's code and the transport's words, never the peer's. + const lines = errors.mock.calls.map((call) => String(call.arguments[0])); + assert.ok(lines.some((line) => /^MCPL server "editor" disconnected \(code=4001, signal=n\/a, reason=websocket closed\) with 1 request\(s\) unanswered: tools\/call \(id=\d+\)$/.test(line)), lines.join('\n')); + assert.ok(!lines.some((line) => line.includes(PEER_REASON)), 'no log line carries the peer\'s close text'); } finally { await conn?.close(); wss.close(); From 2f8796b4339a071cd5bbe9f7cc6a3d0a22ce079e Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sun, 18 Oct 2026 04:42:46 -0400 Subject: [PATCH 03/11] fix(mcpl): the af-w4a-2 round: an error answer with no words says its outcome is unknown, an empty uri beside inline data stays withheld on purpose, and the sheet's tests From af-w4a-2's sheet (wf-2593, verdicts recorded): - I3: a server's error answer with no words adds ", so whether the request took effect is unknown. Check its effect before trying again.", as the connection's other texts for an unknown outcome do. With words it stays the server's own account. - I2, as Ada-1017 decided after Desmond-2579's probe (room-203 #141156, #141202): an empty uri reads as no link only where there's no data, as af-w4a's round has it. Beside inline data, any string uri stays the vector-2 contradiction, as on main: the tool-result lane can't show every inline shape yet, and reading '' as no link there sent inline audio, or an image with no mimeType, into history as raw JSON, taking any native image with it. classifyBlock's comment says why; whichever of this and the tool-result lane's markers lands second makes the rule whole. - I21: close()'s comment says what follows a channel tool's own words. - I16: the host-close test holds two calls pending and checks the line lists both. - I17: the fragment names the log lines the change adds, and leaves out the request on a closed connection, which logs nothing. - Tests: inline audio beside an empty uri stays withheld, on classifyBlock and the history's array path. --- changelog.d/mcpl-agent-words.changed.md | 6 ++--- src/mcpl/references.ts | 7 +++++- src/mcpl/server-connection.ts | 12 +++++----- test/mcpl-agent-words.test.ts | 28 ++++++++++++++++++------ test/mcpl-request-handler-errors.test.ts | 2 +- 5 files changed, 38 insertions(+), 17 deletions(-) diff --git a/changelog.d/mcpl-agent-words.changed.md b/changelog.d/mcpl-agent-words.changed.md index e167b0f47..827d3b6cf 100644 --- a/changelog.d/mcpl-agent-words.changed.md +++ b/changelog.d/mcpl-agent-words.changed.md @@ -1,11 +1,11 @@ - The MCPL texts an agent reads for a failed request, a refused tool call and a block the host can't show now say what happened, whether anything took effect, and what works from there, or that nothing does. Method names, request ids, JSON-RPC codes, close details and field names leave them. The ones an operator needs go to the host's log. - **A failed MCPL request:** - A connection that closes with a call pending reads `The connection to the "" server closed before it answered, so whether the request took effect is unknown.` It then says whether this host is reconnecting, where it read `MCPL server "" disconnected unexpectedly (code=…, signal=…, reason=…) while awaiting (id=N)`. - - The other texts change the same way: a close by this host, a request on a closed connection, a server's error answer (in the server's own words) and a timeout. - - The log gains a line for each with the details the texts dropped. For a close with requests pending, that's its code, signal and reason, whether this host closed it, and the requests left unanswered. + - The other texts change the same way: a close by this host, a request on a closed connection, a server's error answer (in the server's own words, or, with none, saying the outcome is unknown) and a timeout. + - The log gains a line for a close with requests pending, with the close's code, signal and reason, whether this host closed it, and the requests left unanswered; one for a server's error answer, with its method, id and code; and one for a timeout, with its method and id. - **The framework's MCPL tool calls:** - A call to a server missing from the registry, or to a tool its policy denies, says it didn't run and whether anything can change that, in the same words for the model's call and a script's. - An error result with no text reads as one whose effect is unknown, where it read `Tool call failed`. - A conversation's `channel_open` is told where to reply. - - **Blocks the host can't show** read as what they were and why: `[Image not shown: …]`, `[Audio not shown: …]`, `[Reference not shown: …]`, `[Content not shown: …]`, and the text marker `[Text not shown: a text part arrived without readable text.]`. They had read `[Image: no data]`, `[unrecognized content block: …]`, `[inline content withheld: …]` and `[Text not shown: the server's text block had no string text.]`. An image or audio block with an empty link reads as having no data or link on the tool-result lane too. + - **Blocks the host can't show** read as what they were and why: `[Image not shown: …]`, `[Audio not shown: …]`, `[Reference not shown: …]`, `[Content not shown: …]`, and the text marker `[Text not shown: a text part arrived without readable text.]`. They had read `[Image: no data]`, `[unrecognized content block: …]`, `[inline content withheld: …]` and `[Text not shown: the server's text block had no string text.]`. An image or audio block with an empty link and no data reads as having no data or link on the tool-result lane too. - **A reference's expiry and missing server** read `expired`, `treated as expired, since its expiry time can't be read`, `treated as expired, since its expiry time isn't in a form this host accepts` and `not tied to any server, so there's nowhere to fetch it from`. They had read `expired: its expiresAt has passed`, `… (fail closed)` and `unbound: no server is recorded to fetch it from`. diff --git a/src/mcpl/references.ts b/src/mcpl/references.ts index 92812b1a0..b07a15de8 100644 --- a/src/mcpl/references.ts +++ b/src/mcpl/references.ts @@ -97,9 +97,14 @@ export function classifyBlock(block: unknown): BlockClassification { if (hasData) { // Inline form claiming bulk disposition (or smuggling a uri beside // the data) is the vector-2 contradiction: fail closed, withhold. + // A uri of '' counts here too, though it's no link: until the + // tool-result lane gives every inline shape a marker of its own, + // showing that data would put some of it into history as raw JSON. + // Once that lane does, an empty link beside data reads as no link. return { kind: 'inline', contradiction: block.disposition !== undefined || hasUri }; } - // An empty uri is no link, as the push lane's converter reads it. + // With no data, an empty uri is no link, as the push lane's converter + // reads it. if (hasUri && block.uri !== '') return parseReferenceFields(block); return { kind: 'invalid', shown: block.type === 'image' ? IMAGE_NO_DATA_TEXT : AUDIO_NO_DATA_TEXT }; } diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index 53448766d..37aa50b6b 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -1006,10 +1006,11 @@ export class McplServerConnection extends EventEmitter { this.resetPolicyForTransportBoundary(); // Reject all pending requests. Each message is the whole of what an - // agent reads for a tools/call, and reads after a channel tool's own - // words for the requests those make, so it names no method; the log line - // does. It doesn't say whether this host reconnects: a restart closes the - // connection and then connects again. + // agent reads for a tools/call, and what follows a channel tool's own + // words ("Failed to publish to channel: …") for the requests a channel + // tool makes, so it names no method; the log line does. It doesn't say + // whether this host reconnects: a restart closes the connection and then + // connects again. if (this.pendingRequests.size > 0) { console.error(`MCPL server "${this.id}": this host closed its connection with ${this.unansweredRequests()}`); } @@ -1517,7 +1518,8 @@ export class McplServerConnection extends EventEmitter { ); pending.reject(new Error(said ? `The "${this.id}" server answered with an error: ${said}` - : `The "${this.id}" server answered with an error without saying what went wrong.`)); + : `The "${this.id}" server answered with an error without saying what went wrong, so whether the request took effect is unknown. ` + + 'Check its effect before trying again.')); } else { pending.resolve(response.result); } diff --git a/test/mcpl-agent-words.test.ts b/test/mcpl-agent-words.test.ts index c0d319c0e..d567fb21d 100644 --- a/test/mcpl-agent-words.test.ts +++ b/test/mcpl-agent-words.test.ts @@ -19,9 +19,11 @@ import { REFERENCE_LIMITS, REFERENCE_LINK_TOO_LONG_TEXT, REFERENCE_NO_LINK_TEXT, + classifyBlock, referenceStubOrNull, } from '../src/mcpl/references.js'; import { convertBlock } from '../src/mcpl/convert-block.js'; +import { toolResultDataToHistoryString } from '../src/tool-result-history.js'; import { McplServerConnection } from '../src/mcpl/server-connection.js'; import type { McplHostCapabilities, McplServerConfig } from '../src/mcpl/types.js'; @@ -57,6 +59,13 @@ test("a reference block that can't be read says which part is missing, and what assert.equal(referenceStubOrNull(block), text); assert.deepEqual(convertBlock(block as never, 'test'), { type: 'text', text }); } + // Beside inline data, any string uri, an empty one too, stays withheld as + // on main: the tool-result lane can't show every inline shape yet, and + // showing this audio would put its data into history as raw JSON. + const audio = { type: 'audio', data: 'aGVsbG8=', mimeType: 'audio/wav', uri: '' }; + assert.deepEqual(classifyBlock(audio), { kind: 'inline', contradiction: true }); + const stored = toolResultDataToHistoryString([{ type: 'text', text: 't' }, audio]); + assert.ok(stored.includes(INLINE_WITHHELD_TEXT) && !stored.includes('aGVsbG8='), stored); }); // --------------------------------------------------------------------------- @@ -100,6 +109,10 @@ async function within(promise: Promise, what: string): Promise { try { return await Promise.race([promise, late]); } finally { clearTimeout(timer); } } +/** What a server's error answer with no words reads as. */ +const NO_WORDS = 'The "words" server answered with an error without saying what went wrong, so whether the request took effect is unknown. ' + + 'Check its effect before trying again.'; + /** Whether some console.error line matches. */ const logged = (calls: ReadonlyArray<{ arguments: unknown[] }>, line: RegExp): boolean => calls.some((call) => line.test(String(call.arguments[0]))); @@ -114,10 +127,10 @@ test("a server's error answer is shown as the server's, and its method, id and c assert.ok(logged(errors.mock.calls, /^MCPL server "words" answered tools\/call \(id=\d+\) with error -32000: /), 'the operator log has the answer'); // An answer with no words says so, rather than ending on a colon. await assert.rejects(connection.sendToolsCall('mute', {}), - { message: 'The "words" server answered with an error without saying what went wrong.' }); + { message: NO_WORDS }); // A message that isn't a string reads the same, and the log keeps what was sent. await assert.rejects(connection.sendToolsCall('odd', {}), - { message: 'The "words" server answered with an error without saying what went wrong.' }); + { message: NO_WORDS }); assert.ok(logged(errors.mock.calls, /^MCPL server "words" answered tools\/call \(id=\d+\) with error -32000: 42$/), 'the log has the number'); }); @@ -194,14 +207,15 @@ test('a call pending when this host closes the connection says the outcome is un const connection = await McplServerConnection.connect(config(), HOST_CAPS); connection.ready(); // No word on reconnecting: a restart closes the connection, then connects again. - const pending = assert.rejects(connection.sendToolsCall('hang', {}), { + const unknown = { message: `The "words" server didn't answer before this host closed its connection, so whether the request took effect is unknown. ` + 'Check its effect before trying again.', - }); + }; + const pending = [assert.rejects(connection.sendToolsCall('hang', {}), unknown), assert.rejects(connection.sendToolsCall('hang', {}), unknown)]; await connection.close(); - await pending; - assert.ok(logged(errors.mock.calls, /^MCPL server "words": this host closed its connection with 1 request\(s\) unanswered: tools\/call \(id=\d+\)$/), - 'the operator log has the request'); + await Promise.all(pending); + assert.ok(logged(errors.mock.calls, /^MCPL server "words": this host closed its connection with 2 request\(s\) unanswered: tools\/call \(id=(\d+)\), tools\/call \(id=(?!\1\))\d+\)$/), + 'the operator log has both requests'); }); // --------------------------------------------------------------------------- diff --git a/test/mcpl-request-handler-errors.test.ts b/test/mcpl-request-handler-errors.test.ts index 68d0fb63f..f04f630fa 100644 --- a/test/mcpl-request-handler-errors.test.ts +++ b/test/mcpl-request-handler-errors.test.ts @@ -742,7 +742,7 @@ test('a server\'s error answer that no template can convert settles the host\'s await settle(); assert.deepEqual(lines, [ `MCPL server "srv" answered mcpl/manifest (id=${fetch.id}) with error "{\\"toString\\":1}": "{\\"toString\\":1}"`, - '[mcpl] srv: mcpl/manifest fetch failed — previous manifest stands (§17.5): The "srv" server answered with an error without saying what went wrong.', + '[mcpl] srv: mcpl/manifest fetch failed — previous manifest stands (§17.5): The "srv" server answered with an error without saying what went wrong, so whether the request took effect is unknown. Check its effect before trying again.', ]); assert.deepEqual(traces.filter((e) => e.type === 'mcpl:server-error' || e.type === 'mcpl:request-handler-error'), []); assert.equal(framework.manifestRefreshState.get('srv').inFlight, false); From a3ecde007355b333622083a734e72a2c4a7c9ae1 Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sun, 18 Oct 2026 05:33:48 -0400 Subject: [PATCH 04/11] fix(mcpl): the af-w4a-3 round: an error result of only whitespace reads as no words, and the admin puppet's no-words case is tested From af-w4a-3's sheet (wf-2613, verdicts recorded): - I1: mcplToolFailure takes the no-words text when the error result's joined text is blank, as a server's error answer reads whitespace, rather than handing the agent a blank error. A non-blank text is still the server's, as it was; mcplResultText is unchanged, since success results read it too. #490's test gains a whitespace-only case on each path. - I5: #490's puppet test also runs the no-words case, checking the returned error and the stored tool_result. - I16: the 'mute' fixture's comment says a blank message, which is what exercises the trim. - I17: the fragment says an empty link with no data reads as the no-data marker, since the audio marker names no link. --- changelog.d/mcpl-agent-words.changed.md | 4 ++-- src/framework.ts | 15 +++++++++------ test/mcpl-agent-words.test.ts | 2 +- test/mcpl-iserror-failure.test.ts | 12 ++++++++++++ 4 files changed, 24 insertions(+), 9 deletions(-) diff --git a/changelog.d/mcpl-agent-words.changed.md b/changelog.d/mcpl-agent-words.changed.md index 827d3b6cf..f3fbcb63c 100644 --- a/changelog.d/mcpl-agent-words.changed.md +++ b/changelog.d/mcpl-agent-words.changed.md @@ -5,7 +5,7 @@ - The log gains a line for a close with requests pending, with the close's code, signal and reason, whether this host closed it, and the requests left unanswered; one for a server's error answer, with its method, id and code; and one for a timeout, with its method and id. - **The framework's MCPL tool calls:** - A call to a server missing from the registry, or to a tool its policy denies, says it didn't run and whether anything can change that, in the same words for the model's call and a script's. - - An error result with no text reads as one whose effect is unknown, where it read `Tool call failed`. + - An error result with no text, or only whitespace, reads as one whose effect is unknown, where it read `Tool call failed`, or the blank text. - A conversation's `channel_open` is told where to reply. - - **Blocks the host can't show** read as what they were and why: `[Image not shown: …]`, `[Audio not shown: …]`, `[Reference not shown: …]`, `[Content not shown: …]`, and the text marker `[Text not shown: a text part arrived without readable text.]`. They had read `[Image: no data]`, `[unrecognized content block: …]`, `[inline content withheld: …]` and `[Text not shown: the server's text block had no string text.]`. An image or audio block with an empty link and no data reads as having no data or link on the tool-result lane too. + - **Blocks the host can't show** read as what they were and why: `[Image not shown: …]`, `[Audio not shown: …]`, `[Reference not shown: …]`, `[Content not shown: …]`, and the text marker `[Text not shown: a text part arrived without readable text.]`. They had read `[Image: no data]`, `[unrecognized content block: …]`, `[inline content withheld: …]` and `[Text not shown: the server's text block had no string text.]`. An image or audio block with an empty link and no data reads as the no-data marker on the tool-result lane too. - **A reference's expiry and missing server** read `expired`, `treated as expired, since its expiry time can't be read`, `treated as expired, since its expiry time isn't in a form this host accepts` and `not tied to any server, so there's nowhere to fetch it from`. They had read `expired: its expiresAt has passed`, `… (fail closed)` and `unbound: no server is recorded to fetch it from`. diff --git a/src/framework.ts b/src/framework.ts index 5edb8482f..500d59fd5 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -329,18 +329,21 @@ function mcplResultText(content: McpToolResultContent[] | undefined): string { /** * An MCPL tools/call result the server marked `isError`, as a failure: its - * text is the error, and with none, the error says the server gave no words - * and that whether the call did anything is unknown. Its other blocks aren't - * kept. The model's dispatch and an in-process caller (`executeToolCall`, + * text is the error, and with none, or only whitespace, as a server's error + * answer reads it, the error says the server gave no words and that whether + * the call did anything is unknown. Its other blocks aren't kept. The model's + * dispatch and an in-process caller (`executeToolCall`, * `ModuleContext.callTool`) both take a server's error from here, so it reads * the same on each path. `callName` is the tool's name as it was called. */ function mcplToolFailure(content: McpToolResultContent[] | undefined, serverId: string, callName: string): ToolResult { + const said = mcplResultText(content); return { success: false, - error: mcplResultText(content) || - `The "${serverId}" server reported an error for ${callName} without saying what went wrong. ` + - 'Whether it did anything is unknown: check before calling it again.', + error: said.trim() + ? said + : `The "${serverId}" server reported an error for ${callName} without saying what went wrong. ` + + 'Whether it did anything is unknown: check before calling it again.', isError: true, }; } diff --git a/test/mcpl-agent-words.test.ts b/test/mcpl-agent-words.test.ts index d567fb21d..5b35f861b 100644 --- a/test/mcpl-agent-words.test.ts +++ b/test/mcpl-agent-words.test.ts @@ -73,7 +73,7 @@ test("a reference block that can't be read says which part is missing, and what // --------------------------------------------------------------------------- // Stdio server: answers initialize and tools/list; tools/call "refuse" gets a -// JSON-RPC error, "mute" one with an empty message, "odd" one whose message +// JSON-RPC error, "mute" one with a blank message, "odd" one whose message // isn't a string, "exit" ends the process with the call pending, // "answer-then-exit" answers and then ends it, and "hang" is never answered. const SERVER = ` diff --git a/test/mcpl-iserror-failure.test.ts b/test/mcpl-iserror-failure.test.ts index c1e87a057..d925f10de 100644 --- a/test/mcpl-iserror-failure.test.ts +++ b/test/mcpl-iserror-failure.test.ts @@ -17,6 +17,8 @@ const ANSWERS: Record = { two: { isError: true, content: [{ type: 'text', text: 'first' }, { type: 'text', text: 'second' }] }, notString: { isError: true, content: [{ type: 'text', text: { a: 1 } }] }, noText: { isError: true, content: [] }, + // Whitespace alone is no words, as a server's error answer reads it. + blankText: { isError: true, content: [{ type: 'text', text: ' ' }, { type: 'text', text: '\n' }] }, // A nonconformant server's: no content, or content that isn't a list. noContent: { isError: true }, notList: { isError: true, content: 'not a list' }, @@ -38,6 +40,7 @@ const ERRORS: Record = { two: 'first\nsecond', notString: NON_STRING_TEXT_PLACEHOLDER, noText: NO_WORDS, + blankText: NO_WORDS, noContent: NO_WORDS, notList: NO_WORDS, blank: 'after a blank', @@ -122,6 +125,15 @@ test('the admin puppet stores a server\'s error as an error result, and traces i assert.equal(stored.isError, true); assert.equal(stored.content, 'no such row'); assert.deepEqual(traces.slice(before).filter((e) => e.type === 'puppet:tool-call').map((e) => e.isError), [true]); + // With no words, the puppet returns and stores the text the other paths read. + const silent = await f.framework.puppetToolCall('agent', 'mcpl--editor--lookup', { case: 'noText' }); + assert.deepEqual(fields(silent.result), { success: false, data: undefined, error: NO_WORDS, isError: true }); + const storedSilent = f.framework.getAgent('agent')!.getContextManager().getAllMessages() + .flatMap((m) => m.content as unknown as Array>) + .find((b) => b.type === 'tool_result' && b.toolUseId === silent.toolUseId); + assert.ok(storedSilent, 'the no-words pair is stored'); + assert.equal(storedSilent.isError, true); + assert.equal(storedSilent.content, NO_WORDS); }); test('a server\'s error through the model\'s call is traced as tool:failed with its error, and a success as tool:completed', { timeout: 20_000 }, async (t) => { From 7b10c6dae5bbac1daf232cd4af499b3ab75f785a Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sun, 18 Oct 2026 05:58:30 -0400 Subject: [PATCH 05/11] fix(mcpl): a tool call while this host connects to its server says so Greptile's P2 on #506 (discussion_r4306568194), the window af-w4a's I1 raised and I judged intended: connectMcplServerInternal maps a server's tool prefix before it awaits addServer, which registers the server only once the handshake is done. So during a runtime connect or a restart, a call that resolves by prefix (one from an inference begun before a restart took the tools off the list, or a script's) found no server and read "isn't connected, and this host won't reconnect to it on its own ... ask whoever runs this host", while the host was connecting to it. The agent concludes the server stays down until someone acts, and is sent to the operator for something that settles in moments. Ada agreed to the fix in #506 (room-203 #141998). - mcplConnecting: the ids whose connect is in flight, added just before addServer (nothing awaits between the prefix mapping and it) and removed when addServer settles, so a connect that fails reads as gone, which by then it is. - mcplServerAbsentText: both refusal sites (the model's dispatch and a script's executeMcplToolCall) read the set first: "This host is connecting to the "" server that provides , so the call didn't run. Try again in a moment." - Test: connectMcplServerInternal with a handshake the test holds; a call in the window reads the connecting text on both paths, and after a failed connect, the gone text. - reaction-baseline-platform's harness, which builds a framework without its constructor and runs connectMcplServerInternal, gets the field. - The fragment's refused-call bullet gains the connecting case. Merge note (Ada #141998): #265's refusal of a runtime connect for an id already registered and #262's mcpServerIdTaken check ids before a handshake; whichever lands second counts an in-flight id as taken too. --- changelog.d/mcpl-agent-words.changed.md | 2 +- src/framework.ts | 42 +++++++++++++++--- test/mcpl-agent-words.test.ts | 58 +++++++++++++++++++++++++ test/reaction-baseline-platform.test.ts | 1 + 4 files changed, 97 insertions(+), 6 deletions(-) diff --git a/changelog.d/mcpl-agent-words.changed.md b/changelog.d/mcpl-agent-words.changed.md index f3fbcb63c..b59dbee91 100644 --- a/changelog.d/mcpl-agent-words.changed.md +++ b/changelog.d/mcpl-agent-words.changed.md @@ -4,7 +4,7 @@ - The other texts change the same way: a close by this host, a request on a closed connection, a server's error answer (in the server's own words, or, with none, saying the outcome is unknown) and a timeout. - The log gains a line for a close with requests pending, with the close's code, signal and reason, whether this host closed it, and the requests left unanswered; one for a server's error answer, with its method, id and code; and one for a timeout, with its method and id. - **The framework's MCPL tool calls:** - - A call to a server missing from the registry, or to a tool its policy denies, says it didn't run and whether anything can change that, in the same words for the model's call and a script's. + - A call to a server missing from the registry, or to a tool its policy denies, says it didn't run and whether anything can change that, in the same words for the model's call and a script's. One made while this host is connecting to the server, as in a restart, says so and to try again in a moment. - An error result with no text, or only whitespace, reads as one whose effect is unknown, where it read `Tool call failed`, or the blank text. - A conversation's `channel_open` is told where to reply. - **Blocks the host can't show** read as what they were and why: `[Image not shown: …]`, `[Audio not shown: …]`, `[Reference not shown: …]`, `[Content not shown: …]`, and the text marker `[Text not shown: a text part arrived without readable text.]`. They had read `[Image: no data]`, `[unrecognized content block: …]`, `[inline content withheld: …]` and `[Text not shown: the server's text block had no string text.]`. An image or audio block with an empty link and no data reads as the no-data marker on the tool-result lane too. diff --git a/src/framework.ts b/src/framework.ts index 500d59fd5..7360d0a85 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -137,14 +137,24 @@ import { cumulativeDelta } from './usage-accounting.js'; import { stampThinkingTokenEstimates } from './thinking-token-stamp.js'; /** An MCPL tool call refused because its server isn't in the registry: one - * closed for good, one removed, or one never connected, which this host + * closed for good, one removed, or one whose connect failed, which this host * doesn't bring back on its own. A server that's reconnecting stays - * registered, and its closed connection answers the call instead. */ + * registered, and its closed connection answers the call instead. One this + * host is connecting to isn't registered yet, and mcplServerConnectingText + * answers for it. */ function mcplServerGoneText(serverId: string, toolName: string): string { return `The "${serverId}" server that provides ${toolName} isn't connected, and this host won't reconnect to it on its own, ` + "so the call didn't run. If you need it, ask whoever runs this host."; } +/** An MCPL tool call refused while this host connects to its server, as a + * runtime connect or a restart does: the server's prefix routes calls to it + * before the registry holds it, which it does once the handshake is done. */ +function mcplServerConnectingText(serverId: string, toolName: string): string { + return `This host is connecting to the "${serverId}" server that provides ${toolName}, so the call didn't run. ` + + 'Try again in a moment.'; +} + /** An MCPL tool call its server's tool policy denies. The policy hides the * tool from the list it's offered in, so this is a call to a tool the agent * wasn't offered. */ @@ -2238,6 +2248,10 @@ export class AgentFramework { private toolPatternKnownPrefixes: Map = new Map(); /** Maps serverId → McplServerConfig for prefix lookup. */ private mcplServerConfigs: Map = new Map(); + /** Servers whose connect is in flight (connectMcplServerInternal), from the + * registry's handshake until it settles: a call their prefix routes then + * finds no server registered, and is told this host is connecting to it. */ + private mcplConnecting: Set = new Set(); /** Host capabilities advertised during the MCP handshake — stored so servers * can be connected at runtime (connectMcplServer) after initialization. */ private mcplHostCapabilities: McplHostCapabilities | null = null; @@ -15739,7 +15753,7 @@ export class AgentFramework { } const server = this.mcplServerRegistry.getServer(serverId); if (!server) { - return { success: false, error: mcplServerGoneText(serverId, call.name), isError: true }; + return { success: false, error: this.mcplServerAbsentText(serverId, call.name), isError: true }; } const toolName = call.name.slice(prefix.length + 2); // Strip prefix + '--' if (!isToolAllowed(toolName, this.mcplServerConfigs.get(serverId))) { @@ -18942,7 +18956,17 @@ export class AgentFramework { }), }); } - const connection = await this.mcplServerRegistry.addServer(connectionConfig, this.mcplHostCapabilities); + // The prefix above routes this server's tool calls already, and addServer + // registers the server only once its handshake is done, so until it + // settles a call is told this host is connecting to it. A connect that + // fails leaves it unregistered, and a call is told it's gone. + this.mcplConnecting.add(config.id); + let connection: McplServerConnection; + try { + connection = await this.mcplServerRegistry.addServer(connectionConfig, this.mcplHostCapabilities); + } finally { + this.mcplConnecting.delete(config.id); + } // Wire listeners before either startup staging or the runtime global gate // releases control traffic needed for registration and marker service. @@ -20414,6 +20438,14 @@ export class AgentFramework { } } + /** The refusal of an MCPL tool call whose server isn't in the registry, for + * the model's call and a script's alike. */ + private mcplServerAbsentText(serverId: string, toolName: string): string { + return this.mcplConnecting.has(serverId) + ? mcplServerConnectingText(serverId, toolName) + : mcplServerGoneText(serverId, toolName); + } + /** * Dispatch a tool call to an MCPL server. * Strips the configured toolPrefix and routes to the server. @@ -20431,7 +20463,7 @@ export class AgentFramework { toolName: call.name, agentName, moduleName: `mcpl:${serverId}`, - result: { success: false, error: mcplServerGoneText(serverId, call.name), isError: true }, + result: { success: false, error: this.mcplServerAbsentText(serverId, call.name), isError: true }, }); return; } diff --git a/test/mcpl-agent-words.test.ts b/test/mcpl-agent-words.test.ts index 5b35f861b..b9e77a616 100644 --- a/test/mcpl-agent-words.test.ts +++ b/test/mcpl-agent-words.test.ts @@ -236,6 +236,7 @@ function frameworkWith(server: unknown, config: Record = {}) { }; framework.mcplServerRegistry = { getServer: () => server }; framework.mcplServerConfigs = new Map([['words', { id: 'words', ...config }]]); + framework.mcplConnecting = new Set(); framework.emitTrace = () => {}; framework.pushEvent = (event: ToolResultEvent) => { events.push(event); if (event.type === 'tool-result') resolve(); }; return { framework, events, delivered }; @@ -258,6 +259,63 @@ test("an MCPL tool call refused before it runs says it didn't run, and whether a assert.equal((await denied.framework.executeMcplToolCall({ id: 'c4', name: 'words--lookup', input: {} }, 'words', 'words')).error, deniedText); }); +test("a call while this host connects to the tool's server says so, and after a connect that failed, that the server's gone", async () => { + // A runtime connect or a restart's maps the server's prefix, so its tool + // calls route to it, then awaits addServer, which registers the server once + // the handshake is done. Each handshake here waits for the test. + const servers = new Map(); + const handshakes: Array<(ok: boolean) => void> = []; + const { framework, events } = frameworkWith(null); + Object.assign(framework, { + mcplServerRegistry: { + getServer: (id: string) => servers.get(id) ?? null, + addServer: (cfg: { id: string }) => new Promise((resolve, reject) => { + handshakes.push((ok) => { + if (!ok) return reject(new Error('handshake refused')); + const connection = { on: () => {} }; + servers.set(cfg.id, connection); + resolve(connection); + }); + }), + }, + mcplHostCapabilities: {}, + mcplPrefixMap: new Map(), + mcplServerConfigs: new Map(), + channelRegistry: null, + discordAwarenessOutbox: null, + discordAwarenessEmoji: '🔕', + wireMcplEvents: () => {}, + registerMcplServerFeatures: async () => {}, + }); + const connect = (framework as unknown as { + connectMcplServerInternal(config: McplServerConfig, deferAwareness?: boolean): Promise; + }).connectMcplServerInternal.bind(framework); + const call = (id: string) => ({ id, name: 'words--lookup', input: {} }); + const settle = () => new Promise((r) => setImmediate(r)); + + const connecting = connect({ id: 'words', url: 'ws://127.0.0.1:9', toolPrefix: 'words' }, true); + await settle(); + assert.equal(handshakes.length, 1, 'the handshake has begun'); + framework.dispatchMcplToolCall('agent', call('c1'), 'words', 'words'); + const connectingText = 'This host is connecting to the "words" server that provides words--lookup, so the call didn\'t run. Try again in a moment.'; + assert.equal(events[0]?.result?.error, connectingText); + assert.equal((await framework.executeMcplToolCall(call('c2'), 'words', 'words')).error, connectingText, "a script's call reads the same"); + handshakes[0]!(true); + await connecting; + + // A connect that fails leaves the server unregistered, which nothing brings back. + servers.clear(); + const failing = connect({ id: 'words', url: 'ws://127.0.0.1:9', toolPrefix: 'words' }, true); + await settle(); + handshakes[1]!(false); + await assert.rejects(failing, /handshake refused/); + const goneText = 'The "words" server that provides words--lookup isn\'t connected, ' + + "and this host won't reconnect to it on its own, so the call didn't run. If you need it, ask whoever runs this host."; + framework.dispatchMcplToolCall('agent', call('c3'), 'words', 'words'); + assert.equal(events[1]?.result?.error, goneText); + assert.equal((await framework.executeMcplToolCall(call('c4'), 'words', 'words')).error, goneText, "a script's call reads the same"); +}); + test("an MCPL tool's error result with no words says so, and that its effect is unknown", async () => { const silent = frameworkWith({ sendToolsCall: async () => ({ isError: true, content: [] }) }); silent.framework.dispatchMcplToolCall('agent', { id: 'c1', name: 'words--lookup', input: {} }, 'words', 'words'); diff --git a/test/reaction-baseline-platform.test.ts b/test/reaction-baseline-platform.test.ts index 85639b922..38d12dc01 100644 --- a/test/reaction-baseline-platform.test.ts +++ b/test/reaction-baseline-platform.test.ts @@ -25,6 +25,7 @@ async function connectionEnv( mcplHostCapabilities: {}, mcplPrefixMap: new Map(), mcplServerConfigs: new Map(), + mcplConnecting: new Set(), channelRegistry: null, discordAwarenessOutbox: null, discordAwarenessEmoji: '🔕', From 88bf0ff79d3b243c39e050478826fd0409484cba Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sun, 18 Oct 2026 06:35:11 -0400 Subject: [PATCH 06/11] fix(mcpl): the af-w4a-4 round: a request after this host's own close doesn't claim it won't reconnect; texts and tests for the connect window af-w4a-4 (wf-2639, round of af-w4a-3, base 2f8796b43) read 2f8796b43..7b10c6dae. - I8: close() disarms reconnect, so a request after it read "This host won't reconnect to it on its own ... ask whoever runs this host", including during a restart's close, the same disorientation as Greptile's P2. The connection can't tell a restart's close from a removal's, so it now records its own close (closedByThisHost) and a request after it reads "The request wasn't sent: this host closed its connection to the "" server, so nothing ran.", as the pending-request close text already says only what holds in both. A drop without reconnect keeps its text. - I3: a failed connect with reconnect leaves a registered stub that answers (connectWithReconnect's createDisconnectedStub); the comments and the gone text's doc said every failed connect reads as gone. - I1 (intended): the field's doc says one mark per id, since the registry holds one connection per id and two connects of one id at once aren't supported (Ada's merge note, room-203 #141998). - I2: the connecting text is for the connect, a runtime one or a restart's; the fragment says so. The restart's gap between close and connect, with the prefix unmapped, reads main's "Module not found", left. - I4, I10: the connect-window test checks a call after a connect that succeeded (the mark is gone) and a call to another missing server while one connects (only the server being connected to reads so). - I12: mcpl-iserror-failure pins that words with whitespace around them pass through as the server's. - I14: the test's comment matches the gone text's doc. --- changelog.d/mcpl-agent-words.changed.md | 2 +- src/framework.ts | 22 ++++++++++++--------- src/mcpl/server-connection.ts | 18 ++++++++++++----- test/mcpl-agent-words.test.ts | 26 ++++++++++++++++++------- test/mcpl-iserror-failure.test.ts | 3 +++ 5 files changed, 49 insertions(+), 22 deletions(-) diff --git a/changelog.d/mcpl-agent-words.changed.md b/changelog.d/mcpl-agent-words.changed.md index b59dbee91..2f16703d8 100644 --- a/changelog.d/mcpl-agent-words.changed.md +++ b/changelog.d/mcpl-agent-words.changed.md @@ -4,7 +4,7 @@ - The other texts change the same way: a close by this host, a request on a closed connection, a server's error answer (in the server's own words, or, with none, saying the outcome is unknown) and a timeout. - The log gains a line for a close with requests pending, with the close's code, signal and reason, whether this host closed it, and the requests left unanswered; one for a server's error answer, with its method, id and code; and one for a timeout, with its method and id. - **The framework's MCPL tool calls:** - - A call to a server missing from the registry, or to a tool its policy denies, says it didn't run and whether anything can change that, in the same words for the model's call and a script's. One made while this host is connecting to the server, as in a restart, says so and to try again in a moment. + - A call to a server missing from the registry, or to a tool its policy denies, says it didn't run and whether anything can change that, in the same words for the model's call and a script's. One made while this host is connecting to the server, in a runtime connect or a restart's, says so and to try again in a moment. - An error result with no text, or only whitespace, reads as one whose effect is unknown, where it read `Tool call failed`, or the blank text. - A conversation's `channel_open` is told where to reply. - **Blocks the host can't show** read as what they were and why: `[Image not shown: …]`, `[Audio not shown: …]`, `[Reference not shown: …]`, `[Content not shown: …]`, and the text marker `[Text not shown: a text part arrived without readable text.]`. They had read `[Image: no data]`, `[unrecognized content block: …]`, `[inline content withheld: …]` and `[Text not shown: the server's text block had no string text.]`. An image or audio block with an empty link and no data reads as the no-data marker on the tool-result lane too. diff --git a/src/framework.ts b/src/framework.ts index 7360d0a85..e71f4bece 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -137,18 +137,19 @@ import { cumulativeDelta } from './usage-accounting.js'; import { stampThinkingTokenEstimates } from './thinking-token-stamp.js'; /** An MCPL tool call refused because its server isn't in the registry: one - * closed for good, one removed, or one whose connect failed, which this host - * doesn't bring back on its own. A server that's reconnecting stays - * registered, and its closed connection answers the call instead. One this - * host is connecting to isn't registered yet, and mcplServerConnectingText - * answers for it. */ + * closed for good, one removed, or one whose connect failed without + * reconnect, which this host doesn't bring back on its own. A server that's + * reconnecting stays registered, as does the stub a failed connect with + * reconnect leaves, and its closed connection answers the call instead. One + * this host is connecting to isn't registered yet, and + * mcplServerConnectingText answers for it. */ function mcplServerGoneText(serverId: string, toolName: string): string { return `The "${serverId}" server that provides ${toolName} isn't connected, and this host won't reconnect to it on its own, ` + "so the call didn't run. If you need it, ask whoever runs this host."; } -/** An MCPL tool call refused while this host connects to its server, as a - * runtime connect or a restart does: the server's prefix routes calls to it +/** An MCPL tool call refused while this host connects to its server, in a + * runtime connect or a restart's: the server's prefix routes calls to it * before the registry holds it, which it does once the handshake is done. */ function mcplServerConnectingText(serverId: string, toolName: string): string { return `This host is connecting to the "${serverId}" server that provides ${toolName}, so the call didn't run. ` + @@ -2250,7 +2251,9 @@ export class AgentFramework { private mcplServerConfigs: Map = new Map(); /** Servers whose connect is in flight (connectMcplServerInternal), from the * registry's handshake until it settles: a call their prefix routes then - * finds no server registered, and is told this host is connecting to it. */ + * finds no server registered, and is told this host is connecting to it. + * One mark per id: the registry holds one connection per id, and two + * connects of one id at once aren't supported. */ private mcplConnecting: Set = new Set(); /** Host capabilities advertised during the MCP handshake — stored so servers * can be connected at runtime (connectMcplServer) after initialization. */ @@ -18959,7 +18962,8 @@ export class AgentFramework { // The prefix above routes this server's tool calls already, and addServer // registers the server only once its handshake is done, so until it // settles a call is told this host is connecting to it. A connect that - // fails leaves it unregistered, and a call is told it's gone. + // fails without reconnect leaves it unregistered, and a call is told it's + // gone; with reconnect, the stub it leaves is registered and answers. this.mcplConnecting.add(config.id); let connection: McplServerConnection; try { diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index 37aa50b6b..7e973bd30 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -282,6 +282,10 @@ export class McplServerConnection extends EventEmitter { /** Set by `reconnectAfterFailure()` before it closes a live transport, so * the transport close handler reports that teardown as `byHost: true`. */ private hostCloseRequested = false; + /** Set by `close()`: this host has stopped the connection, as a removal and + * a restart both do, so a request after it can't say whether the server + * comes back. */ + private closedByThisHost = false; private closePromise: Promise | null = null; private closeIncomplete = false; private reconnectPromise: Promise | null = null; @@ -994,6 +998,7 @@ export class McplServerConnection extends EventEmitter { async close(): Promise { // Disable reconnect before closing — explicit close means stop retrying this.reconnectEnabled = false; + this.closedByThisHost = true; if (this.reconnectTimer) { clearTimeout(this.reconnectTimer); this.reconnectTimer = null; @@ -1195,11 +1200,14 @@ export class McplServerConnection extends EventEmitter { options: RequestOptions = {}, ): Promise { if (this.closed) { - return Promise.reject(new Error( - `The request wasn't sent: the connection to the "${this.id}" server is closed, so nothing ran. ` + - (this.willReconnect - ? 'This host is reconnecting to it; try again later.' - : "This host won't reconnect to it on its own. If you need it, ask whoever runs this host."), + // After this host's own close, which a restart makes before connecting + // again and a removal makes for good, only that this host closed it holds. + return Promise.reject(new Error(this.closedByThisHost + ? `The request wasn't sent: this host closed its connection to the "${this.id}" server, so nothing ran.` + : `The request wasn't sent: the connection to the "${this.id}" server is closed, so nothing ran. ` + + (this.willReconnect + ? 'This host is reconnecting to it; try again later.' + : "This host won't reconnect to it on its own. If you need it, ask whoever runs this host."), )); } diff --git a/test/mcpl-agent-words.test.ts b/test/mcpl-agent-words.test.ts index b9e77a616..3185d6609 100644 --- a/test/mcpl-agent-words.test.ts +++ b/test/mcpl-agent-words.test.ts @@ -214,6 +214,10 @@ test('a call pending when this host closes the connection says the outcome is un const pending = [assert.rejects(connection.sendToolsCall('hang', {}), unknown), assert.rejects(connection.sendToolsCall('hang', {}), unknown)]; await connection.close(); await Promise.all(pending); + // A request after the close isn't told whether the server comes back either. + await assert.rejects(connection.sendToolsCall('refuse', {}), { + message: `The request wasn't sent: this host closed its connection to the "words" server, so nothing ran.`, + }); assert.ok(logged(errors.mock.calls, /^MCPL server "words": this host closed its connection with 2 request\(s\) unanswered: tools\/call \(id=(\d+)\), tools\/call \(id=(?!\1\))\d+\)$/), 'the operator log has both requests'); }); @@ -245,7 +249,7 @@ function frameworkWith(server: unknown, config: Record = {}) { test("an MCPL tool call refused before it runs says it didn't run, and whether anything can change that", async () => { const gone = frameworkWith(null); gone.framework.dispatchMcplToolCall('agent', { id: 'c1', name: 'words--lookup', input: {} }, 'words', 'words'); - // A server missing from the registry was closed for good, removed, or never connected. + // A server missing from the registry was closed for good, removed, or its connect failed without reconnect. assert.equal(gone.events[0]?.result?.error, `The "words" server that provides words--lookup isn't connected, ` + "and this host won't reconnect to it on its own, so the call didn't run. If you need it, ask whoever runs this host."); assert.equal((await gone.framework.executeMcplToolCall({ id: 'c2', name: 'words--lookup', input: {} }, 'words', 'words')).error, @@ -300,20 +304,28 @@ test("a call while this host connects to the tool's server says so, and after a const connectingText = 'This host is connecting to the "words" server that provides words--lookup, so the call didn\'t run. Try again in a moment.'; assert.equal(events[0]?.result?.error, connectingText); assert.equal((await framework.executeMcplToolCall(call('c2'), 'words', 'words')).error, connectingText, "a script's call reads the same"); + // Only the server being connected to reads so: another one missing is gone. + framework.dispatchMcplToolCall('agent', { id: 'c3', name: 'other--lookup', input: {} }, 'other', 'other'); + assert.equal(events[1]?.result?.error, 'The "other" server that provides other--lookup isn\'t connected, ' + + "and this host won't reconnect to it on its own, so the call didn't run. If you need it, ask whoever runs this host."); handshakes[0]!(true); await connecting; - // A connect that fails leaves the server unregistered, which nothing brings back. + // The mark goes when the connect succeeds: once the server is gone, a call reads so. servers.clear(); + const goneText = 'The "words" server that provides words--lookup isn\'t connected, ' + + "and this host won't reconnect to it on its own, so the call didn't run. If you need it, ask whoever runs this host."; + framework.dispatchMcplToolCall('agent', call('c4'), 'words', 'words'); + assert.equal(events[2]?.result?.error, goneText, 'after a connect that succeeded'); + + // A connect that fails without reconnect leaves the server unregistered, which nothing brings back. const failing = connect({ id: 'words', url: 'ws://127.0.0.1:9', toolPrefix: 'words' }, true); await settle(); handshakes[1]!(false); await assert.rejects(failing, /handshake refused/); - const goneText = 'The "words" server that provides words--lookup isn\'t connected, ' + - "and this host won't reconnect to it on its own, so the call didn't run. If you need it, ask whoever runs this host."; - framework.dispatchMcplToolCall('agent', call('c3'), 'words', 'words'); - assert.equal(events[1]?.result?.error, goneText); - assert.equal((await framework.executeMcplToolCall(call('c4'), 'words', 'words')).error, goneText, "a script's call reads the same"); + framework.dispatchMcplToolCall('agent', call('c5'), 'words', 'words'); + assert.equal(events[3]?.result?.error, goneText, 'after a connect that failed'); + assert.equal((await framework.executeMcplToolCall(call('c6'), 'words', 'words')).error, goneText, "a script's call reads the same"); }); test("an MCPL tool's error result with no words says so, and that its effect is unknown", async () => { diff --git a/test/mcpl-iserror-failure.test.ts b/test/mcpl-iserror-failure.test.ts index d925f10de..73962561f 100644 --- a/test/mcpl-iserror-failure.test.ts +++ b/test/mcpl-iserror-failure.test.ts @@ -19,6 +19,8 @@ const ANSWERS: Record = { noText: { isError: true, content: [] }, // Whitespace alone is no words, as a server's error answer reads it. blankText: { isError: true, content: [{ type: 'text', text: ' ' }, { type: 'text', text: '\n' }] }, + // Words with whitespace around them are the server's, as they came. + padded: { isError: true, content: [{ type: 'text', text: ' no such row\n' }] }, // A nonconformant server's: no content, or content that isn't a list. noContent: { isError: true }, notList: { isError: true, content: 'not a list' }, @@ -41,6 +43,7 @@ const ERRORS: Record = { notString: NON_STRING_TEXT_PLACEHOLDER, noText: NO_WORDS, blankText: NO_WORDS, + padded: ' no such row\n', noContent: NO_WORDS, notList: NO_WORDS, blank: 'after a blank', From 6975a5bfd94bfbd4030581fc4eec513129b19581 Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sun, 18 Oct 2026 07:15:18 -0400 Subject: [PATCH 07/11] fix(mcpl): a request after this host's own close says whom to ask if calling again fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Desmond-2579's point on af-w4a-4's I8 text (room-203 #143052): "The request wasn't sent: this host closed its connection to the "" server, so nothing ran." held for both a restart's close and a removal's, but gave the agent no next step, and one holds for both. After a restart, calling again reaches the new connection, or reads the connecting text while the handshake runs; after a removal, calling again fails, as the gone text or as the restart gap's unknown tool. So the text adds "If calling it again fails, ask whoever runs this host." In channel_publish it reads "… so nothing ran. If calling it again fails, ask whoever runs this host. Nothing was posted." mcpl-agent-words' close test and #247's closed-connection test take it. --- src/mcpl/server-connection.ts | 7 +++++-- test/mcpl-agent-words.test.ts | 3 ++- test/mcpl-request-outcome.test.ts | 3 ++- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/mcpl/server-connection.ts b/src/mcpl/server-connection.ts index dca315435..0ef6bf244 100644 --- a/src/mcpl/server-connection.ts +++ b/src/mcpl/server-connection.ts @@ -1283,9 +1283,12 @@ export class McplServerConnection extends EventEmitter { ): Promise { if (this.closed) { // After this host's own close, which a restart makes before connecting - // again and a removal makes for good, only that this host closed it holds. + // again and a removal makes for good, only that this host closed it + // holds, and one next step holds for both: calling again reaches the + // restarted server, or fails where it's gone. return Promise.reject(new McplRequestError(this.closedByThisHost - ? `The request wasn't sent: this host closed its connection to the "${this.id}" server, so nothing ran.` + ? `The request wasn't sent: this host closed its connection to the "${this.id}" server, so nothing ran. ` + + 'If calling it again fails, ask whoever runs this host.' : `The request wasn't sent: the connection to the "${this.id}" server is closed, so nothing ran. ` + (this.willReconnect ? 'This host is reconnecting to it; try again later.' diff --git a/test/mcpl-agent-words.test.ts b/test/mcpl-agent-words.test.ts index 531944465..aa35ca6a7 100644 --- a/test/mcpl-agent-words.test.ts +++ b/test/mcpl-agent-words.test.ts @@ -216,7 +216,8 @@ test('a call pending when this host closes the connection says the outcome is un await Promise.all(pending); // A request after the close isn't told whether the server comes back either. await assert.rejects(connection.sendToolsCall('refuse', {}), { - message: `The request wasn't sent: this host closed its connection to the "words" server, so nothing ran.`, + message: `The request wasn't sent: this host closed its connection to the "words" server, so nothing ran. ` + + 'If calling it again fails, ask whoever runs this host.', }); assert.ok(logged(errors.mock.calls, /^MCPL server "words": this host closed its connection with 2 request\(s\) unanswered: tools\/call \(id=(\d+)\), tools\/call \(id=(?!\1\))\d+\)$/), 'the operator log has both requests'); diff --git a/test/mcpl-request-outcome.test.ts b/test/mcpl-request-outcome.test.ts index 5382fc00a..27b2a45fd 100644 --- a/test/mcpl-request-outcome.test.ts +++ b/test/mcpl-request-outcome.test.ts @@ -95,7 +95,8 @@ test('a request on a closed connection is not-sent', async () => { connection = await McplServerConnection.connect(config(), HOST_CAPS); await connection.close(); const err = await rejectsWith(connection.sendToolsCall('fail', {}), 'not-sent'); - assert.equal(err.message, 'The request wasn\'t sent: this host closed its connection to the "outcome" server, so nothing ran.'); + assert.equal(err.message, 'The request wasn\'t sent: this host closed its connection to the "outcome" server, so nothing ran. ' + + 'If calling it again fails, ask whoever runs this host.'); connection = null; }); From 8cc1a756080b63902df40b339afa3c173031ece7 Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sun, 18 Oct 2026 09:08:15 -0400 Subject: [PATCH 08/11] fix(mcpl): a runtime connect or restart of a server whose connect is in flight is refused before it writes anything #469's refuseMcplConnect refuses an id already registered, but the registry takes a server only once its handshake is done, so a second connect of an id whose connect is in flight passed it: both connects went on, the second registered over the first without closing it, and the first one's settling cleared mcplConnecting's mark while the second's handshake ran. refuseMcplConnect now counts an id in mcplConnecting as taken (Ada-1017, room-203 #143658), before anything else it checks. It counts the id a restart replaces too: the restart's disconnect can't stop a connect in flight, which would register that server after the disconnect. The refusal reads, for whoever runs the host: MCPL server "" has a connect in flight; try again once it settles. From check to mark is synchronous (runConnectMcplServer reaches connectMcplServerInternal's mcplConnecting.add with no await between), so two connects can't both pass the check. A restart checks again in its connect, after its disconnect's awaits. Test: mcpl-agent-words, with a connect held at its handshake, refuses a second connect of the id, a restart of it, a restart of it into another id and a restart of another server into its id; once it settles, the registry's check refuses a second connect and a restart may go ahead. --- src/framework.ts | 23 +++++++++++----- test/mcpl-agent-words.test.ts | 50 +++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 7 deletions(-) diff --git a/src/framework.ts b/src/framework.ts index 62c6e3e46..df52a7415 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -2348,8 +2348,9 @@ export class AgentFramework { /** Servers whose connect is in flight (connectMcplServerInternal), from the * registry's handshake until it settles: a call their prefix routes then * finds no server registered, and is told this host is connecting to it. - * One mark per id: the registry holds one connection per id, and two - * connects of one id at once aren't supported. */ + * One mark per id: the registry holds one connection per id, and + * refuseMcplConnect refuses a second connect, or a restart, of an id + * whose connect is in flight. */ private mcplConnecting: Set = new Set(); /** Host capabilities advertised during the MCP handshake — stored so servers * can be connected at runtime (connectMcplServer) after initialization. */ @@ -21254,16 +21255,24 @@ export class AgentFramework { /** * Refuse a runtime connect, or a restart, before it writes anything: an id - * already registered, or a tool prefix that equals a module name or another - * server's prefix, or meets another server's (prefixesMeet, #265). create() - * applies the same prefix rules at startup. The connecting id's own entries - * don't count, nor, for a restart, those of the server it replaces - * (`replacing`), which it disconnects first. + * whose connect is in flight or that's already registered, or a tool prefix + * that equals a module name or another server's prefix, or meets another + * server's (prefixesMeet, #265). create() applies the same prefix rules at + * startup. The connecting id's own entries don't count, nor, for a restart, + * those of the server it replaces (`replacing`), which it disconnects first. */ private refuseMcplConnect( config: import('./mcpl/types.js').McplServerConfig, replacing?: string, ): void { + // A connect in flight isn't registered until its handshake is done, so + // the registry's check below misses it. Its id is taken whether this + // connects it or a restart replaces it: the restart's disconnect can't + // stop a connect in flight, which would register the server after it. + const inFlight = [config.id, replacing].find((id) => id !== undefined && this.mcplConnecting.has(id)); + if (inFlight !== undefined) { + throw new Error(`MCPL server "${inFlight}" has a connect in flight; try again once it settles`); + } // The registry refuses a second server under one id too, but only once // the connect has written its config and prefix over the live server's. if (config.id !== replacing && this.mcplServerRegistry?.getServer(config.id)) { diff --git a/test/mcpl-agent-words.test.ts b/test/mcpl-agent-words.test.ts index aa35ca6a7..f56162fb6 100644 --- a/test/mcpl-agent-words.test.ts +++ b/test/mcpl-agent-words.test.ts @@ -329,6 +329,56 @@ test("a call while this host connects to the tool's server says so, and after a assert.equal((await framework.executeMcplToolCall(call('c6'), 'words', 'words')).error, goneText, "a script's call reads the same"); }); +test('a second connect, or a restart, of a server whose connect is in flight is refused before it writes anything', async () => { + // The registry takes the server once its handshake is done, so until then + // only the connecting mark says the id is taken. Each handshake here waits for the test. + const servers = new Map(); + const handshakes: Array<() => void> = []; + const { framework } = frameworkWith(null); + Object.assign(framework, { + mcplServerRegistry: { + getServer: (id: string) => servers.get(id) ?? null, + addServer: (cfg: { id: string }) => new Promise((resolve) => { + handshakes.push(() => { + const connection = { on: () => {} }; + servers.set(cfg.id, connection); + resolve(connection); + }); + }), + }, + moduleRegistry: { getAllModules: () => [] }, + mcplHostCapabilities: {}, + mcplPrefixMap: new Map(), + mcplServerConfigs: new Map(), + channelRegistry: null, + discordAwarenessOutbox: null, + discordAwarenessEmoji: '🔕', + wireMcplEvents: () => {}, + registerMcplServerFeatures: async () => {}, + }); + const fw = framework as unknown as { + connectMcplServerInternal(config: McplServerConfig, deferAwareness?: boolean): Promise; + refuseMcplConnect(config: McplServerConfig, replacing?: string): void; + }; + const words = { id: 'words', url: 'ws://127.0.0.1:9', toolPrefix: 'words' } as McplServerConfig; + const other = { id: 'other', url: 'ws://127.0.0.1:9', toolPrefix: 'other' } as McplServerConfig; + const connecting = fw.connectMcplServerInternal(words, true); + await new Promise((r) => setImmediate(r)); + assert.equal(handshakes.length, 1, 'the handshake has begun'); + const inFlight = { message: 'MCPL server "words" has a connect in flight; try again once it settles' }; + assert.throws(() => fw.refuseMcplConnect(words), inFlight, 'a second connect of it'); + // A restart's disconnect can't stop the connect in flight, which would register the server after it. + assert.throws(() => fw.refuseMcplConnect(words, 'words'), inFlight, 'a restart of it'); + assert.throws(() => fw.refuseMcplConnect(other, 'words'), inFlight, 'a restart of it into another id'); + assert.throws(() => fw.refuseMcplConnect(words, 'other'), inFlight, 'a restart of another server into its id'); + handshakes[0]!(); + await connecting; + // Once the connect settles, the mark is gone: the registry's check refuses a + // second connect, and a restart, which disconnects the server first, may go ahead. + assert.throws(() => fw.refuseMcplConnect(words), { message: 'MCPL server "words" is already registered' }); + assert.doesNotThrow(() => fw.refuseMcplConnect(words, 'words')); +}); + test("an MCPL tool's error result with no words says so, and that its effect is unknown", async () => { const silent = frameworkWith({ sendToolsCall: async () => ({ isError: true, content: [] }) }); silent.framework.dispatchMcplToolCall('agent', { id: 'c1', name: 'words--lookup', input: {} }, 'words', 'words'); From eb0db7a6bad6f9efa071bb673d5614d6f9cb1de9 Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sun, 18 Oct 2026 09:19:20 -0400 Subject: [PATCH 09/11] docs(changelog): a runtime connect or restart of an MCPL server whose connect is in flight is refused The fragment for 8cc1a756, which extends #469's refuseMcplConnect. #469's own fragments describe meeting prefixes; this one says what changes from 0.22.3 for a connect of an id whose connect hasn't settled. --- changelog.d/mcpl-connect-in-flight.fixed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/mcpl-connect-in-flight.fixed.md diff --git a/changelog.d/mcpl-connect-in-flight.fixed.md b/changelog.d/mcpl-connect-in-flight.fixed.md new file mode 100644 index 000000000..7ad3e2b11 --- /dev/null +++ b/changelog.d/mcpl-connect-in-flight.fixed.md @@ -0,0 +1 @@ +- A runtime connect, or a restart, of an MCPL server whose connect is still in flight is now refused before it writes anything, with `MCPL server "" has a connect in flight; try again once it settles`. A second connect of that id went ahead, and its connection replaced the first in the registry without closing it. A restart is refused too while the server it would replace is still connecting, since its disconnect can't stop that connect. From 6b8bbc7087246008c4bd3195cad766d73a8a05cc Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sun, 18 Oct 2026 09:38:45 -0400 Subject: [PATCH 10/11] fix(mcpl): a disconnect of a server whose connect is in flight is refused too Desmond-2579's probe (room-203 #144495): nothing stops a connect in flight, so a disconnect during it removed the server's config and tool prefix, and the handshake then registered the server without them. No call reached it, a later connect was refused as already registered, a restart as not configured, and only another disconnect cleared it, on this branch and on main 8a32054d alike. The reason the restart is refused, that its disconnect can't stop the connect, holds for a plain disconnect, so runDisconnectMcplServer refuses an id in mcplConnecting before it changes anything, with the same words as refuseMcplConnect, now from one helper, mcplConnectInFlightError. connectome-host's mcpl_deploy, mcpl_restart and mcpl_unload quote the error to the agent, so its reader is whoever asked, an operator or an agent. The in-flight test now also refuses a disconnect and checks it left the prefix. mcpl-reconnect-reregistration's harness, built from the prototype, skips field initializers, so it sets mcplConnecting, as mcpl-agent-words' does. The fragment names the disconnect. --- changelog.d/mcpl-connect-in-flight.fixed.md | 2 +- src/framework.ts | 23 +++++++++++++++------ test/mcpl-agent-words.test.ts | 6 +++++- test/mcpl-reconnect-reregistration.test.ts | 2 ++ 4 files changed, 25 insertions(+), 8 deletions(-) diff --git a/changelog.d/mcpl-connect-in-flight.fixed.md b/changelog.d/mcpl-connect-in-flight.fixed.md index 7ad3e2b11..8d374a68b 100644 --- a/changelog.d/mcpl-connect-in-flight.fixed.md +++ b/changelog.d/mcpl-connect-in-flight.fixed.md @@ -1 +1 @@ -- A runtime connect, or a restart, of an MCPL server whose connect is still in flight is now refused before it writes anything, with `MCPL server "" has a connect in flight; try again once it settles`. A second connect of that id went ahead, and its connection replaced the first in the registry without closing it. A restart is refused too while the server it would replace is still connecting, since its disconnect can't stop that connect. +- A runtime connect, a restart or a disconnect of an MCPL server whose connect is still in flight is now refused before it changes anything, with `MCPL server "" has a connect in flight; try again once it settles`. Nothing stops a connect in flight. A second connect went ahead beside it, and its connection replaced the first in the registry without closing it. A disconnect removed the server's config and tool prefix, and the connect then registered the server without them, where no call reached it and only another disconnect cleared it. A restart begins with that disconnect. diff --git a/src/framework.ts b/src/framework.ts index df52a7415..e5c96d74e 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -170,6 +170,15 @@ function mcplToolDeniedText(serverId: string, toolName: string): string { "and calling it again won't change that. If you need it, ask whoever runs this host."; } +/** A runtime connect, restart or disconnect refused because a connect of the + * server is in flight (mcplConnecting). The registry takes a server only once + * its handshake is done, and nothing can stop that connect meanwhile, so it + * would register the server after whatever ran beside it. Whoever asked reads + * it: an operator, or an agent through a host's tool that passes it on. */ +function mcplConnectInFlightError(serverId: string): Error { + return new Error(`MCPL server "${serverId}" has a connect in flight; try again once it settles`); +} + /** Detect a supported image media type from magic bytes (the model API * rejects mislabeled media types, so trust bytes over extensions). * Returns undefined for non-image content. */ @@ -2348,9 +2357,9 @@ export class AgentFramework { /** Servers whose connect is in flight (connectMcplServerInternal), from the * registry's handshake until it settles: a call their prefix routes then * finds no server registered, and is told this host is connecting to it. - * One mark per id: the registry holds one connection per id, and - * refuseMcplConnect refuses a second connect, or a restart, of an id - * whose connect is in flight. */ + * One mark per id: the registry holds one connection per id, and a + * second connect, a restart or a disconnect of an id whose connect is in + * flight is refused (refuseMcplConnect, runDisconnectMcplServer). */ private mcplConnecting: Set = new Set(); /** Host capabilities advertised during the MCP handshake — stored so servers * can be connected at runtime (connectMcplServer) after initialization. */ @@ -21270,9 +21279,7 @@ export class AgentFramework { // connects it or a restart replaces it: the restart's disconnect can't // stop a connect in flight, which would register the server after it. const inFlight = [config.id, replacing].find((id) => id !== undefined && this.mcplConnecting.has(id)); - if (inFlight !== undefined) { - throw new Error(`MCPL server "${inFlight}" has a connect in flight; try again once it settles`); - } + if (inFlight !== undefined) throw mcplConnectInFlightError(inFlight); // The registry refuses a second server under one id too, but only once // the connect has written its config and prefix over the live server's. if (config.id !== replacing && this.mcplServerRegistry?.getServer(config.id)) { @@ -21312,6 +21319,10 @@ export class AgentFramework { if (!this.mcplServerRegistry) { throw new Error('MCPL subsystem is not initialized'); } + // A connect in flight would register the server after this disconnect, + // with its config and prefix gone, where only another disconnect reaches + // it; refused as a second connect of it is (refuseMcplConnect). + if (this.mcplConnecting.has(id)) throw mcplConnectInFlightError(id); const config = this.mcplServerConfigs.get(id); const oldToolNames = new Set(this.mcplTools.map(t => t.name)); diff --git a/test/mcpl-agent-words.test.ts b/test/mcpl-agent-words.test.ts index f56162fb6..e2aa88d72 100644 --- a/test/mcpl-agent-words.test.ts +++ b/test/mcpl-agent-words.test.ts @@ -329,7 +329,7 @@ test("a call while this host connects to the tool's server says so, and after a assert.equal((await framework.executeMcplToolCall(call('c6'), 'words', 'words')).error, goneText, "a script's call reads the same"); }); -test('a second connect, or a restart, of a server whose connect is in flight is refused before it writes anything', async () => { +test('a second connect, a restart or a disconnect of a server whose connect is in flight is refused before it writes anything', async () => { // The registry takes the server once its handshake is done, so until then // only the connecting mark says the id is taken. Each handshake here waits for the test. const servers = new Map(); @@ -359,6 +359,7 @@ test('a second connect, or a restart, of a server whose connect is in flight is const fw = framework as unknown as { connectMcplServerInternal(config: McplServerConfig, deferAwareness?: boolean): Promise; refuseMcplConnect(config: McplServerConfig, replacing?: string): void; + runDisconnectMcplServer(id: string): Promise; }; const words = { id: 'words', url: 'ws://127.0.0.1:9', toolPrefix: 'words' } as McplServerConfig; const other = { id: 'other', url: 'ws://127.0.0.1:9', toolPrefix: 'other' } as McplServerConfig; @@ -371,6 +372,9 @@ test('a second connect, or a restart, of a server whose connect is in flight is assert.throws(() => fw.refuseMcplConnect(words, 'words'), inFlight, 'a restart of it'); assert.throws(() => fw.refuseMcplConnect(other, 'words'), inFlight, 'a restart of it into another id'); assert.throws(() => fw.refuseMcplConnect(words, 'other'), inFlight, 'a restart of another server into its id'); + // A disconnect can't stop it either: the server would register after it, its config and prefix gone. + await assert.rejects(fw.runDisconnectMcplServer('words'), inFlight, 'a disconnect of it'); + assert.equal((framework.mcplPrefixMap as Map).get('words'), 'words', 'the refused disconnect left its prefix'); handshakes[0]!(); await connecting; // Once the connect settles, the mark is gone: the registry's check refuses a diff --git a/test/mcpl-reconnect-reregistration.test.ts b/test/mcpl-reconnect-reregistration.test.ts index 2484f610a..6a03af945 100644 --- a/test/mcpl-reconnect-reregistration.test.ts +++ b/test/mcpl-reconnect-reregistration.test.ts @@ -107,6 +107,8 @@ async function makeHarness() { fw.mcplTools = []; fw.mcplToolRefreshInFlight = false; fw.mcplToolRefreshPending = false; + // A disconnect reads the servers whose connect is in flight. + fw.mcplConnecting = new Set(); fw.channelRegistry = null; fw.inferenceRouter = null; fw.eventGate = null; From da8ee05d1950ca554cbc0ecae553fa026e1ce065 Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sun, 18 Oct 2026 09:51:07 -0400 Subject: [PATCH 11/11] docs(changelog): say which connection a second connect in flight replaced Two connects of one id at once register in whichever order their handshakes settle, so the fragment now says whichever settled last replaced the other, rather than that the second's replaced the first. --- changelog.d/mcpl-connect-in-flight.fixed.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/mcpl-connect-in-flight.fixed.md b/changelog.d/mcpl-connect-in-flight.fixed.md index 8d374a68b..f972c8ff0 100644 --- a/changelog.d/mcpl-connect-in-flight.fixed.md +++ b/changelog.d/mcpl-connect-in-flight.fixed.md @@ -1 +1 @@ -- A runtime connect, a restart or a disconnect of an MCPL server whose connect is still in flight is now refused before it changes anything, with `MCPL server "" has a connect in flight; try again once it settles`. Nothing stops a connect in flight. A second connect went ahead beside it, and its connection replaced the first in the registry without closing it. A disconnect removed the server's config and tool prefix, and the connect then registered the server without them, where no call reached it and only another disconnect cleared it. A restart begins with that disconnect. +- A runtime connect, a restart or a disconnect of an MCPL server whose connect is still in flight is now refused before it changes anything, with `MCPL server "" has a connect in flight; try again once it settles`. Nothing stops a connect in flight. A second connect went ahead beside it, and whichever settled last replaced the other in the registry without closing it. A disconnect removed the server's config and tool prefix, and the connect then registered the server without them, where no call reached it and only another disconnect cleared it. A restart begins with that disconnect.