fix(mcpl): a server's error result is a failure on each path a tool call takes

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.

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<string, unknown>);
+      // 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 —

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<string, unknown> = {
+  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<string, string> = {
+  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<string, unknown>;
+
+async function setup(t: { after(fn: () => unknown): void }) {
+  let ctx: { callTool(call: unknown): Promise<ToolResult> } | 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<ToolResult>((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<Record<string, unknown>>)
+    .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' }]);
+});