From bd51aee5fcdcb7ae4e156a1e225ab5aaebe40832 Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 26 Jun 2026 16:17:58 +0000 Subject: [PATCH] fix(mcp): strip harness intent field at MCP tools/call boundary OMP injects `INTENT_FIELD` (`i`) into every tool's wire schema. The direct model tool-call path strips it via `extractIntent` in agent-loop, but the eval `tool.*` bridge forwards args verbatim, so strict-schema MCP servers (Linear, anything with `additionalProperties:false` / Zod `.strict()`) rejected every call with `-32602 unrecognized_keys: ["i"]`. The eval bridge surfaced the rejection as `hasError: true` instead of throwing, so batch callers reading the value as success silently mutated nothing. Move the strip to the MCP boundary so it owns the contract regardless of caller: `MCPTool.execute` / `DeferredMCPTool.execute` route params through a new `prepareOutboundArgs` that runs `stripHarnessIntent` before `omitUnusedOptionalArgs`. `stripHarnessIntent` leaves `i` in place when the server's own `inputSchema.properties` declares it, so a server that legitimately uses `i` as a parameter is unaffected. Fixes #3575 --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/mcp/tool-bridge.ts | 34 +++++++++- .../coding-agent/test/mcp-tool-args.test.ts | 68 +++++++++++++++++++ 3 files changed, 101 insertions(+), 2 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 12fe49824..df10a873f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -27,6 +27,7 @@ - Fixed stale snapcompact archive state being reintroduced at the AgentSession manual and auto LLM compaction save paths; `preserveData.snapcompact` is now stripped after hook- and result-supplied preserve data are merged, so a prior snapcompact pass can no longer leak frames into a later context-full compaction. ([#3561](https://github.com/can1357/oh-my-pi/pull/3561) by [@serverinspector](https://github.com/serverinspector)) - Fixed the snapcompact→context-full migration shipping the prior archive's plaintext to the summarization provider without secret redaction. When secrets are configured, the migrated archive's `text`/`textHead`/`textTail` regions are now obfuscated alongside the previous summary, while opaque provider-replay state (OpenAI remote-compaction `encrypted_content`) stays byte-identical. ([#3561](https://github.com/can1357/oh-my-pi/pull/3561)) - Fixed fresh interactive launches ignoring `modelRoles.default` when the configured default lives on an extension-registered provider (e.g. an openai-compat plugin's `posthog/claude-opus-4-8`). `createAgentSession` resolved the default role before extension factories registered their providers, so the role-pointed model wasn't visible there; on a fresh launch (no `-c`/`--resume`) the post-extension fallback then went straight to `pickDefaultAvailableModel`, replacing the user's configured default with the first bundled provider default with auth (commonly `openai/gpt-5.5` when `OPENAI_API_KEY` was set). The fallback now retries the default-role lookup against the post-extension allowed-model set — including its explicit thinking selector — before falling back to a bundled provider default. ([#3569](https://github.com/can1357/oh-my-pi/issues/3569)) +- Fixed the `eval` `tool.*` bridge leaking the harness-internal `i` ("intent") field into MCP `tools/call` requests, so strict-schema servers (Linear, anything with `additionalProperties:false` / Zod `.strict()`) rejected every call with `-32602 unrecognized_keys: ["i"]` while the same call via the direct model tool-call path succeeded. `MCPTool.execute` / `DeferredMCPTool.execute` (`packages/coding-agent/src/mcp/tool-bridge.ts`) now strip `INTENT_FIELD` at the MCP boundary, so an MCP call behaves identically whether issued by the model directly or via the eval `tool.*` bridge; servers that legitimately declare `i` as a real parameter keep it untouched. ([#3575](https://github.com/can1357/oh-my-pi/issues/3575)) ## [16.1.22] - 2026-06-26 diff --git a/packages/coding-agent/src/mcp/tool-bridge.ts b/packages/coding-agent/src/mcp/tool-bridge.ts index 3be6e3bca..0a64e7446 100644 --- a/packages/coding-agent/src/mcp/tool-bridge.ts +++ b/packages/coding-agent/src/mcp/tool-bridge.ts @@ -7,6 +7,7 @@ import type { AgentToolUpdateCallback } from "@oh-my-pi/pi-agent-core"; import type { TSchema } from "@oh-my-pi/pi-ai"; import { normalizeSchemaForMCP } from "@oh-my-pi/pi-ai/utils/schema"; import { untilAborted } from "@oh-my-pi/pi-utils"; +import { INTENT_FIELD } from "@oh-my-pi/pi-wire"; import type { SourceMeta } from "../capability/types"; import type { CustomTool, @@ -83,6 +84,35 @@ function omitUnusedOptionalArgs(args: MCPToolArgs, inputSchema: MCPToolDefinitio return cleaned ?? args; } +/** + * Drop the harness-internal intent field (`INTENT_FIELD`) before forwarding + * args to an MCP server. The harness injects `i` into every tool's wire + * schema; the direct model tool-call path strips it via `extractIntent`, but + * the `eval` `tool.*` bridge and any other in-process caller forwards args + * verbatim. Strict-schema servers (Linear, anything with + * `additionalProperties:false` / Zod `.strict()`) reject every call that + * carries `i`. The MCP boundary is the authoritative guard so callers don't + * have to pre-strip. + * + * Leaves `i` in place when the server's own `inputSchema.properties` declares + * it, so a server that legitimately uses `i` as a parameter is unaffected. + */ +function stripHarnessIntent(args: MCPToolArgs, inputSchema: MCPToolDefinition["inputSchema"]): MCPToolArgs { + if (!Object.hasOwn(args, INTENT_FIELD)) return args; + if (inputSchema.properties && Object.hasOwn(inputSchema.properties, INTENT_FIELD)) return args; + const { [INTENT_FIELD]: _intent, ...rest } = args; + return rest; +} + +/** + * Normalize raw tool params into the outbound `tools/call` arguments: strip + * the harness intent field, then drop optional empty placeholders the server + * declares but doesn't require. + */ +function prepareOutboundArgs(params: unknown, inputSchema: MCPToolDefinition["inputSchema"]): MCPToolArgs { + return omitUnusedOptionalArgs(stripHarnessIntent(normalizeToolArgs(params), inputSchema), inputSchema); +} + /** Details included in MCP tool results for rendering */ export interface MCPToolDetails { /** Server name */ @@ -286,7 +316,7 @@ export class MCPTool implements CustomTool { signal?: AbortSignal, ): Promise> { throwIfAborted(signal); - const args = omitUnusedOptionalArgs(normalizeToolArgs(params), this.tool.inputSchema); + const args = prepareOutboundArgs(params, this.tool.inputSchema); const provider = this.connection._source?.provider; const providerName = this.connection._source?.providerName; @@ -385,7 +415,7 @@ export class DeferredMCPTool implements CustomTool { signal?: AbortSignal, ): Promise> { throwIfAborted(signal); - const args = omitUnusedOptionalArgs(normalizeToolArgs(params), this.tool.inputSchema); + const args = prepareOutboundArgs(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 index c7fa70920..42d49c299 100644 --- a/packages/coding-agent/test/mcp-tool-args.test.ts +++ b/packages/coding-agent/test/mcp-tool-args.test.ts @@ -2,6 +2,7 @@ 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 { INTENT_FIELD } from "@oh-my-pi/pi-wire"; import { createMockConnection, createMockTransport } from "./mcp-test-utils"; type CapturedRequest = { @@ -86,4 +87,71 @@ describe("MCP tool arguments", () => { }, ]); }); + + it("strips the harness intent field before tools/call", async () => { + // Regression: the harness injects `i` into every tool's wire schema and + // the eval `tool.*` bridge forwards it verbatim. Strict-schema MCP + // servers (e.g. Linear) reject every such call with + // `unrecognized_keys: ["i"]`. The MCP boundary owns the contract; `i` + // must never reach `tools/call`. + const calls: CapturedRequest[] = []; + const tool = new MCPTool(createCapturedConnection(calls), createSearchToolDefinition()); + + await tool.execute( + "call-1", + { [INTENT_FIELD]: "looking up Foo", symbol: "Foo", language: "TypeScript", file: "" }, + undefined, + unusedContext, + undefined, + ); + + expect(calls).toEqual([ + { + method: "tools/call", + params: { name: "search", arguments: { symbol: "Foo", language: "TypeScript" } }, + }, + ]); + }); + + it("strips the harness intent field 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", + { [INTENT_FIELD]: "deferred lookup", symbol: "Bar", language: "TypeScript" }, + undefined, + unusedContext, + undefined, + ); + + expect(calls).toEqual([ + { + method: "tools/call", + params: { name: "search", arguments: { symbol: "Bar", language: "TypeScript" } }, + }, + ]); + }); + + it("preserves `i` when the server's own schema declares it", async () => { + // A server that legitimately exposes `i` as one of its parameters + // must receive the caller-supplied value untouched. The boundary + // guard checks the server's declared `properties` and steps aside. + const calls: CapturedRequest[] = []; + const definition: MCPToolDefinition = { + name: "echo", + description: "Echo a single token", + inputSchema: { + type: "object", + properties: { i: { type: "string" } }, + required: ["i"], + }, + }; + const tool = new MCPTool(createCapturedConnection(calls), definition); + + await tool.execute("call-1", { i: "hello" }, undefined, unusedContext, undefined); + + expect(calls).toEqual([{ method: "tools/call", params: { name: "echo", arguments: { i: "hello" } } }]); + }); });