From b097019feff547782c1643a278caa2dfa0218526 Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 9 Jul 2026 16:37:29 +0000 Subject: [PATCH 1/2] fix(mcp): resolved local image paths for tool calls - Resolved session '/data/workspaces/can1357__oh-my-pi__4946/.omp-session/2026-07-09T16-06-43-993Z_019f47a1-a619-7000-9062-5f5d863afa45/local' file arguments before forwarding MCP tools/call requests to external servers. - Threaded local protocol options into custom MCP tool context and added focused coverage for image_path attachments. Fixes #4946 --- packages/coding-agent/CHANGELOG.md | 4 ++ .../src/extensibility/custom-tools/types.ts | 3 + packages/coding-agent/src/mcp/tool-bridge.ts | 63 +++++++++++++++++-- .../src/prompts/system/workflow-notice.md | 10 +-- .../coding-agent/src/session/agent-session.ts | 2 + .../coding-agent/test/mcp-tool-args.test.ts | 47 ++++++++++++++ 6 files changed, 118 insertions(+), 11 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index f9b8bb0ef..b64f83105 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed MCP tools receiving session image attachments as raw `local://...` URIs by resolving them to session-local filesystem paths before `tools/call` is sent ([#4946](https://github.com/can1357/oh-my-pi/issues/4946)). + ## [16.3.12] - 2026-07-08 ### Added diff --git a/packages/coding-agent/src/extensibility/custom-tools/types.ts b/packages/coding-agent/src/extensibility/custom-tools/types.ts index 166a9820c..7beb91624 100644 --- a/packages/coding-agent/src/extensibility/custom-tools/types.ts +++ b/packages/coding-agent/src/extensibility/custom-tools/types.ts @@ -23,6 +23,7 @@ import type { Settings } from "../../config/settings"; import type { ExecOptions, ExecResult } from "../../exec/exec"; import type { HookUIContext } from "../../extensibility/hooks/types"; import type * as PiCodingAgent from "../../index"; +import type { LocalProtocolOptions } from "../../internal-urls/local-protocol"; import type { Theme } from "../../modes/theme/theme"; import type { ReadonlySessionManager } from "../../session/session-manager"; import type { TodoItem } from "../../tools/todo"; @@ -96,6 +97,8 @@ export interface CustomToolContext { settings?: Settings; /** Fetch implementation for outbound HTTP; defaults to global fetch when omitted. */ fetch?: FetchImpl; + /** Calling session's `local://` root mapping for tools that bridge out of the OMP process. */ + localProtocolOptions?: LocalProtocolOptions; /** Whether to auto-approve all destructive tool operations (--auto-approve CLI flag) */ autoApprove?: boolean; } diff --git a/packages/coding-agent/src/mcp/tool-bridge.ts b/packages/coding-agent/src/mcp/tool-bridge.ts index 0a64e7446..2a10e4140 100644 --- a/packages/coding-agent/src/mcp/tool-bridge.ts +++ b/packages/coding-agent/src/mcp/tool-bridge.ts @@ -15,8 +15,10 @@ import type { CustomToolResult, RenderResultOptions, } from "../extensibility/custom-tools/types"; +import { resolveLocalUrlToFile } from "../internal-urls/local-protocol"; import type { Theme } from "../modes/theme/theme"; import type { OutputMeta } from "../tools/output-meta"; +import { normalizeLocalScheme } from "../tools/path-utils"; import { ToolAbortError, throwIfAborted } from "../tools/tool-errors"; import { callTool } from "./client"; import { renderMCPCall, renderMCPResult } from "./render"; @@ -104,13 +106,62 @@ function stripHarnessIntent(args: MCPToolArgs, inputSchema: MCPToolDefinition["i return rest; } +async function resolveOutboundLocalUrlArgs( + value: unknown, + context: CustomToolContext, + seen: WeakSet = new WeakSet(), +): Promise { + if (typeof value === "string") { + const normalized = normalizeLocalScheme(value); + if (!normalized.startsWith("local://")) return value; + const localFile = await resolveLocalUrlToFile(normalized, { + cwd: context.sessionManager?.getCwd?.(), + settings: context.settings, + localProtocolOptions: context.localProtocolOptions, + }); + return localFile?.path ?? value; + } + if (typeof value !== "object" || value === null) return value; + if (seen.has(value)) return value; + seen.add(value); + + if (Array.isArray(value)) { + let resolved: unknown[] | undefined; + for (let index = 0; index < value.length; index++) { + const item = value[index]; + const next = await resolveOutboundLocalUrlArgs(item, context, seen); + if (next === item && !resolved) continue; + resolved ??= value.slice(); + resolved[index] = next; + } + return resolved ?? value; + } + + const input = value as Record; + let resolved: Record | undefined; + for (const key in input) { + const item = input[key]; + const next = await resolveOutboundLocalUrlArgs(item, context, seen); + if (next === item && !resolved) continue; + resolved ??= { ...input }; + resolved[key] = next; + } + return resolved ?? value; +} + /** * 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. + * the harness intent field, drop optional empty placeholders the server + * declares but doesn't require, then translate session-local files to paths + * external MCP servers can read. */ -function prepareOutboundArgs(params: unknown, inputSchema: MCPToolDefinition["inputSchema"]): MCPToolArgs { - return omitUnusedOptionalArgs(stripHarnessIntent(normalizeToolArgs(params), inputSchema), inputSchema); +async function prepareOutboundArgs( + params: unknown, + inputSchema: MCPToolDefinition["inputSchema"], + context: CustomToolContext, +): Promise { + const args = omitUnusedOptionalArgs(stripHarnessIntent(normalizeToolArgs(params), inputSchema), inputSchema); + return (await resolveOutboundLocalUrlArgs(args, context)) as MCPToolArgs; } /** Details included in MCP tool results for rendering */ @@ -316,7 +367,7 @@ export class MCPTool implements CustomTool { signal?: AbortSignal, ): Promise> { throwIfAborted(signal); - const args = prepareOutboundArgs(params, this.tool.inputSchema); + const args = await prepareOutboundArgs(params, this.tool.inputSchema, _ctx); const provider = this.connection._source?.provider; const providerName = this.connection._source?.providerName; @@ -415,7 +466,7 @@ export class DeferredMCPTool implements CustomTool { signal?: AbortSignal, ): Promise> { throwIfAborted(signal); - const args = prepareOutboundArgs(params, this.tool.inputSchema); + const args = await prepareOutboundArgs(params, this.tool.inputSchema, _ctx); const provider = this.#fallbackProvider; const providerName = this.#fallbackProviderName; diff --git a/packages/coding-agent/src/prompts/system/workflow-notice.md b/packages/coding-agent/src/prompts/system/workflow-notice.md index 74eddee21..3e62ca5b0 100644 --- a/packages/coding-agent/src/prompts/system/workflow-notice.md +++ b/packages/coding-agent/src/prompts/system/workflow-notice.md @@ -51,20 +51,20 @@ Decompose first, then {{#if taskBatch}}batch the independent leaves{{else}}issue {{#if taskBatch}} task( - context: "# Goal\nReview the auth diff...\n# Constraints\nRead-only...\n# Contract\nReturn findings as severity/file/line/fix...", + context: "# Goal\nReview the auth diff…\n# Constraints\nRead-only…\n# Contract\nReturn findings as severity/file/line/fix…", tasks: [ - { id: "AuthOwner", role: "Auth Storage Reviewer", assignment: "# Target\npackages/ai/src/auth-storage.ts\n# Change\nTrace credential selection...\n# Acceptance\nReturn confirmed findings only..." }, - { id: "PromptOwner", role: "Prompt Contract Reviewer", assignment: "# Target\npackages/coding-agent/src/prompts/**\n# Change\nCheck active-tool guidance...\n# Acceptance\nReturn mismatches and exact prompt lines..." }, + { id: "AuthOwner", role: "Auth Storage Reviewer", assignment: "# Target\npackages/ai/src/auth-storage.ts\n# Change\nTrace credential selection…\n# Acceptance\nReturn confirmed findings only…" }, + { id: "PromptOwner", role: "Prompt Contract Reviewer", assignment: "# Target\npackages/coding-agent/src/prompts/**\n# Change\nCheck active-tool guidance…\n# Acceptance\nReturn mismatches and exact prompt lines…" }, ] ) {{else}} task( role: "Auth Storage Reviewer", - assignment: "# Target\npackages/ai/src/auth-storage.ts\n# Change\nReview the auth diff. Shared contract: read-only; return findings as severity/file/line/fix.\n# Acceptance\nReturn confirmed findings only..." + assignment: "# Target\npackages/ai/src/auth-storage.ts\n# Change\nReview the auth diff. Shared contract: read-only; return findings as severity/file/line/fix.\n# Acceptance\nReturn confirmed findings only…" ) task( role: "Prompt Contract Reviewer", - assignment: "# Target\npackages/coding-agent/src/prompts/**\n# Change\nCheck active-tool guidance. Shared contract: read-only; return mismatches and exact prompt lines.\n# Acceptance\nReturn confirmed findings only..." + assignment: "# Target\npackages/coding-agent/src/prompts/**\n# Change\nCheck active-tool guidance. Shared contract: read-only; return mismatches and exact prompt lines.\n# Acceptance\nReturn confirmed findings only…" ) {{/if}} diff --git a/packages/coding-agent/src/session/agent-session.ts b/packages/coding-agent/src/session/agent-session.ts index 85e544ca1..a7998f103 100644 --- a/packages/coding-agent/src/session/agent-session.ts +++ b/packages/coding-agent/src/session/agent-session.ts @@ -6613,6 +6613,8 @@ export class AgentSession { abort: () => { this.agent.abort(); }, + settings: this.settings, + localProtocolOptions: this.#localProtocolOptions(), }); for (const customTool of mcpTools) { diff --git a/packages/coding-agent/test/mcp-tool-args.test.ts b/packages/coding-agent/test/mcp-tool-args.test.ts index 42d49c299..158bb1431 100644 --- a/packages/coding-agent/test/mcp-tool-args.test.ts +++ b/packages/coding-agent/test/mcp-tool-args.test.ts @@ -1,7 +1,9 @@ import { describe, expect, it } from "bun:test"; +import * as path from "node:path"; 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 { TempDir } from "@oh-my-pi/pi-utils"; import { INTENT_FIELD } from "@oh-my-pi/pi-wire"; import { createMockConnection, createMockTransport } from "./mcp-test-utils"; @@ -40,6 +42,35 @@ function createCapturedConnection(calls: CapturedRequest[]): MCPServerConnection return createMockConnection({ tools: {} }, transport); } +const imageToolDefinition: MCPToolDefinition = { + name: "read_image_with_model", + description: "Read an image from a local filesystem path", + inputSchema: { + type: "object", + properties: { + image_path: { type: "string" }, + }, + required: ["image_path"], + }, +}; + +async function createLocalImageContext( + tempDir: TempDir, +): Promise<{ context: CustomToolContext; expectedPath: string }> { + const artifactsDir = tempDir.join("artifacts"); + const expectedPath = path.join(artifactsDir, "local", "image-issue.png"); + await Bun.write(expectedPath, "png bytes"); + return { + context: { + localProtocolOptions: { + getArtifactsDir: () => artifactsDir, + getSessionId: () => "session-id", + }, + } as CustomToolContext, + expectedPath, + }; +} + describe("MCP tool arguments", () => { it("omits optional empty placeholders before tools/call", async () => { const calls: CapturedRequest[] = []; @@ -154,4 +185,20 @@ describe("MCP tool arguments", () => { expect(calls).toEqual([{ method: "tools/call", params: { name: "echo", arguments: { i: "hello" } } }]); }); + + it("resolves local image arguments before forwarding tools/call", async () => { + using tempDir = TempDir.createSync("@pi-mcp-local-image-"); + const calls: CapturedRequest[] = []; + const { context, expectedPath } = await createLocalImageContext(tempDir); + const tool = new MCPTool(createCapturedConnection(calls), imageToolDefinition); + + await tool.execute("call-1", { image_path: "local://image-issue.png" }, undefined, context, undefined); + + expect(calls).toEqual([ + { + method: "tools/call", + params: { name: "read_image_with_model", arguments: { image_path: expectedPath } }, + }, + ]); + }); }); From 3ebcb3690159ad7dcc36176d5749e8e7aaa96321 Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 9 Jul 2026 17:17:27 +0000 Subject: [PATCH 2/2] fix(mcp): threaded local roots through startup tools - Copied localProtocolOptions through SDK-created custom tool contexts so startup MCP tools resolve '/data/workspaces/can1357__oh-my-pi__4946/.omp-session/2026-07-09T16-06-43-993Z_019f47a1-a619-7000-9062-5f5d863afa45/local' against the active session. - Exposed localProtocolOptions on extension contexts and covered the runner propagation path. Fixes #4946 --- .../src/extensibility/extensions/runner.ts | 3 +++ .../src/extensibility/extensions/types.ts | 3 +++ packages/coding-agent/src/sdk.ts | 3 +++ .../test/extensions-runner.test.ts | 20 +++++++++++++++++++ 4 files changed, 29 insertions(+) diff --git a/packages/coding-agent/src/extensibility/extensions/runner.ts b/packages/coding-agent/src/extensibility/extensions/runner.ts index e0226e0b9..58cc6ee46 100644 --- a/packages/coding-agent/src/extensibility/extensions/runner.ts +++ b/packages/coding-agent/src/extensibility/extensions/runner.ts @@ -7,6 +7,7 @@ import type { KeyId } from "@oh-my-pi/pi-tui"; import { logger } from "@oh-my-pi/pi-utils"; import type { ModelRegistry } from "../../config/model-registry"; import type { Settings } from "../../config/settings"; +import type { LocalProtocolOptions } from "../../internal-urls/local-protocol"; import type { MemoryRuntimeContext } from "../../memory-backend"; import { type Theme, theme } from "../../modes/theme/theme"; import type { SessionManager } from "../../session/session-manager"; @@ -255,6 +256,7 @@ export class ExtensionRunner { private readonly modelRegistry: ModelRegistry, getMemory?: () => MemoryRuntimeContext | undefined, private readonly settings?: Settings, + private readonly localProtocolOptions?: LocalProtocolOptions, ) { this.#uiContext = noOpUIContext; this.#getMemoryFn = getMemory; @@ -537,6 +539,7 @@ export class ExtensionRunner { hasPendingMessages: () => this.#hasPendingMessagesFn(), shutdown: () => this.#shutdownHandler(), getSystemPrompt: () => this.#getSystemPromptFn(), + localProtocolOptions: this.localProtocolOptions, memory: this.#getMemoryFn?.(), }; } diff --git a/packages/coding-agent/src/extensibility/extensions/types.ts b/packages/coding-agent/src/extensibility/extensions/types.ts index 82d513257..ef4596074 100644 --- a/packages/coding-agent/src/extensibility/extensions/types.ts +++ b/packages/coding-agent/src/extensibility/extensions/types.ts @@ -41,6 +41,7 @@ import type { PythonResult } from "../../eval/py/executor"; import type { BashResult } from "../../exec/bash-executor"; import type { ExecOptions, ExecResult } from "../../exec/exec"; import type * as PiCodingAgent from "../../index"; +import type { LocalProtocolOptions } from "../../internal-urls/local-protocol"; import type { MemoryRuntimeContext } from "../../memory-backend"; import type { CustomEditor } from "../../modes/components/custom-editor"; import type { Theme } from "../../modes/theme/theme"; @@ -357,6 +358,8 @@ export interface ExtensionContext { sessionManager: ReadonlySessionManager; /** Model registry for API key resolution */ modelRegistry: ModelRegistry; + /** Calling session's `local://` root mapping for external tool bridges. */ + localProtocolOptions?: LocalProtocolOptions; /** Current model (may be undefined) */ model: Model | undefined; /** Read-only model query facade: list / current / resolve / family. */ diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index 0d27eac07..912821bd5 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -856,6 +856,7 @@ function createCustomToolContext(ctx: ExtensionContext): CustomToolContext { isIdle: ctx.isIdle, hasQueuedMessages: ctx.hasPendingMessages, abort: ctx.abort, + localProtocolOptions: ctx.localProtocolOptions, }; } @@ -2184,6 +2185,7 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} modelRegistry, () => (hasSession ? createSessionMemoryRuntimeContext(session, agentDir, cwd) : undefined), settings, + localProtocolOptions, ); credentialDisabledTarget = extensionRunner; @@ -2202,6 +2204,7 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} session.abort({ reason: USER_INTERRUPT_LABEL }); }, settings, + localProtocolOptions, autoApprove: options.autoApprove ?? false, }); const toolContextStore = new ToolContextStore(getSessionContext); diff --git a/packages/coding-agent/test/extensions-runner.test.ts b/packages/coding-agent/test/extensions-runner.test.ts index d24518737..b7229b46a 100644 --- a/packages/coding-agent/test/extensions-runner.test.ts +++ b/packages/coding-agent/test/extensions-runner.test.ts @@ -72,6 +72,26 @@ describe("ExtensionRunner", () => { }; }; + it("exposes caller localProtocolOptions through extension context", async () => { + const localProtocolOptions = { + getArtifactsDir: () => tempDir.join("artifacts"), + getSessionId: () => "runner-session", + }; + const result = await loadTestExtensions(); + const runner = new ExtensionRunner( + result.extensions, + result.runtime, + tempDir.path(), + sessionManager, + modelRegistry, + undefined, + undefined, + localProtocolOptions, + ); + + expect(runner.createContext().localProtocolOptions).toBe(localProtocolOptions); + }); + describe("shortcut conflicts", () => { it("warns when extension shortcut conflicts with built-in", async () => { const extCode = `