diff --git a/changelog.d/tool-result-lone-surrogate.fixed.md b/changelog.d/tool-result-lone-surrogate.fixed.md new file mode 100644 index 000000000..b239360eb --- /dev/null +++ b/changelog.d/tool-result-lone-surrogate.fixed.md @@ -0,0 +1 @@ +- A lone UTF-16 surrogate in a tool result's text now reaches membrane as U+FFFD, as the context stores it (#695). Before, a text block's text, a reference's stub (from its `name`), an error result's text and the error text of a failure the tool doesn't flag went to membrane with the lone half raw, while the stored copy held U+FFFD. With the tool-result guard on (`toolResultGuard`), such a result left a stored message the context couldn't read back; with a text block or an error result, the agent's next turn sent no request and `stop()` threw. This change doesn't repair a store that already holds such a message. diff --git a/src/agent.ts b/src/agent.ts index 1f88bcd6b..8cc4fd4ac 100644 --- a/src/agent.ts +++ b/src/agent.ts @@ -1921,14 +1921,21 @@ export class Agent { // via buildStoredToolResultContent, with workspace spill). No workspace // is reachable from here, so the house default cap applies as plain // truncation — bounded beats unbounded, for errors too. - const content = results.map((r) => ({ - type: 'tool_result' as const, - toolUseId: r.id, - content: r.result.isError - ? truncateForHistory(r.result.error ?? 'Unknown error', DEFAULT_TOOL_RESULT_INLINE_MAX_CHARS) - : nonErrorResultText(r.result, DEFAULT_TOOL_RESULT_INLINE_MAX_CHARS), - isError: r.result.isError, - })); + const content = results.map((r) => { + // This API's results are unchecked (nonErrorResultText), so each field + // is read once, and only an error that is a string is made well formed; + // any other error value is passed on as it was before. + const isError = r.result.isError; + const error: unknown = isError ? r.result.error ?? 'Unknown error' : undefined; + return { + type: 'tool_result' as const, + toolUseId: r.id, + content: isError + ? truncateForHistory(typeof error === 'string' ? error.toWellFormed() : error as string, DEFAULT_TOOL_RESULT_INLINE_MAX_CHARS) + : nonErrorResultText(r.result, DEFAULT_TOOL_RESULT_INLINE_MAX_CHARS), + isError, + }; + }); return [{ participant: 'user', diff --git a/src/framework.ts b/src/framework.ts index 9a6b73187..2673bc646 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -20631,7 +20631,7 @@ export class AgentFramework { tc: CompletedToolCall, maxChars: number | undefined, ): { text: string; shown: ShownRanges | null } { - if (tc.result.isError) return { text: tc.result.error ?? 'Unknown error', shown: null }; + if (tc.result.isError) return { text: (tc.result.error ?? 'Unknown error').toWellFormed(), shown: null }; const ledger = this.toolImageLedgerFor(agentName); let blocks = null as HistoryBlockSpan[] | null; const text = nonErrorResultText(tc.result, undefined, { @@ -21185,7 +21185,8 @@ export class AgentFramework { if (b.type === 'text') { // A text that isn't a string shows as the converters' placeholder, // rather than sending the whole array to the JSON fallback. - let text = typeof b.text === 'string' ? b.text : NON_STRING_TEXT_PLACEHOLDER; + // Well formed, as the stored copy's text is (nonErrorResultText). + let text = typeof b.text === 'string' ? b.text.toWellFormed() : NON_STRING_TEXT_PLACEHOLDER; if (maxChars && text.length > maxChars) { text = text.slice(0, toolResultCutAt(text, maxChars)) + '\n\n[truncated — original was ' + text.length + ' chars' diff --git a/src/mcpl/references.ts b/src/mcpl/references.ts index c70b043c1..879ec65a9 100644 --- a/src/mcpl/references.ts +++ b/src/mcpl/references.ts @@ -434,7 +434,10 @@ export function unfetchableReason(record: ReferenceRecord, nowMs = Date.now()): /** * RFC-005 treatment for one MCPL wire block, for the model-visible text * lanes. Returns a replacement string, or null when the block should keep - * its existing (inline/text) handling. + * its existing (inline/text) handling. A reference's stub carries text taken + * from the block's own fields, so it's made well formed (a lone UTF-16 + * surrogate becomes U+FFFD), as the context store would make it: a tool + * result's copies then carry the same text (#695). */ export function referenceStubOrNull(block: unknown, provenance?: string, serverId?: string): string | null { const c = classifyBlock(block); @@ -442,7 +445,7 @@ export function referenceStubOrNull(block: unknown, provenance?: string, serverI case 'text': return null; case 'inline': return c.contradiction ? INLINE_WITHHELD_TEXT : null; - case 'reference': return buildReferenceStub(c.testimony, provenance, serverId); + case 'reference': return buildReferenceStub(c.testimony, provenance, serverId).toWellFormed(); case 'invalid': return c.shown; } } diff --git a/src/tool-result-history.ts b/src/tool-result-history.ts index 36e6aa4e4..b1008bbfc 100644 --- a/src/tool-result-history.ts +++ b/src/tool-result-history.ts @@ -9,7 +9,8 @@ * `maxChars` slice and re-introduces, one turn deferred, exactly the silent * hallucination this feature exists to kill. * - * For MCP-shaped content arrays, this helper keeps text blocks verbatim and + * For MCP-shaped content arrays, this helper keeps text blocks verbatim, + * apart from a lone UTF-16 surrogate, which becomes U+FFFD (#695), and * replaces image blocks with a short `[image: mimeType, ~NKB]` placeholder. * A string is already text and is kept as it is. For anything else it falls * back to JSON, written by RFC-005's rules at every depth (historyJson): a @@ -97,6 +98,14 @@ export function toolResultDataToHistoryString( * * Each field is read once: the direct Agent API passes its caller's own * result here, unchecked, and a getter needn't answer the same way twice. + * + * Each text a tool result's copies carry is made well formed where it's + * taken from the result: a lone UTF-16 surrogate becomes U+FFFD, as the + * context store makes it when it writes, so the copy sent to membrane and the + * copy stored carry the same text (#695). Other data goes through JSON, + * which escapes a lone half. toWellFormed() keeps the length, + * so the cut, the spans and the slots are computed from the well-formed text + * as before; a cut at a lone high half no longer steps back over it. */ export function nonErrorResultText( result: { data?: unknown; error?: string }, @@ -106,7 +115,10 @@ export function nonErrorResultText( const data = result.data; if (data === undefined || data === null) { const error = result.error; - if (typeof error === 'string' && error) return maxChars ? truncateForHistory(error, maxChars) : error; + if (typeof error === 'string' && error) { + const text = error.toWellFormed(); + return maxChars ? truncateForHistory(text, maxChars) : text; + } } return toolResultDataToHistoryString(data, maxChars, options); } @@ -139,7 +151,9 @@ function tryHistoryStringFromContentArray(data: unknown, options?: HistorySerial if (b.type === 'text') { // A text that isn't a string shows as the converters' placeholder, // rather than sending the whole array to the JSON fallback. - parts.push({ kind: 'text', text: typeof b.text === 'string' ? b.text : NON_STRING_TEXT_PLACEHOLDER }); + // Read once: a direct Agent API result is unchecked. + const text = b.text; + parts.push({ kind: 'text', text: typeof text === 'string' ? text.toWellFormed() : NON_STRING_TEXT_PLACEHOLDER }); } else if (isInlineContradiction(b)) { // RFC-005 vector 2: checked before the data branch so inline data // claiming bulk disposition is withheld, not rendered. diff --git a/test/tool-result-well-formed.test.ts b/test/tool-result-well-formed.test.ts new file mode 100644 index 000000000..cdcd94629 --- /dev/null +++ b/test/tool-result-well-formed.test.ts @@ -0,0 +1,160 @@ +/** + * A lone UTF-16 surrogate in a tool result's text reaches membrane as U+FFFD, + * as the context store writes it, so the copy sent and the copy stored match + * (#695). Each text a result's copies carry is made well formed where it's + * taken from the result: a text block's text and a reference's stub, in the + * history converter and the native one, and an error result's text, on the + * framework's path and the direct Agent API's. + */ +import { afterEach, test } from 'node:test'; +import assert from 'node:assert/strict'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import type { NormalizedRequest, NormalizedResponse } from '@animalabs/membrane'; +import { AgentFramework, type AgentConfig, type Module, type ProcessEvent, type ToolResult } from '../src/index.js'; +import { MockMembrane, createMockResponse } from './helpers/mock-membrane.js'; + +const LONE = 'cut \ud83d here'; +const MENDED = 'cut \ufffd here'; +// A 1×1 transparent PNG. +const PNG = 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNkYPhfDwAChwGA60e6kgAAAABJRU5ErkJggg=='; +const IMAGE = { type: 'image', data: PNG, mimeType: 'image/png' }; + +const dirs: string[] = []; +afterEach(() => { while (dirs.length) rmSync(dirs.pop()!, { recursive: true, force: true }); }); + +/** Each text a tool result's content holds: a string, or its text blocks'. */ +function texts(content: unknown): string[] { + if (typeof content === 'string') return [content]; + if (!Array.isArray(content)) return []; + return content.flatMap((b) => (b && typeof b === 'object' && typeof (b as { text?: unknown }).text === 'string' + ? [(b as { text: string }).text] : [])); +} + +class Canned implements Module { + readonly name = 'canned'; + constructor(private readonly result: ToolResult) {} + async start() {} + async stop() {} + getTools() { + return [{ name: 'fetch', description: 'Returns the canned result.', inputSchema: { type: 'object' as const, properties: {} } }]; + } + async handleToolCall() { return this.result; } + async onProcess(event: ProcessEvent) { + return event.type === 'external-message' + ? { addMessages: [{ participant: 'user', content: [{ type: 'text' as const, text: 'fetch it' }] }], requestInference: true } + : {}; + } +} + +async function framework(result: ToolResult, membrane: MockMembrane, agent: Partial = {}) { + const dir = mkdtempSync(join(tmpdir(), 'af-well-formed-')); dirs.push(dir); + return AgentFramework.create({ + storePath: join(dir, 'store'), + membrane: membrane.asMembrane(), + agents: [{ name: 'prime', model: 'test-model', systemPrompt: 'You are prime.', allowedTools: 'all', ...agent }], + modules: [new Canned(result)], + syncIntervalMs: 0, + }); +} + +function storedToolResult(f: AgentFramework): unknown { + const blocks = f.getAgent('prime')!.getContextManager().getAllMessages() + .flatMap((m) => m.content.filter((b) => b.type === 'tool_result')); + assert.equal(blocks.length, 1, 'one tool result is stored'); + return (blocks[0] as { content: unknown }).content; +} + +const SHAPES: Array<[string, ToolResult]> = [ + ['a text block beside an image', { success: true, data: [{ type: 'text', text: LONE }, IMAGE] }], + ['a text block alone', { success: true, data: [{ type: 'text', text: LONE }] }], + ['a reference named with it, beside an image', { success: true, data: [{ type: 'resource', uri: 'memo://f417-beside', name: LONE }, IMAGE] }], + ['a reference named with it, alone', { success: true, data: [{ type: 'resource', uri: 'memo://f417-alone', name: LONE }] }], + ['a reference named with it as the whole data', { success: true, data: { type: 'resource', uri: 'memo://f417-bare', name: LONE } }], + ['an error result', { success: false, isError: true, error: LONE }], + ["a failure the tool doesn't flag as an error", { success: false, error: LONE }], +]; + +// With the guard on, the guard stores the result, and on main the stored +// message couldn't be read back: the next turn sent nothing and stop() threw. +for (const guard of [false, true]) for (const [shape, result] of SHAPES) { + test(`a lone surrogate in ${shape} reaches membrane as the stored copy holds it, U+FFFD${guard ? ', with the guard on' : ''}`, async () => { + const membrane = new MockMembrane(); + membrane.pushResponse(createMockResponse([{ type: 'tool_use', id: 'call_fetch', name: 'canned--fetch', input: {} }], 'tool_use')); + membrane.pushResponse(createMockResponse([{ type: 'text', text: 'Handled.' }])); + const f = await framework(result, membrane, { toolResultGuard: guard }); + try { + f.pushEvent({ type: 'external-message', source: 'test', content: 'fetch it', metadata: {} }); + await f.runUntilIdle(); + const sent = (membrane.lastStream?.receivedToolResults[0]?.[0] as { content: unknown } | undefined)?.content; + assert.ok(sent !== undefined, 'a tool result reached membrane'); + const stored = storedToolResult(f); + const sentTexts = texts(sent); + assert.ok(sentTexts.length > 0, 'the result sent carries text'); + for (const t of sentTexts) assert.ok(t.isWellFormed(), `the text sent is well formed: ${JSON.stringify(t)}`); + assert.ok(sentTexts.some((t) => t.includes(MENDED)), 'the lone half is sent as U+FFFD'); + if (typeof sent === 'string') { + assert.equal(sent, stored, 'the copy sent is the copy stored'); + } else { + // Sent as native blocks beside the image: the stored copy is their + // text, with the image as its placeholder. + const storedText = texts(stored).join('\n'); + for (const t of sentTexts) assert.ok(storedText.includes(t), `the stored copy holds the text sent: ${JSON.stringify(t)}`); + } + } finally { + await f.stop(); + } + }); +} + +/** + * One round through the direct Agent API: a call answered with `result`, + * then the next request, whose tool results' content goes to `check`. + */ +async function directRound(result: ToolResult, agentConfig: Partial, check: (sent: unknown[], f: AgentFramework) => void) { + const membrane = new MockMembrane(); + const requests: NormalizedRequest[] = []; + const responses: NormalizedResponse[] = [ + createMockResponse([{ type: 'tool_use', id: 'one', name: 'canned--fetch', input: {} }], 'tool_use'), + createMockResponse([{ type: 'text', text: 'Handled.' }]), + ]; + (membrane as unknown as { stream: (request: NormalizedRequest) => Promise }).stream = async (request) => { + requests.push(structuredClone(request)); + const response = responses.shift(); + assert.ok(response, 'no more inference than scripted'); + return response; + }; + const f = await framework({ success: true, data: 'unused' }, membrane, agentConfig); + try { + const agent = f.getAgent('prime')!; + agent.getContextManager().addMessage('user', [{ type: 'text', text: 'fetch it' }]); + assert.equal((await agent.runInference(f.getAllTools())).toolCalls.length, 1); + agent.provideToolResult('one', result); + await agent.runInference(f.getAllTools()); + assert.equal(requests.length, 2); + const sent = requests[1].messages.flatMap((m) => m.content).filter((b) => b.type === 'tool_result') + .map((b) => (b as { content: unknown }).content); + assert.equal(sent.length, 1, 'the second request carries the tool result'); + check(sent, f); + } finally { + await f.stop(); + } +} + +test('a lone surrogate in an error result given through the direct Agent API reaches membrane as U+FFFD, as it is stored', async () => { + // The guard stores the direct API's results; without it, the caller keeps + // the context, and nothing is stored to compare with. + await directRound({ success: false, isError: true, error: LONE }, { toolResultGuard: true }, (sent, f) => { + assert.equal(sent[0], MENDED, 'the lone half is sent as U+FFFD'); + assert.equal(sent[0], storedToolResult(f), 'the copy sent is the copy stored'); + }); +}); + +test("an error that isn't a string, given through the direct Agent API, is passed on as it was", async () => { + // That API's results are unchecked: only a string error is made well + // formed, and any other value goes on as before. + await directRound({ success: false, isError: true, error: ['not a string'] as unknown as string }, {}, (sent) => { + assert.deepEqual(sent[0], ['not a string']); + }); +});