Merge remote-tracking branch 'origin/farm/8a720b5a/mcp-tools-omit-unused-optional-args'

2026-06-23 20:22:15 +02:00
parent 6591fc29a3 655ab4347d
commit aed01caaae
3 changed files with 120 additions and 2 deletions
@@ -13,6 +13,10 @@

 - Fixed `task.maxConcurrency: 0` serializing subagent spawns instead of running them unbounded. The settings UI labels `0` as "Unlimited", but the session-scoped spawn `Semaphore` clamped `max` via `Math.max(1, max)`, so the second subagent body in a batch always waited for the first to release the seat. The constructor now treats `max <= 0` (and any non-finite input) as unbounded via `Number.POSITIVE_INFINITY`, matching the eval `parallel()`/`pipeline()` worker-pool semantics ([#3305](https://github.com/can1357/oh-my-pi/issues/3305)).

+### Fixed
+
+- Fixed MCP tool calls forwarding empty optional placeholder arguments (`""` and `{}`) to `tools/call`; optional placeholders are now omitted while required fields and meaningful falsy values are preserved. ([#3302](https://github.com/can1357/oh-my-pi/issues/3302))
+
 ## [16.1.16] - 2026-06-23

 ### Breaking Changes
@@ -58,6 +58,31 @@ function normalizeToolArgs(value: unknown): MCPToolArgs {
 	return value as MCPToolArgs;
 }

+function isUnusedOptionalPlaceholder(value: unknown): boolean {
+	return (
+		value === undefined ||
+		value === "" ||
+		(typeof value === "object" && value !== null && !Array.isArray(value) && Object.keys(value).length === 0)
+	);
+}
+
+function omitUnusedOptionalArgs(args: MCPToolArgs, inputSchema: MCPToolDefinition["inputSchema"]): MCPToolArgs {
+	const properties = inputSchema.properties;
+	if (!properties) return args;
+
+	let cleaned: MCPToolArgs | undefined;
+	const required = new Set(inputSchema.required ?? []);
+	for (const [key, value] of Object.entries(args)) {
+		if (required.has(key) || !Object.hasOwn(properties, key) || !isUnusedOptionalPlaceholder(value)) {
+			continue;
+		}
+		cleaned ??= { ...args };
+		delete cleaned[key];
+	}
+
+	return cleaned ?? args;
+}
+
 /** Details included in MCP tool results for rendering */
 export interface MCPToolDetails {
 	/** Server name */
@@ -261,7 +286,7 @@ export class MCPTool implements CustomTool<TSchema, MCPToolDetails> {
 		signal?: AbortSignal,
 	): Promise<CustomToolResult<MCPToolDetails>> {
 		throwIfAborted(signal);
-		const args = normalizeToolArgs(params);
+		const args = omitUnusedOptionalArgs(normalizeToolArgs(params), this.tool.inputSchema);
 		const provider = this.connection._source?.provider;
 		const providerName = this.connection._source?.providerName;

@@ -360,7 +385,7 @@ export class DeferredMCPTool implements CustomTool<TSchema, MCPToolDetails> {
 		signal?: AbortSignal,
 	): Promise<CustomToolResult<MCPToolDetails>> {
 		throwIfAborted(signal);
-		const args = normalizeToolArgs(params);
+		const args = omitUnusedOptionalArgs(normalizeToolArgs(params), this.tool.inputSchema);
 		const provider = this.#fallbackProvider;
 		const providerName = this.#fallbackProviderName;

@@ -0,0 +1,89 @@
+import { describe, expect, it } from "bun:test";
+import type { CustomToolContext } from "@oh-my-pi/pi-coding-agent/extensibility/custom-tools";
+import { DeferredMCPTool, MCPTool, type MCPToolDefinition } from "@oh-my-pi/pi-coding-agent/mcp";
+import type { MCPServerConnection } from "@oh-my-pi/pi-coding-agent/mcp/types";
+import { createMockConnection, createMockTransport } from "./mcp-test-utils";
+
+type CapturedRequest = {
+	method: string;
+	params: Record<string, unknown> | undefined;
+};
+
+const unusedContext = {} as CustomToolContext;
+
+function createSearchToolDefinition(): MCPToolDefinition {
+	return {
+		name: "search",
+		description: "Search symbols or file locations",
+		inputSchema: {
+			type: "object",
+			properties: {
+				symbol: { type: "string" },
+				language: { type: "string" },
+				file: { type: "string" },
+				line: { type: "number" },
+				column: { type: "number" },
+				filters: { type: "object" },
+				exact: { type: "boolean" },
+			},
+			required: ["symbol", "language"],
+		},
+	};
+}
+
+function createCapturedConnection(calls: CapturedRequest[]): MCPServerConnection {
+	const transport = createMockTransport(
+		new Map([["tools/call", [{ content: [{ type: "text", text: "ok" }] }]]]),
+		(method, params) => calls.push({ method, params }),
+	);
+	return createMockConnection({ tools: {} }, transport);
+}
+
+describe("MCP tool arguments", () => {
+	it("omits optional empty placeholders before tools/call", async () => {
+		const calls: CapturedRequest[] = [];
+		const tool = new MCPTool(createCapturedConnection(calls), createSearchToolDefinition());
+
+		await tool.execute(
+			"call-1",
+			{ symbol: "Foo", language: "", file: "", line: 0, filters: {}, exact: false },
+			undefined,
+			unusedContext,
+			undefined,
+		);
+
+		expect(calls).toEqual([
+			{
+				method: "tools/call",
+				params: {
+					name: "search",
+					arguments: { symbol: "Foo", language: "", line: 0, exact: false },
+				},
+			},
+		]);
+	});
+
+	it("omits optional empty placeholders for deferred MCP tools", async () => {
+		const calls: CapturedRequest[] = [];
+		const connection = createCapturedConnection(calls);
+		const tool = new DeferredMCPTool("intellij-index", createSearchToolDefinition(), async () => connection);
+
+		await tool.execute(
+			"call-1",
+			{ symbol: "Foo", language: "TypeScript", file: "", column: "", filters: {} },
+			undefined,
+			unusedContext,
+			undefined,
+		);
+
+		expect(calls).toEqual([
+			{
+				method: "tools/call",
+				params: {
+					name: "search",
+					arguments: { symbol: "Foo", language: "TypeScript" },
+				},
+			},
+		]);
+	});
+});