Skip to content

Commit 49fe87d

Browse files
zw-xyskzw-xysk
committed
fix(mcp): schema-aware null stripping for MCP tool calls
Improve the MCP null-argument fix to be schema-aware: - Only strip null values for fields NOT declared as nullable in the tool's input schema (handles type:null, type:[...,null], anyOf/oneOf with null variant, nullable:true) - Preserves null for schema-meaningful nulls (anyOf [string, null]) This approach is based on competitor PR #96729 (maweibin) which correctly handles the distinction between nullable and non-nullable fields at the MCP boundary. Fixes #96716 Signed-off-by: 赵旺0668001248 <[email protected]>
1 parent c63de8c commit 49fe87d

2 files changed

Lines changed: 150 additions & 12 deletions

File tree

‎src/agents/agent-bundle-mcp-materialize.ts‎

Lines changed: 65 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,10 @@ function mcpContentBlockToToolResult(block: ContentBlock): ToolResultContentBloc
4242
return { type: "text", text: `[audio ${block.mimeType}]` };
4343
case "resource_link": {
4444
const label = block.title ?? block.name;
45-
return { type: "text", text: label ? `[${label}] ${block.uri}` : block.uri };
45+
return {
46+
type: "text",
47+
text: label ? `[${label}] ${block.uri}` : block.uri,
48+
};
4649
}
4750
case "resource": {
4851
const resource = block.resource;
@@ -224,6 +227,58 @@ function addMcpUtilityTool(params: {
224227
params.tools.push(agentTool);
225228
}
226229

230+
/**
231+
* Checks whether a field declared in a JSON Schema object explicitly allows null.
232+
* Handles type: "null", type: ["string", "null"], and anyOf/oneOf with {type: "null"}.
233+
*/
234+
function isFieldNullableInSchema(schema: unknown, key: string): boolean {
235+
if (!schema || typeof schema !== "object") {
236+
return false;
237+
}
238+
const root = schema as Record<string, unknown>;
239+
const properties = root.properties as Record<string, unknown> | undefined;
240+
if (!properties) {
241+
return false;
242+
}
243+
const propertySchema = properties[key];
244+
if (!propertySchema || typeof propertySchema !== "object") {
245+
return false;
246+
}
247+
const ps = propertySchema as Record<string, unknown>;
248+
249+
// Direct type: null or ["...", "null"]
250+
if (ps.type === "null") {
251+
return true;
252+
}
253+
if (Array.isArray(ps.type) && ps.type.includes("null")) {
254+
return true;
255+
}
256+
257+
// nullable: true (OpenAPI extension)
258+
if (ps.nullable === true) {
259+
return true;
260+
}
261+
262+
// anyOf/oneOf with a null variant
263+
for (const unionKey of ["anyOf", "oneOf"] as const) {
264+
const union = ps[unionKey];
265+
if (!Array.isArray(union)) {
266+
continue;
267+
}
268+
if (
269+
union.some((v: unknown) => {
270+
if (!v || typeof v !== "object") return false;
271+
const entry = v as Record<string, unknown>;
272+
return entry.type === "null" || (Array.isArray(entry.type) && entry.type.includes("null"));
273+
})
274+
) {
275+
return true;
276+
}
277+
}
278+
279+
return false;
280+
}
281+
227282
/**
228283
* Projects an already-listed MCP catalog into agent tools. Without `createExecute`,
229284
* the projected tools are inventory-only and throw if execution is attempted.
@@ -403,12 +458,18 @@ export async function materializeBundleMcpToolsForRun(params: {
403458
reservedToolNames: params.reservedToolNames,
404459
createExecute: (tool) => async (_toolCallId: string, input: unknown) => {
405460
params.runtime.markUsed();
406-
// Strip null values from optional arguments — MCP servers may reject null
407-
// where they accept the same argument as absent or undefined.
461+
// Strip null-valued optional arguments before calling MCP. The LLM
462+
// often sends null for unset optional params, but some MCP servers
463+
// treat JSON null differently from an absent key (e.g. coercing null
464+
// to ""). Only strip nulls for fields NOT declared as nullable in the
465+
// tool's input schema, so schema-meaningful nulls (anyOf [string, null])
466+
// are preserved at the boundary.
408467
const cleaned =
409468
input && typeof input === "object" && !Array.isArray(input)
410469
? Object.fromEntries(
411-
Object.entries(input as Record<string, unknown>).filter(([, v]) => v !== null),
470+
Object.entries(input as Record<string, unknown>).filter(
471+
([k, v]) => v !== null || isFieldNullableInSchema(tool.inputSchema, k),
472+
),
412473
)
413474
: input;
414475
const result = await params.runtime.callTool(tool.serverName, tool.toolName, cleaned);

‎src/agents/agent-bundle-mcp-tools.materialize.test.ts‎

Lines changed: 85 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ function makeToolRuntime(
2626
resultText?: string;
2727
diagnostics?: readonly McpToolCatalogDiagnostic[];
2828
supportsParallelToolCalls?: boolean;
29+
callTool?: SessionMcpRuntime["callTool"];
2930
} = {},
3031
): SessionMcpRuntime {
3132
const serverName = params.serverName ?? "bundleProbe";
@@ -74,11 +75,13 @@ function makeToolRuntime(
7475
tools,
7576
...(params.diagnostics ? { diagnostics: params.diagnostics } : {}),
7677
}),
77-
callTool: async () =>
78-
params.result ?? {
79-
content: [{ type: "text", text: params.resultText ?? "FROM-BUNDLE" }],
80-
isError: false,
81-
},
78+
callTool:
79+
params.callTool ??
80+
(async () =>
81+
params.result ?? {
82+
content: [{ type: "text", text: params.resultText ?? "FROM-BUNDLE" }],
83+
isError: false,
84+
}),
8285
dispose: async () => {},
8386
};
8487
}
@@ -182,7 +185,11 @@ describe("createBundleMcpToolRuntime", () => {
182185
},
183186
{
184187
type: "resource",
185-
resource: { uri: "blob://two", blob: "AAAA", mimeType: "application/pdf" },
188+
resource: {
189+
uri: "blob://two",
190+
blob: "AAAA",
191+
mimeType: "application/pdf",
192+
},
186193
},
187194
{ type: "audio", data: "AAAA", mimeType: "audio/mpeg" },
188195
{ type: "image", data: "iVBOR", mimeType: "image/png" },
@@ -219,7 +226,10 @@ describe("createBundleMcpToolRuntime", () => {
219226
const result = await runtime.tools[0].execute("call-bundle-probe", {}, undefined, undefined);
220227

221228
expect(result.content).toHaveLength(1);
222-
expect(result.content[0]).toEqual({ type: "text", text: JSON.stringify({ type: "image" }) });
229+
expect(result.content[0]).toEqual({
230+
type: "text",
231+
text: JSON.stringify({ type: "image" }),
232+
});
223233
});
224234

225235
it("disambiguates bundle MCP tools that collide with existing tool names", async () => {
@@ -322,7 +332,10 @@ describe("createBundleMcpToolRuntime", () => {
322332
toolCount: 0,
323333
resources: { listChanged: false },
324334
prompts: { listChanged: false },
325-
toolFilter: { include: ["resources_*"], exclude: ["resources_read"] },
335+
toolFilter: {
336+
include: ["resources_*"],
337+
exclude: ["resources_read"],
338+
},
326339
},
327340
},
328341
tools: [],
@@ -521,4 +534,68 @@ describe("createBundleMcpToolRuntime", () => {
521534
}),
522535
).toEqual({ parent: { page_id: "page-id" } });
523536
});
537+
538+
it("strips null for non-nullable fields, preserves null for nullable fields", async () => {
539+
const callToolArgs: unknown[] = [];
540+
const runtime = await materializeBundleMcpToolsForRun({
541+
runtime: makeToolRuntime({
542+
tools: [
543+
{
544+
serverName: "eks",
545+
safeServerName: "eks",
546+
toolName: "describe_cluster",
547+
description: "Describe cluster",
548+
inputSchema: {
549+
type: "object",
550+
properties: {
551+
insight_id: { anyOf: [{ type: "string" }, { type: "null" }] },
552+
cluster_name: { type: "string" },
553+
extra: { type: "string" },
554+
},
555+
required: ["cluster_name"],
556+
},
557+
fallbackDescription: "Describe cluster",
558+
},
559+
],
560+
callTool: async (_server: string, _tool: string, args: unknown) => {
561+
callToolArgs.push(args);
562+
return { content: [{ type: "text", text: "ok" }], isError: false };
563+
},
564+
}),
565+
});
566+
567+
// Null for nullable field → preserved
568+
await runtime.tools[0].execute(
569+
"c1",
570+
{ insight_id: null, cluster_name: "test" },
571+
undefined,
572+
undefined,
573+
);
574+
expect(callToolArgs[0]).toEqual({ insight_id: null, cluster_name: "test" });
575+
576+
// Null for non-nullable field → stripped
577+
await runtime.tools[0].execute(
578+
"c2",
579+
{ cluster_name: "test", extra: null },
580+
undefined,
581+
undefined,
582+
);
583+
expect(callToolArgs[1]).toEqual({ cluster_name: "test" });
584+
585+
// Non-null values pass through
586+
await runtime.tools[0].execute(
587+
"c3",
588+
{ insight_id: "eks-123", cluster_name: "test" },
589+
undefined,
590+
undefined,
591+
);
592+
expect(callToolArgs[2]).toEqual({
593+
insight_id: "eks-123",
594+
cluster_name: "test",
595+
});
596+
597+
// All-null non-nullable becomes empty object
598+
await runtime.tools[0].execute("c4", { a: null, b: null }, undefined, undefined);
599+
expect(callToolArgs[3]).toEqual({});
600+
});
524601
});

0 commit comments

Comments
 (0)