diff --git a/clients/cli/README.md b/clients/cli/README.md index e1b3c4ab5..d524c840f 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -78,7 +78,9 @@ npx @modelcontextprotocol/inspector --cli https://my-mcp-server.example.com --tr npx @modelcontextprotocol/inspector --cli https://my-mcp-server.example.com --transport http --method tools/list --header "X-API-Key: your-api-key" ``` -When a server is loaded from a `--catalog`/`--config` file, its per-server settings (headers, connection/request timeouts, and OAuth) are applied to the connection — the same resolution the TUI uses. A `--header` flag overrides the file's headers for that run while leaving the file's timeouts and OAuth in place. +When a server is loaded from a `--catalog`/`--config` file, its per-server settings (headers, connection/request timeouts, OAuth, and `roots`) are applied to the connection — the same resolution the TUI uses. A `--header` flag overrides the file's headers for that run while leaving the file's timeouts and OAuth in place. + +The file is the only durable way to give a run its roots: there is no roots flag, and `--method roots/set` applies only to that one short-lived connection. Roots configured for a server (the same field the web UI's Server Settings writes) are advertised at connect, so a server that asks for `roots/list` — `@modelcontextprotocol/server-filesystem` does, to learn its allowed directories — gets them. **Environment-variable semantics.** `MCP_CATALOG_PATH` is honored only when no ad-hoc target is given (positional command, `--server-url`, or `--transport`) — so a shell that exports it can still run one-off ad-hoc invocations without hitting the catalog/ad-hoc conflict. `MCP_STORAGE_DIR` sets the storage directory used by the OAuth persist backend (`/oauth.json`); the per-file `MCP_INSPECTOR_OAUTH_STATE_PATH` override still takes precedence over it. diff --git a/clients/cli/__tests__/cli.test.ts b/clients/cli/__tests__/cli.test.ts index ca3931008..832cb58a5 100644 --- a/clients/cli/__tests__/cli.test.ts +++ b/clients/cli/__tests__/cli.test.ts @@ -20,6 +20,7 @@ import { getTestMcpServerCommand, createTestServerHttp, createEchoTool, + createListRootsTool, createTestServerInfo, } from "@modelcontextprotocol/inspector-test-server"; import type { MCPServerConfig } from "@modelcontextprotocol/inspector-core/mcp/index.js"; @@ -325,6 +326,89 @@ describe("CLI Tests", () => { }); }); + describe("Roots capability (#1797)", () => { + it("answers a server's roots/list instead of -32601", async () => { + // The CLI used to omit `roots` when constructing its InspectorClient, so + // the capability was never advertised and no `roots/list` handler was + // registered — a server that asked got -32601 Method not found. (And + // `--method roots/set` could not announce the change: the SDK refuses + // `roots/list_changed` from a client that never declared it, so nothing + // reached the wire.) `cli.ts` now always passes the `roots` option — + // empty on this ad-hoc path, since there is no config file. + const server = createTestServerHttp({ + serverInfo: createTestServerInfo(), + tools: [createListRootsTool()], + }); + try { + await server.start(); + + const result = await runCli([ + server.url, + "--cli", + "--method", + "tools/call", + "--tool-name", + "list_roots", + ]); + + expectCliSuccess(result); + const json = expectValidJson(result); + // The tool asks the client for roots and renders what it got. An + // unregistered handler surfaces as an isError result quoting -32601. + expect(json.isError).toBeFalsy(); + expect(JSON.stringify(json)).toContain("Roots:"); + expect(JSON.stringify(json)).not.toContain("-32601"); + } finally { + await server.stop(); + } + }); + + it("answers with the roots configured for the server in the config file", async () => { + // Answering `roots/list` is only half the fix: the roots a user + // configured in mcp.json have to be the answer. They ride in on + // `serverSettings.roots` (lifted by `mcpConfigToServerEntries`), which is + // what web passes too — a CLI that seeded `[]` would let a server fall + // back to its own defaults, the outcome #1797 is about. + const server = createTestServerHttp({ + serverInfo: createTestServerInfo(), + tools: [createListRootsTool()], + }); + let configPath: string | undefined; + try { + await server.start(); + configPath = createTestConfig({ + mcpServers: { + web: { + type: "streamable-http", + url: server.url, + roots: [{ uri: "file:///configured", name: "Configured" }], + } as unknown as MCPServerConfig, + }, + }); + + const result = await runCli([ + "--config", + configPath, + "--server", + "web", + "--cli", + "--method", + "tools/call", + "--tool-name", + "list_roots", + ]); + + expectCliSuccess(result); + const json = expectValidJson(result); + expect(json.isError).toBeFalsy(); + expect(JSON.stringify(json)).toContain("file:///configured"); + } finally { + await server.stop(); + if (configPath) deleteConfigFile(configPath); + } + }); + }); + describe("Config-file settings lifting (#1482)", () => { it("applies a config file's custom header on a tools/call over HTTP", async () => { const server = createTestServerHttp({ diff --git a/clients/cli/__tests__/run-method.test.ts b/clients/cli/__tests__/run-method.test.ts index 242e0cd03..783ba60b2 100644 --- a/clients/cli/__tests__/run-method.test.ts +++ b/clients/cli/__tests__/run-method.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, afterEach } from "vitest"; +import { describe, it, expect, afterEach, vi } from "vitest"; import { getTestMcpServerCommand } from "@modelcontextprotocol/inspector-test-server"; import { InspectorClient } from "@inspector/core/mcp/index.js"; import { createTransportNode } from "@inspector/core/mcp/node/index.js"; @@ -25,6 +25,9 @@ describe("runMethod", () => { progress: false, sample: false, elicit: false, + // Mirrors `cli.ts`, which always passes the option so the capability is + // negotiated and the `roots/list` handler registered (#1797). + roots: [], }, ); await client.connect(); @@ -92,6 +95,33 @@ describe("runMethod", () => { } }); + it("roots/set drops a malformed root rather than advertising it", async () => { + // `--roots-json` only checks that the payload parses to an array, so a + // root with no `uri` used to be stored and then handed to the server in + // the `roots/list` reply — an invalid Root on the wire, echoed back to the + // user as success. `setRoots` normalizes through `cleanRoots` now (#1797). + const c = await connectStdio(); + // cleanRoots reports the drop; suppress it here and assert it happened, so + // the test also covers that a dropped root is not dropped silently. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + const set = await runMethod(c, { + method: "roots/set", + rootsJson: JSON.stringify([ + { name: "no uri" }, + { uri: "file:///keep" }, + ]), + }); + expect(set.kind).toBe("result"); + if (set.kind === "result") { + expect(set.result.roots).toEqual([{ uri: "file:///keep" }]); + } + expect(warn).toHaveBeenCalled(); + } finally { + warn.mockRestore(); + } + }); + it("consumeMethodOutcome writes result json", async () => { let stdout = ""; const original = process.stdout.write; diff --git a/clients/cli/src/cli.ts b/clients/cli/src/cli.ts index a672be42f..87dbceb10 100644 --- a/clients/cli/src/cli.ts +++ b/clients/cli/src/cli.ts @@ -15,6 +15,7 @@ import { listServerEntries, showServerEntry } from "./handlers/servers-list.js"; import { writeFormattedResult } from "./handlers/format-output.js"; import { clearStoredAuthForRelogin } from "./clear-stored-auth-for-relogin.js"; import { InspectorClient } from "@inspector/core/mcp/index.js"; +import { cleanRoots } from "@inspector/core/mcp/serverList.js"; import { createTransportNode, loadServerEntries, @@ -152,6 +153,15 @@ async function callMethod( progress: false, sample: false, elicit: false, + // Advertise the roots configured for this server in mcp.json, exactly as + // web does (`App.tsx`) so both answer `roots/list` with the same content. + // Passing the option (even empty) is what negotiates `capabilities.roots` + // at `initialize` and registers the `roots/list` handler. Omitting it meant + // a server that asks for roots on its own — `server-filesystem` does, at + // `initialize` — got -32601, and `--method roots/set` could not announce + // the change at all: the SDK refuses `roots/list_changed` from a client + // that never declared it, which `setRoots` logged as a send failure (#1797). + roots: cleanRoots(serverSettings?.roots ?? []), serverSettings, // Per-server protocol era (SEP §7.8) from mcp.json → SDK versionNegotiation. // Absent era defaults to legacy in the InspectorClient constructor (#1626). diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index f4adc1eab..62c9e7432 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -20,6 +20,7 @@ import type { GetPromptResult, } from "@modelcontextprotocol/client"; import { InspectorClient } from "@inspector/core/mcp/index.js"; +import { cleanRoots } from "@inspector/core/mcp/serverList.js"; import { eraToVersionNegotiation } from "@inspector/core/mcp/types.js"; import { ManagedToolsState, @@ -304,6 +305,11 @@ function App({ defaultMetadata, }), ...(savedSettings && { serverSettings: savedSettings }), + // Advertise the roots configured for this server in mcp.json, as web + // and the CLI do. The TUI has no roots editor, but a user who set + // roots in the web UI expects them to apply to the same server here + // (#1797). + roots: cleanRoots(savedSettings?.roots ?? []), // Per-server protocol era (SEP §7.8) from mcp.json → SDK // versionNegotiation; absent era defaults to legacy (#1626). ...(savedSettings?.protocolEra && { diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index f37791a9c..aa46f6518 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -44,9 +44,9 @@ import type { } from "@inspector/core/mcp/types.js"; import { DEFAULT_MAX_FETCH_REQUESTS, - DEFAULT_MODERN_LOG_LEVEL, DEFAULT_TASK_TTL_MS, eraToVersionNegotiation, + resolveModernLogLevel, } from "@inspector/core/mcp/types.js"; import { API_SERVER_ENV_VARS, @@ -1216,7 +1216,27 @@ function App() { setConsoleUi(EMPTY_CONSOLE_UI); setProgressByTaskId({}); setCurrentLogLevel("info"); - setModernLogLevel(null); + // Re-seed rather than blank: the client restores its own opt-in from the + // server setting at connect (`resetSessionState`), so blanking here would + // leave the control reading Off while every modern request still carries + // the level — visible on the auth-recovery path, which reconnects the same + // client instance rather than rebuilding it (#1629, #1797). + // `activeServerIdRef` is synced in a passive effect, so it still holds the + // outgoing server's id when this runs from `onDisconnect` — which is what + // lets the re-seed find its settings. Clearing that ref eagerly would take + // the no-server branch below and silently drop this to Off. + // Branch on the *server*, not its settings: an entry with no settings node + // is the common case (`mcp.json` written by hand, never opened in Server + // Settings), and there the default is right — it is what the seed and the + // client both use. Only "no server at all" means Off. + const activeServer = serversRef.current.find( + (s) => s.id === activeServerIdRef.current, + ); + setModernLogLevel( + activeServer + ? (resolveModernLogLevel(activeServer.settings) ?? null) + : null, + ); setPendingStepUp(null); setPendingReauth(null); setReAuthBanner(null); @@ -2337,9 +2357,7 @@ function App() { // setting so the Logs-tab control reflects what the client stamps by // default (the client was seeded the same way in its constructor). "off" // means not opted in (null). Only affects modern connections. - const seededModernLevel = - savedSettings?.modernLogLevel ?? DEFAULT_MODERN_LOG_LEVEL; - setModernLogLevel(seededModernLevel === "off" ? null : seededModernLevel); + setModernLogLevel(resolveModernLogLevel(savedSettings) ?? null); setManagedToolsState(new ManagedToolsState(client)); setPagedToolsState(new PagedToolsState(client)); setPagedPromptsState(new PagedPromptsState(client)); diff --git a/clients/web/src/test/core/mcp/inspectorClient-peer-handler-timing.test.ts b/clients/web/src/test/core/mcp/inspectorClient-peer-handler-timing.test.ts new file mode 100644 index 000000000..84f019cc3 --- /dev/null +++ b/clients/web/src/test/core/mcp/inspectorClient-peer-handler-timing.test.ts @@ -0,0 +1,1289 @@ +import { describe, it, expect, vi } from "vitest"; +import type { + JSONRPCMessage, + Root, + Transport, +} from "@modelcontextprotocol/client"; +import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; +import { ModernGetTaskResultSchema } from "@inspector/core/mcp/modernTaskSchemas.js"; + +/** + * Regression coverage for #1797: a server may talk to us the instant it is + * initialized, and the Inspector must already be able to answer. + * + * The client advertises `roots` (and sampling/elicitation/tasks) on the SDK + * `Client` at construction, so from the moment `connect()` sends + * `notifications/initialized` the server is entitled to call `roots/list`. + * `server-filesystem` does exactly that — it learns its allowed directories that + * way — and used to get `-32601 Method not found` because the handlers were + * registered after the handshake had already resolved. + * + * Driving this over a real HTTP server would make the assertion a race (the + * outcome depends on whether the server's request lands before or after the + * post-connect awaits). The fake transport below removes the timing entirely: it + * delivers the server→client traffic synchronously from inside the `send()` of + * `notifications/initialized`, i.e. at the earliest instant any server could. + * + * `tasks/list` rides along with `roots/list` so the assertion pins the whole + * pre-handshake registration block rather than one member of it — moving any of + * it back after `connect()` fails here. (`sampling/createMessage` and + * `elicitation/create` are deliberately left out: they park a pending request + * awaiting user input, so they have no reply to assert on.) + * + * The `roots/list_changed` notification is injected too, covering the sibling + * `registerPeerNotificationHandlers()`. Note the spec sends that notification + * the *other* way (client→server), so the inbound handler is defensive rather + * than something a conformant server exercises — this pins where it is + * registered, not a behaviour real servers depend on. + * + * One case never connects at all: it covers the constructor's `cleanRoots` + * normalization — same #1797 thread, since answering `roots/list` promptly is + * only useful if what we answer with is well-formed. + * + * The teardown cases cover the flip side of the same move. Registering the + * handlers before the handshake also widened the window in which a server can + * queue a request with us, so every path that ends a connection has to clear + * that queue, announce it, and settle each entry — otherwise the web + * pending-request modal outlives the connection it belongs to, and the server + * is left waiting on a request we accepted and never answered. There are three: a + * failed `connect()`, a mid-session transport close, and an explicit + * `disconnect()`. `disconnect()` and the crash path clear before dispatching + * `disconnect`, so a handler reading the queue sees it empty. `connect()`'s + * own catch dispatches no `disconnect` — though on a real transport its + * `dropCachedTransport()` usually fires `onclose` first, which clears, + * announces and *does* dispatch one, so the catch's own call is really the + * backstop for the auth-recovery sub-case, where the transport is retained and + * no `onclose` fires. The crash and failure paths emit the change events + * immediately; `disconnect()` batches them with its other teardown dispatches. + * + * The outbound direction needs settling too, on its own terms. The raw-wire + * modern `tasks/*` map is ours — the SDK's era gate keeps those frames out of + * its own `_responseHandlers`, so its teardown can't settle them — and a + * Tasks-tab poll in flight when the server dies would otherwise wait out its + * own 30s timeout and blame the timeout for a crash. It is rejected on the two + * paths that can hold one, `disconnect()` and the crash path; the `connect()` + * catch needs no such call, because nothing populates the map before the + * handshake and both terminal paths clear it. + * + * Both directions carry the same caveat, and it is covered for each: those + * paths are every way *out*, which is not every way *in*. An `onerror` without + * an `onclose` only flips the status — it runs none of them, and leaves the + * transport cached for the next `connect()` to reuse — so the queue and the + * raw-wire map are swept start-clean at the top of `connect()` as well. The + * end-clean clears stay where they are regardless: a consumer handling the + * `disconnect` event has to see them already empty, which a sweep on the way + * back in cannot provide. + * + * A further category is session scoping. Receiver tasks, resource + * subscriptions, cancelled task ids, paused task-input aborts and the modern + * log-level opt-in are all scoped to one connection — `tasks/list` is answered + * from that map, and a stale subscription makes the modern subscribe a silent + * no-op — so they belong to the session that created them. Note the contrast + * with the teardown cases above: the peer-request queue is cleared end-clean, + * because its emptiness has to be observable from the `disconnect` event, and + * only swept on the way in as a backstop; this has no such consumer, so it is + * *reset* start-clean at the top of `connect()` and nowhere else — a crash, or + * a failed connect the caller retries on the same instance, means ending a + * session is not the only way a new one begins. + * Reset, not cleared: the log level is re-derived from the server setting + * rather than dropped, so a mid-session override does not carry over and the + * configured level is not lost. The same category covers a release obligation, + * not just a misread one: the reset must close what it drops, since a + * `connect()` reusing a transport an `onerror` left up can be holding the last + * reference to a live stream. That release is best-effort in both directions — + * it is also covered that a `close()` failing either way, synchronously or by + * rejecting, does not escape into the straight-line teardown around the reset's + * other caller, `disconnect()`, whose remaining steps most callers would never + * see skipped. + * + * Some cases cover the other side of the registration gates. Client capabilities + * are fixed at construction, so each gate must key off what was actually + * advertised rather than the option it was derived from: a later `setRoots()` + * must not make a subsequent `connect()` register a roots handler that was + * never advertised, and an `elicit` option that enables no mode must not + * register an elicitation handler. Either mistake throws before the handshake, + * so the client cannot connect at all. + * + * A note on the ordering, since two axes describe this file equally well and + * they cut differently: the cases are grouped by *route* — everything the + * `onerror`-then-reconnect route has to settle sits together, rather than each + * collection sitting with its own category. Where a case is the counterpart of + * one in another group, it is placed next to its counterpart (the two halves of + * the stream-release obligation, one per route) rather than with its route. + * + * The converse category is the advertised capability object itself: it must + * only invite requests we actually serve, and so must the notifications we + * emit. `capabilities.tasks.requests` names the server→client requests we + * accept as tasks, so it is built from the sampling/elicitation capabilities + * rather than from `receiverTasks` alone; and a `roots/list_changed` is an + * invitation to re-read roots, so it is withheld on a client that never + * advertised the capability (the SDK refuses it too — the guard only avoids + * provoking that rejection). Otherwise a server takes an invitation we answer + * `-32601` on, which is where this whole thread started. + */ +class InitializedRacingTransport implements Transport { + onmessage?: (message: JSONRPCMessage) => void; + onclose?: () => void; + onerror?: (error: Error) => void; + + /** Ids for the requests injected at `initialized`, by method. */ + static readonly REQUEST_IDS = { "roots/list": 9001, "tasks/list": 9002 }; + + /** The client's replies to the injected requests, keyed by request id. */ + readonly replies = new Map(); + + /** Resolvers for {@link injectRequest}, keyed by the id awaiting a reply. */ + private readonly waiters = new Map void>(); + + /** + * Whether to fire the server→client burst at `initialized`. Off for the + * `setRoots()` test, which injects by hand once the client is connected. + */ + private readonly burstOnInitialized: boolean; + + constructor(burstOnInitialized = true) { + this.burstOnInitialized = burstOnInitialized; + } + + /** + * Deliver a server→client request outside the `initialized` burst, resolving + * with the client's reply. The handler is async, so the reply lands some + * microtasks later — awaiting it here beats guessing how many. Rejects rather + * than hanging to the vitest timeout if the client never answers, so a + * regression fails where it happened. + */ + injectRequest(method: string, id: number): Promise { + const reply = new Promise((resolve) => { + this.waiters.set(id, resolve); + }); + this.deliver({ jsonrpc: "2.0", id, method }); + return withTimeout(reply, `No reply to injected ${method} (id ${id})`, { + onTimeout: () => this.waiters.delete(id), + }); + } + + async start(): Promise {} + async close(): Promise {} + + async send(message: JSONRPCMessage): Promise { + if ( + "method" in message && + message.method === "initialize" && + "id" in message + ) { + const params = message.params as { protocolVersion: string }; + this.deliver({ + jsonrpc: "2.0", + id: message.id, + result: { + protocolVersion: params.protocolVersion, + capabilities: {}, + serverInfo: { name: "racing-server", version: "1.0.0" }, + }, + }); + return; + } + if ("method" in message && message.method === "notifications/initialized") { + if (!this.burstOnInitialized) return; + // The moment the server learns we are initialized, it talks to us. + for (const [method, id] of Object.entries( + InitializedRacingTransport.REQUEST_IDS, + )) { + this.deliver({ jsonrpc: "2.0", id, method }); + } + this.deliver({ + jsonrpc: "2.0", + method: "notifications/roots/list_changed", + }); + return; + } + if ( + "id" in message && + typeof message.id === "number" && + ("result" in message || "error" in message) + ) { + this.replies.set(message.id, message); + this.waiters.get(message.id)?.(message); + this.waiters.delete(message.id); + } + } + + private deliver(message: JSONRPCMessage): void { + this.onmessage?.(message); + } +} + +/** + * Delivers an `elicitation/create` at `initialized` — the earliest a server + * could — and then fails the `logging/setLevel` that `connect()` issues after + * the handshake, so the connect attempt dies with a peer request already queued. + */ +class ElicitThenFailTransport implements Transport { + onmessage?: (message: JSONRPCMessage) => void; + onclose?: () => void; + onerror?: (error: Error) => void; + + async start(): Promise {} + async close(): Promise {} + + async send(message: JSONRPCMessage): Promise { + if (!("method" in message)) return; + if (message.method === "initialize" && "id" in message) { + const params = message.params as { protocolVersion: string }; + this.onmessage?.({ + jsonrpc: "2.0", + id: message.id, + result: { + protocolVersion: params.protocolVersion, + // Advertise logging so connect() issues the setLevel we fail below. + capabilities: { logging: {} }, + serverInfo: { name: "failing-server", version: "1.0.0" }, + }, + }); + return; + } + if (message.method === "notifications/initialized") { + this.onmessage?.({ + jsonrpc: "2.0", + id: 9201, + method: "elicitation/create", + // `properties` is required — the SDK validates inbound params, and a + // schema without it is rejected before our handler ever enqueues. + params: { + message: "Your name?", + requestedSchema: { + type: "object", + properties: { name: { type: "string" } }, + }, + }, + }); + return; + } + if (message.method === "logging/setLevel" && "id" in message) { + this.onmessage?.({ + jsonrpc: "2.0", + id: message.id, + error: { code: -32603, message: "no logging for you" }, + }); + } + } +} + +/** Connects cleanly, then elicits on demand so a test can kill the transport. */ +class ElicitAfterConnectTransport implements Transport { + onmessage?: (message: JSONRPCMessage) => void; + onclose?: () => void; + onerror?: (error: Error) => void; + + /** Methods of every client→server notification sent, in order. */ + readonly sentNotifications: string[] = []; + + async start(): Promise {} + async close(): Promise {} + + elicit(): void { + this.onmessage?.({ + jsonrpc: "2.0", + id: 9301, + method: "elicitation/create", + params: { + message: "Your name?", + requestedSchema: { + type: "object", + properties: { name: { type: "string" } }, + }, + }, + }); + } + + async send(message: JSONRPCMessage): Promise { + if ("method" in message && !("id" in message)) { + this.sentNotifications.push(message.method); + return; + } + if ( + "method" in message && + message.method === "initialize" && + "id" in message + ) { + const params = message.params as { protocolVersion: string }; + this.onmessage?.({ + jsonrpc: "2.0", + id: message.id, + result: { + protocolVersion: params.protocolVersion, + capabilities: {}, + serverInfo: { name: "dying-server", version: "1.0.0" }, + }, + }); + } + } +} + +/** + * Race `promise` against a short reject, so a regression fails where it + * happened instead of hanging to the vitest timeout — which names the test + * rather than the thing that didn't occur. The timer is cleared on settle so + * nothing armed outlives the test (the #1760 class). `onTimeout` runs only on + * the reject path, for callers with an entry to drop when nobody answered. + */ +function withTimeout( + promise: Promise, + message: string, + { ms = 1000, onTimeout }: { ms?: number; onTimeout?: () => void } = {}, +): Promise { + let timer: ReturnType | undefined; + const expired = new Promise((_, reject) => { + timer = setTimeout(() => { + onTimeout?.(); + reject(new Error(message)); + }, ms); + }); + return Promise.race([promise, expired]).finally(() => clearTimeout(timer)); +} + +/** + * Resolve when the client enqueues a pending peer request. Waits on the + * client's own signal rather than counting microtasks — the enqueue is one tick + * deep today with no margin, and an added await on the SDK's inbound path would + * flake it. + */ +function waitForNewPendingRequest( + client: InspectorClient, + event: "newPendingElicitation" | "newPendingSample", +): Promise { + return withTimeout( + new Promise((resolve) => { + client.addEventListener(event, () => resolve(), { once: true }); + }), + `No ${event} after the request was delivered`, + ); +} + +/** Connects cleanly, then samples on demand so a test can tear the client down. */ +class SampleAfterConnectTransport implements Transport { + onmessage?: (message: JSONRPCMessage) => void; + onclose?: () => void; + onerror?: (error: Error) => void; + + static readonly SAMPLE_ID = 9401; + + /** Methods of every client→server notification sent, in order. */ + readonly sentNotifications: string[] = []; + + /** Resolvers for {@link injectRequest}, keyed by the id awaiting a reply. */ + private readonly waiters = new Map void>(); + + /** + * Deliver a server→client request, resolving with the client's reply. The + * handler is async, so awaiting the reply beats guessing how many microtasks + * it takes; the timeout drops the waiter so a failing run leaves nothing in + * the map. + */ + injectRequest(method: string, id: number): Promise { + const reply = new Promise((resolve) => { + this.waiters.set(id, resolve); + }); + this.onmessage?.({ jsonrpc: "2.0", id, method }); + return withTimeout(reply, `No reply to injected ${method} (id ${id})`, { + onTimeout: () => this.waiters.delete(id), + }); + } + + async start(): Promise {} + async close(): Promise {} + + /** A task-augmented sample, which creates a receiver-task record. */ + sampleAsTask(): void { + this.onmessage?.({ + jsonrpc: "2.0", + id: SampleAfterConnectTransport.SAMPLE_ID + 1, + method: "sampling/createMessage", + params: { + messages: [{ role: "user", content: { type: "text", text: "hi" } }], + maxTokens: 10, + task: { ttl: 60_000 }, + }, + }); + } + + sample(): void { + this.onmessage?.({ + jsonrpc: "2.0", + id: SampleAfterConnectTransport.SAMPLE_ID, + method: "sampling/createMessage", + params: { + messages: [{ role: "user", content: { type: "text", text: "hi" } }], + maxTokens: 10, + }, + }); + } + + async send(message: JSONRPCMessage): Promise { + if ("method" in message && !("id" in message)) { + this.sentNotifications.push(message.method); + return; + } + if ( + "method" in message && + message.method === "initialize" && + "id" in message + ) { + const params = message.params as { protocolVersion: string }; + this.onmessage?.({ + jsonrpc: "2.0", + id: message.id, + result: { + protocolVersion: params.protocolVersion, + capabilities: {}, + serverInfo: { name: "sampling-server", version: "1.0.0" }, + }, + }); + return; + } + if ( + "id" in message && + typeof message.id === "number" && + ("result" in message || "error" in message) + ) { + this.waiters.get(message.id)?.(message); + this.waiters.delete(message.id); + } + } +} + +describe("InspectorClient peer-handler timing (#1797)", () => { + it("serves server→client traffic that arrives with notifications/initialized", async () => { + const roots = [{ uri: "file:///work", name: "Work" }]; + const transport = new InitializedRacingTransport(); + const rootsChanges: unknown[] = []; + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { + environment: { transport: () => ({ transport }) }, + roots, + receiverTasks: true, + }, + ); + client.addEventListener("rootsChange", (event) => { + rootsChanges.push((event as CustomEvent).detail); + }); + + await client.connect(); + + // roots/list — the request that regressed (#1797). + const rootsReply = transport.replies.get( + InitializedRacingTransport.REQUEST_IDS["roots/list"], + ); + expect(rootsReply).toBeDefined(); + // Asserted separately from the `toMatchObject` below because this is the + // assertion that names the pre-fix failure: the reply was an error object + // carrying -32601 Method not found. + expect(rootsReply).not.toHaveProperty("error"); + expect(rootsReply).toMatchObject({ result: { roots } }); + + // tasks/list — pins the rest of the block registered at the same point. + const tasksReply = transport.replies.get( + InitializedRacingTransport.REQUEST_IDS["tasks/list"], + ); + expect(tasksReply).toBeDefined(); + expect(tasksReply).not.toHaveProperty("error"); + expect(tasksReply).toMatchObject({ result: { tasks: [] } }); + + // notifications/roots/list_changed — a notification has no reply, and an + // unhandled one is dropped silently (no wire error), so the effect is the + // only observable: the handler dispatches `rootsChange`. + expect(rootsChanges).toEqual([roots]); + + await client.disconnect(); + }); + + it("normalizes a malformed roots option at construction", () => { + // Core owns the invariant rather than trusting each client to clean at its + // call site — the constructor is the fourth way roots enter the client, and + // the option can come straight off hand-edited mcp.json. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { + environment: { + transport: () => ({ transport: new InitializedRacingTransport() }), + }, + roots: [{ name: "no uri" }, { uri: "file:///keep" }] as Root[], + }, + ); + expect(client.getRoots()).toEqual([{ uri: "file:///keep" }]); + expect(warn).toHaveBeenCalled(); + } finally { + warn.mockRestore(); + } + }); + + it("serves roots set after connect, given roots were advertised at construction", async () => { + // `setRoots()` announces `notifications/roots/list_changed`, inviting the + // server to re-read — so the handler must answer with the *current* roots, + // not the ones passed at construction. It reads `this.roots` live, so it + // does; this pins that. Passing the `roots` option at all — which the CLI + // now always does, empty when nothing is configured + // (`clients/cli/src/cli.ts`) — is what makes the handler exist. A + // client built with no `roots` option cannot serve this: the SDK asserts + // the capability in `setRequestHandler`, and the capability itself is + // fixed at `initialize`. + const transport = new InitializedRacingTransport(false); + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { environment: { transport: () => ({ transport }) }, roots: [] }, + ); + + await client.connect(); + await client.setRoots([{ uri: "file:///late", name: "Late" }]); + + const reply = await transport.injectRequest("roots/list", 9101); + expect(reply).not.toHaveProperty("error"); + expect(reply).toMatchObject({ + result: { roots: [{ uri: "file:///late", name: "Late" }] }, + }); + + await client.disconnect(); + }); + + it("drops a peer request queued during a connect that then fails", async () => { + // Registering the handlers before the handshake (#1797) widened the window + // in which a server can queue a request to include the part of connect() + // that can still fail. The failure path must not leave the queue behind: + // web derives its pending-request modal from these lengths with no status + // gate, so a stranded entry means a live modal on a dead connection, and a + // URL elicitation's waiter would never settle. + const transport = new ElicitThenFailTransport(); + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { + environment: { transport: () => ({ transport }) }, + initialLoggingLevel: "debug", + }, + ); + // The events, not the arrays, are what removes the modal: + // `usePendingClientRequests` tracks its own state off them, so clearing + // without dispatching would leave a live modal on a dead connection — + // invisible to a getter-only assertion. + const elicitationCounts: number[] = []; + client.addEventListener("pendingElicitationsChange", (event) => { + elicitationCounts.push((event as CustomEvent).detail.length); + }); + const sampleCounts: number[] = []; + client.addEventListener("pendingSamplesChange", (event) => { + sampleCounts.push((event as CustomEvent).detail.length); + }); + + await expect(client.connect()).rejects.toThrow(); + + expect(client.getPendingElicitations()).toEqual([]); + expect(client.getPendingSamples()).toEqual([]); + expect(elicitationCounts.at(-1)).toBe(0); + // The helper announces both queues whenever either was non-empty, so the + // sampling event fires here too even though nothing sampled. + expect(sampleCounts.at(-1)).toBe(0); + }); + + it("drops a peer request queued when the connection dies mid-session", async () => { + // The other way a connection ends without `disconnect()`: the server goes + // away. Same hazards — a modal for a dead connection, and a URL + // elicitation's waiter (which blocks `callTool`) never settling. + const transport = new ElicitAfterConnectTransport(); + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { environment: { transport: () => ({ transport }) } }, + ); + const elicitationCounts: number[] = []; + client.addEventListener("pendingElicitationsChange", (event) => { + elicitationCounts.push((event as CustomEvent).detail.length); + }); + + await client.connect(); + const queued = waitForNewPendingRequest(client, "newPendingElicitation"); + transport.elicit(); + await queued; + expect(client.getPendingElicitations()).toHaveLength(1); + + // The server process dies. + transport.onclose?.(); + + expect(client.getPendingElicitations()).toEqual([]); + expect(elicitationCounts.at(-1)).toBe(0); + }); + + it("answers a queued sampling request when the connection is torn down", async () => { + // We accepted the server's `sampling/createMessage`, so dropping it without + // settling means no response frame is ever written and the server waits + // forever. Reachable because the transport can outlive a failed attempt — + // `connect()` keeps it when an auth provider holds it open. + const transport = new SampleAfterConnectTransport(); + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { environment: { transport: () => ({ transport }) } }, + ); + + await client.connect(); + const queued = waitForNewPendingRequest(client, "newPendingSample"); + transport.sample(); + await queued; + const [pending] = client.getPendingSamples(); + expect(pending).toBeDefined(); + + await client.disconnect(); + + expect(client.getPendingSamples()).toEqual([]); + // Settled, not merely discarded: `respond` refuses a request that already + // has an answer, so this throwing is the proof the promise was settled. + // (Whether the response frame reaches the wire depends on whether the + // transport outlived the teardown — on this path `disconnect()` closed it + // first, which is why the assertion is on the settle rather than on a + // recorded reply.) + await expect( + pending!.respond({ + role: "assistant", + content: { type: "text", text: "late" }, + model: "test", + }), + ).rejects.toThrow(/already resolved or rejected/); + }); + + it("has already cleared the queue by the time `disconnect` fires", async () => { + // `disconnect()` clears above its status block so a consumer handling the + // event sees an empty queue, matching the crash path. That ordering is the + // whole point of the clear's position, and moving it back below the block — + // which reads tidier, next to the batched change events — would silently + // restore the asymmetry. This is what notices. + const transport = new ElicitAfterConnectTransport(); + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { environment: { transport: () => ({ transport }) } }, + ); + + await client.connect(); + const queued = waitForNewPendingRequest(client, "newPendingElicitation"); + transport.elicit(); + await queued; + expect(client.getPendingElicitations()).toHaveLength(1); + + let queueDuringDisconnectEvent = -1; + client.addEventListener("disconnect", () => { + queueDuringDisconnectEvent = client.getPendingElicitations().length; + }); + // The array is what a `disconnect` handler reads; the change event is what + // drives the modal. `disconnect()` batches the latter after its `disconnect` + // dispatch, so this is asserted after the await rather than inside the + // listener — pinning that it happens, not where it interleaves. + const elicitationCounts: number[] = []; + client.addEventListener("pendingElicitationsChange", (event) => { + elicitationCounts.push((event as CustomEvent).detail.length); + }); + // This fixture's close() never fires onclose, so the clear under test is + // `disconnect()`'s own, not the crash path's. + await client.disconnect(); + + expect(queueDuringDisconnectEvent).toBe(0); + expect(elicitationCounts.at(-1)).toBe(0); + }); + + it("rejects an in-flight raw-wire request when the connection dies", async () => { + // The modern `tasks/*` frames ride a raw-wire channel the SDK's era gate + // refuses to route, so the SDK's own teardown doesn't know about them. A + // Tasks-tab poll in flight when the server dies would otherwise wait out + // its own 30s timeout and report a timeout for what was a crash. + const transport = new ElicitAfterConnectTransport(); + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { + environment: { transport: () => ({ transport }) }, + // Short, so a regression run doesn't leave the raw-wire request's own + // timer armed past the failing test — and so its message ("timed out + // after 50 ms") beats a generic race message at describing the failure. + // Note this is the *client-wide* request timeout, not a per-call one; + // it is safe here only because this test issues no SDK request after + // connect (`fetchServerInfo` reads cached values, and no + // `setLoggingLevel` since the fixture advertises no logging). Anything + // added later would inherit the 50ms budget. + timeout: 50, + }, + ); + await client.connect(); + + // `rawWireRequest` is private, and every public route to it + // (`getRequestorTask` / `updateRequestorTask` / `cancelRequestorTask`) is + // gated on `isTasksExtensionNegotiated()`, which needs a modern-era + // handshake this fixture doesn't perform — so driving the channel directly + // is the only way to exercise it. The cast asserts only the method's real + // signature. + const pending = ( + client as unknown as { + rawWireRequest: ( + method: string, + params: Record, + schema: { parse: (v: unknown) => unknown }, + ) => Promise; + } + ).rawWireRequest("tasks/get", { taskId: "t1" }, ModernGetTaskResultSchema); + // Attach the rejection handler before the crash so it is never unhandled. + // No tick in between: `rawWireRequest` registers its pending entry + // synchronously (the promise executor runs during the call), so the entry + // is already there — asserting that ordering rather than papering over it. + const settled = expect(pending).rejects.toThrow(/Connection closed/); + + transport.onclose?.(); + + // The short request timeout set above is what makes a regression fail fast + // and name itself — the assertion reports `timed out after 50 ms` against + // the expected /Connection closed/. The race is the backstop for the + // narrower case where the request's own timer is broken too, so nothing + // settles at all. + await withTimeout(settled, "raw-wire request not settled by teardown"); + }); + + it("does not carry receiver tasks into the next session", async () => { + // A record is a task the *server* created with us, and `tasks/list` is + // answered from that map — so one surviving a reconnect reports a task the + // new server never created. `disconnect()` cleared them, but that is not + // the only way a session ends: a crash, or a failed connect the caller + // retries on this same instance, both leave them behind. + const transport = new SampleAfterConnectTransport(); + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { + environment: { transport: () => ({ transport }) }, + receiverTasks: true, + }, + ); + await client.connect(); + + const queued = waitForNewPendingRequest(client, "newPendingSample"); + transport.sampleAsTask(); + await queued; + // Asserted through `tasks/list` — that handler is what a server actually + // sees, and it is the symptom: a surviving record is reported to a server + // that never created it. + expect(await transport.injectRequest("tasks/list", 9501)).toMatchObject({ + result: { tasks: [expect.anything()] }, + }); + + // The server dies; the caller reconnects on the same instance. + transport.onclose?.(); + await client.connect(); + + expect(await transport.injectRequest("tasks/list", 9502)).toMatchObject({ + result: { tasks: [] }, + }); + + await client.disconnect(); + }); + + it("does not carry subscriptions or cancelled task ids into the next session", async () => { + // Same class as the receiver tasks, with sharper symptoms: a stale + // subscription makes the modern `subscribeToResource` early-return, so the + // user's next Subscribe click silently sends nothing; a stale cancelled-id + // mislabels a *new* task sharing that id as cancelled rather than failed. + const transport = new SampleAfterConnectTransport(); + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { environment: { transport: () => ({ transport }) } }, + ); + await client.connect(); + + // Seeded directly: subscribing for real needs a server that answers + // `resources/subscribe` and cancelling needs a live task, neither of which + // adds to what is under test — that a new session starts empty. The cast is + // the only route to `cancelledTaskIds`, which has no public reader. + const internals = client as unknown as { + subscribedResources: Set; + cancelledTaskIds: Set; + modernStreamState: { + active: boolean; + status: string; + honoredUris: string[]; + }; + }; + internals.subscribedResources.add("file:///watched"); + internals.cancelledTaskIds.add("task-1"); + // The stream state a live modern subscription would have left behind. + internals.modernStreamState = { + active: true, + status: "ended", + honoredUris: ["file:///watched"], + }; + // The dispatch is the half the UI tracks — `ResourceSubscriptionsState` + // listens only to the event, never reading the field. + const streamStates: { active: boolean }[] = []; + client.addEventListener("resourceSubscriptionStreamChange", (event) => { + streamStates.push((event as CustomEvent).detail); + }); + // Same for the set: `ResourceSubscriptionsState` reads `event.detail` and + // never the client's field, so clearing without announcing would leave the + // Resources tiles standing on a reconnected session. + const subscriptionLists: string[][] = []; + client.addEventListener("resourceSubscriptionsChange", (event) => { + subscriptionLists.push((event as CustomEvent).detail); + }); + + transport.onclose?.(); + await client.connect(); + + expect(client.getSubscribedResources()).toEqual([]); + expect(internals.cancelledTaskIds.size).toBe(0); + // Cleared with the set it is derived from, not left reading `active` for + // an empty one. + expect(client.getResourceSubscriptionStreamState()).toMatchObject({ + active: false, + }); + expect(streamStates.at(-1)).toMatchObject({ active: false }); + expect(subscriptionLists.at(-1)).toEqual([]); + + await client.disconnect(); + }); + + it("restores the configured modern log level across a reconnect", async () => { + // Two halves of the same member. A mid-session override must not carry into + // the next session, and — the #1629 bug — a `disconnect()` must not leave + // the configured level dropped, which silently stopped stamping + // `_meta` logLevel on everything after a reconnect. + const transport = new SampleAfterConnectTransport(); + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { + environment: { transport: () => ({ transport }) }, + serverSettings: { + headers: [], + env: [], + metadata: [], + connectionTimeout: 0, + requestTimeout: 0, + taskTtl: 0, + maxFetchRequests: 1000, + roots: [], + modernLogLevel: "info", + }, + }, + ); + + await client.connect(); + expect(client.getModernLogLevel()).toBe("info"); + + client.setModernLogLevel("error"); + transport.onclose?.(); + await client.connect(); + expect(client.getModernLogLevel()).toBe("info"); + + await client.disconnect(); + await client.connect(); + expect(client.getModernLogLevel()).toBe("info"); + + await client.disconnect(); + }); + + it("aborts a paused task-input wait when the session ends", async () => { + // The bounded-window member: both registration sites release in a + // `finally`, so nothing leaks permanently — this closes the gap between a + // crash and the loop unwinding on its own. + const transport = new SampleAfterConnectTransport(); + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { environment: { transport: () => ({ transport }) } }, + ); + await client.connect(); + + // Seeded directly: reaching this map for real needs a modern task paused at + // `input_required`, which adds nothing to what is under test. No public + // reader, hence the cast. + const controller = new AbortController(); + ( + client as unknown as { + taskInputAbortControllers: Map; + } + ).taskInputAbortControllers.set("task-1", controller); + + transport.onclose?.(); + await client.connect(); + + expect(controller.signal.aborted).toBe(true); + + await client.disconnect(); + }); + + it("closes a live listen stream the next connect drops", async () => { + // An `onerror` without an `onclose` leaves the transport up, and `connect()` + // reuses it — so the reference the reset drops can be the last one to a + // stream still open on the server. Nothing else can close it afterwards: + // the `closed` handler bails on the bumped generation (production + // behaviour, not asserted here — the stub below has no `closed` promise, so + // that handler never runs). + const transport = new SampleAfterConnectTransport(); + // Counted, because transport *reuse* is what makes "a stream still open on + // the server" true — if a future change recreated it here, the scenario + // would quietly stop applying while the test kept passing. + let created = 0; + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { + environment: { + transport: () => { + created++; + return { transport }; + }, + }, + }, + ); + await client.connect(); + + let closed = false; + // Seeded directly: establishing a real listen stream needs a modern server + // answering `subscriptions/listen`, which adds nothing to what is tested. + // No public writer, hence the cast. + ( + client as unknown as { + modernSubscription: { close: () => Promise } | null; + } + ).modernSubscription = { + close: async () => { + closed = true; + }, + }; + + transport.onerror?.(new Error("stream broke, transport still up")); + await client.connect(); + + expect(closed).toBe(true); + expect(created).toBe(1); + + await client.disconnect(); + }); + + // Distinct reasons per arm, so the filter below excludes the sibling arm's + // late-reported rejection as well as a foreign one. + for (const [label, reason, close] of [ + [ + "throws synchronously", + "close blew up (sync)", + (): Promise => { + throw new Error("close blew up (sync)"); + }, + ], + [ + "returns a rejected promise", + "close blew up (async)", + (): Promise => Promise.reject(new Error("close blew up (async)")), + ], + ] as const) { + it(`continues teardown when the dropped stream's close() ${label}`, async () => { + // The release obligation from the other side: the reset closes what it + // drops, and a `close()` that fails either way is absorbed. Be precise + // about *which* harm is absorbed at this site, because the two are not + // interchangeable: the call is `void`-ed onto an `async` helper, so even + // the synchronous throw becomes a dropped rejection rather than reaching + // `disconnect()` — the teardown below it runs regardless. What the + // wrapping prevents is the rejection going *unhandled*, which ends a Node + // process by default. Hence the listener: it is the assertion that fails + // if the helper's `try/catch` is removed. The two teardown assertions are + // kept for the neighbouring regression — unwrapping this to a bare + // `void closing.close()`, where the synchronous half would propagate. + const unhandled: unknown[] = []; + const onUnhandled = (reason: unknown): void => { + unhandled.push(reason); + }; + // Registered from here, so every path out of the test removes it — a leak + // would otherwise accumulate across the rest of the file into an array + // nobody reads. + process.on("unhandledRejection", onUnhandled); + try { + const transport = new SampleAfterConnectTransport(); + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { environment: { transport: () => ({ transport }) } }, + ); + await client.connect(); + + // Seeded directly, for the reasons `closes a live listen stream the + // next connect drops` and `aborts a paused task-input wait when the + // session ends` give. No public writer for either, hence the casts. + ( + client as unknown as { + modernSubscription: { close: () => Promise } | null; + } + ).modernSubscription = { close }; + + // A downstream teardown step, to witness that teardown continued. + const controller = new AbortController(); + ( + client as unknown as { + taskInputAbortControllers: Map; + } + ).taskInputAbortControllers.set("task-1", controller); + + await expect(client.disconnect()).resolves.toBeUndefined(); + expect(controller.signal.aborted).toBe(true); + + // Node reports an unhandled rejection after the microtask checkpoint, + // so yield to the macrotask queue before reading the listener — nothing + // else in this test would give it a chance to fire. + await new Promise((resolve) => setTimeout(resolve, 0)); + // Filtered to this arm's own reason: the listener is process-wide, so a + // rejection another test — or the sibling arm — left pending and Node + // reported late would otherwise fail here, blaming the wrong one. + expect(unhandled.filter((r) => String(r).includes(reason))).toEqual([]); + } finally { + process.off("unhandledRejection", onUnhandled); + } + }); + } + + it("clears a peer request the next connect would otherwise inherit", async () => { + // Same route as `closes a live listen stream the next connect drops`, for + // the other collection it strands. An + // `onerror` without an `onclose` runs neither teardown path, so the queue + // the three teardown paths clear end-clean is still populated when + // `connect()` reuses the live transport — and the web modal is derived from + // its length with no status gate, so answering it would write a response + // for the old session's request id onto the new connection. `connect()` + // sweeps it start-clean for exactly this route. + const transport = new ElicitAfterConnectTransport(); + // Counted for the same reason as that test: transport *reuse* is the + // premise. + let created = 0; + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { + environment: { + transport: () => { + created++; + return { transport }; + }, + }, + }, + ); + const elicitationCounts: number[] = []; + client.addEventListener("pendingElicitationsChange", (event) => { + elicitationCounts.push((event as CustomEvent).detail.length); + }); + + await client.connect(); + const queued = waitForNewPendingRequest(client, "newPendingElicitation"); + transport.elicit(); + await queued; + expect(client.getPendingElicitations()).toHaveLength(1); + + // Status goes to "error" and stops — no `onclose`, transport left cached. + transport.onerror?.(new Error("stream broke, transport still up")); + expect(client.getPendingElicitations()).toHaveLength(1); + + await client.connect(); + + expect(client.getPendingElicitations()).toEqual([]); + // Announced, not just emptied: the modal reads the change event. + expect(elicitationCounts.at(-1)).toBe(0); + expect(created).toBe(1); + + await client.disconnect(); + }); + + it("sweeps the queue without reporting the outgoing session's task", async () => { + // The sweep must stay *after* `resetSessionState()`, and the two calls read + // as independent. Cancelling a task-augmented peer request settles it + // synchronously into the record callback, which ends in + // `upsertReceiverTask` — a no-op only because `clearReceiverTasks()` just + // emptied the map. Hoisted above the reset, it would emit a + // `notifications/tasks/status` for the ending session's task onto the + // transport this connect is about to reuse. This is what notices. + const transport = new SampleAfterConnectTransport(); + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { + environment: { transport: () => ({ transport }) }, + receiverTasks: true, + }, + ); + await client.connect(); + + const queued = waitForNewPendingRequest(client, "newPendingSample"); + transport.sampleAsTask(); + await queued; + + transport.onerror?.(new Error("stream broke, transport still up")); + // Only the reconnect's frames are under test; the first session legitimately + // reported the task it created. + transport.sentNotifications.length = 0; + await client.connect(); + + expect(transport.sentNotifications).not.toContain( + "notifications/tasks/status", + ); + + await client.disconnect(); + }); + + it("rejects an in-flight raw-wire request the next connect would inherit", async () => { + // The outbound half of the same route. Milder than the queue — the id is + // instance-scoped and monotonic, so a stale entry can't be resolved by the + // new session's traffic — but it is round 30's symptom exactly: without the + // sweep the poll waits out its own 30s timer and blames a timeout for what + // was a crash. + const transport = new ElicitAfterConnectTransport(); + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { + environment: { transport: () => ({ transport }) }, + // Short so a regression names itself, as in the crash-path case above + // — but safe here for a different reason than that one gives: this + // test connects twice, so an SDK `initialize` *is* issued under the + // client-wide budget. It can't race because this fixture answers + // `initialize` synchronously from inside `send()`, so no timer is ever + // in play. A fixture that answered a macrotask later would flake. + timeout: 50, + }, + ); + await client.connect(); + + const pending = ( + client as unknown as { + rawWireRequest: ( + method: string, + params: Record, + schema: { parse: (v: unknown) => unknown }, + ) => Promise; + } + ).rawWireRequest("tasks/get", { taskId: "t1" }, ModernGetTaskResultSchema); + const settled = expect(pending).rejects.toThrow(/Connection ended/); + + // No `onclose`, so nothing settles it on the way out. + transport.onerror?.(new Error("stream broke, transport still up")); + await client.connect(); + + await withTimeout( + settled, + "raw-wire request not settled by the next connect", + ); + + await client.disconnect(); + }); + + it("connects when an elicit option enables no mode", async () => { + // `{ form: false, url: false }` is a valid option that advertises no + // elicitation capability. Registering `elicitation/create` on `this.elicit` + // being truthy would throw "Client does not support elicitation capability" + // before the handshake — so the client could not connect at all. + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { + environment: { + transport: () => ({ transport: new ElicitAfterConnectTransport() }), + }, + elicit: { form: false, url: false }, + }, + ); + + await expect(client.connect()).resolves.toBeUndefined(); + + await client.disconnect(); + }); + + it("can reconnect after setRoots() on a client built without roots", async () => { + // `setRoots()` makes `this.roots` defined on a client that never advertised + // the capability. Gating the `roots/list` registration on that would throw + // "Client does not support roots capability" from `setRequestHandler` on + // every later connect() — before the handshake, so the client could never + // reconnect. The gate reads what was advertised at construction instead. + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { + environment: { + transport: () => ({ transport: new ElicitAfterConnectTransport() }), + }, + }, + ); + + await client.connect(); + await client.setRoots([{ uri: "file:///late" }]); + await client.disconnect(); + + await expect(client.connect()).resolves.toBeUndefined(); + // Stored and readable, but no server can ask for them — the capability was + // never advertised, so no handler is registered. + expect(client.getRoots()).toEqual([{ uri: "file:///late" }]); + + await client.disconnect(); + }); + + it("advertises task requests only for capabilities it advertised", async () => { + // `capabilities.tasks.requests` tells the server which server→client + // requests we accept as tasks. Built from `receiverTasks` alone it would + // invite a task-augmented `elicitation/create` that no handler answers — + // advertise-then-refuse, the shape #1797 is about. + const client = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { + environment: { + transport: () => ({ transport: new ElicitAfterConnectTransport() }), + }, + receiverTasks: true, + elicit: false, + }, + ); + + const capabilities = client.getClientCapabilities(); + expect(capabilities.tasks).toBeDefined(); + expect(capabilities.tasks?.requests?.elicitation).toBeUndefined(); + expect(capabilities.tasks?.requests?.sampling).toBeDefined(); + expect(capabilities.elicitation).toBeUndefined(); + + // Neither advertised: `requests` is omitted rather than sent empty, which + // would claim "I accept no task-augmented requests" instead of saying + // nothing. `tasks` itself stays — list/cancel are still serviceable. + const noRequests = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { + environment: { + transport: () => ({ transport: new ElicitAfterConnectTransport() }), + }, + receiverTasks: true, + sample: false, + elicit: false, + }, + ); + expect(noRequests.getClientCapabilities().tasks).toBeDefined(); + expect(noRequests.getClientCapabilities().tasks?.requests).toBeUndefined(); + }); + + it("announces roots/list_changed only when roots were advertised", async () => { + // Pins the end state rather than this client's guard: the SDK also refuses + // the notification from a client that never declared `roots.listChanged` + // (it rejects, which `setRoots` used to log as a send *failure*), so the + // wire stays clean either way. What this asserts is that a server is never + // invited to re-read roots we have no handler to serve. + const withRoots = new ElicitAfterConnectTransport(); + const advertised = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { + environment: { transport: () => ({ transport: withRoots }) }, + roots: [], + }, + ); + await advertised.connect(); + await advertised.setRoots([{ uri: "file:///a" }]); + expect(withRoots.sentNotifications).toContain( + "notifications/roots/list_changed", + ); + await advertised.disconnect(); + + const withoutRoots = new ElicitAfterConnectTransport(); + const silent = new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + { environment: { transport: () => ({ transport: withoutRoots }) } }, + ); + await silent.connect(); + await silent.setRoots([{ uri: "file:///a" }]); + expect(withoutRoots.sentNotifications).not.toContain( + "notifications/roots/list_changed", + ); + // Still stored locally — only the announcement is withheld. + expect(silent.getRoots()).toEqual([{ uri: "file:///a" }]); + await silent.disconnect(); + }); +}); diff --git a/clients/web/src/test/core/mcp/modernLogLevel.test.ts b/clients/web/src/test/core/mcp/modernLogLevel.test.ts new file mode 100644 index 000000000..c33bef73d --- /dev/null +++ b/clients/web/src/test/core/mcp/modernLogLevel.test.ts @@ -0,0 +1,20 @@ +import { describe, it, expect } from "vitest"; +import { + DEFAULT_MODERN_LOG_LEVEL, + resolveModernLogLevel, +} from "@inspector/core/mcp/types.js"; + +describe("resolveModernLogLevel", () => { + it("falls back to the default when unset", () => { + expect(resolveModernLogLevel()).toBe(DEFAULT_MODERN_LOG_LEVEL); + expect(resolveModernLogLevel({})).toBe(DEFAULT_MODERN_LOG_LEVEL); + }); + + it('treats "off" as not opted in', () => { + expect(resolveModernLogLevel({ modernLogLevel: "off" })).toBeUndefined(); + }); + + it("passes an explicit level through", () => { + expect(resolveModernLogLevel({ modernLogLevel: "error" })).toBe("error"); + }); +}); diff --git a/clients/web/src/test/core/mcp/serverList.test.ts b/clients/web/src/test/core/mcp/serverList.test.ts index 22e9639b0..d6118fbd4 100644 --- a/clients/web/src/test/core/mcp/serverList.test.ts +++ b/clients/web/src/test/core/mcp/serverList.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect } from "vitest"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { cleanRoots, DEFAULT_SEED_CONFIG, @@ -23,6 +23,7 @@ import type { ServerEntry, StoredMCPServer, } from "@inspector/core/mcp/types.js"; +import type { Root } from "@modelcontextprotocol/client"; describe("normalizeServerType", () => { it("defaults missing type to stdio", () => { @@ -102,6 +103,53 @@ describe("cleanRoots", () => { { uri: "file:///b", _meta: { k: 2 } }, ]); }); + + // Every client now feeds this straight from hand-editable mcp.json (#1797), + // where `Root[]` is only a compile-time promise. Malformed input must be + // reported and skipped, not thrown at connect. + describe("malformed input from disk", () => { + let warn: ReturnType; + beforeEach(() => { + warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + }); + afterEach(() => { + warn.mockRestore(); + }); + + it("drops an entry with no uri instead of throwing", () => { + // A hand-edited `"roots": [{ "name": "Work" }]`. + const malformed = [{ name: "Work" }, { uri: "file:///ok" }]; + expect(cleanRoots(malformed as unknown as Root[])).toEqual([ + { uri: "file:///ok" }, + ]); + expect(warn).toHaveBeenCalled(); + }); + + it("drops an entry whose uri is not a string", () => { + const malformed = [{ uri: 42 }, { uri: "file:///ok" }]; + expect(cleanRoots(malformed as unknown as Root[])).toEqual([ + { uri: "file:///ok" }, + ]); + expect(warn).toHaveBeenCalled(); + }); + + it("keeps a root whose name is not a string, dropping just the name", () => { + // `name` is optional, so a bad one costs the name, not the root. + expect( + cleanRoots([ + { uri: "file:///a", name: 42 }, + { uri: "file:///b", name: { x: 1 } }, + ] as unknown as Root[]), + ).toEqual([{ uri: "file:///a" }, { uri: "file:///b" }]); + expect(warn).toHaveBeenCalledTimes(2); + }); + + it("bails to an empty list when roots is not an array", () => { + // A hand-edited `"roots": "file:///work"`. + expect(cleanRoots("file:///work" as unknown as Root[])).toEqual([]); + expect(warn).toHaveBeenCalled(); + }); + }); }); describe("mcpConfigToServerEntries", () => { diff --git a/clients/web/src/test/integration/mcp/inspectorClient-coverage-backfill.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-coverage-backfill.test.ts index 291a1d89f..d873453c1 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-coverage-backfill.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-coverage-backfill.test.ts @@ -57,23 +57,6 @@ async function getTool(client: InspectorClient, name: string): Promise { return tool; } -/** - * Attach a no-op catch to every outstanding receiver-task payload promise so a - * deliberate reject (failure-path test) doesn't bubble up as an unhandled - * rejection. The real consumer (the server polling tasks/result) handles the - * rejection, but it may not have a handler attached at the instant we reject. - */ -function suppressReceiverPayloadRejections(client: InspectorClient): void { - const records = ( - client as unknown as { - receiverTaskRecords: Map }>; - } - ).receiverTaskRecords; - for (const record of records.values()) { - record.payloadPromise.catch(() => {}); - } -} - describe("InspectorClient coverage backfill", () => { let client: InspectorClient | null = null; let server: TestServerHttp | null = null; @@ -200,8 +183,11 @@ describe("InspectorClient coverage backfill", () => { }); describe("setRoots", () => { - it("enables roots when previously undefined and dispatches rootsChange", async () => { - // No roots option → this.roots is undefined initially. + it("stores roots and dispatches rootsChange when none were configured", async () => { + // No roots option → this.roots is undefined initially. Note setRoots does + // *not* enable the capability: this client still has no `roots/list` + // handler and no `capabilities.roots`, so only `getRoots()` reflects the + // change — see the note on setRoots (#1797). client = stdioClient(); await client.connect(); const rootsChange = waitForEvent(client, "rootsChange", { @@ -323,9 +309,6 @@ describe("InspectorClient coverage backfill", () => { .catch((e: unknown) => e); const sample = await samplingPromise; - // Pre-attach a catch to the receiver task's payload promise so its - // rejection (driven below) doesn't surface as an unhandled rejection. - suppressReceiverPayloadRejections(client); // Reject instead of respond — drives the receiver-task error callback, // which sets status "failed" and calls upsertReceiverTask. await sample.reject(new Error("user rejected sampling")); @@ -374,7 +357,6 @@ describe("InspectorClient coverage backfill", () => { .catch((e: unknown) => e); const elicitation = await elicitationPromise; - suppressReceiverPayloadRejections(client); await elicitation.reject(new Error("user declined elicitation")); const outcome = await callPromise; diff --git a/clients/web/src/test/integration/mcp/inspectorClient-subscriptions-era.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-subscriptions-era.test.ts index a0ba41007..d10610a27 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-subscriptions-era.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-subscriptions-era.test.ts @@ -297,6 +297,196 @@ describe("resource subscriptions era fork (#1630)", () => { return fake; } + for (const [label, close] of [ + [ + "throws synchronously", + (): Promise => { + throw new Error("close blew up"); + }, + ], + [ + "returns a rejected promise", + (): Promise => Promise.reject(new Error("close blew up")), + ], + ] as const) { + it(`re-lists when the superseded stream's close() ${label}`, async () => { + // A re-list drops its reference to the previous stream *before* closing + // it, so an escaping failure would both abandon a stream that may still + // be open on the server and abort the refresh before its replacement + // `listen()` — leaving a non-empty subscription set with no stream. The + // close is best-effort against both failure modes for that reason; this + // is the site where being best-effort against the *synchronous* one + // changed behaviour (a `.catch()` alone never caught it). + const started = await startServer({}); + const { connected } = await connect(started.url, "modern"); + await connected.subscribeToResource(RESOURCE_URI); + + const int = internals(connected); + // Swap the live stream for a poisoned one, closing the real stream so + // the wire is torn down (as `installFakeSubscription` does). + const real = int.modernSubscription; + int.modernSubscription = { close } as unknown as McpSubscription; + await real?.close().catch(() => {}); + + // A second URI changes the filter, so this re-lists over the poisoned + // stream. + await expect( + connected.subscribeToResource(RESOURCE_URI_2), + ).resolves.toBeUndefined(); + + expect(connected.getSubscribedResources()).toEqual([ + RESOURCE_URI, + RESOURCE_URI_2, + ]); + const state = connected.getResourceSubscriptionStreamState(); + expect(state.active).toBe(true); + expect(state.status).toBe("acknowledged"); + expect(int.modernSubscription).not.toBeNull(); + }); + } + + it("keeps the optimistic add when a newer refresh superseded the failure", async () => { + // The rollback's premise is that the server filter is unchanged, which is + // exactly what supersession falsifies: the newer refresh built its filter + // from the set *including* this URI, so on success the server honors it. + // Rolling back anyway would leave the set missing a URI the live stream + // carries — and with only one subscribed, an empty set with an active + // stream, the combination `resetSubscriptionStream` exists to prevent. + // Reachable without any close() failure: a rejecting `listen()` does it, + // which is what a user subscribing while the reconnect timer re-lists + // (i.e. exactly when the server is flaky) can produce. + const started = await startServer({}); + const { connected } = await connect(started.url, "modern"); + const int = internals(connected); + + const real = int.client.listen; + // The first listen hangs, so the second can supersede it before it fails. + let failFirst: (error: Error) => void = () => {}; + int.client.listen = () => + new Promise((_, reject) => { + failFirst = reject; + }); + const first = connected.subscribeToResource(RESOURCE_URI); + // Attached before the supersession so the rejection is never unhandled. + const firstSettled = expect(first).rejects.toThrow(/listen boom/); + + // A newer refresh starts and acknowledges, carrying both URIs. + int.client.listen = real; + await connected.subscribeToResource(RESOURCE_URI_2); + + failFirst(new Error("listen boom")); + await firstSettled; + + // The superseded call still reports its failure, but leaves the filter to + // the refresh that owns it. + expect(connected.getSubscribedResources()).toEqual([ + RESOURCE_URI, + RESOURCE_URI_2, + ]); + const state = connected.getResourceSubscriptionStreamState(); + expect(state.active).toBe(true); + expect(state.status).toBe("acknowledged"); + }); + + it("retries rather than stranding the URIs when both overlapping subscribes fail", async () => { + // The other half of the gate: skipping the superseded call's rollback is + // only safe if the superseding call reconciles when *it* fails too. + // Otherwise the set keeps the first URI while the state keeps the + // optimistic "connecting" — a badge that never changes, over a + // subscription no server ever honored, with nothing armed to fix it + // (neither `onModernSubscriptionClosed` nor `onModernReconnectFailed` + // ran). The reconcile hands it to the reconnect machinery instead. + const started = await startServer({}); + const { connected } = await connect(started.url, "modern"); + const int = internals(connected); + + let failFirst: (error: Error) => void = () => {}; + int.client.listen = () => + new Promise((_, reject) => { + failFirst = reject; + }); + const first = connected.subscribeToResource(RESOURCE_URI); + const firstSettled = expect(first).rejects.toThrow(/listen boom/); + + // The superseding call fails on its own listen. + int.client.listen = () => Promise.reject(new Error("listen boom 2")); + await expect( + connected.subscribeToResource(RESOURCE_URI_2), + ).rejects.toThrow(/listen boom 2/); + + failFirst(new Error("listen boom")); + await firstSettled; + + // Its own URI rolled back; the superseded one survives — and rather than + // sitting at "connecting" forever it is now a retry in progress. + expect(connected.getSubscribedResources()).toEqual([RESOURCE_URI]); + expect(connected.getResourceSubscriptionStreamState().status).toBe( + "reconnecting", + ); + + // And the retry makes the state true: the next re-listen acknowledges. + int.client.listen = () => Promise.resolve(makeFakeSub().sub); + await vi.waitFor(() => { + expect(connected.getResourceSubscriptionStreamState().status).toBe( + "acknowledged", + ); + }); + }); + + it("retries rather than stranding the URIs when the unsubscribe's re-listen fails", async () => { + // `unsubscribeFromResource` keeps its removal when the re-listen fails, + // so nothing is rolled back — but the stream is gone with URIs still + // subscribed, and without the reconcile the badge would keep reporting + // the previous success ("acknowledged"/Listening) over no stream at all. + const started = await startServer({}); + const { connected } = await connect(started.url, "modern"); + await connected.subscribeToResource(RESOURCE_URI); + await connected.subscribeToResource(RESOURCE_URI_2); + expect(connected.getResourceSubscriptionStreamState().status).toBe( + "acknowledged", + ); + + const int = internals(connected); + int.client.listen = () => Promise.reject(new Error("re-listen boom")); + await expect( + connected.unsubscribeFromResource(RESOURCE_URI_2), + ).rejects.toThrow(/re-listen boom/); + + expect(connected.getSubscribedResources()).toEqual([RESOURCE_URI]); + expect(connected.getResourceSubscriptionStreamState().status).toBe( + "reconnecting", + ); + }); + + it("never announces an empty subscription set with an active stream", async () => { + // The pair `resetSubscriptionStream` orders its own writes to prevent, + // asserted from the consumer's side at the two sites that empty the set + // outside it: the last-URI unsubscribe (happy path, where the re-listen + // that would set INACTIVE is a round-trip away) and the last-URI rollback + // of a failed subscribe (where the state still reads "connecting"). + const started = await startServer({}); + const { connected } = await connect(started.url, "modern"); + const seen: { size: number; active: boolean }[] = []; + connected.addEventListener("resourceSubscriptionsChange", () => { + seen.push({ + size: connected.getSubscribedResources().length, + active: connected.getResourceSubscriptionStreamState().active, + }); + }); + + await connected.subscribeToResource(RESOURCE_URI); + await connected.unsubscribeFromResource(RESOURCE_URI); + + const int = internals(connected); + int.client.listen = () => Promise.reject(new Error("listen boom")); + await expect(connected.subscribeToResource(RESOURCE_URI)).rejects.toThrow( + /listen boom/, + ); + + expect(seen.length).toBeGreaterThan(0); + expect(seen.filter((s) => s.size === 0 && s.active)).toEqual([]); + }); + it("rolls back the optimistic add when listen() fails", async () => { const started = await startServer({}); const { connected } = await connect(started.url, "modern"); diff --git a/clients/web/src/test/integration/mcp/inspectorClient.test.ts b/clients/web/src/test/integration/mcp/inspectorClient.test.ts index 13c57f75c..7937b219a 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient.test.ts @@ -5560,7 +5560,9 @@ describe("InspectorClient", () => { initialStatus: "working", statusMessage: "running", }); - // Pre-attach a catch so cancel's reject doesn't surface as unhandled + // Capture the rejection's message for the assertion below. (Not for + // unhandled-rejection suppression — `createReceiverTask` marks the + // promise handled at the source.) const payloadResult = record.payloadPromise.catch( (e) => (e as Error).message, ); diff --git a/core/mcp/elicitationCreateMessage.ts b/core/mcp/elicitationCreateMessage.ts index 2c30a6b2b..347773c8a 100644 --- a/core/mcp/elicitationCreateMessage.ts +++ b/core/mcp/elicitationCreateMessage.ts @@ -102,11 +102,17 @@ export class ElicitationCreateMessage { /** * Settle a still-pending elicitation as cancelled, without removing it from - * the queue. Used by `disconnect()` teardown so an awaiting caller — notably - * the error-path `awaitUrlElicitation` that blocks `callTool` — doesn't hang - * forever when the pending queue is dropped wholesale. No-op once already - * resolved; deliberately does not call `onRemove` (the caller clears the - * queue itself, so we must not splice it mid-iteration). + * the queue. Called from `InspectorClient`'s `clearPendingPeerRequests()`, + * which serves every route that drops the queue wholesale — each route out of + * a connection, and the top of `connect()` as a backstop for the one route in + * that settles nothing — so an awaiting caller (notably the error-path + * `awaitUrlElicitation` that blocks `callTool`) doesn't hang forever. The + * category, not a count: that set has grown. No-op once already resolved. + * + * Deliberately does not call `onRemove`: that caller iterates the queue and + * clears it itself, so removing here would splice mid-iteration — skipping + * every other entry and leaving those promises unsettled, the exact hang this + * method exists to prevent. */ cancel(): void { if (this.resolvePromise) { diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 8a4d6705c..29629affb 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -31,10 +31,11 @@ export type { } from "./types.js"; import { getServerType as getServerTypeFromConfig } from "./config.js"; import { - DEFAULT_MODERN_LOG_LEVEL, INACTIVE_SUBSCRIPTION_STREAM_STATE, isTerminalStatus, + resolveModernLogLevel, } from "./types.js"; +import { cleanRoots } from "./serverList.js"; // Fallback client identity, used ONLY when a caller doesn't pass // `clientIdentity`. Real clients supply their own: the Node clients (CLI, TUI) // read the single-source version from the root package.json via @@ -242,6 +243,52 @@ const TOOL_CALL_CANCELLED_REASON = "Tool call cancelled by user"; */ const DEFAULT_TASK_POLL_INTERVAL_MS = 500; +/** + * Close a modern listen stream best-effort, absorbing both failure modes a + * third-party `close()` can produce: a rejected promise and a synchronous + * throw. All three stream closes go through here, and every one of them has + * already dropped its reference to the stream — so in each case an escaping + * failure abandons a stream that may still be open on the server. What it does + * *besides* that differs per site, because the shape of the call does: + * + * - `resetSubscriptionStream` — fire-and-forget (`void`), and the last statement + * of the method. Because this helper is `async`, even a synchronous throw + * becomes a rejection of the promise it returns, which the `void` then drops: + * the caller is unaffected, and the harm is an *unhandled rejection* — fatal + * to a Node process by default, a console error in the browser. That, not + * skipped teardown, is what the wrapping buys at this site. + * - `refreshModernSubscription`'s re-listen close — awaited, with the + * replacement `listen()` after it, so an escaping failure skips the re-listen + * and leaves a non-empty subscription set with no stream. On the reconnect + * caller it is instead absorbed into the backoff run and retried. + * - the superseded-generation discard — awaited, but `return` follows, so + * nothing local is skipped; the failure only propagates out of + * `refreshModernSubscription` to whichever caller a newer refresh had already + * superseded. Self-healing on the reconnect path; on the two user-initiated + * paths it is a report of a failure that did not happen — for + * `unsubscribeFromResource`, whose removal is kept regardless, a "Failed to + * unsubscribe" for an unsubscribe that stuck. Both of those gate their + * failure handling on still owning the re-listen, so a superseded call + * reports its error without editing a filter or a badge that is no longer + * its own (see `subscribeToResource`'s catch for the reasoning). + * + * Note what the generation bump proves in that last case is that a newer + * refresh had *started*, not that it succeeded — it may yet fail at its own + * `listen()`, making an escaping failure here a second one rather than a + * redundant one. + * + * Wrapped identically all the same, so the rule is one rule (#1630, #1797). + */ +async function closeSubscriptionBestEffort( + subscription: Pick, +): Promise { + try { + await subscription.close(); + } catch { + // Best-effort: there is nothing to do about a stream that won't close. + } +} + /** * Extract the method literal from an MCP notification Zod schema (e.g. * `ToolListChangedNotificationSchema`), or `undefined` if the shape isn't @@ -320,8 +367,8 @@ export class InspectorClient extends InspectorClientEventTarget { // request so server logs arrive on each request's stream; `undefined` means // "don't opt in" (logs stay silently absent). Only honored on the modern era. private modernLogLevel?: LoggingLevel; - private sample: boolean; - private elicit: boolean | { form?: boolean; url?: boolean }; + private readonly sample: boolean; + private readonly elicit: boolean | { form?: boolean; url?: boolean }; private progress: boolean; private resetTimeoutOnProgress: boolean; private requestTimeout: number | undefined; @@ -363,6 +410,46 @@ export class InspectorClient extends InspectorClientEventTarget { private pendingElicitations: ElicitationCreateMessage[] = []; // Roots (undefined means roots capability not enabled, empty array means enabled but no roots) private roots: Root[] | undefined; + /** + * Whether `capabilities.roots` was advertised at `initialize`, read off the + * capability object actually sent rather than re-derived from the constructor + * option. Fixed for the client's lifetime, because the capability is + * negotiated at construction and the SDK refuses `registerCapabilities` after + * connect. + * + * The `roots/list` registration gates on *this*, not on `this.roots`, which + * `setRoots()` can make defined later: the SDK throws "Client does not support + * roots capability" from `setRequestHandler` when the capability was never + * advertised, and that throw would land before the handshake on every + * subsequent `connect()` — wedging the client permanently (#1797). + */ + private readonly rootsCapabilityAdvertised: boolean; + /** + * Whether `capabilities.elicitation` was advertised, on the same terms as + * {@link rootsCapabilityAdvertised}. Not the same question as `this.elicit` + * being truthy: `{}` / `{ form: false, url: false }` are valid options that + * enable no mode, so nothing is advertised — and registering + * `elicitation/create` anyway throws "Client does not support elicitation + * capability" before the handshake, leaving the client unable to connect + * at all (#1797). + */ + private readonly elicitationCapabilityAdvertised: boolean; + /** As above, for `capabilities.sampling`. */ + private readonly samplingCapabilityAdvertised: boolean; + /** As above, for `capabilities.tasks` (the receiver-side `tasks/*` polls). */ + private readonly tasksCapabilityAdvertised: boolean; + /** As above, for `capabilities.elicitation.url` (the URL-mode completion). */ + private readonly urlElicitationCapabilityAdvertised: boolean; + /** + * As above, for `capabilities.roots.listChanged` — the predicate the SDK + * asserts before letting us *send* `roots/list_changed`, which is narrower + * than the `capabilities.roots` presence it asserts before letting us + * *register* `roots/list`. They coincide today (roots are only ever + * advertised as `{ listChanged: true }`), but reading each assertion's own + * predicate is what keeps a future conditional advertisement from silently + * un-gating one of them. + */ + private readonly rootsListChangedCapabilityAdvertised: boolean; // Content cache // ListChanged notification configuration private listChangedNotifications: { @@ -430,10 +517,10 @@ export class InspectorClient extends InspectorClientEventTarget { // `cancelRequestorTask` instead, so they don't use this (#1458). private activeToolCallAbortController?: AbortController; // Receiver tasks (server-initiated: server sends createMessage/elicit with params.task, server polls us) - private receiverTasks: boolean; + private readonly receiverTasks: boolean; // Per-extension advertise overrides (#1738); undefined key falls back to the // registry default in ADVERTISABLE_EXTENSIONS. - private advertisedExtensions?: Record; + private readonly advertisedExtensions?: Record; private receiverTaskTtlMs: number | (() => number); private receiverTaskRecords: Map = new Map(); // OAuth support (config owned by oauthManager; client delegates and uses !!oauthManager for "is OAuth configured") @@ -486,15 +573,19 @@ export class InspectorClient extends InspectorClientEventTarget { // the Logs-tab control. Absence means DEFAULT_MODERN_LOG_LEVEL; `"off"` // clears the opt-in. Only stamped on modern connections (see mergeMeta) — // legacy uses `logging/setLevel`. - const settingLevel = - options.serverSettings?.modernLogLevel ?? DEFAULT_MODERN_LOG_LEVEL; - this.modernLogLevel = settingLevel === "off" ? undefined : settingLevel; + this.modernLogLevel = resolveModernLogLevel(options.serverSettings); // Default to the legacy 2025-11-25 era when the caller doesn't pin one, per // the SDK guidance that a debugging tool must not auto-probe (#1626). this.versionNegotiation = options.versionNegotiation ?? { mode: "legacy" }; this.directAuthRecovery = options.directAuthRecovery ?? false; - // Only set roots if explicitly provided (even if empty array) - this enables roots capability - this.roots = options.roots; + // Only set roots if explicitly provided (even if empty array) - this enables + // roots capability. Normalized here as well as in `setRoots`, so core owns + // the invariant rather than trusting each client to clean at its call site + // (they do, and `cleanRoots` is idempotent, so this costs nothing). The + // ternary preserves the `undefined`-vs-`[]` distinction the capability + // advertisement below gates on. + this.roots = + options.roots !== undefined ? cleanRoots(options.roots) : undefined; // Initialize listChangedNotifications config (default: all enabled) this.listChangedNotifications = { tools: options.listChangedNotifications?.tools ?? true, @@ -590,13 +681,24 @@ export class InspectorClient extends InspectorClientEventTarget { } // Receiver tasks: advertise so server can send task-augmented createMessage/elicit and poll us if (this.receiverTasks) { + // `requests` declares which server→client requests we accept as tasks, so + // it must name only capabilities we actually advertised — both are decided + // above. Advertising a channel we then answer `-32601` on is the shape + // #1797 is about, and `{ receiverTasks: true, elicit: false }` would do + // exactly that. + const taskRequests: NonNullable< + NonNullable["requests"] + > = {}; + if (capabilities.sampling) { + taskRequests.sampling = { createMessage: {} }; + } + if (capabilities.elicitation) { + taskRequests.elicitation = { create: {} }; + } capabilities.tasks = { list: {}, cancel: {}, - requests: { - sampling: { createMessage: {} }, - elicitation: { create: {} }, - }, + ...(Object.keys(taskRequests).length > 0 && { requests: taskRequests }), }; } // Assemble the advertised-extensions map from one builder (the single @@ -619,6 +721,19 @@ export class InspectorClient extends InspectorClientEventTarget { } clientOptions.capabilities = capabilities; this.clientCapabilities = capabilities; + // Read off the built capability object rather than re-deriving from + // `options.roots`: the gate and the advertisement must agree, and two + // independent derivations of the same fact can drift (a `readonly` field is + // assignable anywhere in the constructor). + this.rootsCapabilityAdvertised = capabilities.roots !== undefined; + this.elicitationCapabilityAdvertised = + capabilities.elicitation !== undefined; + this.samplingCapabilityAdvertised = capabilities.sampling !== undefined; + this.tasksCapabilityAdvertised = capabilities.tasks !== undefined; + this.urlElicitationCapabilityAdvertised = + capabilities.elicitation?.url !== undefined; + this.rootsListChangedCapabilityAdvertised = + capabilities.roots?.listChanged === true; this.appRendererClientProxy = null; this.clientInfo = options.clientIdentity ?? { @@ -714,6 +829,22 @@ export class InspectorClient extends InspectorClientEventTarget { this.status = "disconnected"; this.dispatchTypedEvent("statusChange", this.status); } + // A mid-session crash ends the connection without going through + // `disconnect()`, so drop anything the server had queued with us — the + // same reasoning as the `connect()` failure path. Cleared before the + // `disconnect` event so a consumer reading the queue while handling it + // sees it empty. `disconnect()` clears at the same point, but batches its + // change events with the rest of its teardown dispatches — so on that + // path the *events* land just after its `disconnect`, not before. + this.clearAndAnnouncePendingPeerRequests(); + // Same for what *we* asked the server. The SDK's chained `_onclose` + // settles its own `_responseHandlers`, but the raw-wire map (the modern + // `tasks/*` frames its era gate refuses to route) is ours and it doesn't + // know about it — so a Tasks-tab poll in flight when the server dies + // would otherwise wait out its own 30s timeout and blame the timeout for + // a crash. Rejecting a settled promise is a no-op and the helper clears + // the map, so this can't double-settle with `disconnect()`. + this.rejectPendingRawWireRequests("Connection closed"); this.dispatchTypedEvent("disconnect"); }; baseTransport.onerror = (error: Error) => { @@ -904,7 +1035,10 @@ export class InspectorClient extends InspectorClientEventTarget { if (!validating) return; internal._requestHandlers.set(method, (request, ctx) => { const task = (request as { params?: { task?: unknown } })?.params?.task; - if (this.receiverTasks && task != null) { + // The advertisement check is redundant here — this wrapper only exists + // when tasks are advertised — but it mirrors the handler branch below + // deliberately: the two must agree, so they read one predicate. + if (this.tasksCapabilityAdvertised && task != null) { return rawHandler(request as CreateMessageRequest & ElicitRequest); } return validating(request, ctx); @@ -939,6 +1073,13 @@ export class InspectorClient extends InspectorClientEventTarget { resolvePayload = resolve; rejectPayload = reject; }); + // Mark it handled. The real consumer is the server polling `tasks/result` + // (`getReceiverTaskPayload` returns this same promise, so a real awaiter + // still sees the rejection), but nothing has attached a handler while the + // task sits in `input_required` — and it can be rejected from there, by an + // explicit `tasks/cancel` or by teardown settling a queued sample. Without + // this, that reject surfaces as an unhandled rejection. + void payloadPromise.catch(() => {}); const record: ReceiverTaskRecord = { task, payloadPromise, @@ -1048,6 +1189,423 @@ export class InspectorClient extends InspectorClientEventTarget { this.transportHasAuthProvider = false; } + /** + * Register the handlers for requests the *server* makes of *us* — + * `roots/list`, `sampling/createMessage`, `elicitation/create`, and the + * receiver-side `tasks/*` polls. + * + * MUST be called before `client.connect()`. The matching capabilities are + * advertised on the `Client` at construction time, so from the moment + * `connect()` sends `notifications/initialized` the server is entitled to + * issue any of these requests. Registering afterwards leaves a window in + * which the SDK `Client` has no handler and answers `-32601 Method not + * found` — which is exactly what a server that asks for roots the instant it + * is initialized (e.g. `server-filesystem`, which learns its allowed + * directories that way) hits, while a server that asks later does not (#1797). + * + * Nothing here depends on the server's capabilities — only on constructor-set + * state — so there is nothing to wait for. Its sibling + * {@link registerPeerNotificationHandlers} does the same for the one + * notification handler in that position; the notification handlers that *do* + * gate on `this.capabilities` stay in `connect()`, after the handshake. + */ + private registerPeerRequestHandlers(): void { + // Gated on what was advertised, like the others — see + // `rootsCapabilityAdvertised`. + if (this.samplingCapabilityAdvertised && this.client) { + const samplingHandler = ( + request: CreateMessageRequest, + ): Promise => { + const paramsTask = (request.params as { task?: { ttl?: number } }) + ?.task; + if (this.tasksCapabilityAdvertised && paramsTask != null) { + const record = this.createReceiverTask({ + ttl: paramsTask.ttl, + initialStatus: "input_required", + statusMessage: "Awaiting user input", + }); + void (async () => { + const samplingRequest = new SamplingCreateMessage( + request, + (result) => { + record.resolvePayload(result); + const now = new Date().toISOString(); + const updated: Task = { + ...record.task, + status: "completed", + lastUpdatedAt: now, + }; + record.task = updated; + this.upsertReceiverTask(updated); + }, + (error) => { + record.rejectPayload(error); + const now = new Date().toISOString(); + const updated: Task = { + ...record.task, + status: "failed", + lastUpdatedAt: now, + statusMessage: + error instanceof Error ? error.message : String(error), + }; + record.task = updated; + this.upsertReceiverTask(updated); + }, + (id) => this.removePendingSample(id), + ); + this.addPendingSample(samplingRequest); + })(); + // Task-augmented (2025-11-25) response: the server sent a + // task-augmented `sampling/createMessage`, so we reply with a + // `CreateTaskResult` (`{ task }`) rather than a `CreateMessageResult`. + // The v2 Client validates a spec handler's result and would reject + // `{ task }` with -32602; `installReceiverTaskResponseBypass` below + // routes this task-augmented branch around that validation so the + // legacy `{ task }` response reaches the wire. `taskResult` is typed + // as `CreateTaskResult` so its shape IS checked; the unavoidable + // `as unknown as CreateMessageResult` bridges the SDK gap — the 2-arg + // `setRequestHandler` overload types a sampling handler's return as + // `CreateMessageResult` only and doesn't model the (deprecated but + // wire-valid) task-augmented `CreateTaskResult`. A handler-result + // union `CreateMessageResult | CreateTaskResult` on the SDK side + // would remove this cast. + const taskResult: CreateTaskResult = { task: record.task }; + return Promise.resolve(taskResult as unknown as CreateMessageResult); + } + return this.enqueuePendingSample(request, "server-request"); + }; + this.client.setRequestHandler("sampling/createMessage", samplingHandler); + // Registration, like the `setRequestHandler` above it — and the whole + // bypass mechanism (install, wrapper branch, handler branch) reads this + // one predicate, so the install can't drift from the branch it controls. + if (this.tasksCapabilityAdvertised) { + this.installReceiverTaskResponseBypass( + "sampling/createMessage", + samplingHandler, + ); + } + } + + // Gated on what was advertised, not on `this.elicit` — see the field's doc: + // an elicit option that enables no mode advertises nothing, and registering + // regardless throws before the handshake. + if (this.elicitationCapabilityAdvertised && this.client) { + const elicitHandler = (request: ElicitRequest): Promise => { + const paramsTask = (request.params as { task?: { ttl?: number } }) + ?.task; + if (this.tasksCapabilityAdvertised && paramsTask != null) { + const record = this.createReceiverTask({ + ttl: paramsTask.ttl, + initialStatus: "input_required", + statusMessage: "Awaiting user input", + }); + void (async () => { + const elicitationRequest = new ElicitationCreateMessage( + request, + (result) => { + record.resolvePayload(result); + const now = new Date().toISOString(); + const updated: Task = { + ...record.task, + status: "completed", + lastUpdatedAt: now, + }; + record.task = updated; + this.upsertReceiverTask(updated); + }, + (id) => this.removePendingElicitation(id), + (error) => { + record.rejectPayload(error); + const now = new Date().toISOString(); + const updated: Task = { + ...record.task, + status: "failed", + lastUpdatedAt: now, + statusMessage: error.message, + }; + record.task = updated; + this.upsertReceiverTask(updated); + }, + ); + this.addPendingElicitation(elicitationRequest); + })(); + // Task-augmented (2025-11-25) response — see the sampling handler + // above. Reply with a `CreateTaskResult` (`{ task }`), routed around + // the v2 Client's result validation by + // `installReceiverTaskResponseBypass` below. `taskResult` is typed so + // its shape is checked; the `as unknown as ElicitResult` bridges the + // same SDK gap as the sampling handler — the 2-arg `setRequestHandler` + // overload types an elicitation handler's return as `ElicitResult` + // only and doesn't model the task-augmented `CreateTaskResult`. + const taskResult: CreateTaskResult = { task: record.task }; + return Promise.resolve(taskResult as unknown as ElicitResult); + } + return this.enqueuePendingElicitation(request, "server-request"); + }; + this.client.setRequestHandler("elicitation/create", elicitHandler); + // Registration, like the `setRequestHandler` above it — and the whole + // bypass mechanism (install, wrapper branch, handler branch) reads this + // one predicate, so the install can't drift from the branch it controls. + if (this.tasksCapabilityAdvertised) { + this.installReceiverTaskResponseBypass( + "elicitation/create", + elicitHandler, + ); + } + } + + // Gated on what was advertised at construction, and it has to be: the SDK + // asserts the matching client capability inside `setRequestHandler`, so + // registering this on a client built without `roots` throws "Client does + // not support roots capability". Since `capabilities.roots` is negotiated at + // `initialize` (set in the constructor) and `registerCapabilities` refuses + // to run after connect, a client that omits the option can never serve + // `roots/list` — which is why every client that may call `setRoots()` later + // must pass `roots` up front (web does; the CLI and TUI now do too — #1797). + if (this.rootsCapabilityAdvertised && this.client) { + this.client.setRequestHandler("roots/list", async () => { + return { roots: this.roots ?? [] }; + }); + } + + // Set up receiver-task request handlers (server polls us for tasks/list, + // tasks/get, tasks/result, tasks/cancel). SDK v2 removed tasks from the + // spec-method set, so these register through the 3-arg custom form with an + // explicit params schema (from the deprecated-but-importable task request + // schemas). The `result` schema is intentionally omitted so the SDK does + // not validate our responder return — matching v1, where only the + // requester validated (our receiver `Task` may omit fields a strict result + // schema would require). + if (this.tasksCapabilityAdvertised && this.client) { + this.client.setRequestHandler( + "tasks/list", + { params: ListTasksRequestSchema.shape.params }, + async () => ({ tasks: this.listReceiverTasks() }), + ); + this.client.setRequestHandler( + "tasks/get", + { params: GetTaskRequestSchema.shape.params }, + async (params) => { + const record = this.getReceiverTask(params.taskId); + if (!record) { + throw new ProtocolError( + ProtocolErrorCode.InvalidParams, + `Unknown taskId: ${params.taskId}`, + ); + } + return record.task; + }, + ); + this.client.setRequestHandler( + "tasks/result", + { params: GetTaskPayloadRequestSchema.shape.params }, + async (params) => this.getReceiverTaskPayload(params.taskId), + ); + this.client.setRequestHandler( + "tasks/cancel", + { params: CancelTaskRequestSchema.shape.params }, + async (params) => this.cancelReceiverTask(params.taskId), + ); + } + } + + /** + * Register the inbound *notification* handlers that depend on nothing but + * constructor-set state, and so belong before the handshake for the same + * reason as {@link registerPeerRequestHandlers} (#1797). + * + * `notifications/roots/list_changed` is the only one — and note it is a + * **client**→server notification in the spec (`ClientNotification`): we send + * it from {@link setRoots}, servers do not normally send it to us. This + * inbound handler is defensive coverage for a non-conformant or experimental + * server, and its body dispatches `rootsChange` with our own already-known + * roots, i.e. a refresh signal carrying no new data. It sits here for + * consistency with the request handlers — it gates on no server capability, + * so there is nothing to wait for, and an unhandled notification is dropped + * silently by the SDK (no wire error) rather than answered `-32601`. + * + * The remaining listChanged handlers gate on `this.capabilities`, which is + * not populated until `fetchServerInfo()` runs, so they stay in `connect()`. + */ + private registerPeerNotificationHandlers(): void { + /* v8 ignore next -- unreachable: the sole caller is connect(), past its + `if (!this.client) throw`; the guard exists only to narrow the type. */ + if (!this.client) return; + this.client.setNotificationHandler( + "notifications/roots/list_changed", + async () => { + // Re-dispatch our already-known roots as a refresh signal for the UI — + // the payload carries no new data (see the note on ownership above). + // Copied, as `setRoots` and `getRoots()` do: a listener must not be + // able to push into the list we advertise. + this.dispatchTypedEvent("rootsChange", [...(this.roots ?? [])]); + }, + ); + } + + /** + * Stop the receiver tasks' TTL timers and drop the records. + * + * These are tasks a *server* created with us, so they belong to the session + * that created them: `listReceiverTasks()` is what the `tasks/list` handler + * answers with, and a record surviving into the next session would report a + * task the new server never created. `disconnect()` clears them, and so does + * `connect()` — the auth-recovery retry reconnects the *same* client + * instance, so ending the session isn't the only way a new one begins + * (#1797). + */ + private clearReceiverTasks(): void { + for (const record of this.receiverTaskRecords.values()) { + if (record.cleanupTimeoutId != null) { + clearTimeout(record.cleanupTimeoutId); + } + } + this.receiverTaskRecords.clear(); + } + + /** + * Reset the modern listen-stream cluster: the subscribed set, the stream + * state derived from it, and the reconnect machinery that reports on it. + * + * One helper because these move together — the rest of the file derives the + * stream's `active` from `subscribedResources.size > 0`, so clearing one + * without the other leaves a combination those readers treat as impossible, + * and a surviving `modernReconnectAttempts` makes a *new* session's first + * drop back off as if it were the old session's nth. Both axes are announced: + * every other mutation of the set dispatches, and so does the stream state. + * + * Closes the stream itself, best-effort. `disconnect()` is the obvious caller + * with a live one, but not the only one: an `onerror` without an `onclose` + * leaves the transport up, and `connect()` then reuses it — so the reference + * dropped here can be the last one to a stream still open on the server. + */ + private resetSubscriptionStream(): void { + const closing = this.modernSubscription; + this.subscribedResources.clear(); + this.modernListenGeneration++; + this.clearModernReconnectTimer(); + this.modernReconnectAttempts = 0; + this.modernSubscription = null; + // Announced only once both have moved: a listener that ran between them + // would see an empty set with an `active` stream — the combination this + // helper exists to prevent, so its own dispatches must not expose it. + this.setModernStreamState(INACTIVE_SUBSCRIPTION_STREAM_STATE); + this.dispatchSubscriptionsChange(); + // After the dispatches, so the ordering above is unaffected, and + // fire-and-forget because nothing downstream depends on it. That is also + // why the wrapping matters here in a different way than at the awaited + // sites: the `void` means a failure cannot reach the caller at all — so it + // cannot cut `disconnect()`'s straight-line teardown short — and what it + // would do instead is go unhandled, which ends a Node process by default. + if (closing) void closeSubscriptionBestEffort(closing); + } + + /** + * Drop the session-scoped state a *new* session would misread — anything the + * next server could be told about, or that would change how we treat its + * traffic. State that only needs settling on the way out (the peer-request + * queues, the raw-wire map, the in-flight tool call) is not reset here — the + * in-flight tool call because `callTool`'s own `finally` releases it once the + * SDK rejects it, the other two because they are settled *end*-clean, by + * `disconnect()` and the crash path, so that a consumer handling the + * `disconnect` event already sees them empty. That is a difference in when, + * not in whether: the routes out do not cover every route *in* (an `onerror` + * without an `onclose` runs neither), so `connect()` sweeps those two beside + * this call as a backstop — see the comment there. + * + * `disconnect()` touches all of this on the way out too — two members through + * the same helpers, three hand-rolled in both places — so a sixth member + * added here has to be added there as well. One of the three is *paired* + * rather than duplicated: `modernLogLevel` is re-derived here and blanked + * there, deliberately (see the comment at that site). It is not the only + * way a session ends — a crash, or a failed connect the caller retries on + * this same instance (the auth-recovery path), both leave it behind. Called + * start-clean from `connect()` so every route in is covered. + * + * Each member has a symptom, not just untidiness: a stale `subscribedResources` + * entry makes the modern `subscribeToResource` early-return, so the user's + * Subscribe click silently sends nothing to the new server; a stale + * `cancelledTaskIds` entry mislabels a *new* task sharing the id as + * `cancelled` rather than `failed`; a stale subscription stream state reads + * `active` for a set that is now empty, which every reader of it treats as + * impossible; a receiver-task record is reported to the new server by + * `tasks/list`; and an un-aborted `taskInputAbortControllers` + * entry delays a paused poll loop unwinding — both registration sites release + * in a `finally`, so nothing leaks permanently; the abort just closes the + * window between the crash and the unwind (#1797). + */ + private resetSessionState(): void { + this.clearReceiverTasks(); + this.resetSubscriptionStream(); + this.cancelledTaskIds.clear(); + for (const [, controller] of this.taskInputAbortControllers) { + controller.abort(new Error("Connection ended")); + } + this.taskInputAbortControllers.clear(); + // Restore the configured opt-in rather than carrying a mid-session + // `setModernLogLevel` override into the next connection — and rather than + // leaving it `undefined` after a `disconnect()` cleared it, which silently + // dropped the user's configured level on reconnect (#1629, #1797). + this.modernLogLevel = resolveModernLogLevel(this.serverSettings); + } + + /** + * Settle and drop the queued peer requests (sampling / elicitation). + * + * Every entry is settled before being dropped rather than discarded: an + * elicitation so an error-path `awaitUrlElicitation` — which blocks + * `callTool` — doesn't hang forever, and a sample so the *server* gets a + * response frame for the request we accepted (the transport can outlive a + * failed attempt; see the `connect()` catch). Callers dispatch the change + * events themselves: + * `disconnect()` batches them with its other teardown dispatches, and + * {@link clearAndAnnouncePendingPeerRequests} emits them immediately + * everywhere else. + */ + private clearPendingPeerRequests(): void { + for (const sample of this.pendingSamples) { + sample.cancel(); + } + this.pendingSamples = []; + for (const elicitation of this.pendingElicitations) { + elicitation.cancel(); + } + this.pendingElicitations = []; + } + + /** + * {@link clearPendingPeerRequests} plus the change events, for every route + * that drops a queue without going through `disconnect()`: the routes *out* + * that end a connection some other way, plus the top of `connect()` as a + * backstop for the one route in that settles nothing (an `onerror` without an + * `onclose` — see the comment at that call). Named as a category rather than + * counted, because the set has grown before. (One of the routes out doesn't + * always end the connection: when a `connect()` failure leaves an auth + * provider holding the transport open, the caller re-authenticates and + * retries over it, and what's dropped is the queue left by the attempt that + * failed.) + * + * The events are the load-bearing half: `usePendingClientRequests` tracks its + * own state off them, so clearing the arrays without dispatching leaves the + * web pending-request modal on screen for a connection that is gone. Guarded + * on a non-empty queue so the paths that overlap (a `connect()` failure whose + * `dropCachedTransport` also fires `onclose`) announce it once. + */ + private clearAndAnnouncePendingPeerRequests(): void { + if ( + this.pendingSamples.length === 0 && + this.pendingElicitations.length === 0 + ) { + return; + } + this.clearPendingPeerRequests(); + this.dispatchTypedEvent("pendingSamplesChange", this.pendingSamples); + this.dispatchTypedEvent( + "pendingElicitationsChange", + this.pendingElicitations, + ); + } + /** * Connect to the MCP server */ @@ -1059,6 +1617,44 @@ export class InspectorClient extends InspectorClientEventTarget { return; } + // Start from a clean session — see `resetSessionState` for why this is + // start-clean rather than relying on `disconnect()`. + this.resetSessionState(); + // The two collections `resetSessionState` excludes as "settled on the way + // out", swept here as well — because one route out settles nothing. An + // `onerror` without an `onclose` only flips status to `"error"`: it runs + // neither teardown path, and it leaves `baseTransport` cached, so a + // `connect()` on this same instance reuses a *live* transport. That is the + // route the subscription-stream close exists for, and it strands these two + // the same way. The peer queue is the sharper of them — the web + // pending-request modal is derived from its length with no status gate, so + // it outlives the session, and a user answering it later would write + // *their* answer for the previous session's request id onto the new + // connection, arbitrarily far past the re-handshake. Note what the sweep + // does instead is emit a *cancel* for that same id, right here: still the + // settle-don't-discard rule, and this is the earliest moment available: + // the old connection is still the one on the wire here, and stays so at + // least until the conditional `dropCachedTransport()` below — which on a + // stdio server never runs at all, so the same transport carries straight + // through the re-handshake. + // + // Both helpers are idempotent (one guards on a non-empty queue, the other + // clears its map and re-rejecting a settled promise is a no-op), so these + // are no-ops on the routes that already ran them; and anything still + // pending here belongs to a session that is, by definition, no longer + // connected. + // + // Must stay *after* `resetSessionState()`, which reads as independent of it + // but is not: cancelling a task-augmented peer request settles it + // synchronously into the record callback, which ends in + // `upsertReceiverTask`. That is a no-op only because `clearReceiverTasks()` + // just emptied the map — hoisted above the reset, it would instead emit a + // `notifications/tasks/status` for the outgoing session's task, onto the + // transport this connect is about to reuse, moments before the reset drops + // the record anyway. + this.clearAndAnnouncePendingPeerRequests(); + this.rejectPendingRawWireRequests("Connection ended"); + const oauthManager = this.oauthManager; if ( this.baseTransport && @@ -1165,6 +1761,12 @@ export class InspectorClient extends InspectorClientEventTarget { this.status = "connecting"; this.dispatchTypedEvent("statusChange", this.status); + // Register the handlers for server→client requests and the + // capability-independent notifications before the handshake — see + // `registerPeerRequestHandlers` for why the ordering is load-bearing. + this.registerPeerRequestHandlers(); + this.registerPeerNotificationHandlers(); + // Optional connect-time timeout from per-server settings. The MCP SDK // has no connect-time timeout option, so we wrap the handshake in a // Promise.race. On timeout, tear the transport down so the next @@ -1224,209 +1826,6 @@ export class InspectorClient extends InspectorClientEventTarget { ); } - // Set up sampling request handler if sampling capability is enabled - if (this.sample && this.client) { - const samplingHandler = ( - request: CreateMessageRequest, - ): Promise => { - const paramsTask = (request.params as { task?: { ttl?: number } }) - ?.task; - if (this.receiverTasks && paramsTask != null) { - const record = this.createReceiverTask({ - ttl: paramsTask.ttl, - initialStatus: "input_required", - statusMessage: "Awaiting user input", - }); - void (async () => { - const samplingRequest = new SamplingCreateMessage( - request, - (result) => { - record.resolvePayload(result); - const now = new Date().toISOString(); - const updated: Task = { - ...record.task, - status: "completed", - lastUpdatedAt: now, - }; - record.task = updated; - this.upsertReceiverTask(updated); - }, - (error) => { - record.rejectPayload(error); - const now = new Date().toISOString(); - const updated: Task = { - ...record.task, - status: "failed", - lastUpdatedAt: now, - statusMessage: - error instanceof Error ? error.message : String(error), - }; - record.task = updated; - this.upsertReceiverTask(updated); - }, - (id) => this.removePendingSample(id), - ); - this.addPendingSample(samplingRequest); - })(); - // Task-augmented (2025-11-25) response: the server sent a - // task-augmented `sampling/createMessage`, so we reply with a - // `CreateTaskResult` (`{ task }`) rather than a `CreateMessageResult`. - // The v2 Client validates a spec handler's result and would reject - // `{ task }` with -32602; `installReceiverTaskResponseBypass` below - // routes this task-augmented branch around that validation so the - // legacy `{ task }` response reaches the wire. `taskResult` is typed - // as `CreateTaskResult` so its shape IS checked; the unavoidable - // `as unknown as CreateMessageResult` bridges the SDK gap — the 2-arg - // `setRequestHandler` overload types a sampling handler's return as - // `CreateMessageResult` only and doesn't model the (deprecated but - // wire-valid) task-augmented `CreateTaskResult`. A handler-result - // union `CreateMessageResult | CreateTaskResult` on the SDK side - // would remove this cast. - const taskResult: CreateTaskResult = { task: record.task }; - return Promise.resolve( - taskResult as unknown as CreateMessageResult, - ); - } - return this.enqueuePendingSample(request, "server-request"); - }; - this.client.setRequestHandler( - "sampling/createMessage", - samplingHandler, - ); - if (this.receiverTasks) { - this.installReceiverTaskResponseBypass( - "sampling/createMessage", - samplingHandler, - ); - } - } - - // Set up elicitation request handler if elicitation capability is enabled - if (this.elicit && this.client) { - const elicitHandler = ( - request: ElicitRequest, - ): Promise => { - const paramsTask = (request.params as { task?: { ttl?: number } }) - ?.task; - if (this.receiverTasks && paramsTask != null) { - const record = this.createReceiverTask({ - ttl: paramsTask.ttl, - initialStatus: "input_required", - statusMessage: "Awaiting user input", - }); - void (async () => { - const elicitationRequest = new ElicitationCreateMessage( - request, - (result) => { - record.resolvePayload(result); - const now = new Date().toISOString(); - const updated: Task = { - ...record.task, - status: "completed", - lastUpdatedAt: now, - }; - record.task = updated; - this.upsertReceiverTask(updated); - }, - (id) => this.removePendingElicitation(id), - (error) => { - record.rejectPayload(error); - const now = new Date().toISOString(); - const updated: Task = { - ...record.task, - status: "failed", - lastUpdatedAt: now, - statusMessage: error.message, - }; - record.task = updated; - this.upsertReceiverTask(updated); - }, - ); - this.addPendingElicitation(elicitationRequest); - })(); - // Task-augmented (2025-11-25) response — see the sampling handler - // above. Reply with a `CreateTaskResult` (`{ task }`), routed around - // the v2 Client's result validation by - // `installReceiverTaskResponseBypass` below. `taskResult` is typed so - // its shape is checked; the `as unknown as ElicitResult` bridges the - // same SDK gap as the sampling handler — the 2-arg `setRequestHandler` - // overload types an elicitation handler's return as `ElicitResult` - // only and doesn't model the task-augmented `CreateTaskResult`. - const taskResult: CreateTaskResult = { task: record.task }; - return Promise.resolve(taskResult as unknown as ElicitResult); - } - return this.enqueuePendingElicitation(request, "server-request"); - }; - this.client.setRequestHandler("elicitation/create", elicitHandler); - if (this.receiverTasks) { - this.installReceiverTaskResponseBypass( - "elicitation/create", - elicitHandler, - ); - } - } - - // Set up roots/list request handler if roots capability is enabled - if (this.roots !== undefined && this.client) { - this.client.setRequestHandler("roots/list", async () => { - return { roots: this.roots ?? [] }; - }); - } - - // Set up receiver-task request handlers (server polls us for tasks/list, - // tasks/get, tasks/result, tasks/cancel). SDK v2 removed tasks from the - // spec-method set, so these register through the 3-arg custom form with an - // explicit params schema (from the deprecated-but-importable task request - // schemas). The `result` schema is intentionally omitted so the SDK does - // not validate our responder return — matching v1, where only the - // requester validated (our receiver `Task` may omit fields a strict result - // schema would require). - if (this.receiverTasks && this.client) { - this.client.setRequestHandler( - "tasks/list", - { params: ListTasksRequestSchema.shape.params }, - async () => ({ tasks: this.listReceiverTasks() }), - ); - this.client.setRequestHandler( - "tasks/get", - { params: GetTaskRequestSchema.shape.params }, - async (params) => { - const record = this.getReceiverTask(params.taskId); - if (!record) { - throw new ProtocolError( - ProtocolErrorCode.InvalidParams, - `Unknown taskId: ${params.taskId}`, - ); - } - return record.task; - }, - ); - this.client.setRequestHandler( - "tasks/result", - { params: GetTaskPayloadRequestSchema.shape.params }, - async (params) => this.getReceiverTaskPayload(params.taskId), - ); - this.client.setRequestHandler( - "tasks/cancel", - { params: CancelTaskRequestSchema.shape.params }, - async (params) => this.cancelReceiverTask(params.taskId), - ); - } - - // Set up notification handler for roots/list_changed from server - if (this.client) { - this.client.setNotificationHandler( - "notifications/roots/list_changed", - async () => { - // Dispatch event to notify UI that server's roots may have changed - // Note: rootsChange is a CustomEvent with Root[] payload, not a signal event - // We'll reload roots when the UI requests them, so we don't need to pass data here - // For now, we'll just dispatch an empty array as a signal to reload - this.dispatchTypedEvent("rootsChange", this.roots || []); - }, - ); - } - // Set up listChanged notification handlers based on config if (this.client) { // Tools listChanged handler @@ -1515,11 +1914,7 @@ export class InspectorClient extends InspectorClientEventTarget { // Elicitation complete notification (URL mode only): server notifies when out-of-band // elicitation completes; we resolve the corresponding pending elicitation - const urlElicitEnabled = - this.elicit && - typeof this.elicit === "object" && - this.elicit.url === true; - if (urlElicitEnabled) { + if (this.urlElicitationCapabilityAdvertised) { this.client.setNotificationHandler( "notifications/elicitation/complete", async (notification) => { @@ -1552,6 +1947,18 @@ export class InspectorClient extends InspectorClientEventTarget { if (this.baseTransport && !this.transportHasAuthProvider) { await this.dropCachedTransport(); } + // The peer handlers are registered before the handshake (#1797), so a + // server can queue a sampling/elicitation request during it — and this is + // where that connect attempt dies. Drop the queue: otherwise the UI keeps + // a live pending-request modal for a connection that never came up, and + // answering it would route to a transport that is either torn down or — + // when an auth provider holds it open, so the caller can re-authenticate + // and retry over it — carrying a queue from an attempt that already + // failed. Note the retention is gated on `transportHasAuthProvider` + // alone, independently of `isConnectAuthRecoveryError` above, which gates + // only the status hold. `disconnect()` does the same clearing, but + // `connect()` only reaches it on the connect-timeout path. + this.clearAndAnnouncePendingPeerRequests(); // Deliberately do NOT dispatch the `error` event here: this is the // awaited `connect()` path, so re-throwing hands the reason straight to // the caller. The `error` event is reserved for non-awaited transitions @@ -1614,6 +2021,11 @@ export class InspectorClient extends InspectorClientEventTarget { this.baseTransport = null; this.transport = null; this.transportHasAuthProvider = false; + // Drop anything the server had queued with us before announcing the + // teardown, so a `disconnect` consumer sees an empty queue here as it does + // on the crash path. The change events stay batched with the other teardown + // dispatches below. + this.clearPendingPeerRequests(); // Update status - any onclose fired during close() above deferred to us // (see `disconnecting`), so this is the single place the explicit-disconnect // path settles the status and emits `disconnect`. @@ -1623,33 +2035,18 @@ export class InspectorClient extends InspectorClientEventTarget { this.dispatchTypedEvent("disconnect"); } - // Clear server state on disconnect (list state is in state managers). - // Settle any outstanding elicitations as cancelled before dropping them, so - // an error-path `awaitUrlElicitation` (which blocks `callTool`) doesn't hang - // forever when the queue is cleared on teardown. - this.pendingSamples = []; - for (const elicitation of this.pendingElicitations) { - elicitation.cancel(); - } - this.pendingElicitations = []; + // Clear the rest of the server state (list state is in state managers). // Clear resource subscriptions on disconnect. Tear down the modern listen // stream (best-effort — the transport is already going away) and bump the // generation so any in-flight re-listen/reconnect bails (#1630). - this.subscribedResources.clear(); - this.modernListenGeneration++; - this.clearModernReconnectTimer(); - this.modernReconnectAttempts = 0; - const closingSubscription = this.modernSubscription; - this.modernSubscription = null; - closingSubscription?.close().catch(() => {}); - this.modernStreamState = INACTIVE_SUBSCRIPTION_STREAM_STATE; - this.dispatchTypedEvent( - "resourceSubscriptionStreamChange", - INACTIVE_SUBSCRIPTION_STREAM_STATE, - ); + this.resetSubscriptionStream(); this.cancelledTaskIds.clear(); // Settle any pending raw-wire (modern tasks/*) requests so their callers - // don't hang past teardown. + // don't hang past teardown. Rejected outright on every disconnect: the + // drain above polls the SDK's own response-handler map, which never holds + // raw-wire ids (those frames go straight through the transport), and it is + // opt-in anyway — every production caller leaves `safeDisconnectTimeout` at + // 0, so nothing is drained for anyone. this.rejectPendingRawWireRequests("Disconnected"); // Abort any task paused at input_required so its poll loop unwinds. for (const [, controller] of this.taskInputAbortControllers) { @@ -1660,13 +2057,7 @@ export class InspectorClient extends InspectorClientEventTarget { // hanging past teardown; drop the controller reference either way. this.activeToolCallAbortController?.abort("Disconnected"); this.activeToolCallAbortController = undefined; - // Clear receiver tasks: stop TTL timers and drop records - for (const record of this.receiverTaskRecords.values()) { - if (record.cleanupTimeoutId != null) { - clearTimeout(record.cleanupTimeoutId); - } - } - this.receiverTaskRecords.clear(); + this.clearReceiverTasks(); this.appRendererClientProxy = null; this.capabilities = undefined; this.serverInfo = undefined; @@ -1675,8 +2066,12 @@ export class InspectorClient extends InspectorClientEventTarget { this.protocolEra = undefined; this.discoverResult = undefined; this.excludedTools = []; - // Drop the modern per-request log-level opt-in so it doesn't leak into the - // next connection's `_meta` (#1629). + // Read as "not opted in" while disconnected. This is no longer what stops + // it leaking into the next connection — `resetSessionState()` re-derives it + // at connect, so removing this would leak nothing (#1629). Note the web + // Logs control deliberately shows the *configured* level in this window + // (`resetSessionScopedUiState`), so the two disagree until the next + // connect re-seeds both; harmless, since nothing is sent meanwhile. this.modernLogLevel = undefined; this.dispatchTypedEvent("pendingSamplesChange", this.pendingSamples); this.dispatchTypedEvent( @@ -1950,7 +2345,11 @@ export class InspectorClient extends InspectorClientEventTarget { return true; } - /** Reject and clear all pending raw-wire requests (on disconnect/teardown). */ + /** + * Reject and clear all pending raw-wire requests — on every route out that + * can hold one, and at the top of `connect()` for the route in that settles + * nothing (see the comment there). + */ private rejectPendingRawWireRequests(reason: string): void { for (const [, pending] of this.pendingRawWireRequests) { clearTimeout(pending.timer); @@ -4198,23 +4597,51 @@ export class InspectorClient extends InspectorClientEventTarget { } /** - * Set roots and notify server if it supports roots/listChanged - * Note: This will enable roots capability if it wasn't already enabled + * Set roots and announce the change to the server. + * + * Note this does **not** enable the roots capability on a client that was + * built without the constructor's `roots` option. `capabilities.roots` is + * negotiated at `initialize` and the SDK refuses `registerCapabilities` + * after connect, so such a client has no `roots/list` handler (see {@link + * registerPeerRequestHandlers}) and would have to answer `-32601` if a + * server asked. So on such a client the roots set here are stored and + * readable via {@link getRoots}, but no server can ask for them and the + * change is not announced — the SDK refuses `roots/list_changed` from a + * client that never declared `roots.listChanged`, so the notification could + * not have gone out anyway. Pass `roots` at construction — `[]` is enough — + * in any client that may call this (#1797). + * + * The argument runs through `cleanRoots`, the same normalizer the + * connect-time and settings-save paths use, so all three ways roots enter the + * client agree and no caller can advertise a `Root` with no `uri` (the CLI's + * `--roots-json` only checks that the JSON is an array). It is idempotent, so + * a caller that already cleaned loses nothing. */ async setRoots(roots: Root[]): Promise { if (!this.client) { throw new Error("Client is not connected"); } - // Enable roots capability if not already enabled - if (this.roots === undefined) { - this.roots = []; + this.roots = cleanRoots(roots); + // Copy, as `getRoots()` does — a listener must not be able to push into the + // list we advertise. + this.dispatchTypedEvent("rootsChange", [...this.roots]); + + // The *server* needn't advertise support for this notification, but the + // *client* must have declared `roots.listChanged` to send it. The SDK + // enforces that itself — `notification()` rejects with "Client does not + // support roots list changed notifications" — so nothing reaches the wire + // on a client built without `roots`, and the server is never invited to + // re-fetch something we'd answer `-32601`. Returning early only avoids + // provoking that rejection and logging it as a *failure*: it isn't one, it + // is a client that was never able to announce (#1797). + if (!this.rootsListChangedCapabilityAdvertised) { + this.logger.warn( + "setRoots() on a client that did not advertise `roots.listChanged`; " + + "roots are stored locally but the change is not announced", + ); + return; } - this.roots = [...roots]; - this.dispatchTypedEvent("rootsChange", this.roots); - - // Send notification to server - clients can send this notification to any server - // The server doesn't need to advertise support for it try { await this.client.notification({ method: "notifications/roots/list_changed", @@ -4310,6 +4737,14 @@ export class InspectorClient extends InspectorClientEventTarget { return filter; } + /** Cancel a pending reconnect re-listen, if any (#1630). */ + private clearModernReconnectTimer(): void { + if (this.modernReconnectTimer !== undefined) { + clearTimeout(this.modernReconnectTimer); + this.modernReconnectTimer = undefined; + } + } + /** * (Re-)establish the modern `subscriptions/listen` stream to match the current * `subscribedResources` set (#1630). Because the stream is not resumable, @@ -4320,14 +4755,6 @@ export class InspectorClient extends InspectorClientEventTarget { * while this one awaits its acknowledgement, the just-opened stream is * discarded rather than overwriting the newer one. */ - /** Cancel a pending reconnect re-listen, if any (#1630). */ - private clearModernReconnectTimer(): void { - if (this.modernReconnectTimer !== undefined) { - clearTimeout(this.modernReconnectTimer); - this.modernReconnectTimer = undefined; - } - } - private async refreshModernSubscription( fromReconnect = false, ): Promise { @@ -4345,7 +4772,7 @@ export class InspectorClient extends InspectorClientEventTarget { const previous = this.modernSubscription; this.modernSubscription = null; if (previous) { - await previous.close().catch(() => {}); + await closeSubscriptionBestEffort(previous); } // Nothing subscribed → keep the stream closed. @@ -4361,7 +4788,7 @@ export class InspectorClient extends InspectorClientEventTarget { // A newer refresh superseded us while awaiting the ack — discard this one. if (generation !== this.modernListenGeneration) { - await subscription.close().catch(() => {}); + await closeSubscriptionBestEffort(subscription); return; } @@ -4377,8 +4804,20 @@ export class InspectorClient extends InspectorClientEventTarget { }); // Observe termination; an unexpected drop reconnects by re-listing. - void subscription.closed.then((reason) => - this.onModernSubscriptionClosed(subscription, reason, generation), + void subscription.closed.then( + (reason) => + this.onModernSubscriptionClosed(subscription, reason, generation), + // The rejection arm exists because a `closed` that rejects carries no + // reason to act on, and an unhandled rejection ends a Node process by + // default. It is scoped to the rejection rather than chained after the + // handler because the handler cannot throw today — it only assigns state, + // dispatches on a native `EventTarget` (which reports listener throws + // rather than propagating them) and arms a timer — and chaining a + // `.catch` after it would silently abandon a re-listen if that ever + // changed. (Closing a stream *resolves* `closed`; what the connect-path + // close newly reaches is the handler running at all, where the reference + // used to be dropped with `closed` pending forever.) + () => {}, ); } @@ -4409,8 +4848,10 @@ export class InspectorClient extends InspectorClientEventTarget { if (!shouldReconnect) { // "stream gone but subscriptions remain" renders the same whether we gave // up after failed reconnects or the server closed it gracefully: keep the - // ended badge while URIs are still subscribed. (On a terminal-status drop - // the disconnect reset clears the set and forces the inactive state.) + // ended badge while URIs are still subscribed. (A `disconnect()` clears + // the set and forces the inactive state; a *crash* leaves both in place + // until the next `connect()` calls `resetSubscriptionStream`, which moves + // them together — so "active with an empty set" is never observable.) this.setModernStreamState({ active: this.subscribedResources.size > 0, status: "ended", @@ -4425,6 +4866,40 @@ export class InspectorClient extends InspectorClientEventTarget { this.scheduleModernReconnect(); } + /** + * Reconcile the stream state after a re-listen *this* call owned and lost + * (#1797). Only correct for a caller that has not been superseded — a newer + * refresh owns the state as well as the filter — so both call sites gate on + * the generation first. + * + * The empty case is the ordinary one: nothing subscribed, no stream, inactive. + * The non-empty one exists because a failed re-listen leaves + * `modernSubscription` null with URIs still subscribed, and nothing else will + * notice: the reconnect machinery is reachable only from a stream that closed + * or a reconnect that failed, and neither happened here. Left alone, the state + * keeps whatever the last success (or the optimistic `"connecting"`) wrote — a + * badge that will never change over subscriptions the server may never have + * honored, recoverable only by an Unsubscribe/Subscribe toggle (a fresh + * Subscribe early-returns on the URI already being in the set). + * + * So it reconnects rather than settling for an honest-but-dead `"ended"`: + * every other route to "stream gone, URIs live" either expects the close or + * has exhausted the retry cap, and this is the one that has made no attempt + * at all. `scheduleModernReconnect` fits as-is — a user-initiated refresh + * already reset `modernReconnectAttempts`, so it starts at the base delay; the + * timer bails on a terminal status or an emptied set; and past the cap + * `onModernReconnectFailed` lands on the same `"ended"` badge. The state + * therefore becomes true or ends after a real attempt. The caller still sees + * its error either way — the retry is about the subscriptions, not the call. + */ + private reconcileModernStreamStateAfterFailedRefresh(): void { + if (this.subscribedResources.size === 0) { + this.setModernStreamState(INACTIVE_SUBSCRIPTION_STREAM_STATE); + return; + } + this.scheduleModernReconnect(); + } + /** * Schedule a reconnect re-listen after the current backoff delay (#1630). * `modernReconnectAttempts` reflects the number of *consecutive failed* @@ -4513,15 +4988,40 @@ export class InspectorClient extends InspectorClientEventTarget { status: "connecting", honoredUris: this.modernStreamState.honoredUris, }); + // Read before the call, to tell "our refresh failed" from "someone else + // took over" in the catch — see there. + const generationBefore = this.modernListenGeneration; try { await this.refreshModernSubscription(); } catch (error) { // Roll back the optimistic add + stream state so both stay consistent - // with the (unchanged) server filter. - this.subscribedResources.delete(uri); - this.dispatchSubscriptionsChange(); - if (this.subscribedResources.size === 0) { - this.setModernStreamState(INACTIVE_SUBSCRIPTION_STREAM_STATE); + // with the server filter — but only while this call still owns that + // filter. A refresh bumps the generation exactly once, synchronously, + // so anything past that bump means something else advanced it, and + // both bumpers make the rollback wrong: + // + // - a newer `refreshModernSubscription` built its filter from the set + // *including* this URI, so if it succeeded the server is honoring + // the subscription; deleting it here would leave the set missing a + // URI the live stream carries, the UI showing it unsubscribed while + // its `resources/updated` keep arriving. + // - `resetSubscriptionStream` (a `disconnect()`, or the start-clean + // reset in `connect()`) already cleared the set and set the state, + // so there is nothing to roll back — and if the caller has since + // re-subscribed on the new session, this stale catch would delete a + // URI that session legitimately holds. + // + // Either way the state is no longer ours to correct. The error is + // still the caller's to see. + if (this.modernListenGeneration === generationBefore + 1) { + // State before the announce, for `resetSubscriptionStream`'s + // reason: on a last-URI rollback, dispatching first would expose an + // empty set with a stream still reading `"connecting"` — the pair + // this file treats as impossible. The reverse intermediate is the + // benign one (and the optimistic add above already exposes it). + this.subscribedResources.delete(uri); + this.reconcileModernStreamStateAfterFailedRefresh(); + this.dispatchSubscriptionsChange(); } throw error; } @@ -4557,8 +5057,31 @@ export class InspectorClient extends InspectorClientEventTarget { if (!this.subscribedResources.delete(uri)) return; // The removal is the user's intent; keep it even if the re-listen fails // (the stale URI simply lingers in the server's honored filter). + // + // Removing the last URI moves the stream to inactive before announcing + // the set, rather than letting the re-listen below do it a round-trip + // later: dispatching first would expose an empty set with an `active` + // stream, which is the pair `resetSubscriptionStream` orders its own + // writes to prevent. The re-listen sets the same state on arrival, and + // sets it on the failure path too (see the catch). + if (this.subscribedResources.size === 0) { + this.setModernStreamState(INACTIVE_SUBSCRIPTION_STREAM_STATE); + } this.dispatchSubscriptionsChange(); - await this.refreshModernSubscription(); + // Same ownership test as `subscribeToResource` — see the long comment + // there. Nothing to undo on this path (the removal is kept + // deliberately), but the *state* still needs reconciling when this call + // owned the re-listen that failed: it leaves no stream behind, and the + // badge would otherwise keep reporting the last success. + const generationBefore = this.modernListenGeneration; + try { + await this.refreshModernSubscription(); + } catch (error) { + if (this.modernListenGeneration === generationBefore + 1) { + this.reconcileModernStreamStateAfterFailedRefresh(); + } + throw error; + } } else { await this.client.unsubscribeResource( { uri }, diff --git a/core/mcp/samplingCreateMessage.ts b/core/mcp/samplingCreateMessage.ts index 4f9539cbb..d4727cf4a 100644 --- a/core/mcp/samplingCreateMessage.ts +++ b/core/mcp/samplingCreateMessage.ts @@ -86,6 +86,42 @@ export class SamplingCreateMessage { this.remove(); } + /** + * Settle a still-pending sample as cancelled, without removing it from the + * queue. Called from `InspectorClient`'s `clearPendingPeerRequests()`, which + * serves every route that drops the queue wholesale — each route out of a + * connection, and the top of `connect()` as a backstop for the one route in + * that settles nothing. The category, not a count: that set has grown. + * + * Unlike an elicitation there is no internal awaiter to unblock, but on the + * plain server-request shape the *server* is one: we accepted its + * `sampling/createMessage`, so dropping the request without settling means no + * response frame is ever written and it waits forever. That is reachable when + * the transport outlives the failed attempt — `connect()` keeps it when an + * auth provider holds it open — so settle rather than discard. Rejecting is + * the right settle: it is what the UI's decline path sends, and the SDK turns + * it into a JSON-RPC error response. No-op once already resolved. + * + * A *task-augmented* sample sits in the same queue but already answered the + * wire request synchronously with a `CreateTaskResult`, so nothing is waiting + * on a frame; settling there is about not leaving the task in + * `input_required` limbo. Its reject reaches the receiver task's payload + * promise, which `InspectorClient` marks handled at creation for exactly this + * reason. + * + * Deliberately does not call `onRemove`, for the same reason as + * `ElicitationCreateMessage.cancel()`: the caller iterates the queue and + * clears it itself, so removing here would splice mid-iteration — skipping + * every other entry and leaving those requests unanswered. + */ + cancel(): void { + if (this.rejectPromise) { + this.rejectPromise(new Error("Connection torn down")); + } + this.resolvePromise = undefined; + this.rejectPromise = undefined; + } + /** * Remove this pending sample from the list */ diff --git a/core/mcp/serverList.ts b/core/mcp/serverList.ts index d46a31e67..56aaae5e6 100644 --- a/core/mcp/serverList.ts +++ b/core/mcp/serverList.ts @@ -88,19 +88,48 @@ export function normalizeServerType( } /** - * Normalize the form's controlled root rows into the shape the Inspector - * advertises and persists: drop rows whose `uri` is blank (the form leaves a - * new row empty mid-edit) and drop a blank/whitespace `name`. Any other fields - * a root carries (e.g. `_meta` from a hand-edited `mcp.json`) are preserved — - * only `uri`/`name` are normalized. Shared by the settings → disk converter - * (`inspectorSettingsToStoredFields`) and the web client's connect-time + - * `setRoots` wiring so the roots told to the server match what hits disk. + * The shared roots normalizer — for a list from the web settings form, from + * `mcp.json`, or from `setRoots()`. Puts them in the shape the Inspector + * advertises and persists: drop entries whose `uri` is blank (the settings form + * leaves a new row empty mid-edit) and drop a blank/whitespace `name`. Any other + * fields a root carries (e.g. `_meta` from a hand-edited `mcp.json`) are + * preserved — only `uri`/`name` are normalized. Every path roots take runs + * through here — the settings → disk converter + * (`inspectorSettingsToStoredFields`), the `InspectorClient` constructor and + * `setRoots`, and all three clients' connect-time wiring — so what the server is + * told matches what hits disk. + * + * `Root[]` is a compile-time type over hand-editable `mcp.json`, and every + * client now feeds this straight from disk (#1797), so the shape is validated + * at runtime too, rather than throwing at connect: a non-array bails to `[]`, + * an entry without a string `uri` is dropped, and a non-string `name` is + * dropped from an otherwise-usable entry. Each case warns. */ export function cleanRoots(roots: Root[]): Root[] { + // Keep: the `Root[]` parameter narrows this branch to `never`, but the type is + // a promise hand-edited `mcp.json` does not keep. Not dead code (#1797). + if (!Array.isArray(roots)) { + console.warn("Ignoring `roots`: expected an array, got", typeof roots); + return []; + } return roots - .filter((r) => r.uri.trim() !== "") + .filter((r) => { + // Keep: unreachable per the parameter type, reachable from disk. + if (typeof r?.uri !== "string") { + console.warn("Dropping root without a string `uri`:", r); + return false; + } + return r.uri.trim() !== ""; + }) .map((r) => { - const trimmedName = r.name?.trim(); + // `?.` would guard null/undefined but not a non-string `name` from disk, + // which `.trim()` throws on — the `uri` case above, one field over. + const rawName = r.name; + if (rawName !== undefined && typeof rawName !== "string") { + console.warn("Dropping non-string `name` on root:", r); + } + const trimmedName = + typeof rawName === "string" ? rawName.trim() : undefined; // Strip `name` off the carried-through rest so a cleared optional name // doesn't persist as `name: ""`; re-add it only when non-empty. const { name: _name, ...rest } = r; diff --git a/core/mcp/types.ts b/core/mcp/types.ts index d190dd15d..b95c65a73 100644 --- a/core/mcp/types.ts +++ b/core/mcp/types.ts @@ -567,6 +567,24 @@ export type ModernLogLevel = LoggingLevel | "off"; */ export const DEFAULT_MODERN_LOG_LEVEL: ModernLogLevel = "debug"; +/** + * The live modern per-request log level a server's settings imply: the + * configured value, or {@link DEFAULT_MODERN_LOG_LEVEL} when unset, with + * `"off"` meaning not opted in. + * + * One derivation, because the client stamps `_meta` from it while the web Logs + * control displays it — computing it separately on each side is how the two + * come to disagree, which is a bad failure for a tool whose job is showing what + * it sent (#1629, #1797). The web maps `undefined` to `null` at its own + * boundary; that is display, not a second derivation. + */ +export function resolveModernLogLevel( + settings?: Pick, +): LoggingLevel | undefined { + const level = settings?.modernLogLevel ?? DEFAULT_MODERN_LOG_LEVEL; + return level === "off" ? undefined : level; +} + /** All modern-log-level values, for form options and the runtime guard. */ export const MODERN_LOG_LEVELS: ModernLogLevel[] = [ "off",