From 2c77c8535a5b5f6795da8d48d91b55c9a6bb6d1a Mon Sep 17 00:00:00 2001 From: Dongmen Laohu <251681749+FernandeZ-hjm@users.noreply.github.com> Date: Mon, 27 Jul 2026 18:41:21 +0800 Subject: [PATCH] fix(mcp): resolve native resource URIs --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/src/cli/read-cli.ts | 43 +++++++++- .../src/internal-urls/mcp-protocol.ts | 15 +++- .../coding-agent/src/internal-urls/router.ts | 23 +++++- packages/coding-agent/src/mcp/manager.ts | 19 +++-- packages/coding-agent/src/tools/read.ts | 4 +- .../fixtures/resources-no-templates-mcp.ts | 10 ++- .../test/internal-urls/mcp-protocol.test.ts | 71 +++++++++++++++++ .../mcp-resource-templates-missing.test.ts | 14 +--- .../test/read-cli-mcp-resource.test.ts | 79 +++++++++++++++++++ 10 files changed, 246 insertions(+), 33 deletions(-) create mode 100644 packages/coding-agent/test/read-cli-mcp-resource.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index ce8a4f32d..89c6ed77e 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -8,6 +8,7 @@ ### Fixed +- Fixed MCP resources with custom URI schemes being treated as missing filesystem paths. `read` and `omp read` now resolve server-advertised native resource URIs such as `ags://capabilities/current-host`, while preserving the existing `mcp://` form. - Fixed Python cell errors (`$` commands and the eval tool) leaking runner-internal traceback frames. Cell syntax errors now render as the bare caret display with a `` filename instead of a `_handle_request_async`/`ast.parse` stack dump, and runtime tracebacks start at user code, matching the Ruby runner's user-frame filtering. ## [17.1.5] - 2026-07-27 diff --git a/packages/coding-agent/src/cli/read-cli.ts b/packages/coding-agent/src/cli/read-cli.ts index afaff1dbf..4d7292917 100644 --- a/packages/coding-agent/src/cli/read-cli.ts +++ b/packages/coding-agent/src/cli/read-cli.ts @@ -8,6 +8,11 @@ import { getProjectDir } from "@oh-my-pi/pi-utils"; import chalk from "chalk"; import { Settings } from "../config/settings"; +import { InternalUrlRouter } from "../internal-urls/router"; +import { discoverAndLoadMCPTools } from "../mcp/loader"; +import { MCPManager } from "../mcp/manager"; +import { discoverAuthStorage } from "../session/auth-broker-config"; +import type { AuthStorage } from "../session/auth-storage"; import type { ToolSession } from "../tools"; import { wrapToolWithMetaNotice } from "../tools/output-meta"; import { ReadTool } from "../tools/read"; @@ -17,6 +22,15 @@ export interface ReadCommandArgs { path: string; } +function shouldDiscoverMcp(path: string): boolean { + const match = path.match(/^([a-z][a-z0-9+.-]*):\/\//i); + if (!match) return false; + const scheme = match[1].toLowerCase(); + if (scheme === "mcp") return true; + if (["conflict", "file", "http", "https"].includes(scheme)) return false; + return InternalUrlRouter.instance().getHandler(scheme) === undefined; +} + export async function runReadCommand(cmd: ReadCommandArgs): Promise { if (!cmd.path) { process.stderr.write(chalk.red("error: path is required\n")); @@ -34,9 +48,26 @@ export async function runReadCommand(cmd: ReadCommandArgs): Promise { getSessionSpawns: () => "*", }; - const tool = wrapToolWithMetaNotice(new ReadTool(session)); + let authStorage: AuthStorage | undefined; + let mcpManager: MCPManager | undefined; + let failed = false; try { + if (shouldDiscoverMcp(cmd.path)) { + authStorage = await discoverAuthStorage(); + const result = await discoverAndLoadMCPTools(cwd, { + enableProjectConfig: settings.get("mcp.enableProjectConfig") ?? true, + filterExa: true, + filterBrowser: settings.get("browser.enabled") ?? false, + cacheStorage: settings.getStorage(), + authStorage, + }); + mcpManager = result.manager; + session.mcpManager = mcpManager; + MCPManager.setInstance(mcpManager); + } + + const tool = wrapToolWithMetaNotice(new ReadTool(session)); const result = await tool.execute("omp-read", { path: cmd.path }); for (const block of result.content) { @@ -52,6 +83,14 @@ export async function runReadCommand(cmd: ReadCommandArgs): Promise { } } catch (err) { process.stderr.write(`${chalk.red(renderError(err))}\n`); - process.exit(1); + failed = true; + } finally { + if (mcpManager) { + await mcpManager.disconnectAll(); + if (MCPManager.instance() === mcpManager) MCPManager.setInstance(undefined); + } + authStorage?.close(); } + + if (failed) process.exit(1); } diff --git a/packages/coding-agent/src/internal-urls/mcp-protocol.ts b/packages/coding-agent/src/internal-urls/mcp-protocol.ts index f1db2e591..1b91f620f 100644 --- a/packages/coding-agent/src/internal-urls/mcp-protocol.ts +++ b/packages/coding-agent/src/internal-urls/mcp-protocol.ts @@ -24,7 +24,9 @@ function extractResourceUri(url: InternalUrl): string { const host = url.rawHost || url.hostname; const rawPathname = url.rawPathname ?? url.pathname; const hasPath = rawPathname && rawPathname !== "/"; - const uri = `${host}${hasPath ? rawPathname : ""}${url.search}${url.hash}`.trim(); + const scheme = url.protocol.replace(/:$/, "").toLowerCase(); + const prefix = scheme === "mcp" ? "" : `${scheme}://`; + const uri = `${prefix}${host}${hasPath ? rawPathname : ""}${url.search}${url.hash}`.trim(); if (!uri) { throw new Error("mcp:// URL requires a resource URI: mcp://"); } @@ -95,10 +97,11 @@ function formatAvailableResources(mcpManager: MCPManager): string { } /** - * Protocol handler for mcp:// URLs. + * Protocol handler for MCP resources. * - * URL form: + * URL forms: * - mcp:// (e.g. mcp://test://notes, mcp://ibkr://portfolio/positions) + * - A resource's native URI when its scheme has no OMP handler (e.g. ags://capabilities/current-host) */ export class McpProtocolHandler implements ProtocolHandler { readonly scheme = "mcp"; @@ -111,7 +114,11 @@ export class McpProtocolHandler implements ProtocolHandler { } const uri = extractResourceUri(url); - const targetServer = resolveTargetServer(mcpManager, uri); + let targetServer = resolveTargetServer(mcpManager, uri); + if (!targetServer) { + await Promise.allSettled(mcpManager.getConnectedServers().map(name => mcpManager.ensureServerResources(name))); + targetServer = resolveTargetServer(mcpManager, uri); + } if (!targetServer) { throw new Error( `No MCP server has resource "${uri}".\n\nAvailable resources:\n${formatAvailableResources(mcpManager)}`, diff --git a/packages/coding-agent/src/internal-urls/router.ts b/packages/coding-agent/src/internal-urls/router.ts index 041cda4f8..052eb4bb9 100644 --- a/packages/coding-agent/src/internal-urls/router.ts +++ b/packages/coding-agent/src/internal-urls/router.ts @@ -79,6 +79,17 @@ export class InternalUrlRouter { return this.#handlers.has(match[1].toLowerCase()); } + /** + * Whether read can resolve this URL through either a native handler or the + * MCP resource fallback. MCP resources may use arbitrary custom schemes. + */ + canResolve(input: string): boolean { + const match = input.match(/^([a-z][a-z0-9+.-]*):\/\//i); + if (!match) return false; + const scheme = match[1].toLowerCase(); + return this.#handlers.has(scheme) || this.#isMcpResourceScheme(scheme); + } + /** Schemes whose handler supports host/path autocomplete. */ completionSchemes(): string[] { const schemes: string[] = []; @@ -98,10 +109,16 @@ export class InternalUrlRouter { return handler.complete(query, context); } - #route(input: string): { parsed: InternalUrl; handler: ProtocolHandler } { + #isMcpResourceScheme(scheme: string): boolean { + return !["file", "http", "https"].includes(scheme) && this.#handlers.has("mcp"); + } + + #route(input: string, allowMcpResource = false): { parsed: InternalUrl; handler: ProtocolHandler } { const parsed = parseInternalUrl(input); const scheme = parsed.protocol.replace(/:$/, "").toLowerCase(); - const handler = this.#handlers.get(scheme); + const handler = + this.#handlers.get(scheme) ?? + (allowMcpResource && this.#isMcpResourceScheme(scheme) ? this.#handlers.get("mcp") : undefined); if (!handler) { const available = Array.from(this.#handlers.keys()) .map(candidate => `${candidate}://`) @@ -113,7 +130,7 @@ export class InternalUrlRouter { /** Resolve an internal URL through its registered protocol handler. */ async resolve(input: string, context?: ResolveContext): Promise { - const { parsed, handler } = this.#route(input); + const { parsed, handler } = this.#route(input, true); const resource = await handler.resolve(parsed, context); return { ...resource, immutable: resource.immutable ?? handler.immutable }; } diff --git a/packages/coding-agent/src/mcp/manager.ts b/packages/coding-agent/src/mcp/manager.ts index 12a8e1ad9..e7c2ca64b 100644 --- a/packages/coding-agent/src/mcp/manager.ts +++ b/packages/coding-agent/src/mcp/manager.ts @@ -1041,13 +1041,7 @@ export class MCPManager { async #loadServerResourcesAndPrompts(name: string, connection: MCPServerConnection): Promise { if (serverSupportsResources(connection.capabilities)) { try { - const [resources] = await Promise.all([listResources(connection), listResourceTemplates(connection)]); - - if (this.#notificationsEnabled && connection.capabilities.resources?.subscribe) { - const uris = resources.map(r => r.uri); - const notificationEpoch = this.#notificationsEpoch; - this.#subscribeAndTrack(name, connection, uris, notificationEpoch); - } + await this.refreshServerResources(name); } catch (error) { logger.debug("Failed to load MCP resources", { path: `mcp:${name}`, error }); } @@ -1161,6 +1155,17 @@ export class MCPManager { return promise; } + /** + * Wait until a connected server's resource catalog has been loaded. + * Coalesces with initial loading and notification-driven refreshes. + */ + async ensureServerResources(name: string): Promise { + const connection = this.#connections.get(name); + if (!connection || !serverSupportsResources(connection.capabilities)) return; + if (connection.resources !== undefined && connection.resourceTemplates !== undefined) return; + await this.refreshServerResources(name); + } + /** * Refresh prompts from a specific server. */ diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index 8772f57f8..8c870875a 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -2250,12 +2250,12 @@ export class ReadTool implements AgentTool { return executeReadUrl(this.session, { path: parsedUrlTarget.path, raw: urlRaw }, signal); } - // Handle internal URLs (agent://, artifact://, memory://, skill://, rule://, local://, mcp://, omp://, issue://, pr://). + // Handle native OMP URLs and custom-scheme resources advertised by MCP servers. // Use the internal-URL-aware splitter so malformed selectors are peeled // off the URL and surfaced via parseSel rather than confusing handlers. const internalRouter = InternalUrlRouter.instance(); let promotedSelector: string | undefined; - if (internalRouter.canHandle(readPath)) { + if (internalRouter.canResolve(readPath)) { const internalTarget = splitInternalUrlSel(readPath); const parsed = parseSel(internalTarget.sel); if (internalTarget.sel !== undefined && parsed.kind === "none") { diff --git a/packages/coding-agent/test/fixtures/resources-no-templates-mcp.ts b/packages/coding-agent/test/fixtures/resources-no-templates-mcp.ts index 9d0c09829..0c2a3c0c9 100755 --- a/packages/coding-agent/test/fixtures/resources-no-templates-mcp.ts +++ b/packages/coding-agent/test/fixtures/resources-no-templates-mcp.ts @@ -1,7 +1,7 @@ #!/usr/bin/env bun /** * Test fixture: a stdio MCP server that advertises the `resources` capability - * and serves `resources/list`, but does NOT implement the optional + * and serves `resources/list` plus `resources/read`, but does NOT implement the optional * `resources/templates/list` method — it answers that request with a JSON-RPC * -32601 ("Method not found") error, exactly like jcodemunch/jdocmunch. * @@ -24,7 +24,7 @@ type JsonRpcRequest = { params?: Record; }; -function buildResult(method: string): Record { +function buildResult(method: string, params?: Record): Record { switch (method) { case "initialize": return { @@ -36,6 +36,10 @@ function buildResult(method: string): Record { return { resources: RESOURCE_URIS.map((uri, i) => ({ uri, name: `Resource ${i}` })), }; + case "resources/read": { + const uri = String(params?.uri ?? ""); + return { contents: [{ uri, text: `fixture content for ${uri}` }] }; + } default: return {}; } @@ -66,7 +70,7 @@ function startServer(): void { return; } - const response = { jsonrpc: "2.0" as const, id: msg.id, result: buildResult(msg.method) }; + const response = { jsonrpc: "2.0" as const, id: msg.id, result: buildResult(msg.method, msg.params) }; process.stdout.write(`${JSON.stringify(response)}\n`); }); rl.on("close", () => process.exit(0)); diff --git a/packages/coding-agent/test/internal-urls/mcp-protocol.test.ts b/packages/coding-agent/test/internal-urls/mcp-protocol.test.ts index 3c8b788d0..195bf9842 100644 --- a/packages/coding-agent/test/internal-urls/mcp-protocol.test.ts +++ b/packages/coding-agent/test/internal-urls/mcp-protocol.test.ts @@ -1,17 +1,23 @@ import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import * as os from "node:os"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { InternalUrlRouter } from "@oh-my-pi/pi-coding-agent/internal-urls"; import { MCPManager } from "@oh-my-pi/pi-coding-agent/mcp/manager"; import type { MCPResource, MCPResourceReadResult, MCPResourceTemplate } from "@oh-my-pi/pi-coding-agent/mcp/types"; +import type { ToolSession } from "@oh-my-pi/pi-coding-agent/tools"; +import { ReadTool } from "@oh-my-pi/pi-coding-agent/tools/read"; function createMockManager(opts: { servers?: string[]; resources?: Map; readResult?: MCPResourceReadResult | undefined; readError?: Error; + ensureResources?: (name: string) => Promise; }) { return { getConnectedServers: () => opts.servers ?? [], getServerResources: (name: string) => opts.resources?.get(name), + ensureServerResources: async (name: string) => opts.ensureResources?.(name), readServerResource: async (_name: string, _uri: string) => { if (opts.readError) throw opts.readError; return opts.readResult; @@ -19,6 +25,16 @@ function createMockManager(opts: { } as unknown as MCPManager; } +function createToolSession(): ToolSession { + return { + cwd: os.tmpdir(), + hasUI: false, + settings: Settings.isolated(), + getSessionFile: () => null, + getSessionSpawns: () => "*", + }; +} + describe("McpProtocolHandler", () => { beforeEach(() => { MCPManager.resetForTests(); @@ -76,6 +92,61 @@ describe("McpProtocolHandler", () => { expect(resource.notes).toEqual(["MCP server: my-server"]); }); + it("lets read consume a native URI advertised by an MCP server", async () => { + const resources = new Map(); + resources.set("ags", { + resources: [{ uri: "ags://capabilities/current-host", name: "current-host" }], + templates: [], + }); + const manager = createMockManager({ + servers: ["ags"], + resources, + readResult: { + contents: [{ uri: "ags://capabilities/current-host", text: "host capabilities" }], + }, + }); + MCPManager.setInstance(manager); + + const result = await new ReadTool(createToolSession()).execute("read-ags-resource", { + path: "ags://capabilities/current-host", + }); + const output = result.content.find(block => block.type === "text"); + + expect(output?.type).toBe("text"); + if (output?.type !== "text") throw new Error("Expected text output"); + expect(output.text).toContain("host capabilities"); + }); + + it("waits for the MCP resource catalog before rejecting a native URI", async () => { + const resources = new Map(); + let ensureCalls = 0; + const manager = createMockManager({ + servers: ["ags"], + resources, + ensureResources: async name => { + ensureCalls += 1; + resources.set(name, { + resources: [{ uri: "ags://capabilities/current-host", name: "current-host" }], + templates: [], + }); + }, + readResult: { + contents: [{ uri: "ags://capabilities/current-host", text: "loaded after discovery" }], + }, + }); + MCPManager.setInstance(manager); + + const result = await new ReadTool(createToolSession()).execute("read-delayed-ags-resource", { + path: "ags://capabilities/current-host", + }); + const output = result.content.find(block => block.type === "text"); + + expect(ensureCalls).toBe(1); + expect(output?.type).toBe("text"); + if (output?.type !== "text") throw new Error("Expected text output"); + expect(output.text).toContain("loaded after discovery"); + }); + it("preserves query parameters in MCP resource URI", async () => { const resources = new Map(); resources.set("query-server", { diff --git a/packages/coding-agent/test/mcp-resource-templates-missing.test.ts b/packages/coding-agent/test/mcp-resource-templates-missing.test.ts index 72eca394b..12347f71b 100644 --- a/packages/coding-agent/test/mcp-resource-templates-missing.test.ts +++ b/packages/coding-agent/test/mcp-resource-templates-missing.test.ts @@ -93,18 +93,8 @@ describe("MCPManager loads resources for a templates-less server", () => { try { await manager.connectServers({ docs: config }, {}); - - // Genuine integration wait: `#loadServerResourcesAndPrompts` runs - // fire-and-forget against a real spawned subprocess and exposes no - // completion promise or event to await, and fake timers cannot drive a - // child process. Poll the live manager with a generous ceiling, exiting - // the instant resources arrive (mirrors sdk-mcp-auto-discovery.test.ts). - const deadline = Date.now() + 10_000; - let resources = manager.getServerResources("docs"); - while ((resources?.resources.length ?? 0) === 0 && Date.now() < deadline) { - await Bun.sleep(25); - resources = manager.getServerResources("docs"); - } + await manager.ensureServerResources("docs"); + const resources = manager.getServerResources("docs"); expect(resources).toBeDefined(); // The -32601 from templates/list must NOT discard the concrete resources. diff --git a/packages/coding-agent/test/read-cli-mcp-resource.test.ts b/packages/coding-agent/test/read-cli-mcp-resource.test.ts new file mode 100644 index 000000000..4c44a0d79 --- /dev/null +++ b/packages/coding-agent/test/read-cli-mcp-resource.test.ts @@ -0,0 +1,79 @@ +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { removeWithRetries } from "@oh-my-pi/pi-utils"; + +const CLI_ENTRY = path.join(import.meta.dir, "..", "src", "cli.ts"); +const FIXTURE_PATH = path.join(import.meta.dir, "fixtures", "resources-no-templates-mcp.ts"); + +describe("omp read MCP resources", () => { + let root: string; + let projectDir: string; + let agentDir: string; + + beforeEach(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), "omp-read-mcp-")); + projectDir = path.join(root, "project"); + agentDir = path.join(root, "agent"); + await Promise.all([fs.mkdir(projectDir), fs.mkdir(agentDir)]); + await Bun.write( + path.join(projectDir, ".mcp.json"), + JSON.stringify({ + mcpServers: { + fixture: { + type: "stdio", + command: process.execPath, + args: [FIXTURE_PATH], + }, + }, + }), + ); + }); + + afterEach(async () => { + await removeWithRetries(root); + }); + + async function runRead(resourceUri: string): Promise<{ exitCode: number; output: string; error: string }> { + const proc = Bun.spawn([process.execPath, CLI_ENTRY, "read", resourceUri], { + cwd: projectDir, + stdout: "pipe", + stderr: "pipe", + env: { + ...process.env, + HOME: root, + NO_COLOR: "1", + PI_CODING_AGENT_DIR: agentDir, + }, + }); + const stdout = new Response(proc.stdout).text(); + const stderr = new Response(proc.stderr).text(); + const [exitCode, output, error] = await Promise.all([proc.exited, stdout, stderr]); + return { exitCode, output, error }; + } + + it("discovers MCP before reading a server-advertised native URI", async () => { + const { exitCode, output, error } = await runRead("test://alpha"); + + expect(exitCode).toBe(0); + expect(error).toBe(""); + expect(output).toContain("fixture content for test://alpha"); + }, 30_000); + + it("keeps the mcp:// wrapper working in the standalone CLI", async () => { + const { exitCode, output, error } = await runRead("mcp://test://beta"); + + expect(exitCode).toBe(0); + expect(error).toBe(""); + expect(output).toContain("fixture content for test://beta"); + }, 30_000); + + it("exits after an MCP resource read error", async () => { + const { exitCode, output, error } = await runRead("test://missing"); + + expect(exitCode).toBe(1); + expect(output).toBe(""); + expect(error).toContain('No MCP server has resource "test://missing"'); + }, 30_000); +});