fix(mcp): preserved native resource URIs and opaque scheme routing
- Native (non-mcp://) resource URIs now pass through byte-for-byte via rawHref; slash elision applies only to the legacy mcp:// wrapper, so catalog://root/ style URIs match exact-equality server lookups. - resources/templates/list failure no longer discards a successful resources/list (Promise.allSettled; templates retried later). - Opaque RFC 3986 URIs (urn:doc, custom:item) are recognized by both the router and read-cli discovery gates, with drive-path and read-selector false positives guarded. - Review follow-up for PR #6790.
This commit is contained in:
@@ -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://<resource-uri>` 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.
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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://<resource-uri>` 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://<resource-uri>");
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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. */
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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<string, { resources: MCPResource[]; templates: MCPResourceTemplate[] }>();
|
||||
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<string, { resources: MCPResource[]; templates: MCPResourceTemplate[] }>();
|
||||
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<string, { resources: MCPResource[]; templates: MCPResourceTemplate[] }>();
|
||||
resources.set("query-server", {
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
|
||||
@@ -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");
|
||||
|
||||
|
||||
Reference in New Issue
Block a user