diff --git a/changelog.d/script-result-flags.added.md b/changelog.d/script-result-flags.added.md new file mode 100644 index 000000000..11015b6ee --- /dev/null +++ b/changelog.d/script-result-flags.added.md @@ -0,0 +1,15 @@ +- **A `code_execution` script can test a result for an error, and read its + structured content, without parsing.** Each tool function's string is + unchanged, and carries two attributes beside it: `.is_error`, `True` for an + error result (a failure the framework reports as `Error: …`, or a + structured result whose `isError` is true) and `False` otherwise, and + `.structured`, the parsed `structuredContent` of a structured result's + JSON object, or `None`. A result whose text merely starts with `Error:` + reads `False`. The string is now a `str` subclass, so code that uses it as + a string keeps working, `json.loads` and `isinstance(r, str)` included, + while an exact type check such as `type(r) is str` now reads `False`, and + a result a script pickles loads only in the script runtime. + `PyRunner`'s `onToolCall` may return a `ScriptToolResult` (exported) to + set both, or a plain string as before, for which both read `None`. + `code_execution`'s description changes, so the first request after the + upgrade that carries it misses the prompt cache from its tool block on. diff --git a/src/code-execution/index.ts b/src/code-execution/index.ts index 58f5f2a7c..062fd1f38 100644 --- a/src/code-execution/index.ts +++ b/src/code-execution/index.ts @@ -14,6 +14,7 @@ export type { ExecResult, PyRunnerOptions, ScriptToolCallHandler, + ScriptToolResult, BackgroundExecOptions, } from './py-runner.js'; export { PYTHON_RUNTIME_SOURCE } from './runtime-py.js'; diff --git a/src/code-execution/py-runner.ts b/src/code-execution/py-runner.ts index a534061b3..397be0054 100644 --- a/src/code-execution/py-runner.ts +++ b/src/code-execution/py-runner.ts @@ -55,11 +55,30 @@ export interface BackgroundExecOptions { lifetimeMs: number; } -/** Resolves inner tool calls. Must never reject — map errors to strings. */ +/** + * An inner tool call's result as the script gets it. `text` is the string + * its await returns, and the runner sets `.is_error` on that string from + * `isError`. With `structuredJson`, `text` is the JSON object a structured + * result reaches a script as, in any of its forms, and `.structured` reads + * its `structuredContent`, None for a form that holds none, such as one past + * the size cap; without it, `.structured` is None. + */ +export interface ScriptToolResult { + text: string; + isError: boolean; + structuredJson?: boolean; +} + +/** + * Resolves inner tool calls. Must never reject — map errors to results; a + * rejection reaches the script as an `Error: …` result. A plain string is + * the result's text with nothing said about it, so the script's + * `.is_error` and `.structured` are None. + */ export type ScriptToolCallHandler = ( toolName: string, args: Record, -) => Promise; +) => Promise; export interface PyRunnerOptions { pythonPath?: string; @@ -530,9 +549,24 @@ export class PyRunner { ? (msg.args as Record) : {}; this.onToolCall(toolName, args) - .catch((err) => `Error: ${err instanceof Error ? err.message : String(err)}`) + .catch((err): ScriptToolResult => ({ + text: `Error: ${err instanceof Error ? err.message : String(err)}`, + isError: true, + })) .then((result) => { - this.reply(child, { op: 'tool_result', id: callId, result }, `result of ${toolName} call ${callId}`); + // Only an object is a ScriptToolResult. Anything else a handler + // resolves with, a string or a value its type rules out, is sent + // as it is, so the call is still answered. + const reply = typeof result === 'object' && result !== null + ? { + op: 'tool_result', + id: callId, + result: result.text, + is_error: result.isError, + ...(result.structuredJson ? { structured_json: true } : {}), + } + : { op: 'tool_result', id: callId, result }; + this.reply(child, reply, `result of ${toolName} call ${callId}`); }); return; } diff --git a/src/code-execution/runtime-py.ts b/src/code-execution/runtime-py.ts index 840f61826..89147ed35 100644 --- a/src/code-execution/runtime-py.ts +++ b/src/code-execution/runtime-py.ts @@ -7,7 +7,8 @@ * * - scripts run with top-level `await` (PyCF_ALLOW_TOP_LEVEL_AWAIT) * - host tools are injected as async functions taking a single dict and - * returning a string (the tool_result text) + * returning the tool_result text as a str (ToolResult), which also + * carries .is_error and .structured * - a pending tool call that gets no response raises TimeoutError with the * same message shape the managed runtime produces * - interpreter globals persist across execs (container-reuse semantics) @@ -29,7 +30,7 @@ * host -> py : {op:"init", tools:[{py_name,tool_name}], call_timeout_s, * background?, log_path?, tail_chars?} * {op:"exec", id, code} - * {op:"tool_result", id, result} + * {op:"tool_result", id, result, is_error?, structured_json?} * {op:"wake_ack", id, error?} * {op:"cancel", id, reason?} * py -> host : {op:"ready"} @@ -49,6 +50,7 @@ export const PYTHON_RUNTIME_SOURCE: string = String.raw` import ast import asyncio +import functools import inspect import io import json @@ -258,6 +260,31 @@ def _deliver_cancel(st): st.task.cancel() +class ToolResult(str): + """A tool's result: the string the tool gave, with two attributes. + + is_error: True if the result is an error, False if not, None if the + host didn't say. + structured: the parsed structuredContent of a structured result's JSON + object, or None. + """ + + # Defaults, since pickle and copy rebuild a str subclass from its text + # alone and then restore the attributes. + def __new__(cls, text="", is_error=None, structured_json=False): + self = str.__new__(cls, text) + self.is_error = is_error + self._structured_json = structured_json + return self + + # Read from the string itself, the one copy sent, on first use only. + @functools.cached_property + def structured(self): + if not self._structured_json: + return None + return json.loads(self).get("structuredContent") + + def _resolve(fut, value): # Defense in depth: the script may have been cancelled since the lookup. if fut is not None and not fut.done(): @@ -307,8 +334,9 @@ def _make_tool_fn(tool_name, py_name): meant.update(kwargs) # A key is what the agent has to write, so a str key is shown # whole up to 60 characters, as names are elsewhere; a longer one, - # or any other key, is shown as a value is. - shown = ", ".join((repr(k) if type(k) is str and len(k) <= 60 else _brief(k)) + ": " + _brief(v) + # or any other key, is shown as a value is. A tool's result counts + # as a str: ToolResult keeps str's repr, so no script code runs. + shown = ", ".join((repr(k) if (type(k) is str or type(k) is ToolResult) and len(k) <= 60 else _brief(k)) + ": " + _brief(v) for k, v in meant.items()) raise TypeError( py_name + " wasn't called: a tool function takes one dict of " @@ -347,7 +375,8 @@ def _make_tool_fn(tool_name, py_name): tool_fn.__qualname__ = py_name tool_fn.__doc__ = ( "Host tool '" + tool_name + "'. Takes a single dict of arguments and " - "returns the tool result as a string." + "returns the tool result as a string, with .is_error and .structured " + "beside it." ) return tool_fn @@ -537,7 +566,10 @@ def _dispatch(msg): ) st.task = _current_exec_task elif op == "tool_result": - _resolve(_pending_tool_futures.get(msg.get("id")), str(msg.get("result", ""))) + _resolve( + _pending_tool_futures.get(msg.get("id")), + ToolResult(str(msg.get("result", "")), msg.get("is_error"), msg.get("structured_json") is True), + ) elif op == "wake_ack": _resolve(_pending_wake_futures.get(msg.get("id")), msg.get("error") or None) elif op == "cancel": diff --git a/src/code-execution/tool-definition.ts b/src/code-execution/tool-definition.ts index dbf37dae9..a14934e00 100644 --- a/src/code-execution/tool-definition.ts +++ b/src/code-execution/tool-definition.ts @@ -42,6 +42,9 @@ export function buildCodeExecutionToolDefinition(opts?: { `workspace path the payload was saved to (null if it wasn't) and "refId" is fetch_reference's ` + 'ref_id. A structured result over 5 MB is saved to the workspace instead, and the string is ' + '{"isError": ..., "oversized": {"chars": ..., "savedTo": ...}}. ' + + 'Each string also carries .is_error (True if the result is an error, else False; test it ' + + 'rather than an "Error:" prefix) and .structured (the parsed "structuredContent" of the ' + + 'JSON object above, or None). ' + "An exact-name lookup dict is also available: tools['mcpl--discord--fetch_history']({...}). " + 'Use top-level await; run independent calls in parallel with asyncio.gather. ' + 'Only what you print() (plus stderr and the exit code) comes back to you — intermediate ' + diff --git a/src/framework.ts b/src/framework.ts index b06394192..c0d092ae7 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -132,7 +132,7 @@ import { type ShownRanges, } from './tool-image-ledger.js'; import { randomUUID, createHash } from 'node:crypto'; -import { PyRunner, buildInjectedTools, formatLimit } from './code-execution/py-runner.js'; +import { PyRunner, buildInjectedTools, formatLimit, type ScriptToolResult } from './code-execution/py-runner.js'; import { buildCodeExecutionToolDefinition, CODE_EXECUTION_TOOL_NAME, @@ -18824,8 +18824,10 @@ export class AgentFramework { /** * Resolve one script-inner tool call to the string the script's tool - * function returns. Never rejects: errors become "Error: ..." strings, - * matching the managed PTC runtime ("Claude's code receives this error"). + * function returns, and whether that result is an error (the script's + * `.is_error`). Never rejects: a failure without structured content becomes + * an "Error: ..." string, matching the managed PTC runtime ("Claude's code + * receives this error"), and a structured one its JSON object. * * A result without structured content is serialized with the HISTORY * serializer: images become placeholders, since a script variable holding @@ -18844,9 +18846,9 @@ export class AgentFramework { origin?: ScriptCallOrigin, /** The foreground script that made the call; none for a background script. */ script?: { endTurn: boolean; turnId?: number }, - ): Promise { + ): Promise { if (toolName === CODE_EXECUTION_TOOL_NAME) { - return 'Error: code_execution cannot be called from within a script'; + return { text: 'Error: code_execution cannot be called from within a script', isError: true }; } // A foreground script's call was made in the turn of the code_execution // call whose script is running when it arrives. A background script runs @@ -18863,10 +18865,10 @@ export class AgentFramework { // it back out of text. const structured = Object.prototype.hasOwnProperty.call(result, 'structured'); if (result.isError && !structured) { - return `Error: ${result.error ?? 'tool call failed'}`; + return { text: `Error: ${result.error ?? 'tool call failed'}`, isError: true }; } if ((result.data === undefined || result.data === null) && !structured) { - return nonErrorResultText(result, AgentFramework.SCRIPT_RESULT_MAX_CHARS); + return { text: nonErrorResultText(result, AgentFramework.SCRIPT_RESULT_MAX_CHARS), isError: false }; } // RFC-005 "eagerly lazy": a script receiving a result is the strongest // available signal that the bytes are wanted (scripts process, they do @@ -18898,9 +18900,9 @@ export class AgentFramework { // context-cap truncation, no file spill. if (structured) return this.scriptStructuredResult(result, toolName); try { - return toolResultDataToHistoryString(result.data, AgentFramework.SCRIPT_RESULT_MAX_CHARS); + return { text: toolResultDataToHistoryString(result.data, AgentFramework.SCRIPT_RESULT_MAX_CHARS), isError: false }; } catch (err) { - return `Error: ${unconvertibleScriptResult(toolName, err)}`; + return { text: `Error: ${unconvertibleScriptResult(toolName, err)}`, isError: true }; } } @@ -18923,9 +18925,13 @@ export class AgentFramework { * that can't be converted to JSON (a cyclic object, a BigInt), which the * dispatch's own check makes unlikely here, gets the same object empty, * `{ content: [], structuredContent: null, isError: true, error }`, the - * error naming the tool. + * error naming the tool. In every form, the script's `.is_error` is the + * `isError` of the object its string holds, and its `.structured` is read + * from that string. */ - private async scriptStructuredResult(result: ToolResult, toolName: string): Promise { + private async scriptStructuredResult(result: ToolResult, toolName: string): Promise { + const asSent = (value: T, json: string = JSON.stringify(value)): ScriptToolResult => + ({ text: json, isError: value.isError, structuredJson: true }); const object = { content: Array.isArray(result.data) ? result.data : [], structuredContent: result.structured, @@ -18937,19 +18943,19 @@ export class AgentFramework { json = JSON.stringify(object); } catch (err) { const error = unconvertibleScriptResult(toolName, err, result.isError ? result.error : undefined); - return JSON.stringify({ content: [], structuredContent: null, isError: true, error }); + return asSent({ content: [], structuredContent: null, isError: true, error }); } - if (json.length <= AgentFramework.SCRIPT_RESULT_MAX_CHARS) return json; + if (json.length <= AgentFramework.SCRIPT_RESULT_MAX_CHARS) return asSent(object, json); const workspace = this.getWorkspaceModule(); const mount = workspace ? this.firstWritableMountName(workspace) : null; if (workspace && mount) { const path = `${mount}/tool-results/${new Date().toISOString().slice(0, 10)}-script-${randomUUID()}.json`; const written = await workspace.writeBinary(path, Buffer.from(json, 'utf8'), 'application/json').catch(() => null); if (written?.success) { - return JSON.stringify({ isError: object.isError, oversized: { chars: json.length, savedTo: path } }); + return asSent({ isError: object.isError, oversized: { chars: json.length, savedTo: path } }); } } - return JSON.stringify({ + return asSent({ isError: true, error: `the result (${json.length} chars) exceeds the ${AgentFramework.SCRIPT_RESULT_MAX_CHARS}-char script result cap ` + `and could not be saved to the workspace, so it could not be delivered; the tool may already have completed`, diff --git a/src/index.ts b/src/index.ts index fd34068f3..dc3748915 100644 --- a/src/index.ts +++ b/src/index.ts @@ -74,6 +74,7 @@ export type { ExecResult, PyRunnerOptions, ScriptToolCallHandler, + ScriptToolResult, } from './code-execution/index.js'; export type { SessionUsage, AgentUsage, SessionUsageSnapshot, UsageUpdatedEvent } from './usage/index.js'; diff --git a/test/code-execution.test.ts b/test/code-execution.test.ts index 36834a0c4..43645ed44 100644 --- a/test/code-execution.test.ts +++ b/test/code-execution.test.ts @@ -20,6 +20,7 @@ import type { ModuleContext, ProcessEvent, ProcessState, + ScriptToolResult, ToolCall, ToolDefinition, ToolResult, @@ -136,6 +137,77 @@ describe('PyRunner (real python3)', () => { } }); + it('gives a script each result as its string, with .is_error and .structured beside it', async () => { + const structured = JSON.stringify({ content: [], structuredContent: { rows: [1, 2] }, isError: false }); + const oversized = JSON.stringify({ isError: true, oversized: { chars: 9, savedTo: null } }); + const results: Record = { + failed: { text: 'Error: boom', isError: true }, + // Text that merely starts like an error, the case .is_error is for. + textlike: { text: 'Error: a line of the file, not a failure', isError: false }, + structured: { text: structured, isError: false, structuredJson: true }, + oversized: { text: oversized, isError: true, structuredJson: true }, + // A handler that returns a plain string says nothing about it. + plain: 'a string, nothing said', + // Unmarked text holding a structuredContent key is still text. + jsonlike: { text: '{"structuredContent": {"a": 1}}', isError: false }, + }; + const runner = new PyRunner({ + onToolCall: async (_name, args) => { + if (args.kind === 'rejects') throw new Error('handler broke'); + // A JS handler can resolve with what its type rules out; the call is + // still answered, as before this change. + if (args.kind === 'nothing') return undefined as unknown as string; + return results[String(args.kind)]!; + }, + }); + try { + const result = await runner.exec( + [ + 'import copy, json, pickle', + 'out = {}', + 'for kind in ["failed", "textlike", "structured", "oversized", "plain", "rejects", "jsonlike", "nothing"]:', + ' r = await test__echo({"kind": kind})', + ' out[kind] = {"text": str(r), "is_error": r.is_error, "structured": r.structured,', + ' "type": type(r).__name__, "is_str": isinstance(r, str)}', + 'r = await test__echo({"kind": "structured"})', + 'out["parsed"] = json.loads(r)["structuredContent"]', + 'out["copies"] = [[c.is_error, c.structured, c == r] for c in (copy.copy(r), pickle.loads(pickle.dumps(r)))]', + 'key = await test__echo({"kind": "plain"})', + 'try:', + ' await test__echo({key: 1}, extra=2)', + 'except TypeError as e:', + ' out["refusal"] = str(e)', + 'print(json.dumps(out))', + ].join('\n'), + ECHO_TOOLS, + ); + assert.strictEqual(result.returnCode, 0, result.stderr); + const out = JSON.parse(result.stdout) as Record; + const seen = (text: string, isError: boolean | null, value: unknown) => + ({ text, is_error: isError, structured: value, type: 'ToolResult', is_str: true }); + assert.deepStrictEqual(out.failed, seen('Error: boom', true, null)); + assert.deepStrictEqual(out.textlike, seen('Error: a line of the file, not a failure', false, null)); + assert.deepStrictEqual(out.structured, seen(structured, false, { rows: [1, 2] })); + // Marked as a structured result's object, but holding no + // structuredContent: none to read. + assert.deepStrictEqual(out.oversized, seen(oversized, true, null)); + assert.deepStrictEqual(out.plain, seen('a string, nothing said', null, null)); + // A handler that rejects reaches the script as an error, flagged. + assert.deepStrictEqual(out.rejects, seen('Error: handler broke', true, null)); + // The string is unchanged for code that parses it itself. + assert.deepStrictEqual(out.parsed, { rows: [1, 2] }); + // copy and pickle keep both attributes. + assert.deepStrictEqual(out.copies, [[false, { rows: [1, 2] }, true], [false, { rows: [1, 2] }, true]]); + assert.deepStrictEqual(out.jsonlike, seen('{"structuredContent": {"a": 1}}', false, null)); + assert.deepStrictEqual(out.nothing, seen('', null, null)); + // A refusal shows a key that is a tool's result as its text, the key the + // agent has to write, as it shows any str key. + assert.match(String(out.refusal), /\{'a string, nothing said': , 'extra': \}/); + } finally { + runner.dispose(); + } + }); + it('supports the exact-name tools[...] escape hatch', async () => { const runner = new PyRunner({ onToolCall: async (_name, args) => `got:${String(args.v)}`, @@ -986,10 +1058,14 @@ describe('framework code_execution integration (real python3)', () => { const code = [ 'import json', 'names = []', + 'flags = []', 'for m in ["alpha", "beta"]:', - ' r = json.loads(await test__echo({"message": m}))', + ' raw = await test__echo({"message": m})', + ' flags.append(raw.is_error)', + ' r = json.loads(raw)', ' names.append(r["echoed"]["message"].upper())', 'print("summary:", "+".join(names))', + 'print("is_error:", flags)', ].join('\n'); membrane.pushResponse(createMockResponse([ @@ -1029,6 +1105,9 @@ describe('framework code_execution integration (real python3)', () => { const wireResult = stream.receivedToolResults[0][0] as { toolUseId: string; content: string }; assert.strictEqual(wireResult.toolUseId, 'call_ce_1'); assert.match(wireResult.content, /summary: ALPHA\+BETA/); + // Through the framework, each result says it isn't an error (a + // handler that passed on only the string would make it None). + assert.match(wireResult.content, /is_error: \[False, False\]/); assert.match(wireResult.content, /"return_code":\s*0/); // ...and NOT the intermediate tool results. assert.ok( @@ -1280,7 +1359,8 @@ describe('framework background scripts + spill (real python3)', () => { const bgCode = [ 'import asyncio', 'await asyncio.sleep(0.2)', - 'await wake_agent({"found": "signal"})', + 'r = await test__echo({"message": "bg"})', + 'await wake_agent({"found": "signal", "is_error": r.is_error})', ].join('\\n'); // Turn 1: spawn the background script, then finish the turn. @@ -1315,8 +1395,11 @@ describe('framework background scripts + spill (real python3)', () => { assert.ok(wakeMessage, 'wake message should be injected'); const text = (wakeMessage.content[0] as { text?: string }).text ?? ''; assert.match(text, /\[background script bg-\d+\] Woke you/); - assert.match(text, /line 3 of your script/); + assert.match(text, /line 4 of your script/); assert.match(text, /"found": "signal"/); + // A background script's results carry the flag too (None if only the + // string were passed on). + assert.match(text, /"is_error": false/); assert.match(text, /workspace file files\/background-scripts\/bg-\d+\.log/); assert.match(text, /Script status: still running|Script status/); } finally { diff --git a/test/mcp-modern-framework.test.ts b/test/mcp-modern-framework.test.ts index acf85fcfe..82e5c88bf 100644 --- a/test/mcp-modern-framework.test.ts +++ b/test/mcp-modern-framework.test.ts @@ -16,7 +16,7 @@ import { join, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; import { AgentFramework, WorkspaceModule } from '../src/index.js'; -import type { EventResponse, Module, ModuleContext, ProcessEvent, ProcessState, ToolDefinition, ToolResult } from '../src/index.js'; +import type { EventResponse, Module, ModuleContext, ProcessEvent, ProcessState, ScriptToolResult, ToolDefinition, ToolResult } from '../src/index.js'; import { MockMembrane, createMockResponse } from './helpers/mock-membrane.js'; const here = dirname(fileURLToPath(import.meta.url)); @@ -161,21 +161,35 @@ test('model path: the result reaches the model, structured-only as JSON', async }); }); +/** A script's result with its JSON string parsed, to compare whole. */ +const parsedText = (result: ScriptToolResult) => ({ ...result, text: JSON.parse(result.text) as unknown }); + test('scripts get structured results whole, as one JSON object', async () => { await withModern(async ({ framework }) => { framework.start(); const handle = (framework as unknown as { - handleScriptToolCall: (agent: string, tool: string, args: Record) => Promise; + handleScriptToolCall: (agent: string, tool: string, args: Record) => Promise; }).handleScriptToolCall.bind(framework); - const out = JSON.parse(await handle('agent', 'mcpl--modern--structured_and_text', {})); - assert.deepEqual(out, { - content: [{ type: 'text', text: 'Found 2 rows.' }], - structuredContent: { rows: [{ id: 1 }, { id: 2 }] }, + // Beside the string: isError, which the script reads as .is_error, and + // structuredJson, which has it read .structured from the string. + assert.deepEqual(parsedText(await handle('agent', 'mcpl--modern--structured_and_text', {})), { + text: { + content: [{ type: 'text', text: 'Found 2 rows.' }], + structuredContent: { rows: [{ id: 1 }, { id: 2 }] }, + isError: false, + }, isError: false, + structuredJson: true, + }); + // A structured tool error is flagged as one beside its string. + assert.deepEqual(parsedText(await handle('agent', 'mcpl--modern--sized', { pad: 1, isError: true })), { + text: sizedObject(1, true), + isError: true, + structuredJson: true, }); // Image plus scores: both views reach the script whole, the image's // payload included (no placeholder stands in for it). - const scored = JSON.parse(await handle('agent', 'mcpl--modern--scored_image', {})); + const scored = JSON.parse((await handle('agent', 'mcpl--modern--scored_image', {})).text); assert.deepEqual(scored, { content: [{ type: 'image', data: 'iVBORw0KGgo=', mimeType: 'image/png' }], structuredContent: { score: 0.9, label: 'cat' }, @@ -184,7 +198,7 @@ test('scripts get structured results whole, as one JSON object', async () => { // A resource link, embedded resources and audio are ordinary MCP blocks // from a modern server, not RFC-005 references: the script reads them as // the server sent them. - const media = JSON.parse(await handle('agent', 'mcpl--modern--structured_media', {})); + const media = JSON.parse((await handle('agent', 'mcpl--modern--structured_media', {})).text); assert.deepEqual(media, { content: [ { type: 'text', text: 'before' }, @@ -198,8 +212,9 @@ test('scripts get structured results whole, as one JSON object', async () => { isError: false, }); // Without structured content the script contract is unchanged (pinned): - // the history rendering, which JSON-encodes a text result. - assert.equal(await handle('agent', 'mcpl--modern--echo', { text: 'x' }), JSON.stringify('echo:x')); + // the history rendering, which JSON-encodes a text result. No + // structuredJson, so the script's .structured is None. + assert.deepEqual(await handle('agent', 'mcpl--modern--echo', { text: 'x' }), { text: JSON.stringify('echo:x'), isError: false }); }); }); @@ -220,7 +235,7 @@ const sizedObject = (pad: number, isError = false) => ({ /** The pad that makes the script's JSON exactly `chars` long. */ const padFor = (chars: number, isError = false) => chars - JSON.stringify(sizedObject(0, isError)).length; const scriptHandle = (framework: AgentFramework) => (framework as unknown as { - handleScriptToolCall: (agent: string, tool: string, args: Record) => Promise; + handleScriptToolCall: (agent: string, tool: string, args: Record) => Promise; }).handleScriptToolCall.bind(framework); async function withWorkspace(fn: (h: Harness & { workspace: WorkspaceModule }) => Promise): Promise { @@ -255,16 +270,22 @@ test('script results: exactly at the cap arrive inline; one past it are saved wh const handle = scriptHandle(framework); const at = await handle('agent', 'mcpl--modern--sized', { pad: padFor(SCRIPT_CAP) }); - assert.equal(at.length, SCRIPT_CAP); - assert.deepEqual(JSON.parse(at), sizedObject(padFor(SCRIPT_CAP))); - - const over = JSON.parse(await handle('agent', 'mcpl--modern--sized', { pad: padFor(SCRIPT_CAP + 1) })); + assert.equal(at.text.length, SCRIPT_CAP); + assert.deepEqual(parsedText(at), { text: sizedObject(padFor(SCRIPT_CAP)), isError: false, structuredJson: true }); + + // Past the cap the string holds where the object went, and is still + // marked as a structured result's object: .structured reads none from it. + const overResult = await handle('agent', 'mcpl--modern--sized', { pad: padFor(SCRIPT_CAP + 1) }); + assert.deepEqual({ isError: overResult.isError, structuredJson: overResult.structuredJson }, { isError: false, structuredJson: true }); + const over = JSON.parse(overResult.text); assert.deepEqual(Object.keys(over).sort(), ['isError', 'oversized']); assert.equal(over.isError, false); assert.equal(over.oversized.chars, SCRIPT_CAP + 1); assert.deepEqual(await savedObject(workspace, over.oversized.savedTo), sizedObject(padFor(SCRIPT_CAP + 1)), 'the whole object, recoverable'); - const failed = JSON.parse(await handle('agent', 'mcpl--modern--sized', { pad: padFor(SCRIPT_CAP + 1, true), isError: true })); + const failedResult = await handle('agent', 'mcpl--modern--sized', { pad: padFor(SCRIPT_CAP + 1, true), isError: true }); + assert.equal(failedResult.isError, true, 'flagged beside the string too'); + const failed = JSON.parse(failedResult.text); assert.equal(failed.isError, true, 'a tool error stays visible without reading the file'); assert.equal(failed.oversized.chars, SCRIPT_CAP + 1); assert.notEqual(failed.oversized.savedTo, over.oversized.savedTo); @@ -273,19 +294,23 @@ test('script results: exactly at the cap arrive inline; one past it are saved wh }); test('script results past the cap: no workspace, or a failed write, is a failure to deliver, never a success', async () => { - const assertUndelivered = (out: { isError: boolean; error: string; oversized: unknown }) => { + const assertUndelivered = (result: ScriptToolResult) => { + // Not delivered: flagged as an error beside a string that says the tool + // may already have completed. + assert.deepEqual({ isError: result.isError, structuredJson: result.structuredJson }, { isError: true, structuredJson: true }); + const out = JSON.parse(result.text) as { isError: boolean; error: string; oversized: unknown }; assert.equal(out.isError, true); assert.match(out.error, /exceeds the 5000000-char script result cap and could not be saved to the workspace, so it could not be delivered; the tool may already have completed/); assert.deepEqual(out.oversized, { chars: SCRIPT_CAP + 1, savedTo: null }); }; await withModern(async ({ framework }) => { framework.start(); - assertUndelivered(JSON.parse(await scriptHandle(framework)('agent', 'mcpl--modern--sized', { pad: padFor(SCRIPT_CAP + 1) }))); + assertUndelivered(await scriptHandle(framework)('agent', 'mcpl--modern--sized', { pad: padFor(SCRIPT_CAP + 1) })); }); await withWorkspace(async ({ framework, workspace }) => { framework.start(); workspace.writeBinary = async () => ({ success: false, error: 'disk full', isError: true }); - assertUndelivered(JSON.parse(await scriptHandle(framework)('agent', 'mcpl--modern--sized', { pad: padFor(SCRIPT_CAP + 1) }))); + assertUndelivered(await scriptHandle(framework)('agent', 'mcpl--modern--sized', { pad: padFor(SCRIPT_CAP + 1) })); }); }); diff --git a/test/refusals-are-error-results.test.ts b/test/refusals-are-error-results.test.ts index 39356fa10..02543a6ff 100644 --- a/test/refusals-are-error-results.test.ts +++ b/test/refusals-are-error-results.test.ts @@ -17,7 +17,7 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { AgentFramework, ApiModule } from '../src/index.js'; -import type { Module, ModuleContext, ToolDefinition, ToolResult } from '../src/index.js'; +import type { Module, ModuleContext, ScriptToolResult, ToolDefinition, ToolResult } from '../src/index.js'; import { TUNE_OUT_DEFAULTS } from '../src/tune-out/coordinator.js'; import { MockMembrane, createMockResponse } from './helpers/mock-membrane.js'; @@ -189,17 +189,23 @@ test('a script, and a module calling in process, get the refusals as errors too' // A script's call is answered through the event loop, so it has to run. framework.start(); const script = (tool: string, input: Record) => (framework as unknown as { - handleScriptToolCall(agent: string, tool: string, args: Record): Promise; + handleScriptToolCall(agent: string, tool: string, args: Record): Promise; }).handleScriptToolCall('scout', tool, input); - assert.equal(await script('tune_out', { channelId: 'disc:guild:none' }), 'Error: unknown channel: disc:guild:none'); - assert.equal(await script('api--lookup', {}), `Error: ${API_UNKNOWN}`); + // Each refusal reaches the script as an error, flagged as one (.is_error). + assert.deepEqual(await script('tune_out', { channelId: 'disc:guild:none' }), { text: 'Error: unknown channel: disc:guild:none', isError: true }); + assert.deepEqual(await script('api--lookup', {}), { text: `Error: ${API_UNKNOWN}`, isError: true }); // A control: the same tool's success still reads as one. const { cadenceSeconds, backlogCap, maxWakes } = TUNE_OUT_DEFAULTS; - assert.equal( + assert.deepEqual( await script('tune_out', { channelId: CHANNEL }), - JSON.stringify({ tunedOut: CHANNEL, cadenceSeconds, backlogCap, maxWakes }), + { text: JSON.stringify({ tunedOut: CHANNEL, cadenceSeconds, backlogCap, maxWakes }), isError: false }, + ); + assert.deepEqual(await script('channel_open', { channelId: CHANNEL }), { text: `Error: ${TUNED_OUT}`, isError: true }); + // A script can't start a script: refused before any dispatch, as an error. + assert.deepEqual( + await script('code_execution', { code: 'print(1)' }), + { text: 'Error: code_execution cannot be called from within a script', isError: true }, ); - assert.equal(await script('channel_open', { channelId: CHANNEL }), `Error: ${TUNED_OUT}`); // A module gets the result as the registry returns it: flagged, with the // refusal's data kept, so it can still read why. assert.deepEqual(await keeper.ctx!.callTool({ id: 'ctx-open', name: 'channel_open', input: { channelId: CHANNEL } }), { diff --git a/test/script-structured-references.test.ts b/test/script-structured-references.test.ts index 465ca069f..b610135c1 100644 --- a/test/script-structured-references.test.ts +++ b/test/script-structured-references.test.ts @@ -17,7 +17,7 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { AgentFramework, WorkspaceModule } from '../src/index.js'; -import type { ToolResult } from '../src/index.js'; +import type { ScriptToolResult, ToolResult } from '../src/index.js'; import { EXPIRY_REASON, INLINE_WITHHELD_TEXT } from '../src/mcpl/references.js'; import type { ReferenceRecord } from '../src/mcpl/references.js'; import { MockMembrane } from './helpers/mock-membrane.js'; @@ -96,7 +96,7 @@ async function withPeers(fn: (h: Harness) => Promise, opts: { eager?: bool mcplServerRegistry: unknown; mcplPrefixMap: Map; getReferenceFetcher: () => unknown; - handleScriptToolCall: (agent: string, tool: string, args: Record) => Promise; + handleScriptToolCall: (agent: string, tool: string, args: Record) => Promise; dispatchMcplToolCall: (agent: string, call: { id: string; name: string; input: unknown }, serverId: string, prefix: string) => void; pushEvent: (event: { type: string; callId?: string; result?: ToolResult }) => void; }; @@ -130,7 +130,9 @@ async function withPeers(fn: (h: Harness) => Promise, opts: { eager?: bool try { await fn({ answer: (result) => { answer = result; }, - script: (tool) => internals.handleScriptToolCall.call(framework, 'agent', tool, {}), + // The string a script's call returns; its flags are pinned in + // mcp-modern-framework and code-execution. + script: async (tool) => (await internals.handleScriptToolCall.call(framework, 'agent', tool, {})).text, modelCall, fetched, workspace, diff --git a/test/tool-result-reference-uri.test.ts b/test/tool-result-reference-uri.test.ts index 6f9f82dbb..64dc82da2 100644 --- a/test/tool-result-reference-uri.test.ts +++ b/test/tool-result-reference-uri.test.ts @@ -11,6 +11,7 @@ import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { AgentFramework } from '../src/index.js'; +import type { ScriptToolResult } from '../src/index.js'; import type { Module, ModuleContext, ToolCall, ToolResult, ToolDefinition } from '../src/types/index.js'; import { nonErrorResultText, toolResultDataToHistoryString } from '../src/tool-result-history.js'; import { parseImagePlaceholders } from '../src/tool-image-ledger.js'; @@ -308,14 +309,14 @@ describe('a model\'s call answered with such data', () => { describe('a script given such data', () => { type ScriptFramework = { dispatchScriptToolCall: (...args: unknown[]) => Promise; - handleScriptToolCall(agentName: string, toolName: string, args: Record): Promise; + handleScriptToolCall(agentName: string, toolName: string, args: Record): Promise; }; it('gets the text history stores, with each reference as its stub and the rest as JSON writes it', async () => { const fw = Object.create(AgentFramework.prototype) as ScriptFramework; const shot = { type: 'image', data: BASE64, mimeType: 'image/png' }; fw.dispatchScriptToolCall = async () => ({ success: true, data: { file: RESOURCE, shot, page: 2 } }); - const text = await fw.handleScriptToolCall('zz-agent', 'zz_tool', {}); + const { text } = await fw.handleScriptToolCall('zz-agent', 'zz_tool', {}); assert.ok(!text.includes(SIGNATURE), text); const result = parsed(text) as { file: string; shot: unknown; page: number }; assert.match(result.file, STUB); @@ -331,7 +332,7 @@ describe('a script given such data', () => { console.error = (...args: unknown[]) => { logged.push(args.map(String).join(' ')); }; let text: string; try { - text = await fw.handleScriptToolCall('zz-agent', 'zz_tool', {}); + ({ text } = await fw.handleScriptToolCall('zz-agent', 'zz_tool', {})); } finally { console.error = error; } diff --git a/test/unconvertible-tool-result.test.ts b/test/unconvertible-tool-result.test.ts index 5b0497fa5..af4e06216 100644 --- a/test/unconvertible-tool-result.test.ts +++ b/test/unconvertible-tool-result.test.ts @@ -13,7 +13,7 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { AgentFramework } from '../src/index.js'; -import type { EventResponse, Module, ModuleContext, ProcessEvent, ProcessState, ToolCall, ToolDefinition, ToolResult } from '../src/index.js'; +import type { EventResponse, Module, ModuleContext, ProcessEvent, ProcessState, ScriptToolResult, ToolCall, ToolDefinition, ToolResult } from '../src/index.js'; import { MockMembrane, createMockResponse } from './helpers/mock-membrane.js'; function cyclic(): Record { @@ -42,6 +42,8 @@ const RESULTS: Record unknown> = { errerror: () => ({ success: false, isError: true, error: new Error('db timeout') }), nullerror: () => ({ success: false, isError: true, error: null }), quietfail: () => ({ success: false, error: 'unknown channel: c1', isError: false }), + // An unflagged failure whose own text starts like an error. + prefixquiet: () => ({ success: false, error: 'Error: quota is fine', isError: false }), noresult: () => undefined, nullresult: () => null, arrayresult: () => [], @@ -275,15 +277,45 @@ test('a script reads a quiet failure as its error text, and an unusable result a // A script's call is answered through the event loop, so it has to run. framework.start(); const script = (tool: string) => (framework as unknown as { - handleScriptToolCall(agent: string, tool: string, args: Record): Promise; + handleScriptToolCall(agent: string, tool: string, args: Record): Promise; }).handleScriptToolCall('assistant', `odd--${tool}`, {}); - assert.equal(await within(script('quietfail'), 10_000, 'the call'), 'unknown channel: c1'); - assert.equal(await within(script('nulldataquiet'), 10_000, 'the call'), 'gone'); - assert.equal(await within(script('cyclic'), 10_000, 'the call'), `Error: ${unconvertible('cyclic', 'Converting circular structure to JSON')}`); - assert.equal(await within(script('getterthrows'), 10_000, 'the call'), `Error: ${unreadable('getterthrows', 'a value with no text form')}`); + // A failure the tool doesn't flag reads as the model reads it, isError + // false: the script's .is_error follows the result's flag, not its text. + assert.deepEqual(await within(script('quietfail'), 10_000, 'the call'), { text: 'unknown channel: c1', isError: false }); + assert.deepEqual(await within(script('nulldataquiet'), 10_000, 'the call'), { text: 'gone', isError: false }); + // Text that only starts like an error isn't flagged as one: the flag is + // the result's, not the text's prefix. + assert.deepEqual(await within(script('prefixquiet'), 10_000, 'the call'), { text: 'Error: quota is fine', isError: false }); + assert.deepEqual(await within(script('cyclic'), 10_000, 'the call'), { text: `Error: ${unconvertible('cyclic', 'Converting circular structure to JSON')}`, isError: true }); + assert.deepEqual(await within(script('getterthrows'), 10_000, 'the call'), { text: `Error: ${unreadable('getterthrows', 'a value with no text form')}`, isError: true }); await within(framework.stop(), 10_000, 'stop()'); }); +test("a result the script's serializer can't convert reaches the script as an error, flagged as one", async (t) => { + // The dispatch's own check (storableToolResult) turns such a result into an + // error first, so these returns are reached only past it: here, with the + // dispatch answering a cyclic result directly. + const { framework } = await setUp(t, []); + const internals = framework as unknown as { + dispatchScriptToolCall: () => Promise; + handleScriptToolCall(agent: string, tool: string, args: Record): Promise; + }; + const reason = 'Converting circular structure to JSON'; + internals.dispatchScriptToolCall = async () => ({ success: true, data: cyclic() }); + const text = await internals.handleScriptToolCall('assistant', 'odd--cyclic', {}); + assert.equal(text.isError, true); + assert.equal(text.structuredJson, undefined); + assert.ok(text.text.startsWith("Error: tool 'odd--cyclic' returned a result that can't be converted to JSON, so the script can't receive it"), text.text); + assert.ok(text.text.includes(reason), text.text); + internals.dispatchScriptToolCall = async () => ({ success: true, structured: cyclic() }); + const structured = await internals.handleScriptToolCall('assistant', 'odd--cyclic', {}); + assert.equal(structured.isError, true); + assert.equal(structured.structuredJson, true); + const object = JSON.parse(structured.text) as { content: unknown[]; structuredContent: unknown; isError: boolean; error: string }; + assert.deepEqual({ ...object, error: undefined }, { content: [], structuredContent: null, isError: true, error: undefined }); + assert.ok(object.error.includes(reason), object.error); +}); + test("the read_image tool's own handler gets the same check", async (t) => { const { framework, membrane, logged } = await setUp(t, ['read_image'], [new OddWorkspace()]); framework.pushEvent({ type: 'external-message', source: 'test', content: 'Look.', metadata: {} });