Skip to content

Commit 3662c8c

Browse files
vincentkocvincentkoc
committed
fix(agents): contain provider schema hook failures
1 parent 8f1ae59 commit 3662c8c

5 files changed

Lines changed: 261 additions & 28 deletions

File tree

‎src/agents/embedded-agent-runner/tool-schema-runtime.test.ts‎

Lines changed: 96 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { describe, expect, it, vi } from "vitest";
1+
import { beforeEach, describe, expect, it, vi } from "vitest";
22

33
const mocks = vi.hoisted(() => ({
44
inspectProviderToolSchemasWithPlugin: vi.fn(),
@@ -22,6 +22,10 @@ const { logProviderToolSchemaDiagnostics, normalizeProviderToolSchemas } =
2222
await import("./tool-schema-runtime.js");
2323

2424
describe("tool schema runtime diagnostics", () => {
25+
beforeEach(() => {
26+
vi.clearAllMocks();
27+
});
28+
2529
it("stays quiet when a provider reports no diagnostics", () => {
2630
mocks.inspectProviderToolSchemasWithPlugin.mockReturnValueOnce([]);
2731

@@ -57,6 +61,42 @@ describe("tool schema runtime diagnostics", () => {
5761
);
5862
});
5963

64+
it("keeps original tools when provider schema normalization throws", () => {
65+
const tools = [{ name: "alpha" }] as never;
66+
mocks.normalizeProviderToolSchemasWithPlugin.mockImplementationOnce(() => {
67+
throw new Error("normalizer exploded");
68+
});
69+
70+
expect(
71+
normalizeProviderToolSchemas({
72+
provider: "example",
73+
tools,
74+
}),
75+
).toBe(tools);
76+
77+
expect(mocks.log.warn).toHaveBeenCalledWith(
78+
"provider tool schema normalization failed for example; keeping original tool schemas",
79+
{ provider: "example", toolCount: 1, error: "normalizer exploded" },
80+
);
81+
});
82+
83+
it("rethrows provider schema normalization errors when requested", () => {
84+
const tools = [{ name: "alpha" }] as never;
85+
mocks.normalizeProviderToolSchemasWithPlugin.mockImplementationOnce(() => {
86+
throw new Error("normalizer exploded");
87+
});
88+
89+
expect(() =>
90+
normalizeProviderToolSchemas({
91+
provider: "example",
92+
tools,
93+
throwOnProviderToolSchemaError: true,
94+
}),
95+
).toThrow("normalizer exploded");
96+
97+
expect(mocks.log.warn).not.toHaveBeenCalled();
98+
});
99+
60100
it("logs one summarized warning for provider tool schema diagnostics", () => {
61101
mocks.inspectProviderToolSchemasWithPlugin.mockReturnValueOnce([
62102
{ toolName: "alpha", toolIndex: 0, violations: ["one", "two"] },
@@ -84,4 +124,59 @@ describe("tool schema runtime diagnostics", () => {
84124
},
85125
);
86126
});
127+
128+
it("does not throw when provider schema diagnostics throw or are malformed", () => {
129+
mocks.inspectProviderToolSchemasWithPlugin.mockImplementationOnce(() => {
130+
throw new Error("inspector exploded");
131+
});
132+
133+
expect(() =>
134+
logProviderToolSchemaDiagnostics({
135+
provider: "example",
136+
tools: [{ name: "alpha" }] as never,
137+
}),
138+
).not.toThrow();
139+
140+
expect(mocks.log.warn).toHaveBeenCalledWith(
141+
"provider tool schema diagnostics failed for example",
142+
{ provider: "example", toolCount: 1, error: "inspector exploded" },
143+
);
144+
145+
const malformedDiagnostics = [
146+
{
147+
get toolName() {
148+
throw new Error("tool name exploded");
149+
},
150+
get violations() {
151+
throw new Error("violations exploded");
152+
},
153+
},
154+
];
155+
mocks.inspectProviderToolSchemasWithPlugin.mockReturnValueOnce(malformedDiagnostics);
156+
157+
expect(() =>
158+
logProviderToolSchemaDiagnostics({
159+
provider: "example",
160+
tools: [{ name: "alpha" }] as never,
161+
}),
162+
).not.toThrow();
163+
164+
expect(mocks.log.warn).toHaveBeenLastCalledWith(
165+
"provider tool schema diagnostics: 1 tool for example: unknown (1 violation)",
166+
{
167+
provider: "example",
168+
toolCount: 1,
169+
diagnosticCount: 1,
170+
tools: ["0:alpha"],
171+
diagnostics: [
172+
{
173+
index: undefined,
174+
tool: "unknown",
175+
violations: ["diagnostic is unreadable"],
176+
violationCount: 1,
177+
},
178+
],
179+
},
180+
);
181+
});
87182
});

‎src/agents/embedded-agent-runner/tool-schema-runtime.ts‎

Lines changed: 158 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,13 @@ type ProviderToolSchemaParams<TSchemaType extends TSchema = TSchema, TResult = u
2222
model?: ProviderRuntimeModel;
2323
runtimeHandle?: ProviderRuntimePluginHandle;
2424
allowRuntimePluginLoad?: boolean;
25+
throwOnProviderToolSchemaError?: boolean;
26+
};
27+
28+
type ReadableProviderToolSchemaDiagnostic = {
29+
toolName: string;
30+
toolIndex?: number;
31+
violations: string[];
2532
};
2633

2734
function buildProviderToolSchemaContext<TSchemaType extends TSchema = TSchema, TResult = unknown>(
@@ -49,15 +56,27 @@ export function normalizeProviderToolSchemas<
4956
TResult = unknown,
5057
>(params: ProviderToolSchemaParams<TSchemaType, TResult>): AgentTool<TSchemaType, TResult>[] {
5158
const provider = params.provider.trim();
52-
const pluginNormalized = normalizeProviderToolSchemasWithPlugin({
53-
provider,
54-
config: params.config,
55-
workspaceDir: params.workspaceDir,
56-
env: params.env,
57-
runtimeHandle: params.runtimeHandle,
58-
allowRuntimePluginLoad: params.allowRuntimePluginLoad,
59-
context: buildProviderToolSchemaContext(params, provider),
60-
});
59+
let pluginNormalized: unknown;
60+
try {
61+
pluginNormalized = normalizeProviderToolSchemasWithPlugin({
62+
provider,
63+
config: params.config,
64+
workspaceDir: params.workspaceDir,
65+
env: params.env,
66+
runtimeHandle: params.runtimeHandle,
67+
allowRuntimePluginLoad: params.allowRuntimePluginLoad,
68+
context: buildProviderToolSchemaContext(params, provider),
69+
});
70+
} catch (error) {
71+
if (params.throwOnProviderToolSchemaError) {
72+
throw error;
73+
}
74+
log.warn(
75+
`provider tool schema normalization failed for ${provider}; keeping original tool schemas`,
76+
{ provider, toolCount: readToolCount(params.tools), error: describeProviderHookError(error) },
77+
);
78+
return params.tools;
79+
}
6180
return Array.isArray(pluginNormalized)
6281
? (pluginNormalized as AgentTool<TSchemaType, TResult>[])
6382
: params.tools;
@@ -68,31 +87,42 @@ export function normalizeProviderToolSchemas<
6887
*/
6988
export function logProviderToolSchemaDiagnostics(params: ProviderToolSchemaParams): void {
7089
const provider = params.provider.trim();
71-
const diagnostics = inspectProviderToolSchemasWithPlugin({
72-
provider,
73-
config: params.config,
74-
workspaceDir: params.workspaceDir,
75-
env: params.env,
76-
runtimeHandle: params.runtimeHandle,
77-
allowRuntimePluginLoad: params.allowRuntimePluginLoad,
78-
context: buildProviderToolSchemaContext(params, provider),
79-
});
90+
let diagnostics: unknown;
91+
try {
92+
diagnostics = inspectProviderToolSchemasWithPlugin({
93+
provider,
94+
config: params.config,
95+
workspaceDir: params.workspaceDir,
96+
env: params.env,
97+
runtimeHandle: params.runtimeHandle,
98+
allowRuntimePluginLoad: params.allowRuntimePluginLoad,
99+
context: buildProviderToolSchemaContext(params, provider),
100+
});
101+
} catch (error) {
102+
log.warn(`provider tool schema diagnostics failed for ${provider}`, {
103+
provider,
104+
toolCount: readToolCount(params.tools),
105+
error: describeProviderHookError(error),
106+
});
107+
return;
108+
}
80109
if (!Array.isArray(diagnostics)) {
81110
return;
82111
}
83-
if (diagnostics.length === 0) {
112+
const readableDiagnostics = readProviderToolSchemaDiagnostics(diagnostics);
113+
if (readableDiagnostics.length === 0) {
84114
return;
85115
}
86116

87-
const summary = summarizeProviderToolSchemaDiagnostics(diagnostics);
117+
const summary = summarizeProviderToolSchemaDiagnostics(readableDiagnostics);
88118
log.warn(
89-
`provider tool schema diagnostics: ${diagnostics.length} ${diagnostics.length === 1 ? "tool" : "tools"} for ${params.provider}: ${summary}`,
119+
`provider tool schema diagnostics: ${readableDiagnostics.length} ${readableDiagnostics.length === 1 ? "tool" : "tools"} for ${provider}: ${summary}`,
90120
{
91-
provider: params.provider,
92-
toolCount: params.tools.length,
93-
diagnosticCount: diagnostics.length,
94-
tools: params.tools.map((tool, index) => `${index}:${tool.name}`),
95-
diagnostics: diagnostics.map((diagnostic) => ({
121+
provider,
122+
toolCount: readToolCount(params.tools),
123+
diagnosticCount: readableDiagnostics.length,
124+
tools: summarizeToolNames(params.tools),
125+
diagnostics: readableDiagnostics.map((diagnostic) => ({
96126
index: diagnostic.toolIndex,
97127
tool: diagnostic.toolName,
98128
violations: diagnostic.violations.slice(0, 12),
@@ -103,7 +133,7 @@ export function logProviderToolSchemaDiagnostics(params: ProviderToolSchemaParam
103133
}
104134

105135
function summarizeProviderToolSchemaDiagnostics(
106-
diagnostics: readonly ProviderToolSchemaDiagnostic[],
136+
diagnostics: readonly ReadableProviderToolSchemaDiagnostic[],
107137
) {
108138
const visible = diagnostics.slice(0, 6).map((diagnostic) => {
109139
const violationCount = diagnostic.violations.length;
@@ -112,3 +142,104 @@ function summarizeProviderToolSchemaDiagnostics(
112142
const remaining = diagnostics.length - visible.length;
113143
return remaining > 0 ? `${visible.join(", ")}, +${remaining} more` : visible.join(", ");
114144
}
145+
146+
function describeProviderHookError(error: unknown): string {
147+
if (error instanceof Error && error.message) {
148+
return error.message;
149+
}
150+
return String(error);
151+
}
152+
153+
function readToolCount(tools: readonly unknown[]): number {
154+
try {
155+
return tools.length;
156+
} catch {
157+
return 0;
158+
}
159+
}
160+
161+
function summarizeToolNames(tools: readonly AnyAgentTool[]): string[] {
162+
const count = readToolCount(tools);
163+
const names: string[] = [];
164+
for (let index = 0; index < count; index += 1) {
165+
try {
166+
const name = normalizeDiagnosticText(tools[index]?.name) ?? "unknown";
167+
names.push(`${index}:${name}`);
168+
} catch {
169+
names.push(`${index}:unknown`);
170+
}
171+
}
172+
return names;
173+
}
174+
175+
function readProviderToolSchemaDiagnostics(
176+
diagnostics: readonly ProviderToolSchemaDiagnostic[],
177+
): ReadableProviderToolSchemaDiagnostic[] {
178+
let count: number;
179+
try {
180+
count = diagnostics.length;
181+
} catch {
182+
return [{ toolName: "unknown", violations: ["diagnostics are unreadable"] }];
183+
}
184+
const readableDiagnostics: ReadableProviderToolSchemaDiagnostic[] = [];
185+
for (let index = 0; index < count; index += 1) {
186+
try {
187+
readableDiagnostics.push(readProviderToolSchemaDiagnostic(diagnostics[index]));
188+
} catch {
189+
readableDiagnostics.push({
190+
toolName: "unknown",
191+
toolIndex: index,
192+
violations: ["diagnostic is unreadable"],
193+
});
194+
}
195+
}
196+
return readableDiagnostics;
197+
}
198+
199+
function readProviderToolSchemaDiagnostic(
200+
diagnostic: ProviderToolSchemaDiagnostic,
201+
): ReadableProviderToolSchemaDiagnostic {
202+
const readableDiagnostic: ReadableProviderToolSchemaDiagnostic = {
203+
toolName: "unknown",
204+
violations: ["diagnostic is unreadable"],
205+
};
206+
try {
207+
readableDiagnostic.toolName = normalizeDiagnosticText(diagnostic.toolName) ?? "unknown";
208+
} catch {
209+
return readableDiagnostic;
210+
}
211+
try {
212+
if (typeof diagnostic.toolIndex === "number" && Number.isInteger(diagnostic.toolIndex)) {
213+
readableDiagnostic.toolIndex = diagnostic.toolIndex;
214+
}
215+
} catch {
216+
// Keep the diagnostic visible even if optional metadata is hostile.
217+
}
218+
try {
219+
if (Array.isArray(diagnostic.violations)) {
220+
readableDiagnostic.violations = normalizeViolationTexts(diagnostic.violations);
221+
}
222+
} catch {
223+
readableDiagnostic.violations = ["diagnostic is unreadable"];
224+
}
225+
return readableDiagnostic;
226+
}
227+
228+
function normalizeViolationTexts(violations: readonly unknown[]): string[] {
229+
const normalized: string[] = [];
230+
for (const entry of violations) {
231+
const violation = normalizeDiagnosticText(entry);
232+
if (violation) {
233+
normalized.push(violation);
234+
}
235+
}
236+
return normalized.length > 0 ? normalized : ["diagnostic has no readable violations"];
237+
}
238+
239+
function normalizeDiagnosticText(value: unknown): string | undefined {
240+
if (typeof value === "string") {
241+
const trimmed = value.trim();
242+
return trimmed.length > 0 ? trimmed : undefined;
243+
}
244+
return undefined;
245+
}

‎src/agents/runtime-plan/tools.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ type AgentRuntimeToolPolicyParams<TSchemaType extends TSchema = TSchema, TResult
2727
model?: ProviderRuntimeModel;
2828
runtimeHandle?: ProviderRuntimePluginHandle;
2929
allowProviderRuntimePluginLoad?: boolean;
30+
throwOnProviderToolSchemaError?: boolean;
3031
onPreNormalizationSchemaDiagnostics?: (
3132
diagnostics: readonly RuntimeToolSchemaDiagnostic[],
3233
tools: readonly AgentTool<TSchemaType, TResult>[],
@@ -110,6 +111,9 @@ export function normalizeAgentRuntimeTools<
110111
model: params.model,
111112
runtimeHandle: params.runtimeHandle,
112113
allowRuntimePluginLoad: params.allowProviderRuntimePluginLoad,
114+
...(params.throwOnProviderToolSchemaError !== undefined
115+
? { throwOnProviderToolSchemaError: params.throwOnProviderToolSchemaError }
116+
: {}),
113117
});
114118
const normalizedTools = Array.isArray(normalized) ? normalized : normalizableTools;
115119
return preserveRuntimeToolMetadata(normalizableTools, normalizedTools);

‎src/commands/doctor/shared/active-tool-schema-warnings.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,7 @@ export function collectActiveToolSchemaProjectionWarnings(params: {
182182
modelId: modelRef.model,
183183
modelApi: runtimeModelContext.modelApi,
184184
model: runtimeModelContext.model,
185+
throwOnProviderToolSchemaError: true,
185186
onPreNormalizationSchemaDiagnostics: (diagnostics) =>
186187
preNormalizationDiagnostics.push(...diagnostics),
187188
});

‎src/flows/doctor-core-checks.runtime.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -620,6 +620,7 @@ function collectBundleMcpRuntimeToolSchemaFindings(params: {
620620
modelId: params.modelRef.model,
621621
modelApi: params.model.api,
622622
model: params.model,
623+
throwOnProviderToolSchemaError: true,
623624
onPreNormalizationSchemaDiagnostics: (diagnostics, sourceTools) => {
624625
preNormalizationFindings.push(
625626
...diagnostics.map((diagnostic) =>
@@ -714,6 +715,7 @@ function collectAgentRuntimeToolSchemaFindings(params: {
714715
modelId: params.modelRef.model,
715716
modelApi: params.model.api,
716717
model: params.model,
718+
throwOnProviderToolSchemaError: true,
717719
onPreNormalizationSchemaDiagnostics: (diagnostics, sourceTools) => {
718720
preNormalizationFindings.push(
719721
...diagnostics.map((diagnostic) =>

0 commit comments

Comments
 (0)