Skip to content

Commit a33d86e

Browse files
vincentkocvincentkoc
committed
fix(codex): quarantine unreadable dynamic tool descriptors
1 parent abc3fa0 commit a33d86e

2 files changed

Lines changed: 226 additions & 21 deletions

File tree

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

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

374+
it("quarantines unreadable dynamic tool descriptors before bridge registration", async () => {
375+
const warn = vi.spyOn(embeddedAgentLog, "warn").mockImplementation(() => undefined);
376+
const unreadableName = createTool({ name: "fuzzplugin_unreadable_name" });
377+
Object.defineProperty(unreadableName, "name", {
378+
get() {
379+
throw new Error("fuzzplugin dynamic tool name is unreadable");
380+
},
381+
});
382+
const unreadableParameters = createTool({ name: "fuzzplugin_unreadable_parameters" });
383+
Object.defineProperty(unreadableParameters, "parameters", {
384+
get() {
385+
throw new Error("fuzzplugin dynamic tool schema is unreadable");
386+
},
387+
});
388+
const unreadableDescription = createTool({ name: "fuzzplugin_unreadable_description" });
389+
Object.defineProperty(unreadableDescription, "description", {
390+
get() {
391+
throw new Error("fuzzplugin dynamic tool description is unreadable");
392+
},
393+
});
394+
let flakyNameReads = 0;
395+
const flakyName = createTool({ name: "fuzzplugin_flaky_name" });
396+
Object.defineProperty(flakyName, "name", {
397+
get() {
398+
flakyNameReads += 1;
399+
if (flakyNameReads > 1) {
400+
throw new Error("fuzzplugin dynamic tool name was reread");
401+
}
402+
return "fuzzplugin_flaky_name";
403+
},
404+
});
405+
406+
const bridge = createCodexDynamicToolBridge({
407+
tools: [unreadableName, createTool({ name: "message" }), unreadableParameters],
408+
registeredTools: [
409+
unreadableName,
410+
createTool({ name: "message" }),
411+
unreadableParameters,
412+
unreadableDescription,
413+
flakyName,
414+
],
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", "fuzzplugin_flaky_name"]);
420+
expect(bridge.telemetry.quarantinedTools).toEqual([
421+
{
422+
tool: "tool[0]",
423+
violations: ["tool[0].name is unreadable"],
424+
},
425+
{
426+
tool: "fuzzplugin_unreadable_parameters",
427+
violations: ["fuzzplugin_unreadable_parameters.inputSchema is unreadable"],
428+
},
429+
{
430+
tool: "fuzzplugin_unreadable_description",
431+
violations: ["fuzzplugin_unreadable_description.description is unreadable"],
432+
},
433+
]);
434+
expect(warn).toHaveBeenCalledWith(
435+
expect.stringContaining(
436+
"tool[0], fuzzplugin_unreadable_parameters, fuzzplugin_unreadable_description",
437+
),
438+
expect.objectContaining({
439+
tools: [
440+
{
441+
tool: "tool[0]",
442+
violations: ["tool[0].name is unreadable"],
443+
},
444+
{
445+
tool: "fuzzplugin_unreadable_parameters",
446+
violations: ["fuzzplugin_unreadable_parameters.inputSchema is unreadable"],
447+
},
448+
{
449+
tool: "fuzzplugin_unreadable_description",
450+
violations: ["fuzzplugin_unreadable_description.description is unreadable"],
451+
},
452+
],
453+
}),
454+
);
455+
expect(flakyNameReads).toBe(1);
456+
});
457+
374458
it("can expose all dynamic tools directly for compatibility", () => {
375459
const bridge = createCodexDynamicToolBridge({
376460
tools: [createTool({ name: "web_search" }), createTool({ name: "message" })],

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

Lines changed: 142 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,11 @@ type CodexToolResultHookContext = Omit<CodexDynamicToolHookContext, "config">;
5151

5252
type ProjectedCodexDynamicTool = {
5353
tool: AnyAgentTool;
54+
sourceTool: AnyAgentTool;
55+
name: string;
56+
description: string;
5457
inputSchema: JsonValue;
58+
beforeToolCallWrapped: boolean;
5559
};
5660

5761
type CodexDynamicToolSchemaQuarantine = {
@@ -101,19 +105,20 @@ export function createCodexDynamicToolBridge(params: {
101105
const registeredProjection = params.registeredTools
102106
? projectCodexDynamicTools(params.registeredTools)
103107
: availableProjection;
104-
const availableTools = availableProjection.tools.map(({ tool, inputSchema }) => {
105-
if (isToolWrappedWithBeforeToolCallHook(tool)) {
106-
setBeforeToolCallDiagnosticsEnabled(tool, false);
107-
return { tool, inputSchema };
108+
const availableTools = availableProjection.tools.map((projectedTool) => {
109+
if (projectedTool.beforeToolCallWrapped) {
110+
setBeforeToolCallDiagnosticsEnabled(projectedTool.sourceTool, false);
111+
return projectedTool;
108112
}
109113
return {
110-
tool: wrapToolWithBeforeToolCallHook(tool, params.hookContext, { emitDiagnostics: false }),
111-
inputSchema,
114+
...projectedTool,
115+
tool: wrapToolWithBeforeToolCallHook(projectedTool.tool, params.hookContext, {
116+
emitDiagnostics: false,
117+
}),
112118
};
113119
});
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));
120+
const toolMap = new Map(availableTools.map(({ name, tool }) => [name, tool]));
121+
const registeredToolNames = new Set(registeredProjection.tools.map((tool) => tool.name));
117122
const quarantinedTools = dedupeQuarantinedDynamicTools([
118123
...availableProjection.quarantinedTools,
119124
...registeredProjection.quarantinedTools,
@@ -142,17 +147,19 @@ export function createCodexDynamicToolBridge(params: {
142147
]);
143148

144149
return {
145-
availableSpecs: availableTools.map(({ tool, inputSchema }) =>
150+
availableSpecs: availableTools.map(({ name, description, inputSchema }) =>
146151
createCodexDynamicToolSpec({
147-
tool,
152+
name,
153+
description,
148154
inputSchema,
149155
loading: params.loading ?? "searchable",
150156
directToolNames,
151157
}),
152158
),
153-
specs: registeredProjection.tools.map(({ tool, inputSchema }) =>
159+
specs: registeredProjection.tools.map(({ name, description, inputSchema }) =>
154160
createCodexDynamicToolSpec({
155-
tool,
161+
name,
162+
description,
156163
inputSchema,
157164
loading: params.loading ?? "searchable",
158165
directToolNames,
@@ -286,17 +293,18 @@ export function createCodexDynamicToolBridge(params: {
286293
}
287294

288295
function createCodexDynamicToolSpec(params: {
289-
tool: AnyAgentTool;
296+
name: string;
297+
description: string;
290298
inputSchema: JsonValue;
291299
loading: CodexDynamicToolsLoading;
292300
directToolNames: ReadonlySet<string>;
293301
}): CodexDynamicToolSpec {
294302
const base = {
295-
name: params.tool.name,
296-
description: params.tool.description,
303+
name: params.name,
304+
description: params.description,
297305
inputSchema: params.inputSchema,
298306
};
299-
if (params.loading === "direct" || params.directToolNames.has(params.tool.name)) {
307+
if (params.loading === "direct" || params.directToolNames.has(params.name)) {
300308
return base;
301309
}
302310
return {
@@ -312,17 +320,130 @@ function projectCodexDynamicTools(tools: readonly AnyAgentTool[]): {
312320
} {
313321
const projectedTools: ProjectedCodexDynamicTool[] = [];
314322
const quarantinedTools: CodexDynamicToolSchemaQuarantine[] = [];
315-
for (const tool of tools) {
316-
const projection = projectRuntimeToolInputSchema(tool.parameters, `${tool.name}.inputSchema`);
323+
let toolCount: number;
324+
try {
325+
toolCount = tools.length;
326+
} catch {
327+
return {
328+
tools: projectedTools,
329+
quarantinedTools: [{ tool: "tool[0]", violations: ["tool[0] is unreadable"] }],
330+
};
331+
}
332+
for (let toolIndex = 0; toolIndex < toolCount; toolIndex += 1) {
333+
let tool: AnyAgentTool;
334+
try {
335+
const candidate = tools[toolIndex];
336+
if (!candidate) {
337+
throw new Error("missing dynamic tool entry");
338+
}
339+
tool = candidate;
340+
} catch {
341+
const toolName = `tool[${toolIndex}]`;
342+
quarantinedTools.push({ tool: toolName, violations: [`${toolName} is unreadable`] });
343+
continue;
344+
}
345+
346+
const nameRead = readDynamicToolDescriptorField(tool, "name");
347+
const toolName =
348+
nameRead.readable && typeof nameRead.value === "string" && nameRead.value
349+
? nameRead.value
350+
: `tool[${toolIndex}]`;
351+
const descriptorViolations = nameRead.readable ? [] : [`${toolName}.name is unreadable`];
352+
const parametersRead = readDynamicToolDescriptorField(tool, "parameters");
353+
if (!parametersRead.readable) {
354+
quarantinedTools.push({
355+
tool: toolName,
356+
violations: [...descriptorViolations, `${toolName}.inputSchema is unreadable`],
357+
});
358+
continue;
359+
}
360+
if (descriptorViolations.length > 0) {
361+
quarantinedTools.push({ tool: toolName, violations: descriptorViolations });
362+
continue;
363+
}
364+
const descriptionRead = readDynamicToolDescriptorField(tool, "description");
365+
if (!descriptionRead.readable) {
366+
quarantinedTools.push({
367+
tool: toolName,
368+
violations: [`${toolName}.description is unreadable`],
369+
});
370+
continue;
371+
}
372+
if (typeof descriptionRead.value !== "string") {
373+
quarantinedTools.push({
374+
tool: toolName,
375+
violations: [`${toolName}.description must be a string`],
376+
});
377+
continue;
378+
}
379+
380+
const projection = projectRuntimeToolInputSchema(
381+
parametersRead.value,
382+
`${toolName}.inputSchema`,
383+
);
317384
if (projection.violations.length > 0) {
318-
quarantinedTools.push({ tool: tool.name, violations: projection.violations });
385+
quarantinedTools.push({
386+
tool: toolName,
387+
violations: [...descriptorViolations, ...projection.violations],
388+
});
389+
continue;
390+
}
391+
const executeRead = readDynamicToolDescriptorField(tool, "execute");
392+
if (!executeRead.readable || typeof executeRead.value !== "function") {
393+
quarantinedTools.push({ tool: toolName, violations: [`${toolName}.execute is unreadable`] });
319394
continue;
320395
}
321-
projectedTools.push({ tool, inputSchema: projection.schema as JsonValue });
396+
const prepareArgumentsRead = readDynamicToolDescriptorField(tool, "prepareArguments");
397+
if (!prepareArgumentsRead.readable) {
398+
quarantinedTools.push({
399+
tool: toolName,
400+
violations: [`${toolName}.prepareArguments is unreadable`],
401+
});
402+
continue;
403+
}
404+
const labelRead = readDynamicToolDescriptorField(tool, "label");
405+
const displaySummaryRead = readDynamicToolDescriptorField(tool, "displaySummary");
406+
const executionModeRead = readDynamicToolDescriptorField(tool, "executionMode");
407+
const snapshotTool: AnyAgentTool = {
408+
name: toolName,
409+
label: labelRead.readable && typeof labelRead.value === "string" ? labelRead.value : toolName,
410+
description: descriptionRead.value,
411+
parameters: parametersRead.value,
412+
execute: executeRead.value,
413+
...(prepareArgumentsRead.readable && typeof prepareArgumentsRead.value === "function"
414+
? { prepareArguments: prepareArgumentsRead.value }
415+
: {}),
416+
...(executionModeRead.readable &&
417+
(executionModeRead.value === "parallel" || executionModeRead.value === "sequential")
418+
? { executionMode: executionModeRead.value }
419+
: {}),
420+
...(displaySummaryRead.readable && typeof displaySummaryRead.value === "string"
421+
? { displaySummary: displaySummaryRead.value }
422+
: {}),
423+
} as AnyAgentTool;
424+
projectedTools.push({
425+
tool: snapshotTool,
426+
sourceTool: tool,
427+
name: toolName,
428+
description: descriptionRead.value,
429+
inputSchema: projection.schema as JsonValue,
430+
beforeToolCallWrapped: isToolWrappedWithBeforeToolCallHook(tool),
431+
});
322432
}
323433
return { tools: projectedTools, quarantinedTools };
324434
}
325435

436+
function readDynamicToolDescriptorField<TField extends keyof AnyAgentTool>(
437+
tool: AnyAgentTool,
438+
field: TField,
439+
): { readable: true; value: AnyAgentTool[TField] } | { readable: false } {
440+
try {
441+
return { readable: true, value: tool[field] };
442+
} catch {
443+
return { readable: false };
444+
}
445+
}
446+
326447
function warnQuarantinedDynamicTools(tools: readonly CodexDynamicToolSchemaQuarantine[]): void {
327448
if (tools.length === 0) {
328449
return;

0 commit comments

Comments
 (0)