diff --git a/clients/web/src/test/core/react/useInspectorClient.test.tsx b/clients/web/src/test/core/react/useInspectorClient.test.tsx index 56287702..f24e3692 100644 --- a/clients/web/src/test/core/react/useInspectorClient.test.tsx +++ b/clients/web/src/test/core/react/useInspectorClient.test.tsx @@ -144,6 +144,76 @@ describe("useInspectorClient", () => { expect(result.current.excludedTools).toEqual([]); }); + it("subscribes to malformedListItemsChange and updates (#1909)", () => { + const client = new FakeInspectorClient(); + const { result } = renderHook(() => useInspectorClient(client)); + expect(result.current.malformedListItems).toEqual([]); + const malformed = [ + { + method: "resources/templates/list", + index: 1, + label: "array_annotations", + reason: "annotations: Invalid input (expected object)", + }, + ]; + act(() => { + client.setMalformedListItems(malformed); + }); + expect(result.current.malformedListItems).toEqual(malformed); + }); + + it("seeds malformedListItems from the client it is given (#1909)", () => { + // A client that already salvaged before this hook mounted (a screen opened + // after the connect-time list load) must show the report immediately. + const client = new FakeInspectorClient({ status: "connected" }); + const malformed = [ + { method: "tools/list", index: 0, reason: "name: expected string" }, + ]; + client.setMalformedListItems(malformed); + const { result } = renderHook(() => useInspectorClient(client)); + expect(result.current.malformedListItems).toEqual(malformed); + }); + + it("resets malformedListItems to [] when client becomes null (#1909)", () => { + const client = new FakeInspectorClient({ status: "connected" }); + client.setMalformedListItems([ + { method: "tools/list", index: 0, reason: "name: expected string" }, + ]); + const { result, rerender } = renderHook(({ c }) => useInspectorClient(c), { + initialProps: { c: client as FakeInspectorClient | null }, + }); + expect(result.current.malformedListItems.length).toBe(1); + rerender({ c: null }); + expect(result.current.malformedListItems).toEqual([]); + }); + + it("re-seeds malformedListItems on a client swap, not the old server's (#1909)", () => { + const first = new FakeInspectorClient({ status: "connected" }); + first.setMalformedListItems([ + { method: "tools/list", index: 0, reason: "name: expected string" }, + ]); + const second = new FakeInspectorClient({ status: "connected" }); + const { result, rerender } = renderHook(({ c }) => useInspectorClient(c), { + initialProps: { c: first as FakeInspectorClient }, + }); + expect(result.current.malformedListItems).toHaveLength(1); + rerender({ c: second }); + expect(result.current.malformedListItems).toEqual([]); + }); + + it("stops listening for malformedListItemsChange after unmount (#1909)", () => { + const client = new FakeInspectorClient({ status: "connected" }); + const { result, unmount } = renderHook(() => useInspectorClient(client)); + unmount(); + act(() => { + client.setMalformedListItems([ + { method: "tools/list", index: 0, reason: "name: expected string" }, + ]); + }); + // A listener left attached would update state on an unmounted tree. + expect(result.current.malformedListItems).toEqual([]); + }); + it("resets protocolEra / discoverResult to defaults when client becomes null", () => { const client = new FakeInspectorClient({ status: "connected", diff --git a/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts index d864e9b1..db04bb13 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts @@ -8,6 +8,7 @@ import { import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; import { MessageLogState } from "@inspector/core/mcp/state/index.js"; import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; +import { eraToVersionNegotiation } from "@inspector/core/mcp/types.js"; /** * Per-item list salvage against a real connection (#1909). @@ -23,6 +24,8 @@ import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; * real. */ +const MODERN_VERSION = "2026-07-28"; + interface ListPage { items: unknown[]; nextCursor?: string; @@ -37,10 +40,14 @@ function jsonRpcResult(id: unknown, result: unknown) { * methods it was configured with, returning entries verbatim (malformed ones * included). */ -function startMalformedServer(initialPages: { - resourceTemplates?: ListPage[]; - tools?: ListPage[]; -}): Promise<{ +function startMalformedServer( + initialPages: { + resourceTemplates?: ListPage[]; + tools?: ListPage[]; + }, + /** Negotiate the modern era, which is what turns the SEP-2243 gate on. */ + modern = false, +): Promise<{ url: string; stop: () => Promise; calls: string[]; @@ -82,14 +89,41 @@ function startMalformedServer(initialPages: { return; } - const send = (result: unknown) => { + const send = (result: Record) => { res.writeHead(200, { "Content-Type": "application/json" }); - res.end(JSON.stringify(jsonRpcResult(body.id, result))); + // The 2026-07-28 codec requires `resultType` on every result; the legacy + // leg ignores it. + res.end( + JSON.stringify( + jsonRpcResult( + body.id, + modern + ? { + resultType: "complete", + // The modern leg also requires the cache hints on a list + // result; `ttlMs: 0` means "don't cache", which keeps each + // call in these tests a real round trip. + ttlMs: 0, + cacheScope: "public", + ...result, + } + : result, + ), + ), + ); }; if (body.method === "initialize") { send({ - protocolVersion: "2025-06-18", + protocolVersion: modern ? MODERN_VERSION : "2025-06-18", + capabilities: { resources: {}, tools: {} }, + serverInfo: { name: "malformed-list-server", version: "1.0.0" }, + }); + return; + } + if (body.method === "server/discover") { + send({ + supportedVersions: [MODERN_VERSION], capabilities: { resources: {}, tools: {} }, serverInfo: { name: "malformed-list-server", version: "1.0.0" }, }); @@ -159,10 +193,13 @@ describe("InspectorClient list salvage (#1909)", () => { let client: InspectorClient | null = null; let stopServer: (() => Promise) | null = null; - async function connectTo(url: string) { + async function connectTo(url: string, era?: "legacy" | "modern") { client = new InspectorClient( { type: "streamable-http", url }, - { environment: { transport: createTransportNode } }, + { + environment: { transport: createTransportNode }, + ...(era ? { versionNegotiation: eraToVersionNegotiation(era) } : {}), + }, ); await client.connect(); return client; @@ -247,6 +284,37 @@ describe("InspectorClient list salvage (#1909)", () => { expect(seen[0]).toMatchObject([{ label: "array_annotations" }]); }); + it("marks the ORIGINAL rejected response, not a salvage page", async () => { + // The re-walk answers the same method again, so a mark taken afterwards + // would land on a page that succeeded and leave the invalid response + // rendering clean — the exact lie #1953 exists to remove. + const server = await startMalformedServer({ + resourceTemplates: [{ items: [PHP_EMPTY_ANNOTATIONS, VALID_TEMPLATE] }], + }); + stopServer = server.stop; + const connected = await connectTo(server.url); + const log = new MessageLogState(connected); + + await connected.listAllResourceTemplates(); + + const templateCalls = log + .getMessages() + .filter( + (entry) => + "method" in entry.message && + entry.message.method === "resources/templates/list", + ); + // Two exchanges: the strict aggregate that was rejected, then the re-walk. + expect(templateCalls.length).toBeGreaterThanOrEqual(2); + expect(templateCalls[0]?.clientError).toContain( + "Dropped 1 malformed entry", + ); + for (const later of templateCalls.slice(1)) { + expect(later.clientError).toBeUndefined(); + } + log.destroy(); + }); + it("marks the Protocol entry rejected even though the list rendered", async () => { const server = await startMalformedServer({ resourceTemplates: [{ items: [PHP_EMPTY_ANNOTATIONS, VALID_TEMPLATE] }], @@ -302,6 +370,50 @@ describe("InspectorClient list salvage (#1909)", () => { expect(connected.getMalformedListItems()).toEqual([]); }); + it("does not readmit a SEP-2243-excluded tool while salvaging", async () => { + // The strict aggregate is filtered by the SDK, so the salvage path has to + // reapply the rule: otherwise one ordinary malformed tool anywhere in the + // list quietly readmits every invalid-header tool beside it, turning a + // rendering fix into a spec violation (#1632 + #1909). + const server = await startMalformedServer( + { + tools: [ + { + items: [ + { name: "ok_tool", inputSchema: { type: "object" } }, + // Malformed: trips the salvage fallback. + { name: "broken", inputSchema: "not-a-schema" }, + // Schema-valid, but its x-mcp-header is not an RFC 9110 token. + { + name: "invalid_header_tool", + inputSchema: { + type: "object", + properties: { + value: { + type: "string", + "x-mcp-header": "Bad Header", + }, + }, + }, + }, + ], + }, + ], + }, + true, + ); + stopServer = server.stop; + const connected = await connectTo(server.url, "modern"); + expect(connected.getProtocolEra()).toBe("modern"); + + const { tools } = await connected.listAllTools(); + expect(tools.map((t) => t.name)).toEqual(["ok_tool"]); + // Still reported as excluded rather than silently gone. + expect(connected.getExcludedTools().map((x) => x.tool.name)).toEqual([ + "invalid_header_tool", + ]); + }); + it("rethrows when the rejection is not about any single entry", async () => { // Every entry validates; the result is bad for another reason (a cursor of // the wrong type). There is no per-item story, so the original error must diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index e37994d5..785c0c35 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -1616,6 +1616,15 @@ export class InspectorClient extends InspectorClientEventTarget { // (#1953). this.outboundRequestMethods.clear(); this.lastAnsweredRequestByMethod.clear(); + // Per-session for the same reason: a report names entries of the PREVIOUS + // server's list. Cleared here as well as in `disconnect()` because the + // route out that tears down nothing (`onerror` with no `onclose`) would + // otherwise carry it into the next connection — indefinitely, if that + // server never lists the method again (#1909). + if (this.malformedListItems.size > 0) { + this.malformedListItems.clear(); + this.dispatchTypedEvent("malformedListItemsChange", []); + } for (const [, controller] of this.taskInputAbortControllers) { controller.abort(new Error("Connection ended")); } @@ -2372,8 +2381,6 @@ export class InspectorClient extends InspectorClientEventTarget { private withModernTaskEnvelope( params: Record, ): Record { - const existingMeta = - (params._meta as Record | undefined) ?? {}; const clientCapabilities = { ...this.clientCapabilities, // Force-stamp the tasks extension regardless of what the client @@ -2385,11 +2392,31 @@ export class InspectorClient extends InspectorClientEventTarget { [TASKS_EXTENSION_KEY]: {}, }, }; - // Use the NEGOTIATED protocol version so the envelope agrees with the - // `MCP-Protocol-Version` header the transport stamps from the same source — - // a future modern-family revision would negotiate a different string, and - // the two must not disagree. The raw channel only runs on a connected modern - // session, so this is always set; the constant is a defensive fallback. + return this.withModernEnvelope(params, clientCapabilities); + } + + /** + * Stamp the `_meta` envelope every raw-wire request needs on the modern leg: + * the negotiated protocol version, the client identity, and the client + * capabilities. + * + * Split out of {@link withModernTaskEnvelope} so the list-salvage re-walk + * (#1909) can reuse it WITHOUT that method's force-stamped tasks extension — + * a `tools/list` has no business claiming an extension the user may have + * deliberately turned off (#1738). + * + * The NEGOTIATED protocol version is used so the envelope agrees with the + * `MCP-Protocol-Version` header the transport stamps from the same source — a + * future modern-family revision would negotiate a different string, and the + * two must not disagree. The raw channel only runs on a connected modern + * session, so this is always set; the constant is a defensive fallback. + */ + private withModernEnvelope( + params: Record, + clientCapabilities: ClientCapabilities = this.clientCapabilities, + ): Record { + const existingMeta = + (params._meta as Record | undefined) ?? {}; /* v8 ignore start -- fallback only if getProtocolVersion() is unset, which can't happen on the connected modern session this runs on. Bracketed so the ignore is reflow-proof however prettier splits the statement. */ @@ -3154,6 +3181,7 @@ export class InspectorClient extends InspectorClientEventTarget { itemsKey: "tools", itemSchema: ToolSchema, metadata: options?.metadata, + finalize: (salvaged) => this.excludeInvalidXMcpHeaderTools(salvaged), aggregate: async () => [ ...( await this.client!.listTools( @@ -3261,12 +3289,19 @@ export class InspectorClient extends InspectorClientEventTarget { itemSchema, aggregate, metadata, + finalize, }: { method: string; itemsKey: string; itemSchema: z.ZodType; aggregate: () => Promise; metadata?: Record; + /** + * Filter applied to SALVAGED entries only, to reproduce a filter the SDK's + * strict aggregate applies for us. Without it the fallback would return a + * longer list than the strict path — see the `tools/list` caller. + */ + finalize?: (items: T[]) => T[]; }): Promise { try { const items = await this.invokeMcpClient(aggregate, { method }); @@ -3274,10 +3309,35 @@ export class InspectorClient extends InspectorClientEventTarget { return items; } catch (err) { if (!isClientDecodeRejection(err)) throw err; - return this.salvageList({ method, itemsKey, itemSchema, metadata, err }); + const salvaged = await this.salvageList({ + method, + itemsKey, + itemSchema, + metadata, + err, + }); + return finalize ? finalize(salvaged) : salvaged; } } + /** + * Drop the tools the SDK would have excluded from `tools/list` for invalid + * `x-mcp-header` annotations (SEP-2243). + * + * The strict aggregate is filtered by the SDK before we see it, so the + * salvage path has to reapply the same rule itself: otherwise one ordinary + * malformed tool anywhere in the list would trip the fallback and quietly + * readmit every invalid-header tool alongside it — turning a rendering fix + * into a spec violation. The dropped tools are still reported through + * `refreshExcludedTools`, which lists them with their reason. + */ + private excludeInvalidXMcpHeaderTools(tools: Tool[]): Tool[] { + if (!this.excludesInvalidXMcpHeaderTools()) return tools; + return tools.filter( + (tool) => scanXMcpHeaderDeclarations(tool.inputSchema).valid, + ); + } + /** * Re-walk a list method with pagination validated but entries left raw, then * validate each entry on its own — keeping the good ones and reporting the @@ -3310,6 +3370,13 @@ export class InspectorClient extends InspectorClientEventTarget { metadata?: Record; err: unknown; }): Promise { + // Capture the correlation id BEFORE re-walking. `markResponseRejected` + // resolves "the last response received for this method", and the re-walk + // below answers that same method one or more times — so marking afterwards + // would stamp the rejection onto a salvage page that succeeded and leave + // the actually-invalid response rendering as a clean success, which is the + // precise lie #1953 exists to remove. + const rejectedResponseId = this.lastAnsweredRequestByMethod.get(method); const valid: T[] = []; const malformed: MalformedListItem[] = []; const seenCursors = new Set(); @@ -3320,13 +3387,27 @@ export class InspectorClient extends InspectorClientEventTarget { ...(effectiveMeta ? { _meta: effectiveMeta } : {}), ...(cursor ? { cursor } : {}), }; + // The modern (2026-07-28) codec validates a result against the SPEC + // schema for the method before the caller's schema is consulted, so a + // lenient `client.request` is rejected exactly as the strict one was — + // there is no salvage without going around it. The raw-wire channel that + // exists for the modern `tasks/*` methods does precisely that: the frame + // goes straight through the transport (still logged for the Protocol / + // Network tabs) and only the caller's schema is applied. Legacy keeps the + // ordinary SDK path, which honors request options and `_meta` for us. const page = await this.invokeMcpClient( () => - this.client!.request( - { method, params }, - LenientListPageSchema, - this.getRequestOptions(metadata?.progressToken), - ), + this.isModernEra() + ? this.rawWireRequest( + method, + this.withModernEnvelope(params), + LenientListPageSchema, + ) + : this.client!.request( + { method, params }, + LenientListPageSchema, + this.getRequestOptions(metadata?.progressToken), + ), { method }, ); const salvaged = salvageListItems({ @@ -3349,7 +3430,12 @@ export class InspectorClient extends InspectorClientEventTarget { // this fallback cannot explain — surface it rather than hide it. if (malformed.length === 0) throw err; - this.markResponseRejected(method, summarizeMalformed(malformed)); + if (rejectedResponseId !== undefined) { + this.dispatchTypedEvent("responseRejected", { + id: rejectedResponseId, + reason: summarizeMalformed(malformed), + }); + } this.setMalformedListItems(method, malformed); this.logger.warn( { method, malformed }, @@ -3382,7 +3468,7 @@ export class InspectorClient extends InspectorClientEventTarget { const seenCursors = new Set(); let cursor: string | undefined; do { - const page = await this.listTools(cursor, metadata); + const page = await this.listToolsForScan(cursor, metadata); for (const tool of page.tools) { const scan = scanXMcpHeaderDeclarations(tool.inputSchema); if (!scan.valid) excluded.push({ tool, reason: scan.reason }); @@ -3400,6 +3486,54 @@ export class InspectorClient extends InspectorClientEventTarget { return excluded; } + /** + * One page of the RAW `tools/list` for the SEP-2243 scan, tolerating a + * malformed sibling entry (#1632 + #1909). + * + * The scan needs the unfiltered list, so it uses the strict single-page call + * — but that call rejects the whole page when any entry is non-conforming, + * which would blank the excluded set and leave a tool that vanished for an + * invalid header with no explanation at all. That is the same failure #1909 + * is about, one level down. So on a decode rejection the page is re-fetched + * leniently and scanned entry by entry; the malformed ones are not "excluded + * tools" and are reported through the salvage channel instead. + */ + private async listToolsForScan( + cursor: string | undefined, + metadata: Record | undefined, + ): Promise<{ tools: Tool[]; nextCursor?: string }> { + try { + return await this.listTools(cursor, metadata); + } catch (err) { + if (!isClientDecodeRejection(err)) throw err; + const effectiveMeta = this.mergeMeta(metadata); + const params = { + ...(effectiveMeta ? { _meta: effectiveMeta } : {}), + ...(cursor ? { cursor } : {}), + }; + const page = this.isModernEra() + ? await this.rawWireRequest( + "tools/list", + this.withModernEnvelope(params), + LenientListPageSchema, + ) + : await this.client!.request( + { method: "tools/list", params }, + LenientListPageSchema, + this.getRequestOptions(metadata?.progressToken), + ); + const { valid } = salvageListItems({ + method: "tools/list", + items: rawItemsOf(page, "tools"), + schema: ToolSchema, + }); + return { + tools: valid, + ...(page.nextCursor !== undefined && { nextCursor: page.nextCursor }), + }; + } + } + /** * Call a tool. Caller must provide the Tool (e.g. from a state manager). * @param tool The tool to call (use tool.name for the request)