Skip to content

Commit c923c11

Browse files
committed
Harden spilled tool result materialization
1 parent 56e455b commit c923c11

7 files changed

Lines changed: 187 additions & 51 deletions

src/mcp/plugin.test.ts

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,7 @@
11
import { describe, test, expect } from "bun:test";
22
import { mcpClientToAgentTools } from "./plugin.js";
33
import { createPermissionGate } from "../permission/gate.js";
4-
import {
5-
MAX_RESULT_CHARS,
6-
spillBlobKey,
7-
} from "../plugins/result-truncation-plugin.js";
4+
import { MAX_RESULT_CHARS, spillBlobKey } from "../plugins/result-truncation-plugin.js";
85
import { toolOutputAbsolutePath } from "../plugins/tool-result-materialize.js";
96
import { CREDENTIAL_REDACTION } from "../plugins/tool-result-secret-scrub.js";
107
import type { MCPClient } from "./client.js";
@@ -116,6 +113,38 @@ describe("mcpClientToAgentTools", () => {
116113
expect(result.content).toContain("output truncated");
117114
});
118115

116+
test("scrubs escaped secrets after oversized JSON pretty materialization", async () => {
117+
const gate = skipGate();
118+
const store = fakeBlobStore();
119+
const escapedSecret = `sk-\\u006cive-${"b".repeat(24)}`;
120+
const minified = `{"secret":"${escapedSecret}","pad":"${"x".repeat(MAX_RESULT_CHARS)}"}`;
121+
expect(minified).not.toContain("sk-live-");
122+
123+
const client = fakeClient(minified);
124+
const [tool] = mcpClientToAgentTools(client, gate, {
125+
getBlobWriter: () => store.writeBlob,
126+
});
127+
if (tool?.kind !== "full") throw new Error("expected full tool");
128+
129+
const result = await tool.handler(
130+
{
131+
id: "c-mcp-json-secret",
132+
name: "mcp__acme__fetch_secret",
133+
arguments: {},
134+
},
135+
new AbortController().signal,
136+
);
137+
138+
const spilled = new TextDecoder().decode(
139+
store.blobs.get(spillBlobKey("c-mcp-json-secret"))!.bytes,
140+
);
141+
expect(result.content).toContain(CREDENTIAL_REDACTION);
142+
expect(result.content).not.toContain("sk-live-");
143+
expect(spilled).toContain(CREDENTIAL_REDACTION);
144+
expect(spilled).not.toContain("sk-live-");
145+
expect(spilled).not.toContain(escapedSecret);
146+
});
147+
119148
test("spills oversized plain text under :full and names contextDir path", async () => {
120149
const gate = skipGate();
121150
const store = fakeBlobStore();
@@ -138,8 +167,6 @@ describe("mcpClientToAgentTools", () => {
138167
expect(entry?.contentType).toBe("text/plain");
139168
expect(new TextDecoder().decode(entry!.bytes)).toBe(huge);
140169
expect(result.content).toContain(`tool-output:///${key}`);
141-
expect(result.content).toContain(
142-
toolOutputAbsolutePath(contextDir, key, "text/plain"),
143-
);
170+
expect(result.content).toContain(toolOutputAbsolutePath(contextDir, key, "text/plain"));
144171
});
145172
});

src/plugins/result-truncation-plugin.test.ts

Lines changed: 37 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,27 @@ describe("truncateToolResultContent", () => {
133133
expect(truncated).not.toContain(pretty.slice(-40));
134134
});
135135

136+
test("oversized JSON with an escaped secret is scrubbed after pretty materialization", async () => {
137+
const store = fakeBlobStore();
138+
const escapedSecret = `sk-\\u006cive-${"a".repeat(24)}`;
139+
const minified = `{"secret":"${escapedSecret}","pad":"${"x".repeat(MAX_RESULT_CHARS)}"}`;
140+
expect(minified).not.toContain("sk-live-");
141+
142+
const truncated = await truncateToolResultContent(minified, MAX_RESULT_CHARS, {
143+
callId: "call-json-secret",
144+
writeBlob: store.writeBlob,
145+
});
146+
147+
const spilled = new TextDecoder().decode(
148+
store.blobs.get(spillBlobKey("call-json-secret"))!.bytes,
149+
);
150+
expect(truncated).toContain(CREDENTIAL_REDACTION);
151+
expect(truncated).not.toContain("sk-live-");
152+
expect(spilled).toContain(CREDENTIAL_REDACTION);
153+
expect(spilled).not.toContain("sk-live-");
154+
expect(spilled).not.toContain(escapedSecret);
155+
});
156+
136157
test("NDJSON over the gate is spilled unchanged as application/x-ndjson", async () => {
137158
const store = fakeBlobStore();
138159
const lines = Array.from({ length: 200 }, (_, i) =>
@@ -208,28 +229,24 @@ describe("truncateToolResultContent", () => {
208229
},
209230
);
210231

211-
test(
212-
"a same-keyed reactor spill cannot clobber the :full blob (CL-6908)",
213-
async () => {
214-
const store = fakeBlobStore();
215-
const original = "p".repeat(50_000);
216-
await truncateToolResultContent(original, MAX_RESULT_CHARS, {
217-
callId: "call-1",
218-
writeBlob: store.writeBlob,
219-
});
220-
221-
// Simulate a lossy same-id write the reactor would do on an over-cap result.
222-
await store.writeBlob("call-1", new TextEncoder().encode("LOSSY"), "text/plain");
232+
test("a same-keyed reactor spill cannot clobber the :full blob (CL-6908)", async () => {
233+
const store = fakeBlobStore();
234+
const original = "p".repeat(50_000);
235+
await truncateToolResultContent(original, MAX_RESULT_CHARS, {
236+
callId: "call-1",
237+
writeBlob: store.writeBlob,
238+
});
223239

224-
const blobReader = createBlobReader(store);
225-
const recovered = new TextDecoder().decode(
226-
await blobReader.read(`tool-output:///${spillBlobKey("call-1")}`),
227-
);
228-
expect(recovered).toBe(original);
229-
expect(new TextDecoder().decode(store.blobs.get("call-1")!.bytes)).toBe("LOSSY");
230-
},
231-
);
240+
// Simulate a lossy same-id write the reactor would do on an over-cap result.
241+
await store.writeBlob("call-1", new TextEncoder().encode("LOSSY"), "text/plain");
232242

243+
const blobReader = createBlobReader(store);
244+
const recovered = new TextDecoder().decode(
245+
await blobReader.read(`tool-output:///${spillBlobKey("call-1")}`),
246+
);
247+
expect(recovered).toBe(original);
248+
expect(new TextDecoder().decode(store.blobs.get("call-1")!.bytes)).toBe("LOSSY");
249+
});
233250
});
234251
});
235252

src/plugins/result-truncation-plugin.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import {
55
toolOutputAbsolutePath,
66
type MaterializedToolResult,
77
} from "./tool-result-materialize.js";
8+
import { scrubSecretShapedToolResultContent } from "./tool-result-secret-scrub.js";
89

910
const TRUNCATABLE_TOOLS = new Set(["read_file", "grep", "run_shell", "search_files", "web_fetch"]);
1011

@@ -65,8 +66,7 @@ function truncationNotice(args: {
6566
`Use offset/limit or a narrower query.]`
6667
);
6768
}
68-
const pathBit =
69-
absolutePath !== undefined ? ` (session path: ${absolutePath})` : "";
69+
const pathBit = absolutePath !== undefined ? ` (session path: ${absolutePath})` : "";
7070
return (
7171
`\n[output truncated at ${maxChars.toLocaleString()} chars — ` +
7272
`${remaining.toLocaleString()} more chars omitted here. The full result ` +
@@ -107,7 +107,8 @@ async function spillAndTruncate(
107107
maxChars: number,
108108
spill?: TruncationSpillOptions,
109109
): Promise<string> {
110-
const { text, contentType } = materialized;
110+
const { contentType } = materialized;
111+
const text = scrubSecretShapedToolResultContent(materialized.text);
111112
if (text.length <= maxChars) return text;
112113

113114
if (spill === undefined) {

src/plugins/tool-result-materialize.test.ts

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -62,11 +62,7 @@ describe("materializeToolResultRecord", () => {
6262

6363
describe("toolOutputAbsolutePath", () => {
6464
test("mirrors store naming including :full → _full and .json extension", () => {
65-
const abs = toolOutputAbsolutePath(
66-
"/tmp/session/context",
67-
"call-42:full",
68-
"application/json",
69-
);
65+
const abs = toolOutputAbsolutePath("/tmp/session/context", "call-42:full", "application/json");
7066
expect(abs).toBe("/tmp/session/context/tool-output/call-42_full.json");
7167
});
7268

src/plugins/tool-result-materialize.ts

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,7 @@
11
import path from "node:path";
22

33
/** MIME written with spilled tool-output blobs. */
4-
export type ToolResultContentType =
5-
| "application/json"
6-
| "application/x-ndjson"
7-
| "text/plain";
4+
export type ToolResultContentType = "application/json" | "application/x-ndjson" | "text/plain";
85

96
export interface MaterializedToolResult {
107
text: string;
Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
import { describe, expect, test } from "bun:test";
2+
import { createToolRunner } from "@intx/agent";
3+
import { createBlobReader, type ToolCall, type ToolResult } from "@intx/types/runtime";
4+
5+
import { createCodexToolProxies, type CodexRunManageTasks } from "../agent/codex-tool-proxies.js";
6+
import {
7+
MAX_RESULT_CHARS,
8+
truncateToolResultContent,
9+
} from "../plugins/result-truncation-plugin.js";
10+
import { createCodexProxyRunTool } from "./run.js";
11+
12+
function fakeBlobStore() {
13+
const blobs = new Map<string, { bytes: Uint8Array; contentType: string }>();
14+
return {
15+
blobs,
16+
writeBlob: async (key: string, bytes: Uint8Array, contentType: string) => {
17+
blobs.set(key, { bytes, contentType });
18+
},
19+
readBlob: async (key: string) => {
20+
const entry = blobs.get(key);
21+
if (entry === undefined) throw new Error(`Blob not found: ${key}`);
22+
return entry.bytes;
23+
},
24+
};
25+
}
26+
27+
const unusedManageTasks: CodexRunManageTasks = async () => ({ content: "unused" });
28+
29+
function extractToolOutputURI(content: unknown): string {
30+
const match = /tool-output:\/\/\/\S+/.exec(String(content));
31+
if (match === null) throw new Error(`missing tool-output URI in ${String(content)}`);
32+
return match[0].replace(/[.\]]+$/, "");
33+
}
34+
35+
describe("createCodexProxyRunTool", () => {
36+
test("oversized proxied shell calls get distinct recoverable spill URIs", async () => {
37+
const store = fakeBlobStore();
38+
const outputs: [string, string] = [
39+
`${"a".repeat(MAX_RESULT_CHARS)}FIRST-TAIL`,
40+
`${"b".repeat(MAX_RESULT_CHARS)}SECOND-TAIL`,
41+
];
42+
const seenCallIds: string[] = [];
43+
const posixTools = {
44+
run: async (call: ToolCall): Promise<ToolResult> => {
45+
seenCallIds.push(call.id);
46+
const index = seenCallIds.length - 1;
47+
return {
48+
callId: call.id,
49+
content: await truncateToolResultContent(outputs[index] ?? "", MAX_RESULT_CHARS, {
50+
callId: call.id,
51+
writeBlob: store.writeBlob,
52+
}),
53+
};
54+
},
55+
};
56+
57+
const tools = createCodexToolProxies({
58+
isCodex: true,
59+
runTool: createCodexProxyRunTool(posixTools),
60+
readRawFile: async () => ({ content: "unused" }),
61+
runManageTasks: unusedManageTasks,
62+
});
63+
const runner = createToolRunner(tools);
64+
65+
const first = await runner.run(
66+
{ id: "outer-1", name: "shell", arguments: { command: "first" } },
67+
new AbortController().signal,
68+
);
69+
const second = await runner.run(
70+
{ id: "outer-2", name: "shell", arguments: { command: "second" } },
71+
new AbortController().signal,
72+
);
73+
74+
const firstURI = extractToolOutputURI(first.content);
75+
const secondURI = extractToolOutputURI(second.content);
76+
expect(firstURI).not.toBe(secondURI);
77+
expect(seenCallIds).toEqual(["codex-proxy-1", "codex-proxy-2"]);
78+
79+
const reader = createBlobReader(store);
80+
expect(new TextDecoder().decode(await reader.read(firstURI))).toBe(outputs[0]);
81+
expect(new TextDecoder().decode(await reader.read(secondURI))).toBe(outputs[1]);
82+
});
83+
});

src/subagent/run.ts

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,13 @@ import { type } from "arktype";
2323
import { createPosixTools } from "@intx/tools-posix";
2424
import { createDynamicToolRunner } from "../tui/dynamic-tool-runner.js";
2525
import type { ReactorEmittedEvent } from "@intx/inference";
26-
import type { BlobReader, InboundMessage, ToolDefinition } from "@intx/types/runtime";
26+
import type {
27+
BlobReader,
28+
InboundMessage,
29+
ToolCall,
30+
ToolDefinition,
31+
ToolResult,
32+
} from "@intx/types/runtime";
2733

2834
import { seedPricingMetadataFromCache } from "../cost/pricing-metadata.js";
2935
import { defaultPricingCachePath } from "../cost/pricing-fetcher.js";
@@ -321,6 +327,25 @@ const submitResultDefinition: ToolDefinition = {
321327
},
322328
};
323329

330+
interface CodexProxyToolRunner {
331+
run(call: ToolCall, signal: AbortSignal): Promise<ToolResult>;
332+
}
333+
334+
export function createCodexProxyRunTool(posixTools: CodexProxyToolRunner): CodexRunTool {
335+
let invocation = 0;
336+
return async (name, args) => {
337+
invocation += 1;
338+
const result = await posixTools.run(
339+
{ id: `codex-proxy-${invocation}`, name, arguments: args },
340+
new AbortController().signal,
341+
);
342+
return {
343+
content: typeof result.content === "string" ? result.content : JSON.stringify(result.content),
344+
...(result.isError === true ? { isError: true } : {}),
345+
};
346+
};
347+
}
348+
324349
// Spin up an isolated, autonomous agent loop, hand it one task, and return
325350
// its final report. `params.cwd` is either the dispatcher's own cwd (shared
326351
// mode) or a worktree snapshotted from the dispatcher's last commit
@@ -411,17 +436,7 @@ export async function runSubAgent(params: RunSubAgentParams): Promise<RunSubAgen
411436
// filter, so implement/docs allowlists can keep it when Codex. allowDelete
412437
// follows whether delete_file is in the leaf capability include list (docs
413438
// omits it; implement includes it).
414-
const runTool: CodexRunTool = async (name, args) => {
415-
const result = await posixTools.run(
416-
{ id: "codex-proxy", name, arguments: args },
417-
new AbortController().signal,
418-
);
419-
return {
420-
content:
421-
typeof result.content === "string" ? result.content : JSON.stringify(result.content),
422-
...(result.isError === true ? { isError: true } : {}),
423-
};
424-
};
439+
const runTool = createCodexProxyRunTool(posixTools);
425440
// manage_tasks is not a posix tool — task state here is owned by the
426441
// director observing manage_tasks tool_calls in the model's own output,
427442
// not by this handler's return value (see applyManageTasksToolCall in

0 commit comments

Comments
 (0)