From 0e62d7a64e35577ad2c22d0338ca39f3d9f800ac Mon Sep 17 00:00:00 2001 From: Crispin-2713 Date: Thu, 22 Oct 2026 15:39:22 -0400 Subject: [PATCH 1/2] fix(tool-results): a lone surrogate in a tool result's text reaches membrane as U+FFFD, as it's stored A tool result's text was taken from the result as it was: a text block's text in both converters, a reference's stub (whose name keeps a lone half through sanitizeLabel), an error result's text, the error text of a failure the tool doesn't flag, and the direct Agent API's error text. The request sent to membrane then carried the lone half while the context stored U+FFFD. With the tool-result guard on, the stored message couldn't be read back, and an agent given a text block or an error result sent no request on its next turn, and stop() threw (#695). Each of those texts is now made well formed where it's taken (toWellFormed, which keeps the length), and the stub where referenceStubOrNull builds it. The direct Agent API's results are unchecked, so there an error that isn't a string is left as it was, and a text block's text is read once. Fixes #695. Co-Authored-By: Claude Opus 5.5 --- .../tool-result-lone-surrogate.fixed.md | 1 + src/agent.ts | 23 ++- src/framework.ts | 5 +- src/mcpl/references.ts | 7 +- src/tool-result-history.ts | 20 ++- test/tool-result-well-formed.test.ts | 142 ++++++++++++++++++ 6 files changed, 183 insertions(+), 15 deletions(-) create mode 100644 changelog.d/tool-result-lone-surrogate.fixed.md create mode 100644 test/tool-result-well-formed.test.ts 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..e896f10b7 --- /dev/null +++ b/test/tool-result-well-formed.test.ts @@ -0,0 +1,142 @@ +/** + * 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(); + } + }); +} + +test('a lone surrogate in an error result given through the direct Agent API reaches membrane as U+FFFD, as it is stored', async () => { + 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; + }; + // The guard stores the direct API's results; without it, the caller keeps + // the context, and nothing is stored to compare with. + const f = await framework({ success: true, data: 'unused' }, membrane, { toolResultGuard: true }); + 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', { success: false, isError: true, error: LONE }); + 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'); + 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'); + } finally { + await f.stop(); + } +}); From d89968517c283952b6e1fa5b396f9154c60700d8 Mon Sep 17 00:00:00 2001 From: Crispin-2713 Date: Thu, 22 Oct 2026 15:52:19 -0400 Subject: [PATCH 2/2] test: an error that isn't a string, given through the direct Agent API, is passed on as it was RevEngine's review of #699 (low): the direct API's typeof check on its error, which passes a non-string error on as main does, had no test, so calling toWellFormed() on any error value would have passed. The two direct-API tests now share one round's harness. Co-Authored-By: Claude Opus 5.5 --- test/tool-result-well-formed.test.ts | 32 ++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/test/tool-result-well-formed.test.ts b/test/tool-result-well-formed.test.ts index e896f10b7..cdcd94629 100644 --- a/test/tool-result-well-formed.test.ts +++ b/test/tool-result-well-formed.test.ts @@ -108,7 +108,11 @@ for (const guard of [false, true]) for (const [shape, result] of SHAPES) { }); } -test('a lone surrogate in an error result given through the direct Agent API reaches membrane as U+FFFD, as it is stored', async () => { +/** + * 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[] = [ @@ -121,22 +125,36 @@ test('a lone surrogate in an error result given through the direct Agent API rea assert.ok(response, 'no more inference than scripted'); return response; }; - // The guard stores the direct API's results; without it, the caller keeps - // the context, and nothing is stored to compare with. - const f = await framework({ success: true, data: 'unused' }, membrane, { toolResultGuard: true }); + 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', { success: false, isError: true, error: LONE }); + 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'); - 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'); + 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']); + }); });