Merge PR #4950: fix(mcp): resolve local image paths for tool calls (@roboomp)

2026-07-14 18:45:22 +02:00
parent 5a79ef3308 3ebcb36901
commit 4b5c32a092
10 changed files with 144 additions and 11 deletions
@@ -352,6 +352,7 @@
 - Fixed Cursor-provider turns whose assistant message contains intro text, tool calls, and trailing final text rendering the final answer above the tool output instead of at the transcript tail ([#4871](https://github.com/can1357/oh-my-pi/issues/4871)).
 - Fixed `omp -p` looking hung while a text-mode prompt is in flight by writing a one-shot working indicator to stderr before awaiting the model response. ([#4901](https://github.com/can1357/oh-my-pi/issues/4901))
 - Fixed autolearn auto-continue firing a capture turn after an aborted stop (Esc/cancel): the controller now skips any `agent_end` whose last assistant message has `stopReason: "aborted"`.
+- 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

@@ -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;
 }
@@ -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";
@@ -256,6 +257,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;
@@ -538,6 +540,7 @@ export class ExtensionRunner {
 			hasPendingMessages: () => this.#hasPendingMessagesFn(),
 			shutdown: () => this.#shutdownHandler(),
 			getSystemPrompt: () => this.#getSystemPromptFn(),
+			localProtocolOptions: this.localProtocolOptions,
 			memory: this.#getMemoryFn?.(),
 		};
 	}
@@ -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";
@@ -420,6 +421,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. */
@@ -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<object> = new WeakSet(),
+): Promise<unknown> {
+	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<string, unknown>;
+	let resolved: Record<string, unknown> | 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<MCPToolArgs> {
+	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<TSchema, MCPToolDetails> {
 		signal?: AbortSignal,
 	): Promise<CustomToolResult<MCPToolDetails>> {
 		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<TSchema, MCPToolDetails> {
 		signal?: AbortSignal,
 	): Promise<CustomToolResult<MCPToolDetails>> {
 		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;

@@ -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}}

@@ -865,6 +865,7 @@ function createCustomToolContext(ctx: ExtensionContext): CustomToolContext {
 		isIdle: ctx.isIdle,
 		hasQueuedMessages: ctx.hasPendingMessages,
 		abort: ctx.abort,
+		localProtocolOptions: ctx.localProtocolOptions,
 	};
 }

@@ -2218,6 +2219,7 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {}
 			modelRegistry,
 			() => (hasSession ? createSessionMemoryRuntimeContext(session, agentDir, cwd) : undefined),
 			settings,
+			localProtocolOptions,
 		);

 		credentialDisabledTarget = extensionRunner;
@@ -2236,6 +2238,7 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {}
 				session.abort({ reason: USER_INTERRUPT_LABEL });
 			},
 			settings,
+			localProtocolOptions,
 			autoApprove: options.autoApprove ?? false,
 		});
 		const toolContextStore = new ToolContextStore(getSessionContext);
@@ -7242,6 +7242,8 @@ export class AgentSession {
 			abort: () => {
 				this.agent.abort();
 			},
+			settings: this.settings,
+			localProtocolOptions: this.#localProtocolOptions(),
 		});

 		for (const customTool of mcpTools) {
@@ -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 = `
@@ -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 } },
+			},
+		]);
+	});
 });