Files
oh-my-pi/packages/coding-agent/test/mcp-roots-list.test.ts
T
Miroslav Drbal [ApoC] 21a86a693d feat(mcp): implement roots/list and server-to-client request handling (#474)
* feat(mcp): implement roots/list and server-to-client request handling

Add support for MCP server-to-client JSON-RPC requests across both
stdio and HTTP transports, enabling servers to query client capabilities
such as roots/list during initialization.

Transport layer (types.ts, stdio.ts, http.ts):
- Add onRequest callback to MCPTransport interface for server-initiated
  requests; add toJsonRpcError helper for error code propagation
- Classify incoming messages by checking method+id (request), id-only
  (response), method-only (notification); guard against id:null per
  JSON-RPC 2.0 spec
- StdioTransport: detect server requests in #handleMessage, respond via
  #sendResponse writing JSON-RPC response to subprocess stdin
- HttpTransport: detect server requests via #dispatchSSEMessage across
  all SSE streams (dedicated listener, POST response drain, notify
  piggybacking), respond via #sendServerResponse POST with proper
  Accept header and session ID
- Refactor startSSEListener to resolve once SSE GET connects (not when
  stream ends), enabling await before notifications/initialized; reset
  #sseConnection via .finally() for reconnection after transient failure
- #parseSSEResponse continues reading after capturing the primary
  response to drain piggybacked server requests/notifications; clears
  timeout after capture so drain phase is unbounded
- notify() reads text/event-stream response bodies for piggybacked
  messages; cancels non-SSE response bodies to release connections
- #sendServerResponse includes AbortSignal.timeout and cancels response
  body; fire-and-forget handlers wrapped in try/catch to prevent
  unhandled rejections

Client wiring (client.ts):
- Add onRequest to connectToServer options, wire to transport before
  initialization
- Add awaitable onInitialized hook in initializeConnection, called
  between initialize response (which sets session ID) and initialized
  notification, so SSE stream is open when server sends roots/list
- Pass only signal to transport.request (not full options object)
- Hoist transport ref to outer scope; close on timeout/abort to prevent
  orphaned transports when SSE GET hangs

Manager (manager.ts):
- Wire onRequest handler in connectServers for all MCP connections
- Handle roots/list by returning project CWD as file:// URI via
  pathToFileURL; return -32601 for unsupported methods

Tests (mcp-roots-list.test.ts):
- toJsonRpcError: code extraction, defaults, non-Error values
- Message classification spec tests: request/response/notification/
  unknown dispatch, id:null and id:0 edge cases
- Roots response shape: file:// URI generation, Windows paths, spaces

* fix(mcp): return SSE response immediately instead of blocking on stream drain

The #parseSSEResponse loop continued iterating the SSE stream after
capturing the response for the expected request ID. Since clearTimeout
was called after capture, a server that holds the SSE stream open for
follow-up events (permitted by Streamable HTTP) would block the
request() call indefinitely.

Return the result as soon as it's captured and drain remaining
messages in a detached background task via #readSSEStream, which
already handles dispatch and error swallowing.

* fix(mcp): handle batched JSON-RPC messages in both transports

JSON-RPC 2.0 section 6 allows sending an array of request/notification
objects as a batch. If a server sent a batch, the message classifier
in both transports would fail the 'method in message' check on the
array object and silently drop all contained messages.

Add an Array.isArray guard at the top of #handleMessage (stdio) and
#dispatchSSEMessage (http) that recurses into each element. Defensive
measure — no known MCP server sends batches today, but the guard is
cheap and correct per the JSON-RPC spec.

* fix(mcp): address second Codex review round

- http: break from SSE loop before starting background drain to avoid
  ReadableStream locked error (the for-await iterator still holds the
  reader when #drainSSEBackground was called inline)
- types: toJsonRpcError now accepts plain { code, message } objects,
  not just Error instances, so onRequest handlers can throw structured
  JSON-RPC errors without wrapping in Error
- test: relax Windows path name assertion to toBeTruthy since
  path.basename is platform-dependent for backslash paths; add tests
  for plain-object toJsonRpcError

* fix(mcp): address third Codex review round

- parseSSEResponse: flatten JSON-RPC batch arrays before checking for
  the expected response, so a server that batches the primary response
  with piggybacked requests/notifications in a single SSE event still
  has the response extracted correctly
- sendServerResponse: retry once on 401/403 via onAuthError, matching
  the auth-refresh logic in #executeRequest; prevents server-initiated
  request replies from failing after token expiry on long-lived SSE
  sessions

---------

Co-authored-by: Miroslav Drbal <miroslav.drbal@gendigital.com>
2026-03-18 23:02:22 +01:00

120 lines
4.7 KiB
TypeScript

import { describe, expect, it } from "bun:test";
import * as path from "node:path";
import * as url from "node:url";
import { toJsonRpcError } from "../src/mcp/types";
describe("toJsonRpcError", () => {
it("extracts code from Error with .code property", () => {
const err = Object.assign(new Error("not found"), { code: -32601 });
const result = toJsonRpcError(err);
expect(result).toEqual({ code: -32601, message: "not found" });
});
it("defaults to -32603 when Error has no code", () => {
const result = toJsonRpcError(new Error("boom"));
expect(result).toEqual({ code: -32603, message: "boom" });
});
it("handles non-Error values", () => {
const result = toJsonRpcError("string error");
expect(result).toEqual({ code: -32603, message: "Internal error" });
});
it("ignores non-numeric code", () => {
const err = Object.assign(new Error("bad"), { code: "ENOENT" });
expect(toJsonRpcError(err).code).toBe(-32603);
});
it("preserves code and message from plain objects", () => {
const result = toJsonRpcError({ code: -32601, message: "Method not found" });
expect(result).toEqual({ code: -32601, message: "Method not found" });
});
it("falls back for plain objects missing code or message", () => {
expect(toJsonRpcError({ code: 42 })).toEqual({ code: -32603, message: "Internal error" });
expect(toJsonRpcError({ message: "hi" })).toEqual({ code: -32603, message: "Internal error" });
expect(toJsonRpcError(null)).toEqual({ code: -32603, message: "Internal error" });
});
});
describe("message classification", () => {
// Specification test: pins the expected JSON-RPC message classification rules.
// Does not exercise the actual transport methods — changes to #handleMessage
// won't fail this test. Tests the contract shape, not the wiring.
function classify(message: Record<string, unknown>): "request" | "response" | "notification" | "unknown" {
// Mirrors the classification in StdioTransport.#handleMessage
if ("method" in message && "id" in message && message.id != null) return "request";
if ("id" in message && message.id != null) return "response";
if ("method" in message) return "notification";
return "unknown";
}
it("classifies server request (method + id)", () => {
expect(classify({ jsonrpc: "2.0", method: "roots/list", id: 1 })).toBe("request");
expect(classify({ jsonrpc: "2.0", method: "roots/list", id: "abc" })).toBe("request");
expect(classify({ jsonrpc: "2.0", method: "roots/list", id: 0 })).toBe("request");
});
it("classifies response (id, no method)", () => {
expect(classify({ jsonrpc: "2.0", id: 1, result: {} })).toBe("response");
expect(classify({ jsonrpc: "2.0", id: 1, error: { code: -1, message: "fail" } })).toBe("response");
expect(classify({ jsonrpc: "2.0", id: 0, result: {} })).toBe("response");
});
it("classifies notification (method, no id)", () => {
expect(classify({ jsonrpc: "2.0", method: "notifications/tools/list_changed" })).toBe("notification");
});
it("treats id:null as notification, not request", () => {
// Per JSON-RPC 2.0 spec, id MUST NOT be null in requests
expect(classify({ jsonrpc: "2.0", method: "roots/list", id: null })).toBe("notification");
});
it("classifies message without id key as notification", () => {
// When id key is absent entirely (vs present with null value)
expect(classify({ jsonrpc: "2.0", method: "notifications/tools/list_changed", params: {} })).toBe("notification");
});
it("classifies message with neither method nor id as unknown", () => {
expect(classify({ jsonrpc: "2.0" })).toBe("unknown");
});
});
describe("roots response shape", () => {
// Specification test: pins the MCP roots/list response shape.
// Does not exercise MCPManager.#getRoots — tests the contract, not the wiring.
function getRoots(cwd: string): { roots: Array<{ uri: string; name: string }> } {
return {
roots: [
{
uri: url.pathToFileURL(cwd).href,
name: path.basename(cwd),
},
],
};
}
it("returns a single root with file:// URI and directory name", () => {
const result = getRoots("/home/user/project");
expect(result.roots).toHaveLength(1);
expect(result.roots[0].uri).toStartWith("file:///");
expect(result.roots[0].name).toBe("project");
});
it("produces valid file:// URI on Windows-style paths", () => {
// path.basename and pathToFileURL are platform-dependent for
// Windows paths; only assert the URI format, not the name.
const result = getRoots("C:\\Users\\dev\\myproject");
expect(result.roots[0].uri).toMatch(/^file:\/\/\//);
expect(result.roots[0].name).toBeTruthy();
});
it("handles paths with spaces", () => {
const result = getRoots("/home/user/my project");
expect(result.roots[0].uri).toContain("my%20project");
expect(result.roots[0].name).toBe("my project");
});
});