diff --git a/clients/web/src/test/core/mcp/listSalvage.test.ts b/clients/web/src/test/core/mcp/listSalvage.test.ts index 08445051..f0fd7fdc 100644 --- a/clients/web/src/test/core/mcp/listSalvage.test.ts +++ b/clients/web/src/test/core/mcp/listSalvage.test.ts @@ -66,14 +66,16 @@ describe("LenientListPageSchema", () => { ).toBe(false); }); - it("reads an absent or non-array items key as no entries", () => { - expect(rawItemsOf({}, "tools")).toEqual([]); + it("distinguishes an empty page from one that is not a list at all", () => { + // Reading either as "no entries" would let a top-level violation pass as a + // silently truncated list. expect( - rawItemsOf( - LenientListPageSchema.parse({ tools: "not-a-list" }) as never, - "tools", - ), + rawItemsOf(LenientListPageSchema.parse({ tools: [] }), "tools"), ).toEqual([]); + expect(rawItemsOf({}, "tools")).toBeUndefined(); + expect( + rawItemsOf(LenientListPageSchema.parse({ tools: "not-a-list" }), "tools"), + ).toBeUndefined(); }); }); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-excluded-tools.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-excluded-tools.test.ts index fa3f9843..8f3bbf88 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-excluded-tools.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-excluded-tools.test.ts @@ -120,6 +120,28 @@ describe("excluded tools (SEP-2243 x-mcp-header)", () => { expect(excluded.map((x) => x.tool.name)).toEqual(["invalid_header_tool"]); }); + it("does not carry an excluded set into the next connection (#1909)", async () => { + // `resetSessionState()` is the start-clean path, and it is the ONLY one the + // route out that tears down nothing (`onerror` with no `onclose`) reaches. + // A set left behind names tools of the PREVIOUS server — and would persist + // indefinitely against a server that never lists tools again. + const started = await start({}); + const connected = await connect(started.url, "modern"); + await connected.listAllTools(); + expect(connected.getExcludedTools()).toHaveLength(1); + + const events: number[] = []; + connected.addEventListener("excludedToolsChange", (e) => { + events.push(e.detail.length); + }); + // Reconnecting runs the start-clean reset; assert before anything re-lists. + await connected.disconnect(); + await connected.connect(); + + expect(connected.getExcludedTools()).toEqual([]); + expect(events).toContain(0); + }); + it("reports no excluded tools on a legacy connection (no exclusion there)", async () => { const started = await start(undefined); const connected = await connect(started.url, "legacy"); 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 db04bb13..039dcbc0 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 @@ -51,6 +51,8 @@ function startMalformedServer( url: string; stop: () => Promise; calls: string[]; + /** Every `cursor` param the server was sent, in order. */ + cursors: (string | undefined)[]; /** Swap what the server serves next, to model a server that got fixed. */ setPages: (next: { resourceTemplates?: ListPage[]; @@ -60,10 +62,21 @@ function startMalformedServer( const calls: string[] = []; let pages = initialPages; + const cursors: (string | undefined)[] = []; + // Cursor string -> the page index it addresses, learned as pages are served. + // Modeling it this way (rather than parsing the cursor as a number) is what + // makes an EMPTY-STRING cursor meaningful: a client that drops it sends no + // cursor at all, lands back on page one, and duplicates its entries. + const cursorToIndex = new Map(); const pageFor = (key: "resourceTemplates" | "tools", cursor?: string) => { + cursors.push(cursor); const configured = pages[key] ?? []; - const index = cursor === undefined ? 0 : Number(cursor); - return configured[index] ?? { items: [] }; + const index = cursor === undefined ? 0 : (cursorToIndex.get(cursor) ?? 0); + const page = configured[index] ?? { items: [] }; + if (page.nextCursor !== undefined) { + cursorToIndex.set(page.nextCursor, index + 1); + } + return page; }; const handler = async (req: IncomingMessage, res: ServerResponse) => { @@ -160,6 +173,7 @@ function startMalformedServer( resolve({ url: `http://127.0.0.1:${port}/mcp`, calls, + cursors, setPages: (next) => { pages = next; }, @@ -414,6 +428,62 @@ describe("InspectorClient list salvage (#1909)", () => { ]); }); + it("surfaces the ORIGINAL error when the re-walk itself fails", async () => { + // A malformed `nextCursor` fails the lenient page schema too, so the walk + // throws its own error. The original is what the caller's list actually + // failed on — and what `rejectedResponseId` was captured for — so that is + // the one that must surface. + const server = await startMalformedServer({ + resourceTemplates: [ + { + items: [PHP_EMPTY_ANNOTATIONS, VALID_TEMPLATE], + nextCursor: 42 as unknown as string, + }, + ], + }); + stopServer = server.stop; + const connected = await connectTo(server.url); + + await expect(connected.listAllResourceTemplates()).rejects.toThrow( + /annotations/, + ); + expect(connected.getMalformedListItems()).toEqual([]); + }); + + it("keeps the strict error when a page is not a list at all", async () => { + // `{ resourceTemplates: "nope" }` is a top-level violation the per-item + // pass cannot explain; treating it as an empty page would return a + // silently truncated list and discard the error that was right about it. + const server = await startMalformedServer({ + resourceTemplates: [{ items: "nope" as unknown as unknown[] }], + }); + stopServer = server.stop; + const connected = await connectTo(server.url); + + await expect(connected.listAllResourceTemplates()).rejects.toThrow(); + expect(connected.getMalformedListItems()).toEqual([]); + }); + + it("sends an empty-string cursor rather than re-fetching page one", async () => { + // "" is a valid cursor. A truthiness check would drop it, re-request page + // one, and duplicate its entries until the repeated-cursor guard fired. + const server = await startMalformedServer({ + resourceTemplates: [ + { items: [PHP_EMPTY_ANNOTATIONS, VALID_TEMPLATE], nextCursor: "" }, + { items: [{ ...VALID_TEMPLATE, name: "page_two" }] }, + ], + }); + stopServer = server.stop; + const connected = await connectTo(server.url); + + const { resourceTemplates } = await connected.listAllResourceTemplates(); + expect(resourceTemplates.map((t) => t.name)).toEqual([ + "full_annotations", + "page_two", + ]); + expect(server.cursors).toContain(""); + }); + 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 785c0c35..ac6d5f00 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -1616,15 +1616,23 @@ 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 + // Per-session for the same reason: both name 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). + // otherwise carry them into the next connection — indefinitely, if that + // server never lists the method again. Safe to clear unconditionally at + // connect: `listAllTools` recomputes the excluded set on every aggregate + // and the salvage report on every list, so this can only drop a stale set, + // never a live one (#1909; the `excludedTools` half is the same latent bug + // from #1632, fixed here rather than left as a known sibling). if (this.malformedListItems.size > 0) { this.malformedListItems.clear(); this.dispatchTypedEvent("malformedListItemsChange", []); } + if (this.excludedTools.length > 0) { + this.excludedTools = []; + this.dispatchTypedEvent("excludedToolsChange", []); + } for (const [, controller] of this.taskInputAbortControllers) { controller.abort(new Error("Connection ended")); } @@ -3381,50 +3389,75 @@ export class InspectorClient extends InspectorClientEventTarget { const malformed: MalformedListItem[] = []; const seenCursors = new Set(); let cursor: string | undefined; - do { - const effectiveMeta = this.mergeMeta(metadata); - const params = { - ...(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.isModernEra() - ? this.rawWireRequest( - method, - this.withModernEnvelope(params), - LenientListPageSchema, - ) - : this.client!.request( - { method, params }, - LenientListPageSchema, - this.getRequestOptions(metadata?.progressToken), - ), - { method }, - ); - const salvaged = salvageListItems({ - method, - items: rawItemsOf(page, itemsKey), - schema: itemSchema, - startIndex: valid.length + malformed.length, - }); - valid.push(...salvaged.valid); - malformed.push(...salvaged.malformed); - cursor = page.nextCursor; - if (cursor !== undefined) { - /* v8 ignore next -- defensive: a spec-compliant server never repeats a cursor; this guards a non-converging server from an infinite walk (mirrors the SDK's drainList guard) */ - if (seenCursors.has(cursor)) break; - seenCursors.add(cursor); + try { + do { + const effectiveMeta = this.mergeMeta(metadata); + const params = { + ...(effectiveMeta ? { _meta: effectiveMeta } : {}), + // `!== undefined`, not truthiness: "" is a valid cursor, and dropping it + // would re-request page one and duplicate its entries until the + // repeated-cursor guard below stopped the walk. + ...(cursor !== undefined && { 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.isModernEra() + ? this.rawWireRequest( + method, + this.withModernEnvelope(params), + LenientListPageSchema, + ) + : this.client!.request( + { method, params }, + LenientListPageSchema, + this.getRequestOptions(metadata?.progressToken), + ), + { method }, + ); + const items = rawItemsOf(page, itemsKey); + // The page's list member is missing or isn't a list: a top-level + // violation, not a per-item one. Reading it as "no entries" would return + // a silently truncated list and discard the strict error that was right + // about it. + if (items === undefined) throw err; + const salvaged = salvageListItems({ + method, + items, + schema: itemSchema, + startIndex: valid.length + malformed.length, + }); + valid.push(...salvaged.valid); + malformed.push(...salvaged.malformed); + cursor = page.nextCursor; + if (cursor !== undefined) { + /* v8 ignore next -- defensive: a spec-compliant server never repeats a cursor; this guards a non-converging server from an infinite walk (mirrors the SDK's drainList guard) */ + if (seenCursors.has(cursor)) break; + seenCursors.add(cursor); + } + } while (cursor !== undefined); + } catch (walkErr) { + // The re-walk itself failed — its own decode rejection (a malformed + // `nextCursor`, an unsalvageable page), or the connection going away + // mid-walk. Either way the ORIGINAL error is the one to surface: it is + // what the caller's list actually failed on, and `rejectedResponseId` + // above was captured for that response, not this one. The secondary + // failure is logged so it stays diagnosable rather than vanishing. + if (walkErr !== err) { + this.logger.warn( + { err: walkErr, method }, + "Salvage re-walk failed; surfacing the original list error", + ); } - } while (cursor !== undefined); + throw err; + } // Nothing per-item was wrong, so the strict rejection was about something // this fallback cannot explain — surface it rather than hide it. @@ -3462,6 +3495,7 @@ export class InspectorClient extends InspectorClientEventTarget { metadata?: Record, ): Promise { const excluded: ExcludedTool[] = []; + const malformed: MalformedListItem[] = []; // Gated to connections that actually exclude; otherwise this is a pure // no-op (no round trip). The raw `listTools` below guards the connection. if (this.excludesInvalidXMcpHeaderTools()) { @@ -3469,6 +3503,13 @@ export class InspectorClient extends InspectorClientEventTarget { let cursor: string | undefined; do { const page = await this.listToolsForScan(cursor, metadata); + malformed.push( + ...page.malformed.map((entry) => ({ + ...entry, + // Index against the aggregate this walk is building, not the page. + index: entry.index + excluded.length + page.tools.length, + })), + ); for (const tool of page.tools) { const scan = scanXMcpHeaderDeclarations(tool.inputSchema); if (!scan.valid) excluded.push({ tool, reason: scan.reason }); @@ -3481,6 +3522,15 @@ export class InspectorClient extends InspectorClientEventTarget { } } while (cursor !== undefined); } + // Report what the scan itself had to drop. This walk is deliberately + // un-cached, so it can see a malformed tool the `listAllTools` aggregate + // never did — that aggregate may have been served from the response cache, + // in which case its salvage never ran and cleared the report instead. Only + // ever ADDS: a scan that finds nothing must not clear a report the salvage + // path just wrote from its own walk of the same list. + if (malformed.length > 0) { + this.setMalformedListItems("tools/list", malformed); + } this.excludedTools = excluded; this.dispatchTypedEvent("excludedToolsChange", excluded); return excluded; @@ -3501,15 +3551,21 @@ export class InspectorClient extends InspectorClientEventTarget { private async listToolsForScan( cursor: string | undefined, metadata: Record | undefined, - ): Promise<{ tools: Tool[]; nextCursor?: string }> { + ): Promise<{ + tools: Tool[]; + nextCursor?: string; + malformed: MalformedListItem[]; + }> { try { - return await this.listTools(cursor, metadata); + const page = await this.listTools(cursor, metadata); + return { ...page, malformed: [] }; } catch (err) { if (!isClientDecodeRejection(err)) throw err; const effectiveMeta = this.mergeMeta(metadata); const params = { ...(effectiveMeta ? { _meta: effectiveMeta } : {}), - ...(cursor ? { cursor } : {}), + // See `salvageList`: "" is a valid cursor. + ...(cursor !== undefined && { cursor }), }; const page = this.isModernEra() ? await this.rawWireRequest( @@ -3522,14 +3578,19 @@ export class InspectorClient extends InspectorClientEventTarget { LenientListPageSchema, this.getRequestOptions(metadata?.progressToken), ); - const { valid } = salvageListItems({ + const items = rawItemsOf(page, "tools"); + // Not a list at all — a top-level violation the per-item pass can't + // explain, so the strict error stands (mirrors `salvageList`). + if (items === undefined) throw err; + const { valid, malformed } = salvageListItems({ method: "tools/list", - items: rawItemsOf(page, "tools"), + items, schema: ToolSchema, }); return { tools: valid, ...(page.nextCursor !== undefined && { nextCursor: page.nextCursor }), + malformed, }; } } diff --git a/core/mcp/listSalvage.ts b/core/mcp/listSalvage.ts index a144c2bd..5d12f5da 100644 --- a/core/mcp/listSalvage.ts +++ b/core/mcp/listSalvage.ts @@ -93,10 +93,22 @@ export const LenientListPageSchema = z.looseObject({ export type LenientListPage = z.infer; -/** Read the primitive array out of a leniently-parsed page. */ -export function rawItemsOf(page: LenientListPage, itemsKey: string): unknown[] { +/** + * Read the primitive array out of a leniently-parsed page. + * + * Returns `undefined` — NOT an empty array — when the member is missing or is + * not an array, because those are different facts. An empty page is a normal + * server answer; a page whose `tools` is the string `"invalid"` is a top-level + * schema violation this fallback cannot explain, and reading it as "no entries" + * would let the caller return a silently truncated list while the strict error + * that was correct about it is discarded. + */ +export function rawItemsOf( + page: LenientListPage, + itemsKey: string, +): unknown[] | undefined { const value = (page as Record)[itemsKey]; - return Array.isArray(value) ? value : []; + return Array.isArray(value) ? value : undefined; } /**