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 new file mode 100644 index 000000000..1a416b4b3 --- /dev/null +++ b/changelog.d/verbatim-string-tool-results.changed.md @@ -0,0 +1,12 @@ +- 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. + + For a result without an error, `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..8a9488386 100644 --- a/test/code-execution.test.ts +++ b/test/code-execution.test.ts @@ -521,6 +521,135 @@ 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("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(); 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']); });