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' }]); +});