From 60348404a02119b58bb16dd48584d7602f9628f7 Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 23 Jun 2026 10:29:36 +0000 Subject: [PATCH 1/2] fix(mcp): omitted unused optional tool args Pruned empty optional MCP argument placeholders before tools/call while preserving required fields and meaningful falsy values. Added regression coverage for active and deferred MCP tools. Fixes #3302 --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/mcp/tool-bridge.ts | 29 +++++- .../coding-agent/test/mcp-tool-args.test.ts | 89 +++++++++++++++++++ 3 files changed, 117 insertions(+), 2 deletions(-) create mode 100644 packages/coding-agent/test/mcp-tool-args.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 1bff46648..5e1b619ad 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -27,6 +27,7 @@ ### 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)) - Fixed `local://` URLs decoding images as corrupted text (mojibake) instead of showing the image - Fixed `omp --resume` hanging instead of exiting when the startup session picker is cancelled (Esc) or there are no sessions to resume. Startup arms long-lived handles (theme/appearance listeners, settings save timer, model registry), so the cancel/empty paths' bare `return` left the event loop alive and the process stuck after the picker cleared the alternate screen. These paths now exit cleanly via `process.exit(0)`, matching the `--version`/`--export` early-exit convention. The in-session `/resume` picker is unaffected — it keeps its own cancel handler that just closes the overlay. - Fixed the `/resume` session picker scrolling down after a session is deleted. The delete-confirmation dialog was mounted as a sibling below the picker's bottom border, briefly growing the picker past the terminal height; the TUI committed the picker's header rows into native scrollback to fit, and when the dialog closed `windowTop` stayed pinned at the new commit boundary, leaving the header stranded above the viewport. The picker now hosts the `SessionList` in a single content slot and swaps the dialog INTO that slot (replacing the `SessionList`) while it is open, so the dialog only competes with the `SessionList`'s rendered budget — not the `SessionList` AND the picker chrome — and the picker frame stays inside the viewport. ([#3283](https://github.com/can1357/oh-my-pi/issues/3283)) diff --git a/packages/coding-agent/src/mcp/tool-bridge.ts b/packages/coding-agent/src/mcp/tool-bridge.ts index 6d6e75db8..3be6e3bca 100644 --- a/packages/coding-agent/src/mcp/tool-bridge.ts +++ b/packages/coding-agent/src/mcp/tool-bridge.ts @@ -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 { signal?: AbortSignal, ): Promise> { 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 { signal?: AbortSignal, ): Promise> { throwIfAborted(signal); - const args = normalizeToolArgs(params); + const args = omitUnusedOptionalArgs(normalizeToolArgs(params), this.tool.inputSchema); const provider = this.#fallbackProvider; const providerName = this.#fallbackProviderName; diff --git a/packages/coding-agent/test/mcp-tool-args.test.ts b/packages/coding-agent/test/mcp-tool-args.test.ts new file mode 100644 index 000000000..c7fa70920 --- /dev/null +++ b/packages/coding-agent/test/mcp-tool-args.test.ts @@ -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 | 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" }, + }, + }, + ]); + }); +}); From 655ab4347d6a0321e9e3c90fabacae6f421f5e4c Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 23 Jun 2026 10:33:38 +0000 Subject: [PATCH 2/2] docs(changelog): moved MCP fix entry to unreleased section --- packages/coding-agent/CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 5e1b619ad..57b94c41f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### 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 @@ -27,7 +31,6 @@ ### 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)) - Fixed `local://` URLs decoding images as corrupted text (mojibake) instead of showing the image - Fixed `omp --resume` hanging instead of exiting when the startup session picker is cancelled (Esc) or there are no sessions to resume. Startup arms long-lived handles (theme/appearance listeners, settings save timer, model registry), so the cancel/empty paths' bare `return` left the event loop alive and the process stuck after the picker cleared the alternate screen. These paths now exit cleanly via `process.exit(0)`, matching the `--version`/`--export` early-exit convention. The in-session `/resume` picker is unaffected — it keeps its own cancel handler that just closes the overlay. - Fixed the `/resume` session picker scrolling down after a session is deleted. The delete-confirmation dialog was mounted as a sibling below the picker's bottom border, briefly growing the picker past the terminal height; the TUI committed the picker's header rows into native scrollback to fit, and when the dialog closed `windowTop` stayed pinned at the new commit boundary, leaving the header stranded above the viewport. The picker now hosts the `SessionList` in a single content slot and swaps the dialog INTO that slot (replacing the `SessionList`) while it is open, so the dialog only competes with the `SessionList`'s rendered budget — not the `SessionList` AND the picker chrome — and the picker frame stays inside the viewport. ([#3283](https://github.com/can1357/oh-my-pi/issues/3283))