From 5c7f515710b44610be775bb1c5ad0668e7d5b11b Mon Sep 17 00:00:00 2001 From: Anarchid Date: Fri, 16 Oct 2026 14:22:27 +0300 Subject: [PATCH 1/3] fix(tool-results): send a string result as its text, not JSON-quoted A tool result whose data is a string was JSON-encoded on its way to the model: quoted, with every newline, tab and quote escaped. A module tool that answers with a line-per-item listing reached context, the live wire and its spill file as one escaped line, which workspace--read could then page only by character and workspace--grep matched as a single line. toolResultDataToHistoryString now keeps a string as it is; objects, other scalars and non-MCP arrays still go as JSON, MCP content arrays as their blocks. The live wire's fallback in toMembraneToolResult follows the same rule, so the four sites that share the serializer (stored history, the live wire, scripts, the direct Agent API) stay converged. Tests: the serializer keeps a string verbatim and truncates its text; an oversized string spills as its own lines and grep finds one by line number; a small one reaches history and the wire byte-identical; a code_execution script gets the string itself. Two tests that pinned the quoted form are updated. Co-Authored-By: Claude Opus 5.5 --- .../verbatim-string-tool-results.changed.md | 14 +++++ src/framework.ts | 9 +-- src/tool-result-history.ts | 10 +++- src/types/events.ts | 5 +- test/code-execution.test.ts | 54 +++++++++++++++++ test/tool-result-guard.test.ts | 6 +- test/tool-result-history.test.ts | 20 ++++++- test/tool-result-spill.test.ts | 58 +++++++++++++++++++ test/unconvertible-tool-result.test.ts | 2 +- 9 files changed, 166 insertions(+), 12 deletions(-) create mode 100644 changelog.d/verbatim-string-tool-results.changed.md diff --git a/changelog.d/verbatim-string-tool-results.changed.md b/changelog.d/verbatim-string-tool-results.changed.md new file mode 100644 index 000000000..fc9964b99 --- /dev/null +++ b/changelog.d/verbatim-string-tool-results.changed.md @@ -0,0 +1,14 @@ +- A tool result whose `data` is a string now reaches the model as that text. + It was JSON-encoded on the way, quoted with every newline, tab and quote + escaped, so a multi-line answer, such as a module tool's line-per-item + listing, arrived as one escaped line: in context, on the live wire, and in + its spill file under `tool-results/`, which `workspace--read` could then + page only by character and `workspace--grep` matched as a single line. A + spill file now holds the string as the tool returned it. Objects, numbers, + booleans and arrays other than MCP content arrays are still sent as JSON, + and MCP content arrays as their blocks. + + A `code_execution` script's tool function returns the same text, so it now + gets a string result as it is. A script that ran `json.loads` on such a + result to unquote it now gets an error unless the string itself is JSON. + `data: ''` is stored as no text, as `data: undefined` already was. diff --git a/src/framework.ts b/src/framework.ts index 94f1035ff..b0060306f 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -14851,9 +14851,10 @@ export class AgentFramework { }; } // MCPL tool results arrive as `data: McpToolResultContent[]` — preserve image - // blocks natively rather than JSON-stringifying them away. Anything else - // (objects, scalars) falls through to JSON. The error path was handled - // above, so isError is always false on these return paths. + // blocks natively rather than JSON-stringifying them away. A string goes + // as it is, as history stores it; anything else (objects, other scalars) + // falls through to JSON. The error path was handled above, so isError is + // always false on these return paths. const blocks = this.tryNativeToolResultContent(afResult.data, maxChars, precomputed?.filePath ?? null); if (blocks) { return { toolUseId: callId, content: blocks, isError: false }; @@ -14864,7 +14865,7 @@ export class AgentFramework { // JSON.stringify returns the VALUE undefined (not a string) for undefined // input — a module tool returning `{ success: true }` with no data would // otherwise make `content.length` below throw a TypeError mid-turn. - let content = JSON.stringify(afResult.data) ?? ''; + let content = typeof afResult.data === 'string' ? afResult.data : JSON.stringify(afResult.data) ?? ''; if (maxChars && content.length > maxChars) { content = safeSlice(content, 0, maxChars) + '\n\n[truncated — original was ' + content.length + ' chars]'; diff --git a/src/tool-result-history.ts b/src/tool-result-history.ts index 73ddf431e..3e99ecfb1 100644 --- a/src/tool-result-history.ts +++ b/src/tool-result-history.ts @@ -11,7 +11,8 @@ * * For MCP-shaped content arrays, this helper keeps text blocks verbatim and * replaces image blocks with a short `[image: mimeType, ~NKB]` placeholder. - * For anything else it falls back to JSON. + * A string is already text and is kept as it is. For anything else it falls + * back to JSON. */ import { INLINE_WITHHELD_TEXT, isInlineContradiction, referenceStubOrNull } from './mcpl/references.js'; @@ -52,9 +53,14 @@ export function toolResultDataToHistoryString( options?: HistorySerializeOptions, ): string { const fromArray = tryHistoryStringFromContentArray(data, options); + // A string is the text the tool means to say, kept as it is: JSON would + // quote it and escape every newline, so a multi-line result would reach + // the model, a script and its spill file as one escaped line. // JSON leaves out undefined, a function and a symbol: a result with no data // is stored as no text, as an empty content array is. - const str = fromArray ?? (JSON.stringify(data) as string | undefined) ?? ''; + const str = fromArray + ?? (typeof data === 'string' ? data : JSON.stringify(data) as string | undefined) + ?? ''; return maxChars ? truncateForHistory(str, maxChars) : str; } diff --git a/src/types/events.ts b/src/types/events.ts index 0ef8de110..f3e5e12a6 100644 --- a/src/types/events.ts +++ b/src/types/events.ts @@ -186,7 +186,10 @@ export interface ModuleEvent { export interface ToolResult { /** Whether the tool succeeded */ success: boolean; - /** Result data (if success) */ + /** + * Result data (if success). The model reads a string as that text, an MCP + * content array as its blocks, and anything else as JSON. + */ data?: unknown; /** Error message (if failure) */ error?: string; diff --git a/test/code-execution.test.ts b/test/code-execution.test.ts index 5deec59aa..075433758 100644 --- a/test/code-execution.test.ts +++ b/test/code-execution.test.ts @@ -521,6 +521,60 @@ describe('framework code_execution integration (real python3)', () => { } }); + it('a module tool that returns a string hands the script that string, not its JSON encoding', async () => { + // The tool contract: a script's tool function returns "the same text the + // tool would have returned to you directly". For a string result that + // is the string itself, newlines and quotes intact. + const listing = 'As of 10:00.\n[review] org/repo#1 "Fix it"'; + class ListingModule extends ScriptToolModule { + override getTools(): ToolDefinition[] { + return [...super.getTools(), { + name: 'listing', + description: 'A line-per-item listing.', + inputSchema: { type: 'object', properties: {} }, + }]; + } + override async handleToolCall(call: ToolCall): Promise { + if (call.name.endsWith('listing')) return { success: true, data: listing }; + return super.handleToolCall(call); + } + } + const { tempDir, storePath } = tempStorePath('pytc-string-'); + const membrane = new MockMembrane(); + const code = [ + 'r = await test__listing({})', + 'lines = r.split("\\n")', + 'print("count:", len(lines))', + 'print("second:", lines[1])', + 'print("quoted:", r.startswith(\'"\'))', + ].join('\n'); + membrane.pushResponse(createMockResponse([ + { type: 'tool_use', id: 'call_ce_str', name: 'code_execution', input: { code } }, + ], 'tool_use')); + membrane.pushResponse(createMockResponse([{ type: 'text', text: 'Done.' }])); + + const framework = await createFrameworkWithCodeExecution(storePath, membrane, new ListingModule()); + try { + const created = await framework.createEphemeralAgent({ + name: 'worker', model: 'test-model', systemPrompt: 'Do the task.', allowedTools: 'all', + }); + created.contextManager.addMessage('user', [{ type: 'text', text: 'Go.' }]); + const promise = framework.runEphemeralToCompletion(created.agent, created.contextManager); + framework.start(); + await promise; + + const wireResult = membrane.lastStream!.receivedToolResults[0][0] as { content: string }; + const out = JSON.parse(wireResult.content) as { stdout: string; return_code: number }; + assert.strictEqual(out.return_code, 0, wireResult.content); + assert.match(out.stdout, /count: 2\n/); + assert.match(out.stdout, /second: \[review\] org\/repo#1 "Fix it"\n/); + assert.match(out.stdout, /quoted: False/); + } finally { + await framework.stop(); + rmSync(tempDir, { recursive: true, force: true }); + } + }); + it('two code_execution calls in one response: one runs, the other is refused, the turn completes', async () => { const { tempDir, storePath } = tempStorePath('pytc-two-'); const membrane = new MockMembrane(); diff --git a/test/tool-result-guard.test.ts b/test/tool-result-guard.test.ts index 1f999935a..f97ece6fd 100644 --- a/test/tool-result-guard.test.ts +++ b/test/tool-result-guard.test.ts @@ -1814,14 +1814,14 @@ test('#281: a withheld result over the inline cap is written as stored, and its const withheld = await h.workspace.readBinary(path); assert.ok('data' in withheld); const text = (withheld as { data: Buffer }).data.toString('utf8'); - // The data is a string, stored as its JSON: 407 characters with the quotes. - assert.match(text, /^"long-x{94}\n\n\[truncated — showing 100 of 407 chars; full content: workspace file /, + // The data is a string, stored as it is: 405 characters. + assert.match(text, /^long-x{95}\n\n\[truncated — showing 100 of 405 chars; full content: workspace file /, 'the withheld file holds the capped text, as stored'); const whole = /full content: workspace file (work\/tool-results\/[^\s\]"]+\.txt)/.exec(text)?.[1]; assert.ok(whole && whole !== path, `its note names the file with the whole output: ${text}`); const full = await h.workspace.readBinary(whole); assert.ok('data' in full); - assert.match((full as { data: Buffer }).data.toString('utf8'), /^"long-x{400}"$/); + assert.match((full as { data: Buffer }).data.toString('utf8'), /^long-x{400}$/); assert.doesNotMatch(stub, /full result/, 'the stub says "its result", which the file holds'); } finally { await h.framework.stop(); } }); diff --git a/test/tool-result-history.test.ts b/test/tool-result-history.test.ts index 9fe34494a..8787af7f7 100644 --- a/test/tool-result-history.test.ts +++ b/test/tool-result-history.test.ts @@ -54,12 +54,30 @@ test('mixed known + unknown block shape falls back to JSON (all-or-nothing)', () test('non-array data: JSON stringify', () => { assert.equal(toolResultDataToHistoryString({ ok: true }), '{"ok":true}'); - assert.equal(toolResultDataToHistoryString('hello'), '"hello"'); assert.equal(toolResultDataToHistoryString(42), '42'); + assert.equal(toolResultDataToHistoryString(true), 'true'); // JSON leaves undefined out (JSON.stringify gives undefined): no data is no text. assert.equal(toolResultDataToHistoryString(undefined), ''); }); +test('string data: kept as it is, not JSON-quoted', () => { + // A module tool that answers in prose or a line-per-item listing returns + // a string. JSON would quote it and turn every newline into `\n`, so the + // model (and the spill file, and a script) got one escaped line. + const listing = 'As of 10:00. 3 open.\n[review] a/b#1 "Fix it"\n[merge] a/b#2 tab\there'; + assert.equal(toolResultDataToHistoryString(listing), listing); + assert.equal(toolResultDataToHistoryString('hello'), 'hello'); + assert.equal(toolResultDataToHistoryString(''), ''); + // A string that happens to hold JSON is the tool's text too: not re-encoded. + assert.equal(toolResultDataToHistoryString('{"ok":true}'), '{"ok":true}'); +}); + +test('string data: maxChars truncates the text itself with a marker', () => { + const out = toolResultDataToHistoryString('line\n'.repeat(100), 50); + assert.ok(out.startsWith('line\nline\n'), 'truncation keeps real newlines'); + assert.match(out, /\[truncated — original was 500 chars\]/); +}); + test('maxChars truncates the resulting string with a marker', () => { const data = [{ type: 'text', text: 'a'.repeat(1000) }]; const out = toolResultDataToHistoryString(data, 100); diff --git a/test/tool-result-spill.test.ts b/test/tool-result-spill.test.ts index 87444c5cd..79f2f9536 100644 --- a/test/tool-result-spill.test.ts +++ b/test/tool-result-spill.test.ts @@ -426,6 +426,64 @@ describe('tool-result spill completion (issue #89)', () => { } }); + it('spills a plain string result as its text: lines intact, greppable, not one JSON-quoted line', async () => { + // A module tool that answers with a line-per-item listing returns a + // string. It used to be JSON-quoted on the way to history, so its spill + // file held one escaped line that could only be paged by character. + const body = Array.from({ length: 3000 }, (_, i) => `[review] org/repo#${i} "title ${i}"\tsize ${'t'.repeat(10)}`).join('\n'); + const h = await startSpillTurn({ + prefix: 'spill-string-', + result: { success: true, data: body }, + withWorkspace: true, + }); + try { + const stored = await waitForStoredToolResult(h.framework); + assert.ok(stored, 'tool result should be stored'); + assert.ok(stored.content.startsWith('[review] org/repo#0 "title 0"\t'), 'inline head is the text itself, unquoted'); + + const refMatch = stored.content.match(/workspace file (files\/tool-results\/\S+\.txt)/); + assert.ok(refMatch, 'reference should name the spill file'); + const file = await h.workspace!.readBinary(refMatch[1]); + assert.ok('data' in file, 'spill file should be readable'); + const saved = (file as { data: Buffer }).data.toString('utf8'); + assert.strictEqual(saved, body, 'the spill file holds the string verbatim'); + assert.strictEqual(saved.split('\n').length, 3000, 'one line per item'); + + // grep finds one item, on its own line. + const grep = await h.workspace!.handleToolCall({ + id: 'grep', name: 'grep', input: { pattern: 'repo#2999 ', path: refMatch[1] }, + }); + assert.strictEqual(grep.success, true, grep.error); + const matches = (grep.data as { results: Array<{ matches: Array<{ line: number; text: string }> }> }) + .results.flatMap((r) => r.matches); + assert.deepStrictEqual(matches.map((m) => [m.line, m.text]), [[3000, body.split('\n')[2999]]]); + } finally { + await h.framework.stop(); + rmSync(h.tempDir, { recursive: true, force: true }); + } + }); + + it('a plain string result reaches history and the live wire as its text, byte-identical', async () => { + const body = 'As of 10:00. 2 open.\n[review] org/repo#1 "Fix it"\n[merge] org/repo#2 done'; + const h = await startSpillTurn({ + prefix: 'string-wire-', + result: { success: true, data: body }, + withWorkspace: false, + }); + try { + const stored = await waitForStoredToolResult(h.framework); + assert.ok(stored, 'tool result should be stored'); + assert.strictEqual(stored.content, body, 'history holds the string, not its JSON encoding'); + const wire = h.membrane.lastStream?.receivedToolResults[0] as + Array<{ content: unknown }> | undefined; + assert.ok(wire && wire.length === 1, 'one wire tool result expected'); + assert.strictEqual(wire[0].content, body, 'the wire carries the same text'); + } finally { + await h.framework.stop(); + rmSync(h.tempDir, { recursive: true, force: true }); + } + }); + it('falls back to EXPLICIT plain truncation with no writable workspace', async () => { const h = await startSpillTurn({ prefix: 'spill-nows-', diff --git a/test/unconvertible-tool-result.test.ts b/test/unconvertible-tool-result.test.ts index 5b0497fa5..2d056db74 100644 --- a/test/unconvertible-tool-result.test.ts +++ b/test/unconvertible-tool-result.test.ts @@ -378,6 +378,6 @@ test("the direct Agent API reads each field of its caller's result once", async ]; const built = (agent as unknown as { buildToolResultMessages(r: unknown[]): Array<{ content: Array<{ content: string }> }> }) .buildToolResultMessages(results); - assert.deepEqual(built[0].content.map((block) => block.content), [JSON.stringify('the payload'), 'what failed']); + assert.deepEqual(built[0].content.map((block) => block.content), ['the payload', 'what failed']); }); From 56783fb9022909f7b852ede213a718e7472847c5 Mon Sep 17 00:00:00 2001 From: Hilda-2681 Date: Fri, 16 Oct 2026 14:24:59 -0400 Subject: [PATCH 2/3] test(code-execution): an MCP tool's text-only result reaches the model and a script as that text The MCPL tool-result mapping (src/framework.ts, the "Convert MCP tool result to framework ToolResult" block) hands a result whose blocks are all text on as their joined text, so an MCP tool's text answer is string data too, and the serializer's string rule reaches it as it reaches a module tool's string. This pins that case end to end: a real stdio MCPL server whose tool answers with one text block, called once by the model and once from a code_execution script (real python3). The model's tool_result holds the text as the server sent it, and the script gets the same text: it splits into its two lines, and it doesn't start with a quote. The server accepts the framework's initial featureSets/update, as the fixtures' servers do, so the test doesn't wait out the policy request's 15 s timeout. On main's src it fails at the model's assertion: the model gets the text as a JSON string literal. With that assertion left out, it fails at the script's, since the script gets the same literal, one line, and has no second line to print. It passes with this branch's string rule. Co-Authored-By: Claude Opus 5.5 --- test/code-execution.test.ts | 75 +++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/test/code-execution.test.ts b/test/code-execution.test.ts index 075433758..8a9488386 100644 --- a/test/code-execution.test.ts +++ b/test/code-execution.test.ts @@ -575,6 +575,81 @@ describe('framework code_execution integration (real python3)', () => { } }); + it("an MCP tool's text-only result reaches the model and a script as that text, not its JSON encoding", async () => { + // The MCPL tool-result mapping hands a result whose blocks are all text + // on as their joined text, so an MCP tool's text answer is string data + // too: the model and a script get the text as the server sent it. + const listing = 'As of 10:00.\n[review] org/repo#1 "Fix it"'; + // A stdio MCPL server whose one tool answers with a single text block + // holding the `text` it was given. It accepts the framework's initial + // featureSets/update, as the fixtures' servers do, so the connection + // doesn't wait out the policy request's timeout. + const server = ` + let buf = ''; + process.stdin.on('data', (c) => { + buf += c.toString('utf8'); + let i; + while ((i = buf.indexOf('\\n')) >= 0) { + const line = buf.slice(0, i); buf = buf.slice(i + 1); + if (!line.trim()) continue; + let m; try { m = JSON.parse(line); } catch { continue; } + const reply = (result) => process.stdout.write(JSON.stringify({ jsonrpc: '2.0', id: m.id, result }) + '\\n'); + if (m.method === 'initialize') reply({ capabilities: { experimental: { mcpl: { version: '0.4' } } } }); + else if (m.method === 'tools/list') reply({ tools: [{ name: 'echo', description: 'e', inputSchema: { type: 'object' } }] }); + else if (m.method === 'tools/call') reply({ content: [{ type: 'text', text: String(m.params.arguments.text) }] }); + else if (m.method === 'featureSets/update') { if (m.id !== undefined) reply({ accepted: true }); } + } + }); + `; + const { tempDir, storePath } = tempStorePath('pytc-mcp-string-'); + const membrane = new MockMembrane(); + const code = [ + `r = await mcpl__echo__echo({"text": ${JSON.stringify(listing)}})`, + 'lines = r.split("\\n")', + 'print("count:", len(lines))', + 'print("second:", lines[1])', + 'print("quoted:", r.startswith(\'"\'))', + ].join('\n'); + membrane.pushResponse(createMockResponse([ + { type: 'tool_use', id: 'call_mcp_direct', name: 'mcpl--echo--echo', input: { text: listing } }, + { type: 'tool_use', id: 'call_mcp_script', name: 'code_execution', input: { code } }, + ], 'tool_use')); + membrane.pushResponse(createMockResponse([{ type: 'text', text: 'Done.' }])); + + const framework = await AgentFramework.create({ + storePath, + membrane: membrane.asMembrane(), + agents: [], + modules: [], + mcplServers: [{ id: 'echo', command: process.execPath, args: ['-e', server] }], + syncIntervalMs: 0, + codeExecution: { enabled: true }, + }); + try { + const created = await framework.createEphemeralAgent({ + name: 'worker', model: 'test-model', systemPrompt: 'Do the task.', allowedTools: 'all', + }); + created.contextManager.addMessage('user', [{ type: 'text', text: 'Go.' }]); + const promise = framework.runEphemeralToCompletion(created.agent, created.contextManager); + framework.start(); + await promise; + + const wire = membrane.lastStream!.receivedToolResults[0] as Array<{ toolUseId: string; content: unknown }>; + const direct = wire.find((r) => r.toolUseId === 'call_mcp_direct'); + assert.strictEqual(direct?.content, listing, 'the model gets the text, not its JSON encoding'); + const script = wire.find((r) => r.toolUseId === 'call_mcp_script') as { content: string } | undefined; + assert.ok(script, 'the script call has a result'); + const out = JSON.parse(script.content) as { stdout: string; return_code: number }; + assert.strictEqual(out.return_code, 0, script.content); + assert.match(out.stdout, /count: 2\n/); + assert.match(out.stdout, /second: \[review\] org\/repo#1 "Fix it"\n/); + assert.match(out.stdout, /quoted: False/); + } finally { + await framework.stop(); + rmSync(tempDir, { recursive: true, force: true }); + } + }); + it('two code_execution calls in one response: one runs, the other is refused, the turn completes', async () => { const { tempDir, storePath } = tempStorePath('pytc-two-'); const membrane = new MockMembrane(); From af11da09a107ab92c15c9bd4c3205435cfce49bf Mon Sep 17 00:00:00 2001 From: Hilda-2681 Date: Sat, 17 Oct 2026 02:27:45 -0400 Subject: [PATCH 3/3] changelog: file the change to scripts' tool results as breaking A code_execution script's tool function returns the same text the model reads. With this change, a script that ran json.loads on a string result to unquote it now raises, and one that parsed a tool's JSON text with two json.loads calls now raises at the second, since the first already returns the data. CONTRIBUTING's "Breaking changes" asks that such a change be filed as breaking, so that npm version opens a new minor for it and a dependent's caret range doesn't take it at its next fresh install. - verbatim-string-tool-results-scripts.breaking.md: an audience-scoped entry for script authors, with who acts, the migration and what's unchanged. - verbatim-string-tool-results.changed.md: its two sentences about scripts move to that entry. The model's and the spill files' change stays a change. Its `data: ''` sentence now says it holds for a result without an error, since nonErrorResultText falls back to a result's error only when `data` is undefined or null. Co-Authored-By: Claude Opus 5.5 --- .../verbatim-string-tool-results-scripts.breaking.md | 8 ++++++++ changelog.d/verbatim-string-tool-results.changed.md | 6 ++---- 2 files changed, 10 insertions(+), 4 deletions(-) create mode 100644 changelog.d/verbatim-string-tool-results-scripts.breaking.md diff --git a/changelog.d/verbatim-string-tool-results-scripts.breaking.md b/changelog.d/verbatim-string-tool-results-scripts.breaking.md new file mode 100644 index 000000000..006ef8e12 --- /dev/null +++ b/changelog.d/verbatim-string-tool-results-scripts.breaking.md @@ -0,0 +1,8 @@ +- **Script authors (`code_execution`):** a script's tool function now + returns a tool's text result as that text, where it returned the text + JSON-encoded. That covers every MCP tool whose result is text only, and + every module tool whose `data` is a string. Drop a `json.loads` that only + unquoted such a result. Where the text is itself JSON, one `json.loads` now + returns the data, so drop the second. Unchanged: an object, a number, a + boolean, or an array other than an MCP content array still arrives as its + JSON. diff --git a/changelog.d/verbatim-string-tool-results.changed.md b/changelog.d/verbatim-string-tool-results.changed.md index fc9964b99..1a416b4b3 100644 --- a/changelog.d/verbatim-string-tool-results.changed.md +++ b/changelog.d/verbatim-string-tool-results.changed.md @@ -8,7 +8,5 @@ booleans and arrays other than MCP content arrays are still sent as JSON, and MCP content arrays as their blocks. - A `code_execution` script's tool function returns the same text, so it now - gets a string result as it is. A script that ran `json.loads` on such a - result to unquote it now gets an error unless the string itself is JSON. - `data: ''` is stored as no text, as `data: undefined` already was. + For a result without an error, `data: ''` is stored as no text, as + `data: undefined` already was.