From 8fa90d1485ff1c1c2d4ac03367785469c18d159e Mon Sep 17 00:00:00 2001 From: Alfred-2570 Date: Sun, 18 Oct 2026 02:47:00 -0400 Subject: [PATCH] fix(mcpl): a server's error result is a failure on each path a tool call takes An MCPL server's tools/call result marked isError reached an in-process caller (executeToolCall, ModuleContext.callTool, and the admin puppet, which calls through them) as success: true, with the server's blocks as data. The model's call read the same result as a failure, with its text as the error. Both paths now take a server's error from one function, mcplToolFailure: success false, isError true, and as the error the result's text (mcplResultText: text blocks joined by line breaks, empty ones left out, a text that isn't a string as the converters' placeholder, and content that isn't a list as no text), or "Tool call failed" when it has none. Its blocks that aren't text aren't kept, as the model's call never kept them. So the puppet now stores a server's error as an error result. A success is unchanged on each path: an in-process caller gets the server's own blocks, and the model's call the joined text, or the blocks when any isn't text. The model's call traced a server's error result as tool:completed, and so did a channel tool's error result. Both are now traced as tool:failed with the error, in the form the gate, workspace and agent dispatchers already use. Co-Authored-By: Claude Opus 5.5 --- .../fix-mcpl-iserror-is-a-failure.fixed.md | 2 + src/framework.ts | 89 +++++++---- test/mcpl-iserror-failure.test.ts | 141 ++++++++++++++++++ 3 files changed, 202 insertions(+), 30 deletions(-) create mode 100644 changelog.d/fix-mcpl-iserror-is-a-failure.fixed.md create mode 100644 test/mcpl-iserror-failure.test.ts diff --git a/changelog.d/fix-mcpl-iserror-is-a-failure.fixed.md b/changelog.d/fix-mcpl-iserror-is-a-failure.fixed.md new file mode 100644 index 000000000..6babc89ce --- /dev/null +++ b/changelog.d/fix-mcpl-iserror-is-a-failure.fixed.md @@ -0,0 +1,2 @@ +- An MCPL server's error result (`isError: true`) is now a failure for a caller in process too. `executeToolCall` and a module's `ModuleContext.callTool` got such a result as a success, `success: true` with the server's blocks as `data`, so a caller that checks `success` took a refused call for one that worked. They now get the failure the model's call gets: `success: false`, `isError: true`, and as `error` the result's text blocks joined by line breaks, a text that isn't a string shown as the converters' placeholder, or `Tool call failed` when it has no text. Its blocks that aren't text, which such a caller used to get in `data`, aren't kept, as the model's call never kept them. A success is unchanged: the server's blocks as `data`. The admin puppet (`puppetToolCall`) calls through the same path, so it now stores a server's error as an error result, where it stored it as an ordinary result the agent read as the tool's answer, and its `puppet:tool-call` trace now says `isError: true`. +- A server's error result through the model's call, and a channel tool's error result, such as `channel_open`'s refusal of a channel it doesn't know, are now traced as `tool:failed` with their error, where they were traced as `tool:completed`, so a host that shows failed tools, as connectome-host's terminal view does, now shows these among them. What the agent reads is unchanged. diff --git a/src/framework.ts b/src/framework.ts index 6d854a393..c30822d31 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -92,7 +92,7 @@ import { coalescingSubjectKey, type AssemblyResult, type CoalescedOccurrence, type CoalescingPlacement, type CoalescingSnapshot, type CoalescingReceiptRecord, } from './mcpl/push-coalescer.js'; -import type { ChannelIncomingMessage, ChannelIncomingMessageResult, McplContentBlock, PushEventResult } from './mcpl/types.js'; +import type { ChannelIncomingMessage, ChannelIncomingMessageResult, McplContentBlock, McpToolResultContent, PushEventResult } from './mcpl/types.js'; import { isSilentHeartbeatMarker, isVisiblyEmptyContent, NON_STRING_TEXT_PLACEHOLDER } from './mcpl/visible-content.js'; import { parseProsePrefix, parseHybridProsePrefix } from './mcpl/prose-grammar.js'; import { ProseStreamRouter } from './mcpl/prose-stream-router.js'; @@ -291,6 +291,32 @@ function extractChannelIdFromToolResult( } } +/** + * The text of an MCPL tools/call result: its text blocks' text, joined by + * line breaks, with empty ones left out. A text block whose text isn't a + * string, which only a nonconformant server sends, shows as the converters' + * placeholder, and content that isn't a list, which only such a server sends + * too, has no text. + */ +function mcplResultText(content: McpToolResultContent[] | undefined): string { + return (Array.isArray(content) ? content : []) + .filter((c) => c.type === 'text') + .map((c) => (typeof c.text === 'string' ? c.text : NON_STRING_TEXT_PLACEHOLDER)) + .filter((text) => text !== '') + .join('\n'); +} + +/** + * 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. + */ +function mcplToolFailure(content: McpToolResultContent[] | undefined): ToolResult { + return { success: false, error: mcplResultText(content) || 'Tool call failed', isError: true }; +} + /** * True when an incoming channel message explicitly addressed the agent. * Primary signal is the MCPL RFC-001 `chat:addressed` tag (mention / reply / @@ -15418,6 +15444,10 @@ export class AgentFramework { } 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); return { success: true, data: result.content, @@ -19953,7 +19983,28 @@ export class AgentFramework { }) .then((result) => { const durationMs = Date.now() - startTime; - this.emitTrace({ type: 'tool:completed', module: `mcpl:${serverId}`, tool: toolName, callId: call.id, durationMs }); + // Convert MCP tool result to framework ToolResult. A server's error + // is a failure (mcplToolFailure, which an in-process caller's error + // takes too). When a success contains non-text blocks (e.g. images + // from an MCP tool like zulip-mcp's fetch_attachment), pass the full + // content array through so toMembraneToolResult can preserve image + // blocks natively. Text-only results still collapse to a joined + // string for backward compatibility with callers that expect data to + // be string-ish. + const converted: ToolResult = result.isError + ? mcplToolFailure(result.content) + : { + success: true, + data: result.content?.some((c) => c.type !== 'text') + ? result.content + : (mcplResultText(result.content) || undefined), + isError: false, + }; + this.emitTrace({ + type: converted.isError ? 'tool:failed' : 'tool:completed', + module: `mcpl:${serverId}`, tool: toolName, callId: call.id, durationMs, + ...(converted.isError ? { error: converted.error } : {}), + }); // Record checkpoint from stateful tool response (Step 8). // Explicit response attribution is authoritative. Otherwise use the @@ -20016,39 +20067,13 @@ export class AgentFramework { } } - // Convert MCP tool result to framework ToolResult. - // When the result contains non-text blocks (e.g. images from an MCP - // tool like zulip-mcp's fetch_attachment), pass the full content array - // through so toMembraneToolResult can preserve image blocks natively. - // Text-only results still collapse to a joined string for backward - // compatibility with callers that expect data to be string-ish. - // A text block whose text isn't a string, which only a nonconformant - // server sends, shows as the converter's placeholder, where it used to - // vanish (null, 0, false) or read as String(text), "[object Object]". - const textContent = result.content - ?.filter((c) => c.type === 'text') - .map((c) => (typeof c.text === 'string' ? c.text : NON_STRING_TEXT_PLACEHOLDER)) - .filter((text) => text !== '') - .join('\n'); - const hasNonText = result.content?.some((c) => c.type !== 'text'); - const data = result.isError - ? undefined - : hasNonText - ? result.content - : (textContent || undefined); - this.pushEvent({ type: 'tool-result', callId: call.id, toolName: call.name, agentName, moduleName: `mcpl:${serverId}`, - result: { - success: !result.isError, - data, - error: result.isError ? (textContent || 'Tool call failed') : undefined, - isError: result.isError ?? false, - }, + result: converted, }); }) .catch((error) => { @@ -20130,7 +20155,11 @@ export class AgentFramework { this.channelRegistry!.handleChannelToolCall(call.name, call.input, { kind: 'agent', agentName }) .then((result) => { const durationMs = Date.now() - startTime; - this.emitTrace({ type: 'tool:completed', module: 'channels', tool: call.name, callId: call.id, durationMs }); + this.emitTrace({ + type: result.isError ? 'tool:failed' : 'tool:completed', + module: 'channels', tool: call.name, callId: call.id, durationMs, + ...(result.isError ? { error: result.error } : {}), + }); // A successful channel_open plants the agent's feet in the opened // channel. The agent's own deliberate open is the strongest "my next // words go here" signal in the system — stronger than any injection — diff --git a/test/mcpl-iserror-failure.test.ts b/test/mcpl-iserror-failure.test.ts new file mode 100644 index 000000000..4e6e70422 --- /dev/null +++ b/test/mcpl-iserror-failure.test.ts @@ -0,0 +1,141 @@ +/** + * A server's error result (`isError: true`) is a failure on each path a tool + * call takes: an in-process caller (`executeToolCall`, + * `ModuleContext.callTool`, and the admin puppet, which calls through them) + * gets the failure the model's call reads, and the traces of the model's call + * and of a channel tool name it as `tool:failed`. A success is unchanged on + * each path. + */ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { fixture } from './helpers/coalescing-fixture.js'; +import { NON_STRING_TEXT_PLACEHOLDER } from '../src/mcpl/visible-content.js'; +import type { ToolResult } from '../src/index.js'; + +const ANSWERS: Record = { + one: { isError: true, content: [{ type: 'text', text: 'no such row' }] }, + 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: [] }, + // A nonconformant server's: no content, or content that isn't a list. + noContent: { isError: true }, + notList: { isError: true, content: 'not a list' }, + blank: { isError: true, content: [{ type: 'text', text: '' }, { type: 'text', text: 'after a blank' }] }, + image: { isError: true, content: [{ type: 'image', data: 'aGk=', mimeType: 'image/png' }, { type: 'text', text: 'see the image' }] }, + ok: { content: [{ type: 'text', text: 'a' }, { type: 'text', text: 'b' }] }, + okImage: { content: [{ type: 'image', data: 'aGk=', mimeType: 'image/png' }, { type: 'text', text: 'caption' }] }, + okEmpty: { content: [] }, +}; + +/** What each error answer reads as: its text, or `Tool call failed` 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', + blank: 'after a blank', + image: 'see the image', +}; + +type Trace = Record; + +async function setup(t: { after(fn: () => unknown): void }) { + let ctx: { callTool(call: unknown): Promise } | undefined; + const probe = { + name: 'probe', + async start(c: typeof ctx) { ctx = c; }, + async stop() {}, + getTools() { return []; }, + async handleToolCall() { return { success: false, error: 'no tools', isError: true }; }, + async onProcess() { return {}; }, + }; + const f = await fixture({ + tools: [{ name: 'lookup', description: 'Look something up.', inputSchema: { type: 'object' } }], + toolsCall: (params) => ANSWERS[(params.arguments as { case: string }).case], + framework: { modules: [probe] } as never, + }); + t.after(f.close); + const traces: Trace[] = []; + f.framework.onTrace((e) => { traces.push(e as unknown as Trace); }); + const internals = f.framework as unknown as { pushEvent(event: unknown): void; dispatchToolCall(agent: string, call: unknown): void }; + /** The result a model's call of `name` gets, as the framework pushes it. */ + const viaModel = (id: string, name: string, input: unknown) => new Promise((resolve) => { + const original = internals.pushEvent; + internals.pushEvent = (event: unknown) => { + const e = event as { type: string; callId?: string; result?: ToolResult }; + if (e.type === 'tool-result' && e.callId === id) { internals.pushEvent = original; resolve(e.result!); } + else original.call(internals, event); + }; + internals.dispatchToolCall('agent', { id, name, input }); + }); + /** A call's terminal trace events: its type, module and error. */ + const terminal = (id: string) => traces + .filter((e) => e.callId === id && (e.type === 'tool:completed' || e.type === 'tool:failed')) + .map((e) => ({ type: e.type, module: e.module, ...(e.error !== undefined ? { error: e.error } : {}) })); + return { f, ctx: () => ctx!, traces, viaModel, terminal }; +} + +/** A result's four fields, so a field left out and one set to undefined compare alike. */ +const fields = (r: ToolResult) => ({ success: r.success, data: r.data, error: r.error, isError: r.isError }); + +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) => { + const { f, ctx, viaModel } = await setup(t); + for (const [c, error] of Object.entries(ERRORS)) { + const failure = { success: false, data: undefined, error, isError: true }; + assert.deepEqual(fields(await f.framework.executeToolCall(lookup(`x-${c}`, c))), failure, `executeToolCall, ${c}`); + assert.deepEqual(fields(await ctx().callTool(lookup(`m-${c}`, c))), failure, `ModuleContext.callTool, ${c}`); + assert.deepEqual(fields(await viaModel(`d-${c}`, 'mcpl--editor--lookup', { case: c })), failure, `the model's call, ${c}`); + } +}); + +test('an in-process caller\'s success is still the server\'s own blocks, and the model\'s call still reads text as one string', { timeout: 20_000 }, async (t) => { + const { f, ctx, viaModel } = await setup(t); + const blocks = (ANSWERS.ok as { content: unknown[] }).content; + assert.deepEqual(fields(await f.framework.executeToolCall(lookup('x-ok', 'ok'))), { success: true, data: blocks, error: undefined, isError: undefined }); + assert.deepEqual(fields(await ctx().callTool(lookup('m-ok', 'ok'))), { success: true, data: blocks, error: undefined, isError: undefined }); + const withImage = (ANSWERS.okImage as { content: unknown[] }).content; + assert.deepEqual(fields(await f.framework.executeToolCall(lookup('x-okImage', 'okImage'))), { success: true, data: withImage, error: undefined, isError: undefined }); + assert.deepEqual(fields(await viaModel('d-ok', 'mcpl--editor--lookup', { case: 'ok' })), { success: true, data: 'a\nb', error: undefined, isError: false }); + assert.deepEqual(fields(await viaModel('d-okImage', 'mcpl--editor--lookup', { case: 'okImage' })), { success: true, data: withImage, error: undefined, isError: false }); + assert.deepEqual(fields(await f.framework.executeToolCall(lookup('x-okEmpty', 'okEmpty'))), { success: true, data: [], error: undefined, isError: undefined }); + assert.deepEqual(fields(await viaModel('d-okEmpty', 'mcpl--editor--lookup', { case: 'okEmpty' })), { success: true, data: undefined, error: undefined, isError: false }); +}); + +test('the admin puppet stores a server\'s error as an error result, and traces it as one', { timeout: 20_000 }, async (t) => { + const { f, traces } = await setup(t); + const before = traces.length; + const { toolUseId, result } = await f.framework.puppetToolCall('agent', 'mcpl--editor--lookup', { case: 'one' }); + assert.deepEqual(fields(result), { success: false, data: undefined, error: 'no such row', isError: true }); + const stored = f.framework.getAgent('agent')!.getContextManager().getAllMessages() + .flatMap((m) => m.content as unknown as Array>) + .find((b) => b.type === 'tool_result' && b.toolUseId === toolUseId); + assert.ok(stored, 'the pair is stored'); + 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]); +}); + +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) => { + const { viaModel, terminal } = await setup(t); + 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' }]); + await viaModel('d-ok', 'mcpl--editor--lookup', { case: 'ok' }); + assert.deepEqual(terminal('d-ok'), [{ type: 'tool:completed', module: 'mcpl:editor' }]); +}); + +test('a channel tool\'s error result is traced as tool:failed with its error, and a success as tool:completed', { timeout: 20_000 }, async (t) => { + const { viaModel, terminal } = await setup(t); + const refused = await viaModel('ch-open', 'channel_open', { channelId: 'nope' }); + assert.equal(refused.isError, true); + assert.deepEqual(terminal('ch-open'), [{ type: 'tool:failed', module: 'channels', error: refused.error }]); + assert.equal(typeof refused.error, 'string'); + const listed = await viaModel('ch-list', 'channel_list', {}); + assert.equal(listed.success, true); + assert.deepEqual(terminal('ch-list'), [{ type: 'tool:completed', module: 'channels' }]); +});