diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 67a9380aa..d09ae5d74 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -17,6 +17,7 @@ - Fixed a disabled higher-priority MCP server no longer disabling a same-named lower-priority one: disabled servers are now suppressed after key-level dedupe instead of dropped before it, so a project `foo` with `enabled: false` keeps the user-level `foo` off while still not starving a differently-named equivalent connection. - Fixed the MCP tool-name collision winner flipping when the current owner reconnects: the winner is now chosen by a stable server+tool key instead of tool-array insertion order, which reconnects reorder. - 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 three gaps in native MCP resource URI resolution: server-advertised URIs whose path is exactly `/` (e.g. `catalog://root/`) are now preserved byte-for-byte instead of losing the trailing slash to reconstruction; opaque resource URIs (`urn:example:document`, `custom:item`) are recognized by the `read` and `omp read` resolver gates instead of falling through to filesystem handling; and a failing `resources/templates/list` no longer discards a successful `resources/list`, which previously produced a false missing-resource error. - Fixed custom LSP servers sending `languageId: "plaintext"` for extensions outside the built-in language map by honoring an optional per-server `languageId` in `lsp.json` for disk and in-memory document opens ([#6800](https://github.com/can1357/oh-my-pi/issues/6800)). - Fixed interactive extension confirmations ignoring `dialogOptions`, and cancelled handler-owned dialogs when the extension watchdog times out so stale approval UI cannot outlive a blocked tool call ([#6805](https://github.com/can1357/oh-my-pi/issues/6805)). - Fixed the per-handler extension context snapshotting the live `ctx.model` getter, so a handler calling `pi.setModel()` and then reading `ctx.model` saw the stale model; the scoped context now delegates to the base context instead of spreading it. diff --git a/packages/coding-agent/src/cli/read-cli.ts b/packages/coding-agent/src/cli/read-cli.ts index 4d7292917..52c9f86d2 100644 --- a/packages/coding-agent/src/cli/read-cli.ts +++ b/packages/coding-agent/src/cli/read-cli.ts @@ -8,6 +8,7 @@ import { getProjectDir } from "@oh-my-pi/pi-utils"; import chalk from "chalk"; import { Settings } from "../config/settings"; +import { extractUriScheme } from "../internal-urls/parse"; import { InternalUrlRouter } from "../internal-urls/router"; import { discoverAndLoadMCPTools } from "../mcp/loader"; import { MCPManager } from "../mcp/manager"; @@ -23,9 +24,11 @@ export interface ReadCommandArgs { } function shouldDiscoverMcp(path: string): boolean { - const match = path.match(/^([a-z][a-z0-9+.-]*):\/\//i); - if (!match) return false; - const scheme = match[1].toLowerCase(); + // MCP resource URIs may be hierarchical (`test://notes`) or opaque + // (`urn:example:document`); `extractUriScheme` recognizes both while + // rejecting Windows drive paths and selector-shaped filesystem inputs. + const scheme = extractUriScheme(path); + if (!scheme) return false; if (scheme === "mcp") return true; if (["conflict", "file", "http", "https"].includes(scheme)) return false; return InternalUrlRouter.instance().getHandler(scheme) === undefined; diff --git a/packages/coding-agent/src/internal-urls/mcp-protocol.ts b/packages/coding-agent/src/internal-urls/mcp-protocol.ts index 1b91f620f..2dbc72f13 100644 --- a/packages/coding-agent/src/internal-urls/mcp-protocol.ts +++ b/packages/coding-agent/src/internal-urls/mcp-protocol.ts @@ -21,12 +21,19 @@ function getUriTemplateMatchScore( } function extractResourceUri(url: InternalUrl): string { + const scheme = url.protocol.replace(/:$/, "").toLowerCase(); + if (scheme !== "mcp") { + // Server-advertised native URI (hierarchical or opaque). Preserve the + // input byte-for-byte: `resolveTargetServer` matches by exact string + // equality, so e.g. `catalog://root/` must keep its trailing slash. + return url.rawHref ?? url.href; + } + // Legacy `mcp://` wrapper: reconstruct the wrapped URI and + // elide a bare trailing `/` that URL parsing adds to host-only forms. const host = url.rawHost || url.hostname; const rawPathname = url.rawPathname ?? url.pathname; const hasPath = rawPathname && rawPathname !== "/"; - const scheme = url.protocol.replace(/:$/, "").toLowerCase(); - const prefix = scheme === "mcp" ? "" : `${scheme}://`; - const uri = `${prefix}${host}${hasPath ? rawPathname : ""}${url.search}${url.hash}`.trim(); + const uri = `${host}${hasPath ? rawPathname : ""}${url.search}${url.hash}`.trim(); if (!uri) { throw new Error("mcp:// URL requires a resource URI: mcp://"); } diff --git a/packages/coding-agent/src/internal-urls/parse.ts b/packages/coding-agent/src/internal-urls/parse.ts index 3153412f4..9f698d807 100644 --- a/packages/coding-agent/src/internal-urls/parse.ts +++ b/packages/coding-agent/src/internal-urls/parse.ts @@ -13,6 +13,36 @@ import type { InternalUrl } from "./types"; const SCHEME_HOST_RE = /^([a-z][a-z0-9+.-]*):\/\/([^/?#]*)/i; const PATHNAME_RE = /^[a-z][a-z0-9+.-]*:\/\/[^/?#]*(\/[^?#]*)?/i; +// Opaque URI form (`urn:example:document`, `custom:item`) — an RFC 3986 scheme +// followed by `:` without `//`. Guarded separately in `extractUriScheme`. +const OPAQUE_URI_RE = /^([a-z][a-z0-9+.-]*):(.+)$/is; +// A read-tool selector chain (`12`, `1-20,30+5`, `raw`, `raw:2-4`, `conflicts`) +// so `Makefile:12` or `notes:raw` is not mistaken for an opaque URI. +const SELECTOR_CHUNK_SRC = String.raw`(?:raw|conflicts|-?\d+(?:[-+]\d+)?(?:,\d+(?:[-+]\d+)?)*)`; +const SELECTOR_CHAIN_RE = new RegExp(`^${SELECTOR_CHUNK_SRC}(?::${SELECTOR_CHUNK_SRC})*$`, "i"); + +/** + * Extract the lowercased scheme from a URI-shaped input, or `undefined` when + * the input does not look like a URI. + * + * Accepts both hierarchical (`scheme://…`) and opaque (`scheme:rest`) forms — + * MCP resource URIs may be opaque (`urn:example:document`, `custom:item`). + * The opaque form is guarded against path-like false positives: + * - Windows drive paths (`C:\…`, `C:/…`, `C:foo`) — single-letter scheme. + * - Filenames with extensions (`foo.ts:50`) — dot in the scheme segment. + * - Read-tool selector tails (`Makefile:12`, `README:raw:1-20`). + */ +export function extractUriScheme(input: string): string | undefined { + const hierarchical = input.match(SCHEME_HOST_RE); + if (hierarchical) return hierarchical[1].toLowerCase(); + const opaque = input.match(OPAQUE_URI_RE); + if (!opaque) return undefined; + const [, scheme, rest] = opaque; + if (scheme.length === 1) return undefined; + if (scheme.includes(".")) return undefined; + if (SELECTOR_CHAIN_RE.test(rest)) return undefined; + return scheme.toLowerCase(); +} /** * Parse an internal URL into an InternalUrl. @@ -68,5 +98,6 @@ export function parseInternalUrl(input: string): InternalUrl { const result = parsed as InternalUrl; result.rawHost = rawHost; result.rawPathname = pathMatch?.[1] ?? parsed.pathname; + result.rawHref = input; return result; } diff --git a/packages/coding-agent/src/internal-urls/router.ts b/packages/coding-agent/src/internal-urls/router.ts index 052eb4bb9..5e9970f74 100644 --- a/packages/coding-agent/src/internal-urls/router.ts +++ b/packages/coding-agent/src/internal-urls/router.ts @@ -13,7 +13,7 @@ import { LocalProtocolHandler } from "./local-protocol"; import { McpProtocolHandler } from "./mcp-protocol"; import { MemoryProtocolHandler } from "./memory-protocol"; import { OmpProtocolHandler } from "./omp-protocol"; -import { parseInternalUrl } from "./parse"; +import { extractUriScheme, parseInternalUrl } from "./parse"; import { RuleProtocolHandler } from "./rule-protocol"; import { SkillProtocolHandler } from "./skill-protocol"; import { SshProtocolHandler } from "./ssh-protocol"; @@ -81,13 +81,16 @@ export class InternalUrlRouter { /** * Whether read can resolve this URL through either a native handler or the - * MCP resource fallback. MCP resources may use arbitrary custom schemes. + * MCP resource fallback. MCP resources may use arbitrary custom schemes and + * may be opaque (`urn:example:document`) rather than hierarchical. */ 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); + const scheme = extractUriScheme(input); + if (!scheme) return false; + // Registered handlers only accept the hierarchical `scheme://` form; + // opaque inputs reach the MCP resource fallback alone. + if (this.#handlers.has(scheme)) return this.canHandle(input); + return this.#isMcpResourceScheme(scheme); } /** Schemes whose handler supports host/path autocomplete. */ diff --git a/packages/coding-agent/src/internal-urls/types.ts b/packages/coding-agent/src/internal-urls/types.ts index a1ae61f87..4890e6202 100644 --- a/packages/coding-agent/src/internal-urls/types.ts +++ b/packages/coding-agent/src/internal-urls/types.ts @@ -70,6 +70,12 @@ export interface InternalUrl extends URL { * Raw pathname extracted from input, preserving traversal markers before URL normalization. */ rawPathname?: string; + /** + * Exact input string this URL was parsed from, before any normalization. + * Set by `parseInternalUrl`; used where byte-exact URI matching matters + * (e.g. MCP resource URIs compared by string equality). + */ + rawHref?: string; } /** diff --git a/packages/coding-agent/src/mcp/manager.ts b/packages/coding-agent/src/mcp/manager.ts index e7c2ca64b..3c0b2a7f5 100644 --- a/packages/coding-agent/src/mcp/manager.ts +++ b/packages/coding-agent/src/mcp/manager.ts @@ -1101,8 +1101,17 @@ export class MCPManager { connection.resources = undefined; connection.resourceTemplates = undefined; - // Reload - const [resources] = await Promise.all([listResources(connection), listResourceTemplates(connection)]); + // Reload. Template listing failures must not discard a successful + // resources/list — let both settle, then continue without templates. + const [resourcesResult, templatesResult] = await Promise.allSettled([ + listResources(connection), + listResourceTemplates(connection), + ]); + if (templatesResult.status === "rejected") { + logger.debug("Failed to list MCP resource templates", { path: `mcp:${name}`, error: templatesResult.reason }); + } + if (resourcesResult.status === "rejected") throw resourcesResult.reason; + const resources = resourcesResult.value; if (this.#notificationsEnabled && connection.capabilities.resources?.subscribe) { const newUris = new Set(resources.map(r => r.uri)); const oldUris = this.#subscribedResources.get(name); 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 0c2a3c0c9..2d815f1cd 100755 --- a/packages/coding-agent/test/fixtures/resources-no-templates-mcp.ts +++ b/packages/coding-agent/test/fixtures/resources-no-templates-mcp.ts @@ -15,7 +15,16 @@ import * as readline from "node:readline"; /** Concrete resource URIs the fixture advertises via `resources/list`. */ -export const RESOURCE_URIS = ["test://alpha", "test://beta"]; +export const RESOURCE_URIS = ["test://alpha", "test://beta", "urn:fixture:gamma"]; + +/** + * JSON-RPC error code returned for `resources/templates/list`. Defaults to + * -32601 ("Method not found"); tests may override via the + * `FIXTURE_TEMPLATES_ERROR_CODE` env var (e.g. -32603) to simulate a server + * whose templates listing fails outright. + */ +const TEMPLATES_ERROR_CODE = Number(process.env.FIXTURE_TEMPLATES_ERROR_CODE ?? "-32601"); +const TEMPLATES_ERROR_MESSAGE = TEMPLATES_ERROR_CODE === -32601 ? "Method not found" : "Internal error"; type JsonRpcRequest = { jsonrpc: "2.0"; @@ -60,11 +69,11 @@ function startServer(): void { if (msg.id === undefined || msg.id === null) return; if (msg.method === "resources/templates/list") { - // Optional method this server doesn't implement. + // Optional method this server doesn't implement (or fails, per env). const error = { jsonrpc: "2.0" as const, id: msg.id, - error: { code: -32601, message: "Method not found" }, + error: { code: TEMPLATES_ERROR_CODE, message: TEMPLATES_ERROR_MESSAGE }, }; process.stdout.write(`${JSON.stringify(error)}\n`); return; 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 195bf9842..96c3b43ca 100644 --- a/packages/coding-agent/test/internal-urls/mcp-protocol.test.ts +++ b/packages/coding-agent/test/internal-urls/mcp-protocol.test.ts @@ -147,6 +147,61 @@ describe("McpProtocolHandler", () => { expect(output.text).toContain("loaded after discovery"); }); + it("resolves a native URI whose path is exactly a trailing slash", async () => { + const resources = new Map(); + resources.set("catalog", { + resources: [{ uri: "catalog://root/", name: "root" }], + templates: [], + }); + const manager = createMockManager({ + servers: ["catalog"], + resources, + readResult: { contents: [{ uri: "catalog://root/", text: "catalog root" }] }, + }); + MCPManager.setInstance(manager); + const router = InternalUrlRouter.instance(); + + const resource = await router.resolve("catalog://root/"); + expect(resource.content).toBe("catalog root"); + expect(resource.notes).toEqual(["MCP server: catalog"]); + }); + + it("resolves an opaque resource URI advertised by an MCP server", async () => { + const resources = new Map(); + resources.set("registry", { + resources: [{ uri: "urn:example:document", name: "document" }], + templates: [], + }); + const manager = createMockManager({ + servers: ["registry"], + resources, + readResult: { contents: [{ uri: "urn:example:document", text: "opaque payload" }] }, + }); + MCPManager.setInstance(manager); + + const result = await new ReadTool(createToolSession()).execute("read-opaque-resource", { + path: "urn:example:document", + }); + 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("opaque payload"); + }); + + it("recognizes opaque URIs in canResolve without swallowing path-like inputs", () => { + const router = InternalUrlRouter.instance(); + expect(router.canResolve("urn:example:document")).toBe(true); + expect(router.canResolve("custom:item")).toBe(true); + // Windows drive paths and selector-shaped filesystem inputs stay on the + // filesystem path. + expect(router.canResolve("C:\\Temp\\notes.txt")).toBe(false); + expect(router.canResolve("C:/tmp/notes.txt")).toBe(false); + expect(router.canResolve("Makefile:12")).toBe(false); + expect(router.canResolve("foo.ts:50-80")).toBe(false); + expect(router.canResolve("README:raw")).toBe(false); + }); + 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 12347f71b..bb2ff1469 100644 --- a/packages/coding-agent/test/mcp-resource-templates-missing.test.ts +++ b/packages/coding-agent/test/mcp-resource-templates-missing.test.ts @@ -105,4 +105,30 @@ describe("MCPManager loads resources for a templates-less server", () => { await manager.disconnectAll(); } }, 20_000); + + it("keeps concrete resources when resources/templates/list fails outright", async () => { + const manager = new MCPManager(workDir); + const config: MCPStdioServerConfig = { + type: "stdio", + command: BUN_EXEC, + args: [FIXTURE_PATH], + // Fixture answers resources/templates/list with -32603 instead of + // -32601 — a hard failure, not "method not found". + env: { FIXTURE_TEMPLATES_ERROR_CODE: "-32603" }, + }; + + try { + await manager.connectServers({ docs: config }, {}); + await manager.ensureServerResources("docs"); + const resources = manager.getServerResources("docs"); + + expect(resources).toBeDefined(); + // The still-in-flight resources/list result must not be discarded. + expect(resources?.resources.map(r => r.uri).sort()).toEqual([...RESOURCE_URIS].sort()); + // Templates listing failed; treated as none until a later refresh. + expect(resources?.templates).toEqual([]); + } finally { + await manager.disconnectAll(); + } + }, 20_000); }); diff --git a/packages/coding-agent/test/read-cli-mcp-resource.test.ts b/packages/coding-agent/test/read-cli-mcp-resource.test.ts index 4c44a0d79..80bbed7cf 100644 --- a/packages/coding-agent/test/read-cli-mcp-resource.test.ts +++ b/packages/coding-agent/test/read-cli-mcp-resource.test.ts @@ -61,6 +61,14 @@ describe("omp read MCP resources", () => { expect(output).toContain("fixture content for test://alpha"); }, 30_000); + it("discovers MCP before reading a server-advertised opaque URI", async () => { + const { exitCode, output, error } = await runRead("urn:fixture:gamma"); + + expect(exitCode).toBe(0); + expect(error).toBe(""); + expect(output).toContain("fixture content for urn:fixture:gamma"); + }, 30_000); + it("keeps the mcp:// wrapper working in the standalone CLI", async () => { const { exitCode, output, error } = await runRead("mcp://test://beta");