fix(tools): a module's tool result that can't be stored becomes an error naming the tool (#309)

changelog.d/309-unconvertible-tool-result.fixed.md

@@ -0,0 +1,27 @@
+- A module's tool result that JSON can't hold, such as a cyclic object, a
+  BigInt or a `toJSON` that throws, is now answered as an error result
+  naming the tool wherever the framework stores or sends it: a model's
+  call, a script's inner call, the `utils` meta-tool for a model,
+  `read_image`, and the pair `puppetToolCall` stores. So is a result that
+  isn't a result object (`undefined`, `null`, an array, a string, an `Error`
+  returned rather than thrown), a result whose getters or Proxy traps throw
+  while it's read, and a non-error result whose data is a function or a
+  symbol, which JSON leaves out whole. The error keeps the module's own
+  error text and its `endTurn` when they can be read. Before, the first
+  code to store, send or trace such a result threw, and the agent's turn
+  never settled (#309). Those paths now read a module's result once and use
+  its JSON form, so its getters and `toJSON` never run a second time. A
+  caller that uses the result in process (`ModuleContext.callTool`,
+  `executeToolCall`) still gets the module's own result, and
+  `puppetToolCall` still returns it, or the error result where it used to
+  reject.
+- A module's `error` is sent as text: an `Error`'s message, an object's own
+  `message`, any other value's JSON, or a BigInt's or symbol's `String()`.
+  One that says nothing (`null`, `false` or empty), and a throw with no
+  text, read as "Unknown error", as a missing one does. A result's
+  `success`, `isError` and `endTurn` are sent as booleans.
+- A tool result with no data is stored as empty text, as an empty content
+  array is. Before, storing it threw, and the turn never settled. A
+  failure the tool doesn't flag as an error for the model
+  (`{ success: false, error, isError: false }`) with no data reads as its
+  error text, for the model and for a script.

src/agent.ts

@@ -12,7 +12,7 @@ import {
   type StoredRowLike,
 } from './silent-heartbeat.js';
 import {
-  toolResultDataToHistoryString,
+  nonErrorResultText,
   truncateForHistory,
   DEFAULT_TOOL_RESULT_INLINE_MAX_CHARS,
 } from './tool-result-history.js';
@@ -1277,7 +1277,7 @@ export class Agent {
       toolUseId: r.id,
       content: r.result.isError
         ? truncateForHistory(r.result.error ?? 'Unknown error', DEFAULT_TOOL_RESULT_INLINE_MAX_CHARS)
-        : toolResultDataToHistoryString(r.result.data, DEFAULT_TOOL_RESULT_INLINE_MAX_CHARS),
+        : nonErrorResultText(r.result, DEFAULT_TOOL_RESULT_INLINE_MAX_CHARS),
       isError: r.result.isError,
     }));
 

src/framework.ts

@@ -64,7 +64,7 @@ import { ProcessQueueImpl } from './queue.js';
 import { DEFAULT_SHUTDOWN_TIMEOUT_MS, validateShutdownTimeout, shutdownErrorMessage } from './shutdown.js';
 import { REFUSAL_REACTIONS, REFUSAL_REACTION_FALLBACK } from './refusal-reactions.js';
 import { Agent } from './agent.js';
-import { ModuleRegistry, isStateExistsError } from './module-registry.js';
+import { ModuleRegistry, checkedToolResult, isStateExistsError, storableToolResult, thrownResult } from './module-registry.js';
 import { McplServerRegistry } from './mcpl/server-registry.js';
 import { FeatureSetManager } from './mcpl/feature-set-manager.js';
 import { computeGrant, CapabilityGrant, expandAdvertisementShorthand } from './mcpl/capability-grant.js';
@@ -90,6 +90,7 @@ import { ConversationRouter, CONVERSATION_ROUTING_DEPRECATION_NOTICE } from './m
 import { safeSlice } from './safe-slice.js';
 import type { WorkspaceModule } from './modules/workspace/index.js';
 import {
+  nonErrorResultText,
   toolResultDataToHistoryString,
   truncateForHistory,
   DEFAULT_TOOL_RESULT_INLINE_MAX_CHARS,
@@ -11151,13 +11152,15 @@ export class AgentFramework {
     let ownedToEnd = false;
     try {
       const started = Date.now();
-      const result = await this.executeToolCall({
-        id: toolUseId,
-        name: toolName,
-        input,
-        callerAgentName: agentName,
-      });
+      const call = { id: toolUseId, name: toolName, input, callerAgentName: agentName };
+      const returned = await this.executeToolCall(call);
       const durationMs = Date.now() - started;
+      // The pair is stored as a model's call would be, so the result stored,
+      // traced and logged is its checked form. The caller gets the tool's
+      // own result back, as an in-process caller does, unless nothing
+      // usable could be stored: then it gets the error result stored.
+      const checked = checkedToolResult(call, returned);
+      const result = checked.result;
 
       // Store the pair through the same shapes the ordinary path uses. Build
       // the result blocks BEFORE storing the tool_use: the spill path awaits.
@@ -11205,7 +11208,7 @@ export class AgentFramework {
         `[puppet] ${agentName}: ${toolName} → ${result.isError ? 'ERROR' : 'ok'} ` +
         `in ${durationMs}ms (${toolUseId})${ownedToEnd ? '' : ' — deferred behind a live turn'}`,
       );
-      return { toolUseId, result };
+      return { toolUseId, result: checked.usable ? returned : result };
     } finally {
       if (this.activeTurnTokens.get(agentName) === turnToken) this.activeTurnTokens.delete(agentName);
       // Messages deferred while we held the token land now, after the pair
@@ -11268,10 +11271,11 @@ export class AgentFramework {
     // ephemerals, ModuleContext.callTool) reach utilities the same way the
     // model does.
     if (call.name === 'utils') {
-      return this.handleUtilsToolCall(call.callerAgentName ?? '__ephemeral__', call);
+      return this.handleUtilsToolCall(call.callerAgentName ?? '__ephemeral__', call, { inProcess: true });
     }
 
-    // Module tools
+    // Module tools. The caller uses the result in process, so it gets the
+    // module's own result, as the module returned it.
     return this.moduleRegistry.handleToolCall(call);
   }
 
@@ -11773,7 +11777,7 @@ export class AgentFramework {
       // stays reachable by provenance after the bytes leave the wire.
       const raw = tc.result.isError
         ? tc.result.error ?? 'Unknown error'
-        : toolResultDataToHistoryString(tc.result.data, undefined, {
+        : nonErrorResultText(tc.result, undefined, {
           imageRef: (blockIndex, image) => ledger.retain({
             toolCallId: tc.id,
             toolName: tc.name,
@@ -11921,7 +11925,7 @@ export class AgentFramework {
     if (result.isError) {
       return `Error: ${result.error ?? 'tool call failed'}`;
     }
-    if (result.data === undefined) return '';
+    if (result.data === undefined || result.data === null) return nonErrorResultText(result, 5_000_000);
     // RFC-005 "eagerly lazy": a script receiving a result is the strongest
     // available signal that the bytes are wanted (scripts process, they do
     // not browse) — materialize every referenced payload now, so the stub
@@ -12527,7 +12531,7 @@ export class AgentFramework {
     const startTime = Date.now();
 
     this.moduleRegistry
-      .handleToolCall({ ...call, callerAgentName: agentName })
+      .storableToolCall({ ...call, callerAgentName: agentName })
       .then((result) => {
         const durationMs = Date.now() - startTime;
         this.emitTrace({
@@ -15526,7 +15530,11 @@ export class AgentFramework {
           typeof workspace.getTools === 'function' &&
           workspace.getTools().some((tool) => tool.name === 'read_image')
         ) {
-          const result = await workspace.handleToolCall({ ...call, name: 'read_image' });
+          // The workspace module's own handler, called directly: its result
+          // gets the same check a module's result gets in the registry. The
+          // agent's name is for the check's log line, not the handler.
+          const moduleCall = { ...call, name: 'read_image' };
+          const result = storableToolResult({ ...moduleCall, callerAgentName: agentName }, await workspace.handleToolCall(moduleCall));
           if (result.success && Array.isArray(result.data)) {
             const image = result.data.find(
               (block): block is { type: 'image'; data: string; mimeType: string } =>
@@ -15571,11 +15579,7 @@ export class AgentFramework {
           ],
         });
       } catch (error) {
-        finish({
-          success: false,
-          error: error instanceof Error ? error.message : String(error),
-          isError: true,
-        });
+        finish(thrownResult(error));
       }
     })();
   }
@@ -15978,7 +15982,7 @@ export class AgentFramework {
    *  (Module.getUtilities). Shared by the model-facing dispatcher and the
    *  programmatic path (executeToolCall), so code_execution scripts and
    *  ephemerals reach utilities too. */
-  private async handleUtilsToolCall(agentName: string, call: ToolCall): Promise<ToolResult> {
+  private async handleUtilsToolCall(agentName: string, call: ToolCall, options: { inProcess?: boolean } = {}): Promise<ToolResult> {
     const input = (call.input ?? {}) as { action?: unknown; name?: unknown; args?: unknown };
     const utilities = this.moduleRegistry.getAllUtilities();
     const available = (): string =>
@@ -16013,13 +16017,12 @@ export class AgentFramework {
           };
         }
         // Same dispatch as a first-class tool call — the module cannot tell
-        // which surface the call came through.
-        return this.moduleRegistry.handleToolCall({
-          id: call.id,
-          name: def.name,
-          input: args,
-          callerAgentName: agentName,
-        });
+        // which surface the call came through. An in-process caller gets the
+        // module's own result; the model's path takes its checked form.
+        const utilityCall = { id: call.id, name: def.name, input: args, callerAgentName: agentName };
+        return options.inProcess
+          ? this.moduleRegistry.handleToolCall(utilityCall)
+          : this.moduleRegistry.storableToolCall(utilityCall);
       }
       default:
         return { success: false, isError: true, error: 'action must be "list", "describe", or "run"' };

src/module-registry.ts

@@ -273,7 +273,20 @@ export class ModuleRegistry {
   }
 
   /**
-   * Handle a tool call by routing to the appropriate module.
+   * A module tool's call for a path that stores or sends its result: the
+   * model's dispatch, a script's inner call, the `utils` meta-tool for a
+   * model. The result is checked and read once, in its JSON form
+   * ({@link storableToolResult}).
+   */
+  async storableToolCall(call: ToolCall): Promise<ToolResult> {
+    return storableToolResult(call, await this.handleToolCall(call));
+  }
+
+  /**
+   * Handle a tool call by routing to the appropriate module: the module's own
+   * result, unchecked, as it returned it, for a caller that uses the result
+   * in process (`ModuleContext.callTool`, `executeToolCall`, or a host
+   * calling the registry). A module's throw is answered as an error result.
    */
   async handleToolCall(call: ToolCall): Promise<ToolResult> {
     // Parse module--tool name
@@ -309,11 +322,7 @@ export class ModuleRegistry {
     try {
       return await module.handleToolCall(moduleCall);
     } catch (error) {
-      return {
-        success: false,
-        error: error instanceof Error ? error.message : String(error),
-        isError: true,
-      };
+      return thrownResult(error);
     }
   }
 
@@ -792,3 +801,149 @@ class ProcessStateImpl implements ProcessState {
     this.ctx.pushEvent(event);
   }
 }
+
+/** A result's flags, which consumers read for their truth: booleans here. */
+const RESULT_FLAGS = ['success', 'isError', 'endTurn'] as const;
+
+/**
+ * A module's tool result as the framework can store, send and trace it. A
+ * module runs in the host's process, so it can return what JSON can't hold,
+ * such as a cyclic object, a BigInt or a `toJSON` that throws, no result
+ * object at all, or one whose getters or Proxy traps throw, or answer
+ * differently each time they're read. Everything after this (the stored
+ * history, the request sent to the model, traces, the tool-result guard's
+ * audit record, a script's result) would throw on it, and the agent's turn
+ * would never settle.
+ *
+ * So the result is read once, here, and this never throws. Each field a
+ * consumer reads is read by name, so a result class's getters count, and any
+ * other own field is kept, except a `toJSON`: consumers read a result's
+ * fields, never a `toJSON` of the result. What's returned is the JSON form of
+ * those fields, parsed, so every consumer holds plain data and no module code
+ * runs again. It's taken where the framework stores or sends a result
+ * ({@link ModuleRegistry.storableToolCall}). A caller that uses the result
+ * in process (`ModuleContext.callTool`, `executeToolCall`) gets the module's
+ * own result, as the module returned it ({@link ModuleRegistry.handleToolCall}).
+ * The flags are booleans, read for their truth as consumers read them.
+ * `error` is its text form, which is all a consumer reads of it, so an error
+ * value JSON can't hold costs nothing. An error that says nothing (null,
+ * false or empty) is left out, so the result reads as having no error text.
+ *
+ * A result that can't be read or converted becomes an error result naming the
+ * tool, keeping the module's own error text and its `endTurn` when they could
+ * be read. So does data that is a function or a symbol, which JSON leaves out
+ * whole.
+ */
+export function storableToolResult(call: ToolCall, result: unknown): ToolResult {
+  return checkedToolResult(call, result).result;
+}
+
+/**
+ * {@link storableToolResult}, saying whether the module's result was usable:
+ * read and converted (`usable`), or replaced by the error result naming the
+ * tool. A caller that stores the result and hands the module's own result
+ * back, as `puppetToolCall` does, hands back the error result instead when
+ * there was nothing usable to store.
+ */
+export function checkedToolResult(call: ToolCall, result: unknown): { result: ToolResult; usable: boolean } {
+  // What's been read so far, kept by an error result if a later read throws.
+  let ownText: string | undefined;
+  let endTurn = false;
+  const unusable = (problem: string): { result: ToolResult; usable: boolean } => {
+    const own = ownText ? `, with this error: ${ownText}` : '.';
+    const error = `Tool '${call.name}' ${problem}. The call itself completed${own}`;
+    console.error(`[tools] ${call.callerAgentName ? `agent ${call.callerAgentName}: ` : ''}${error}`);
+    return { result: { success: false, isError: true, error, ...(endTurn ? { endTurn: true } : {}) }, usable: false };
+  };
+  const reasonOf = (error: unknown) => thrownText(error).split('\n')[0] || 'no reason given';
+  try {
+    if (!result || typeof result !== 'object' || Array.isArray(result) || result instanceof Error) {
+      const what = result === undefined || result === null ? String(result)
+        : Array.isArray(result) ? 'an array' : result instanceof Error ? 'an Error' : `a ${typeof result}`;
+      if (result instanceof Error) ownText = errorText(result);
+      return unusable(`returned ${what} instead of a result object, so there's nothing to store or send`);
+    }
+    const candidate = result as Record<string, unknown>;
+    // No prototype, so an own field named like an inherited one is a field.
+    const fields: Record<string, unknown> = Object.create(null);
+    const read = (key: string) => { if (key in candidate) fields[key] = candidate[key]; };
+    // endTurn first and the error's text next, so an error result keeps
+    // whichever of them could be read when a later read throws.
+    read('endTurn');
+    endTurn = Boolean(fields.endTurn);
+    read('error');
+    ownText = errorText(fields.error);
+    for (const key of ['success', 'isError', 'data']) read(key);
+    for (const key of Object.keys(candidate)) {
+      if (key !== 'toJSON' && !(key in fields)) fields[key] = candidate[key];
+    }
+    // The error is kept as its text, which consumers read; its value isn't.
+    delete fields.error;
+    // The flags are kept as their truth, which is all consumers read of them,
+    // before anything is serialized, so a flag's own value (a BigInt, a
+    // function, an object with a toJSON) neither breaks nor runs anything.
+    for (const flag of RESULT_FLAGS) {
+      if (fields[flag] !== undefined) fields[flag] = Boolean(fields[flag]);
+    }
+    // An error result reads as its error, so only a non-error result's data matters.
+    if (!fields.isError && (typeof fields.data === 'function' || typeof fields.data === 'symbol')) {
+      return unusable(`returned a ${typeof fields.data} as its data, which JSON leaves out, so there's nothing to store or send`);
+    }
+    let json: string;
+    try {
+      json = JSON.stringify(fields);
+    } catch (error) {
+      return unusable(`returned a result that can't be converted to JSON, so it can't be stored or sent (${reasonOf(error)})`);
+    }
+    const storable = JSON.parse(json) as Record<string, unknown>;
+    if (ownText !== undefined) storable.error = ownText;
+    return { result: storable as unknown as ToolResult, usable: true };
+  } catch (error) {
+    return unusable(`returned a result that can't be read, so it can't be stored or sent (${reasonOf(error)})`);
+  }
+}
+
+/** A result's `error` as text, always a string or none: a string as it is,
+ *  an `Error`'s message read the same way (a module's value, so not always a
+ *  string), an object's own `message`, any other value's JSON, and a
+ *  primitive JSON can't hold (a BigInt, a symbol) as its String(). None for a
+ *  value that says nothing: absent, null, false or empty, which consumers
+ *  then report as "Unknown error". */
+function errorText(error: unknown): string | undefined {
+  if (error === undefined || error === null || error === false || error === '') return undefined;
+  if (typeof error === 'string') return error;
+  if (error instanceof Error) return errorText(error.message);
+  const message = (error as { message?: unknown }).message;
+  if (typeof message === 'string' && message) return message;
+  try {
+    const json = JSON.stringify(error) as string | undefined;
+    if (json !== undefined) return json;
+  } catch {
+    // A cyclic or BigInt-holding value: nothing faithful to show as JSON.
+  }
+  if (typeof error !== 'object' && typeof error !== 'function') return thrownText(error);
+  return 'an error value with no text form';
+}
+
+/** A module's throw, answered as an error result: its text, or none when it
+ *  has none, so consumers say "Unknown error". */
+export function thrownResult(error: unknown): ToolResult {
+  const text = thrownText(error);
+  return { success: false, isError: true, ...(text ? { error: text } : {}) };
+}
+
+/** A thrown value's whole text, always a string, for a result's error, and
+ *  for a value that has none too. An `Error`'s message is a module's value,
+ *  so it may not be a string: one that isn't is given its String(), and an
+ *  absent one is empty. String() throws on an object without a prototype,
+ *  and such a value is named as having no text form. */
+function thrownText(value: unknown): string {
+  try {
+    if (!(value instanceof Error)) return String(value);
+    const message: unknown = value.message;
+    if (typeof message === 'string') return message;
+    return message === undefined || message === null ? '' : String(message);
+  } catch {
+    return 'a value with no text form';
+  }
+}