From 7b51121f7cee21a5938f10ac3b5762e0bb41a58d Mon Sep 17 00:00:00 2001 From: Kaoru0822 <208281322+kaoru0822-kitauji@users.noreply.github.com> Date: Fri, 9 Oct 2026 14:03:12 +0000 Subject: [PATCH] fix(mcp): measure tool errors against their text-only delivery Since #1277 an error result reaches the client without structuredContent, but toErrorToolResult and the transport recovery still budgeted it as structured JSON plus escaped text. An error whose text fit between about half and all of the response budget was replaced by resource_constraint, and without Evidence retention the original diagnostic was lost. Count only the escaped text for errors. The boundary, process-global and real Ghidra large-result probes now use diagnostics that exceed the text-only budget. Co-Authored-By: Claude Opus 5.5 --- docs/mcp-contracts.md | 3 +- scripts/lib/real-ghidra-large-results.mjs | 8 ++-- src/server/EvidenceMcpServer.ts | 3 +- src/server/toolResult.ts | 8 +++- src/server/toolResultEncoding.test.ts | 19 ++++++++ src/server/toolResultEncoding.ts | 25 ++++++++-- .../mcp/oversizedErrorDeliveryMcp.test.ts | 48 ++++++++++++++++++- .../mcpDeliveryEnvironment.test.ts | 3 +- 8 files changed, 103 insertions(+), 14 deletions(-) diff --git a/docs/mcp-contracts.md b/docs/mcp-contracts.md index 16ce69d6b5..150cf71a97 100644 --- a/docs/mcp-contracts.md +++ b/docs/mcp-contracts.md @@ -201,7 +201,8 @@ retained session history or an explicit bundle for transfer. REA prepares complete MCP results within the pinned stdio client's 10 MiB receive-buffer budget, including both text and structured representations and -room for the JSON-RPC envelope. Response budget settings are captured at startup; +room for the JSON-RPC envelope. Tool errors carry only their text +representation, so only that text counts against the budget. Response budget settings are captured at startup; restart or recreate the server to apply changes. If a result cannot fit, REA returns `resource_constraint` with `details.resource: "transport"` before constructing a document-sized string. Analysis Evidence remains complete in the current diff --git a/scripts/lib/real-ghidra-large-results.mjs b/scripts/lib/real-ghidra-large-results.mjs index 76738567e1..92dde5045a 100644 --- a/scripts/lib/real-ghidra-large-results.mjs +++ b/scripts/lib/real-ghidra-large-results.mjs @@ -64,11 +64,13 @@ export async function verifyGhidraLargeResults({ assert.equal(expected[0], 0x5a); assert.equal(expected.at(-1), 0xa5); const before = await call("binary_session"); - // This request fits the SDK receive limit, but repeating the producer's - // diagnostic in text and structured content cannot fit the response. + // An error reaches the wire once, as escaped JSON text. Each quote costs + // two request bytes but four response bytes once the echoed diagnostic is + // escaped again as text, so this request fits the SDK receive limit while + // the error response cannot. const missingSelector = "REA_MISSING_" + - "x".repeat(Math.ceil(STDIO_DEFAULT_MAX_BUFFER_SIZE / 2) + 64 * 1024); + '"'.repeat(Math.ceil(STDIO_DEFAULT_MAX_BUFFER_SIZE / 4) + 64 * 1024); const lookupError = await reject("procedure_address", { procedure: missingSelector, }); diff --git a/src/server/EvidenceMcpServer.ts b/src/server/EvidenceMcpServer.ts index f2e2396ddf..706ad05772 100644 --- a/src/server/EvidenceMcpServer.ts +++ b/src/server/EvidenceMcpServer.ts @@ -153,7 +153,8 @@ class ErrorEvidenceTransport implements Transport { return message; const structured = jsonObjectSchema.parse(message.result.structuredContent); const budget = this.delivery.resultBudgetBytes; - const encoded = encodeToolResult(structured, budget); + // The delivered error omits structuredContent, so only its text counts. + const encoded = encodeToolResult(structured, budget, "text"); if (encoded.ok) return message; const operation = tool ?? "mcp_tool_error"; const originalError = analysisErrorProjectionSchema.safeParse( diff --git a/src/server/toolResult.ts b/src/server/toolResult.ts index df7be01f8c..90f54dda18 100644 --- a/src/server/toolResult.ts +++ b/src/server/toolResult.ts @@ -65,7 +65,13 @@ export class ToolResultDelivery { /** Project an error without allocating oversized repeated MCP text. */ toErrorToolResult(error: AnalysisError): CallToolResult { const structuredContent = { error: projectAnalysisError(error) }; - const encoded = encodeToolResult(structuredContent, this.resultBudgetBytes); + // Errors reach the wire as text only; the structured copy is a private + // carrier the transport removes after its own budget check. + const encoded = encodeToolResult( + structuredContent, + this.resultBudgetBytes, + "text", + ); // The transport retains an oversized projected error before replacing it // with a recoverable delivery constraint, using this same budget. return { diff --git a/src/server/toolResultEncoding.test.ts b/src/server/toolResultEncoding.test.ts index c6b20d3f55..3225ddd8b9 100644 --- a/src/server/toolResultEncoding.test.ts +++ b/src/server/toolResultEncoding.test.ts @@ -41,6 +41,25 @@ describe("MCP result encoding budget", () => { }); }); + it.each([ + { error: { code: "invalid_request", message: 'δΈ­ζ–‡"\n'.repeat(30000) } }, + { error: { message: "πŸ˜€\ud800" } }, + ])("counts only escaped text for text-only error delivery", (candidate) => { + const textBytes = Buffer.byteLength( + JSON.stringify({ + content: [{ type: "text", text: JSON.stringify(candidate) }], + }), + ); + const encoded = encodeToolResult(candidate, textBytes, "text"); + if (!encoded.ok) throw new Error("Expected complete encoding"); + expect(encoded.text).toBe(JSON.stringify(candidate)); + expect(encoded.bytes).toBe(textBytes); + expect(encodeToolResult(candidate, textBytes - 1, "text")).toMatchObject({ + ok: false, + bytesAtLeast: textBytes, + }); + }); + it("stops before reading the remainder of an oversized result", () => { const candidate: JsonValue = { text: "x".repeat(1000000) }; Object.defineProperty(candidate, "remainder", { diff --git a/src/server/toolResultEncoding.ts b/src/server/toolResultEncoding.ts index c72b38859b..54d6efa6f3 100644 --- a/src/server/toolResultEncoding.ts +++ b/src/server/toolResultEncoding.ts @@ -13,6 +13,16 @@ const RESULT_ENVELOPE_BYTES = }), ) - 4; +const TEXT_RESULT_ENVELOPE_BYTES = Buffer.byteLength( + JSON.stringify({ content: [{ type: "text", text: "" }] }), +); + +/** + * MCP representations a result carries on the wire. Successful results repeat + * the JSON as structured content and escaped text; error results carry text only. + */ +export type ToolResultRepresentations = "structured-and-text" | "text"; + /** Result of encoding the complete repeated MCP representations within a wire budget. */ export type ToolResultEncoding = | { readonly ok: true; readonly text: string; readonly bytes: number } @@ -23,18 +33,23 @@ export type ToolResultEncoding = readonly constraint: "receive-buffer" | "string-length"; }; -/** Account for structured JSON and escaped text before allocating the complete text. */ +/** Account for the delivered JSON representations before allocating the complete text. */ export const encodeToolResult = ( value: JsonValue, budgetBytes: number, + representations: ToolResultRepresentations = "structured-and-text", ): ToolResultEncoding => { + const structured = representations === "structured-and-text"; const parts: string[] = []; - let bytes = RESULT_ENVELOPE_BYTES; - let codeUnits = RESULT_ENVELOPE_BYTES; + let bytes = structured ? RESULT_ENVELOPE_BYTES : TEXT_RESULT_ENVELOPE_BYTES; + let codeUnits = bytes; for (const part of bufferedJsonParts(jsonParts(value))) { const escapedPart = JSON.stringify(part); - bytes += Buffer.byteLength(part) + Buffer.byteLength(escapedPart) - 2; - codeUnits += part.length + escapedPart.length - 2; + bytes += + (structured ? Buffer.byteLength(part) : 0) + + Buffer.byteLength(escapedPart) - + 2; + codeUnits += (structured ? part.length : 0) + escapedPart.length - 2; if (bytes > budgetBytes || codeUnits > MCP_RESULT_STRING_LIMIT) return { ok: false, diff --git a/tests/boundary/mcp/oversizedErrorDeliveryMcp.test.ts b/tests/boundary/mcp/oversizedErrorDeliveryMcp.test.ts index ea2809b7ce..ba90eafdfe 100644 --- a/tests/boundary/mcp/oversizedErrorDeliveryMcp.test.ts +++ b/tests/boundary/mcp/oversizedErrorDeliveryMcp.test.ts @@ -17,17 +17,22 @@ import { ToolResultDelivery } from "../../../src/server/toolResult.js"; const delivery = new ToolResultDelivery(STDIO_DEFAULT_MAX_BUFFER_SIZE); -const oversizedFailure = () => +const failureWithName = (nameBytes: number) => delivery.toErrorToolResult( new AnalysisInputError("procedure_address", undefined, [ { path: ["procedure"], reason: "invalid_value", - message: `Unknown Ghidra procedure name or address: REA_MISSING_${"x".repeat(Math.ceil(STDIO_DEFAULT_MAX_BUFFER_SIZE / 2) + 64 * 1024)}`, + message: `Unknown Ghidra procedure name or address: REA_MISSING_${"x".repeat(nameBytes)}`, }, ]), ); +// The delivered error carries its JSON once, as text, so only a diagnostic +// larger than the whole budget is oversized. +const oversizedFailure = () => + failureWithName(STDIO_DEFAULT_MAX_BUFFER_SIZE + 64 * 1024); + const exerciseDeliveryFailure = async ( recordEvidence: EvidenceWriter["recordEvidence"] | undefined, retention: "retained" | "unavailable" | "failed", @@ -143,3 +148,42 @@ it("retains the complete oversized diagnostic before removing its private struct ); expect(retained[0]?.raw_result).toEqual(oversizedFailure()); }); + +it("delivers an error whose text fits although structured and text copies would not", async () => { + const nameBytes = Math.ceil(STDIO_DEFAULT_MAX_BUFFER_SIZE / 2) + 64 * 1024; + const retained: Evidence[] = []; + const server = new EvidenceMcpServer( + { name: "mid-sized-error-test", version: "1" }, + { capabilities: {} }, + (evidence) => { + retained.push(evidence); + return ok("added"); + }, + delivery, + ); + server.registerTool("mid_failure", { inputSchema: {} }, async () => + failureWithName(nameBytes), + ); + const client = new Client({ name: "mid-sized-error-client", version: "1" }); + const [clientTransport, serverTransport] = + InMemoryTransport.createLinkedPair(); + try { + await server.connect(serverTransport); + await client.connect(clientTransport); + const failure = await client.callTool({ + name: "mid_failure", + arguments: {}, + }); + expect(failure.isError).toBe(true); + expect(failure.structuredContent).toBeUndefined(); + expect(parseMcpToolError(failure)).toEqual( + failureWithName(nameBytes).structuredContent, + ); + expect(retained).toHaveLength(0); + expect(Buffer.byteLength(JSON.stringify(failure))).toBeLessThan( + STDIO_DEFAULT_MAX_BUFFER_SIZE, + ); + } finally { + await Promise.all([client.close(), server.close()]); + } +}); diff --git a/tests/process-global/mcpDeliveryEnvironment.test.ts b/tests/process-global/mcpDeliveryEnvironment.test.ts index 33bb18ed51..32c9061af3 100644 --- a/tests/process-global/mcpDeliveryEnvironment.test.ts +++ b/tests/process-global/mcpDeliveryEnvironment.test.ts @@ -62,8 +62,9 @@ it("keeps independent server budgets for successful results and oversized errors largeEnvironment.REA_MCP_MAX_RESPONSE_BYTES = String(smallBudget); vi.stubEnv("REA_MCP_MAX_RESPONSE_BYTES", "invalid-ambient-after-selection"); + // Error text alone exceeds the small budget and fits the large one. const diagnostic = "observed failure ".repeat( - Math.ceil((smallBudget / 2 + 65536) / 17), + Math.ceil((smallBudget + 65536) / 17), ); for (const server of [small, large]) server.registerTool("selected_failure", { inputSchema: {} }, async () =>