Skip to content

Commit 946ea6a

Browse files
vincentkocvincentkoc
committed
fix(codex): quarantine unreadable dynamic tools
1 parent 8aeaadf commit 946ea6a

2 files changed

Lines changed: 201 additions & 25 deletions

File tree

‎extensions/codex/src/app-server/dynamic-tools.test.ts‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -371,6 +371,84 @@ describe("createCodexDynamicToolBridge", () => {
371371
expect(badExecute).not.toHaveBeenCalled();
372372
});
373373

374+
it("quarantines unreadable dynamic tool entries before Codex registration", () => {
375+
const message = createTool({ name: "message" });
376+
const tools = [message] as AnyAgentTool[];
377+
const proxy = new Proxy(tools, {
378+
get(target, property, receiver) {
379+
if (property === "1") {
380+
throw new Error("fuzzplugin dynamic tool entry getter exploded");
381+
}
382+
if (property === "length") {
383+
return 2;
384+
}
385+
return Reflect.get(target, property, receiver);
386+
},
387+
});
388+
389+
const bridge = createCodexDynamicToolBridge({
390+
tools: proxy,
391+
signal: new AbortController().signal,
392+
});
393+
394+
expect(bridge.availableSpecs.map((tool) => tool.name)).toEqual(["message"]);
395+
expect(bridge.specs.map((tool) => tool.name)).toEqual(["message"]);
396+
expect(bridge.telemetry.quarantinedTools).toEqual([
397+
{
398+
tool: "tool[1]",
399+
violations: ["tool[1] is unreadable"],
400+
},
401+
]);
402+
});
403+
404+
it("quarantines unreadable dynamic tool fields before Codex registration", () => {
405+
const unreadable = createTool({ name: "fuzzplugin_unreadable" });
406+
Object.defineProperty(unreadable, "parameters", {
407+
enumerable: true,
408+
get() {
409+
throw new Error("fuzzplugin dynamic tool parameters getter exploded");
410+
},
411+
});
412+
413+
const bridge = createCodexDynamicToolBridge({
414+
tools: [unreadable, createTool({ name: "message" })],
415+
signal: new AbortController().signal,
416+
});
417+
418+
expect(bridge.availableSpecs.map((tool) => tool.name)).toEqual(["message"]);
419+
expect(bridge.specs.map((tool) => tool.name)).toEqual(["message"]);
420+
expect(bridge.telemetry.quarantinedTools).toEqual([
421+
{
422+
tool: "fuzzplugin_unreadable",
423+
violations: ["fuzzplugin_unreadable.inputSchema is unreadable"],
424+
},
425+
]);
426+
});
427+
428+
it("quarantines unreadable dynamic tool descriptions before Codex registration", () => {
429+
const unreadable = createTool({ name: "fuzzplugin_unreadable_description" });
430+
Object.defineProperty(unreadable, "description", {
431+
enumerable: true,
432+
get() {
433+
throw new Error("fuzzplugin dynamic tool description getter exploded");
434+
},
435+
});
436+
437+
const bridge = createCodexDynamicToolBridge({
438+
tools: [unreadable, createTool({ name: "message" })],
439+
signal: new AbortController().signal,
440+
});
441+
442+
expect(bridge.availableSpecs.map((tool) => tool.name)).toEqual(["message"]);
443+
expect(bridge.specs.map((tool) => tool.name)).toEqual(["message"]);
444+
expect(bridge.telemetry.quarantinedTools).toEqual([
445+
{
446+
tool: "fuzzplugin_unreadable_description",
447+
violations: ["fuzzplugin_unreadable_description.description is unreadable"],
448+
},
449+
]);
450+
});
451+
374452
it("can expose all dynamic tools directly for compatibility", () => {
375453
const bridge = createCodexDynamicToolBridge({
376454
tools: [createTool({ name: "web_search" }), createTool({ name: "message" })],

‎extensions/codex/src/app-server/dynamic-tools.ts‎

Lines changed: 123 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,8 @@ type CodexToolResultHookContext = Omit<CodexDynamicToolHookContext, "config">;
5151

5252
type ProjectedCodexDynamicTool = {
5353
tool: AnyAgentTool;
54+
toolName: string;
55+
description: string;
5456
inputSchema: JsonValue;
5557
};
5658

@@ -59,6 +61,17 @@ type CodexDynamicToolSchemaQuarantine = {
5961
violations: readonly string[];
6062
};
6163

64+
type CodexDynamicToolEntryRead =
65+
| {
66+
readonly ok: true;
67+
readonly tool: AnyAgentTool;
68+
readonly toolIndex: number;
69+
}
70+
| {
71+
readonly ok: false;
72+
readonly quarantine: CodexDynamicToolSchemaQuarantine;
73+
};
74+
6275
export type CodexDynamicToolBridge = {
6376
availableSpecs: CodexDynamicToolSpec[];
6477
specs: CodexDynamicToolSpec[];
@@ -101,19 +114,22 @@ export function createCodexDynamicToolBridge(params: {
101114
const registeredProjection = params.registeredTools
102115
? projectCodexDynamicTools(params.registeredTools)
103116
: availableProjection;
104-
const availableTools = availableProjection.tools.map(({ tool, inputSchema }) => {
105-
if (isToolWrappedWithBeforeToolCallHook(tool)) {
106-
setBeforeToolCallDiagnosticsEnabled(tool, false);
107-
return { tool, inputSchema };
108-
}
109-
return {
110-
tool: wrapToolWithBeforeToolCallHook(tool, params.hookContext, { emitDiagnostics: false }),
111-
inputSchema,
112-
};
113-
});
114-
const toolMap = new Map(availableTools.map(({ tool }) => [tool.name, tool]));
115-
const registeredTools = registeredProjection.tools.map(({ tool }) => tool);
116-
const registeredToolNames = new Set(registeredTools.map((tool) => tool.name));
117+
const availableTools = availableProjection.tools.map(
118+
({ tool, toolName, description, inputSchema }) => {
119+
if (isToolWrappedWithBeforeToolCallHook(tool)) {
120+
setBeforeToolCallDiagnosticsEnabled(tool, false);
121+
return { tool, toolName, description, inputSchema };
122+
}
123+
return {
124+
tool: wrapToolWithBeforeToolCallHook(tool, params.hookContext, { emitDiagnostics: false }),
125+
toolName,
126+
description,
127+
inputSchema,
128+
};
129+
},
130+
);
131+
const toolMap = new Map(availableTools.map(({ toolName, tool }) => [toolName, tool]));
132+
const registeredToolNames = new Set(registeredProjection.tools.map((tool) => tool.toolName));
117133
const quarantinedTools = dedupeQuarantinedDynamicTools([
118134
...availableProjection.quarantinedTools,
119135
...registeredProjection.quarantinedTools,
@@ -142,17 +158,19 @@ export function createCodexDynamicToolBridge(params: {
142158
]);
143159

144160
return {
145-
availableSpecs: availableTools.map(({ tool, inputSchema }) =>
161+
availableSpecs: availableTools.map(({ toolName, description, inputSchema }) =>
146162
createCodexDynamicToolSpec({
147-
tool,
163+
toolName,
164+
description,
148165
inputSchema,
149166
loading: params.loading ?? "searchable",
150167
directToolNames,
151168
}),
152169
),
153-
specs: registeredProjection.tools.map(({ tool, inputSchema }) =>
170+
specs: registeredProjection.tools.map(({ toolName, description, inputSchema }) =>
154171
createCodexDynamicToolSpec({
155-
tool,
172+
toolName,
173+
description,
156174
inputSchema,
157175
loading: params.loading ?? "searchable",
158176
directToolNames,
@@ -286,17 +304,18 @@ export function createCodexDynamicToolBridge(params: {
286304
}
287305

288306
function createCodexDynamicToolSpec(params: {
289-
tool: AnyAgentTool;
307+
toolName: string;
308+
description: string;
290309
inputSchema: JsonValue;
291310
loading: CodexDynamicToolsLoading;
292311
directToolNames: ReadonlySet<string>;
293312
}): CodexDynamicToolSpec {
294313
const base = {
295-
name: params.tool.name,
296-
description: params.tool.description,
314+
name: params.toolName,
315+
description: params.description,
297316
inputSchema: params.inputSchema,
298317
};
299-
if (params.loading === "direct" || params.directToolNames.has(params.tool.name)) {
318+
if (params.loading === "direct" || params.directToolNames.has(params.toolName)) {
300319
return base;
301320
}
302321
return {
@@ -312,17 +331,96 @@ function projectCodexDynamicTools(tools: readonly AnyAgentTool[]): {
312331
} {
313332
const projectedTools: ProjectedCodexDynamicTool[] = [];
314333
const quarantinedTools: CodexDynamicToolSchemaQuarantine[] = [];
315-
for (const tool of tools) {
316-
const projection = projectRuntimeToolInputSchema(tool.parameters, `${tool.name}.inputSchema`);
334+
for (const entry of readCodexDynamicToolEntries(tools)) {
335+
if (!entry.ok) {
336+
quarantinedTools.push(entry.quarantine);
337+
continue;
338+
}
339+
340+
const nameRead = readCodexDynamicToolField(entry.tool, "name");
341+
if (!nameRead.readable) {
342+
const diagnosticToolName = `tool[${entry.toolIndex}]`;
343+
quarantinedTools.push({
344+
tool: diagnosticToolName,
345+
violations: [`${diagnosticToolName}.name is unreadable`],
346+
});
347+
continue;
348+
}
349+
const diagnosticToolName = nameRead.value;
350+
const descriptionRead = readCodexDynamicToolField(entry.tool, "description");
351+
if (!descriptionRead.readable) {
352+
quarantinedTools.push({
353+
tool: diagnosticToolName,
354+
violations: [`${diagnosticToolName}.description is unreadable`],
355+
});
356+
continue;
357+
}
358+
const parametersRead = readCodexDynamicToolField(entry.tool, "parameters");
359+
if (!parametersRead.readable) {
360+
quarantinedTools.push({
361+
tool: diagnosticToolName,
362+
violations: [`${diagnosticToolName}.inputSchema is unreadable`],
363+
});
364+
continue;
365+
}
366+
367+
const projection = projectRuntimeToolInputSchema(
368+
parametersRead.value,
369+
`${diagnosticToolName}.inputSchema`,
370+
);
317371
if (projection.violations.length > 0) {
318-
quarantinedTools.push({ tool: tool.name, violations: projection.violations });
372+
quarantinedTools.push({ tool: diagnosticToolName, violations: projection.violations });
319373
continue;
320374
}
321-
projectedTools.push({ tool, inputSchema: projection.schema as JsonValue });
375+
projectedTools.push({
376+
tool: entry.tool,
377+
toolName: diagnosticToolName,
378+
description: typeof descriptionRead.value === "string" ? descriptionRead.value : "",
379+
inputSchema: projection.schema as JsonValue,
380+
});
322381
}
323382
return { tools: projectedTools, quarantinedTools };
324383
}
325384

385+
function readCodexDynamicToolEntries(tools: readonly AnyAgentTool[]): CodexDynamicToolEntryRead[] {
386+
let length: number;
387+
try {
388+
length = tools.length;
389+
} catch {
390+
return [unreadableCodexDynamicToolEntry(0)];
391+
}
392+
const entries: CodexDynamicToolEntryRead[] = [];
393+
for (let toolIndex = 0; toolIndex < length; toolIndex += 1) {
394+
try {
395+
entries.push({ ok: true, tool: tools[toolIndex], toolIndex });
396+
} catch {
397+
entries.push(unreadableCodexDynamicToolEntry(toolIndex));
398+
}
399+
}
400+
return entries;
401+
}
402+
403+
function unreadableCodexDynamicToolEntry(toolIndex: number): CodexDynamicToolEntryRead {
404+
return {
405+
ok: false,
406+
quarantine: {
407+
tool: `tool[${toolIndex}]`,
408+
violations: [`tool[${toolIndex}] is unreadable`],
409+
},
410+
};
411+
}
412+
413+
function readCodexDynamicToolField<TField extends "name" | "description" | "parameters">(
414+
tool: AnyAgentTool,
415+
field: TField,
416+
): { readable: true; value: AnyAgentTool[TField] } | { readable: false } {
417+
try {
418+
return { readable: true, value: tool[field] };
419+
} catch {
420+
return { readable: false };
421+
}
422+
}
423+
326424
function warnQuarantinedDynamicTools(tools: readonly CodexDynamicToolSchemaQuarantine[]): void {
327425
if (tools.length === 0) {
328426
return;

0 commit comments

Comments
 (0)