fix(mcp): resolve native resource URIs
This commit is contained in:
@@ -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://<resource-uri>` 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 `<cell>` 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
|
||||
|
||||
@@ -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<void> {
|
||||
if (!cmd.path) {
|
||||
process.stderr.write(chalk.red("error: path is required\n"));
|
||||
@@ -34,9 +48,26 @@ export async function runReadCommand(cmd: ReadCommandArgs): Promise<void> {
|
||||
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<void> {
|
||||
}
|
||||
} 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);
|
||||
}
|
||||
|
||||
@@ -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://<resource-uri>");
|
||||
}
|
||||
@@ -95,10 +97,11 @@ function formatAvailableResources(mcpManager: MCPManager): string {
|
||||
}
|
||||
|
||||
/**
|
||||
* Protocol handler for mcp:// URLs.
|
||||
* Protocol handler for MCP resources.
|
||||
*
|
||||
* URL form:
|
||||
* URL forms:
|
||||
* - mcp://<resource-uri> (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)}`,
|
||||
|
||||
@@ -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<InternalResource> {
|
||||
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 };
|
||||
}
|
||||
|
||||
@@ -1041,13 +1041,7 @@ export class MCPManager {
|
||||
async #loadServerResourcesAndPrompts(name: string, connection: MCPServerConnection): Promise<void> {
|
||||
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<void> {
|
||||
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.
|
||||
*/
|
||||
|
||||
@@ -2250,12 +2250,12 @@ export class ReadTool implements AgentTool<typeof readSchema, ReadToolDetails> {
|
||||
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") {
|
||||
|
||||
@@ -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<string, unknown>;
|
||||
};
|
||||
|
||||
function buildResult(method: string): Record<string, unknown> {
|
||||
function buildResult(method: string, params?: Record<string, unknown>): Record<string, unknown> {
|
||||
switch (method) {
|
||||
case "initialize":
|
||||
return {
|
||||
@@ -36,6 +36,10 @@ function buildResult(method: string): Record<string, unknown> {
|
||||
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));
|
||||
|
||||
@@ -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<string, { resources: MCPResource[]; templates: MCPResourceTemplate[] }>;
|
||||
readResult?: MCPResourceReadResult | undefined;
|
||||
readError?: Error;
|
||||
ensureResources?: (name: string) => Promise<void>;
|
||||
}) {
|
||||
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<string, { resources: MCPResource[]; templates: MCPResourceTemplate[] }>();
|
||||
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<string, { resources: MCPResource[]; templates: MCPResourceTemplate[] }>();
|
||||
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<string, { resources: MCPResource[]; templates: MCPResourceTemplate[] }>();
|
||||
resources.set("query-server", {
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
Reference in New Issue
Block a user