diff --git a/clients/web/src/test/core/mcp/listSalvage.test.ts b/clients/web/src/test/core/mcp/listSalvage.test.ts index f547eee0..2cdf3f3d 100644 --- a/clients/web/src/test/core/mcp/listSalvage.test.ts +++ b/clients/web/src/test/core/mcp/listSalvage.test.ts @@ -2,8 +2,10 @@ import { describe, it, expect } from "vitest"; import { z } from "zod/v4"; import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client"; import { + LIST_MAX_PAGES, ModernResultEnvelopeSchema, describeIssues, + listPaginationExceeded, isClientDecodeRejection, isSalvageableRejection, labelForRawItem, @@ -317,4 +319,21 @@ describe("summarizeMalformed", () => { expect(summary).toContain("index 0"); expect(summary).toContain("(+1 more)"); }); + + describe("the page bound", () => { + it("matches the cap the SDK client is configured with", () => { + // The value is passed to the SDK as `listMaxPages`, so the strict + // aggregate and the fallback walks are bounded by the same number. + expect(LIST_MAX_PAGES).toBe(64); + }); + + it("names the method and the cap it hit", () => { + // The walk's failure is logged rather than surfaced (the original + // validation error is what the caller sees), so the message is the only + // record of why salvage gave up. + const error = listPaginationExceeded("resources/templates/list"); + expect(error.message).toContain("resources/templates/list"); + expect(error.message).toContain("64 pages"); + }); + }); }); 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 b9b1f8a6..c9224ff1 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 @@ -9,6 +9,7 @@ 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"; +import { LIST_MAX_PAGES } from "@inspector/core/mcp/listSalvage.js"; /** * Per-item list salvage against a real connection (#1909). @@ -608,4 +609,91 @@ describe("InspectorClient list salvage (#1909)", () => { await expect(connected.listAllResourceTemplates()).rejects.toThrow(); expect(connected.getMalformedListItems()).toEqual([]); }); + + it("stops the salvage walk at the page cap instead of paging forever", async () => { + // A server whose `nextCursor` never converges but never REPEATS slips the + // repeated-cursor guard: every cursor is fresh, so the walk had no reason + // to stop and would page (and accumulate) without bound. The bound is the + // same `LIST_MAX_PAGES` the SDK aggregate is configured with. + const server = await startMalformedServer({ + resourceTemplates: Array.from( + { length: LIST_MAX_PAGES + 40 }, + (_, i) => ({ + items: [PHP_EMPTY_ANNOTATIONS], + nextCursor: `cursor-${i}`, + }), + ), + }); + stopServer = server.stop; + const connected = await connectTo(server.url); + + // Truncation is not salvage: with entries still unfetched, returning what + // we have would present a partial list as complete, so the original + // validation error stands. + await expect(connected.listAllResourceTemplates()).rejects.toThrow(); + expect(connected.getMalformedListItems()).toEqual([]); + + const listCalls = server.calls.filter( + (method) => method === "resources/templates/list", + ); + // One strict aggregate attempt (rejected on page one), then a salvage walk + // bounded at exactly the cap. Without the bound this runs to 104 pages — + // and against a truly endless server, forever. + expect(listCalls).toHaveLength(1 + LIST_MAX_PAGES); + }); + + it("stops the exclusion scan at the page cap too", async () => { + // The SEP-2243 scan walks pages of its own, outside the SDK aggregate's + // cap, so it needs the same bound. Driven directly because a non-converging + // list trips `listAllTools`'s salvage walk first. + const server = await startMalformedServer( + { + tools: Array.from({ length: LIST_MAX_PAGES + 40 }, (_, i) => ({ + items: [{ name: `tool_${i}`, inputSchema: { type: "object" } }], + nextCursor: `cursor-${i}`, + })), + }, + true, + ); + stopServer = server.stop; + const connected = await connectTo(server.url, "modern"); + + await expect(connected.refreshExcludedTools()).rejects.toThrow( + /exceeded 64 pages/, + ); + expect( + server.calls.filter((method) => method === "tools/list"), + ).toHaveLength(LIST_MAX_PAGES); + }); + + it("marks the scan's refused response when its own fallback fails", async () => { + // `tools` is not a list at all, so the scan's lenient re-fetch cannot + // explain the rejection and rethrows. `listAllTools` catches a failing scan + // by design (it must never fail the tools list), so nothing downstream + // would mark the response the client demonstrably refused — it would render + // as a clean exchange. + const server = await startMalformedServer( + { tools: [{ items: "not-a-list" }] }, + true, + ); + stopServer = server.stop; + const connected = await connectTo(server.url, "modern"); + const log = new MessageLogState(connected); + + await expect(connected.refreshExcludedTools()).rejects.toThrow(); + + const marked = log + .getMessages() + .filter( + (entry) => + "method" in entry.message && + entry.message.method === "tools/list" && + entry.clientError !== undefined, + ); + // The mark carries the ORIGINAL decode error: the fallback's own failure is + // a different event from the response being marked. + expect(marked).toHaveLength(1); + expect(marked[0]?.clientError).toBeTruthy(); + log.destroy(); + }); }); diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index cbdba8ba..cc7f0b65 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -172,8 +172,10 @@ import { AjvJsonSchemaValidator } from "@modelcontextprotocol/client/validators/ import { z } from "zod/v4"; import { validateToolOutput } from "./toolOutputValidation.js"; import { + LIST_MAX_PAGES, ModernResultEnvelopeSchema, isSalvageableRejection, + listPaginationExceeded, toolItemSchemaForEra, nextCursorOf, lenientListPageSchema, @@ -688,7 +690,13 @@ export class InspectorClient extends InspectorClientEventTarget { capabilities?: ClientCapabilities; versionNegotiation?: VersionNegotiationOptions; inputRequired?: InputRequiredOptions; + listMaxPages?: number; } = { + // Bound the SDK's auto-aggregate with the SAME constant the #1909 salvage + // re-walks use, so the strict path and its fallback cannot drift apart. + // This is the SDK's own default today; setting it explicitly is what makes + // the shared value a fact rather than a copied guess. + listMaxPages: LIST_MAX_PAGES, // Per-server protocol era (SEP §7.8), threaded from config via // `eraToVersionNegotiation` and defaulted to `{ mode: "legacy" }` in the // constructor. "legacy" keeps the wire byte-identical to a 2025 client; @@ -3442,8 +3450,15 @@ export class InspectorClient extends InspectorClientEventTarget { const malformed: MalformedListItem[] = []; const seenCursors = new Set(); let cursor: string | undefined; + let pages = 0; try { do { + // Bounded like the SDK aggregate this stands in for: a server emitting + // endlessly UNIQUE cursors never trips the repeated-cursor guard below, + // so without this the walk would run forever. Aborting surfaces the + // original validation error rather than a truncated list. + if (pages >= LIST_MAX_PAGES) throw listPaginationExceeded(method); + pages += 1; const effectiveMeta = this.mergeMeta(metadata); const params = { ...(effectiveMeta ? { _meta: effectiveMeta } : {}), @@ -3569,7 +3584,14 @@ export class InspectorClient extends InspectorClientEventTarget { if (this.excludesInvalidXMcpHeaderTools()) { const seenCursors = new Set(); let cursor: string | undefined; + let pages = 0; do { + // Same bound as `salvageList`, for the same reason: this walk is also + // outside the SDK aggregate's `listMaxPages` cap. Throwing (rather than + // committing a partial set) keeps the caller's "the excluded list may + // be incomplete" warning the honest description of what happened. + if (pages >= LIST_MAX_PAGES) throw listPaginationExceeded("tools/list"); + pages += 1; const page = await this.listToolsForScan(cursor, metadata); malformed.push( ...page.malformed.map((entry) => ({ @@ -3641,53 +3663,90 @@ export class InspectorClient extends InspectorClientEventTarget { // client refused the result. const rejectedResponseId = this.lastAnsweredRequestByMethod.get("tools/list"); - const effectiveMeta = this.mergeMeta(metadata); - const params = { - ...(effectiveMeta ? { _meta: effectiveMeta } : {}), - // See `salvageList`: "" is a valid cursor. - ...(cursor !== undefined && { cursor }), - }; - const pageSchema = lenientListPageSchema(ListToolsResultSchema, "tools"); - const page = this.isModernEra() - ? await this.rawWireRequest( - "tools/list", - this.withModernEnvelope(params), - pageSchema, - ) - : await this.client!.request( - { method: "tools/list", params }, - pageSchema, - this.getRequestOptions(metadata?.progressToken), + try { + const effectiveMeta = this.mergeMeta(metadata); + const params = { + ...(effectiveMeta ? { _meta: effectiveMeta } : {}), + // See `salvageList`: "" is a valid cursor. + ...(cursor !== undefined && { cursor }), + }; + const pageSchema = lenientListPageSchema( + ListToolsResultSchema, + "tools", + ); + const page = this.isModernEra() + ? await this.rawWireRequest( + "tools/list", + this.withModernEnvelope(params), + pageSchema, + ) + : await this.client!.request( + { method: "tools/list", params }, + pageSchema, + this.getRequestOptions(metadata?.progressToken), + ); + // Same era-envelope check as `salvageList` — the raw-wire path bypasses + // the codec that would otherwise enforce it. + if ( + this.isModernEra() && + !ModernResultEnvelopeSchema.safeParse(page).success + ) { + throw err; + } + 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, + schema: toolItemSchemaForEra(this.isModernEra()), + }); + if (rejectedResponseId !== undefined && malformed.length > 0) { + this.dispatchTypedEvent("responseRejected", { + id: rejectedResponseId, + reason: summarizeMalformed(malformed), + }); + } + const nextCursor = nextCursorOf(page); + return { + tools: valid, + ...(nextCursor !== undefined && { nextCursor }), + malformed, + }; + } catch (fallbackErr) { + // The fallback could not explain the rejection — the re-fetch itself + // failed, or the page carried a violation that is not per-item (a bad + // modern envelope, a `tools` that is not a list). + // + // `salvageList` can simply rethrow here, because ITS caller + // (`ManagedListState.refresh`) marks the Protocol entry from the error + // it receives. This walk has no such backstop: `listAllTools` catches + // and LOGS a failing scan so it can never fail the tools list, so an + // unmarked rethrow leaves the response the client demonstrably refused + // (`isSalvageableRejection` above proves it was an `InvalidResult`) + // rendering as a clean exchange — the exact lie #1953 removes and the + // one this method's own capture above exists to prevent. So mark it + // here, with the ORIGINAL decode error as the reason: the fallback's + // own failure is a different event, and the response being marked is + // the one `err` was raised for. + if (rejectedResponseId !== undefined) { + this.dispatchTypedEvent("responseRejected", { + id: rejectedResponseId, + reason: err instanceof Error ? err.message : String(err), + }); + } + if (fallbackErr !== err) { + this.logger.warn( + { err: fallbackErr }, + "Excluded-tools salvage re-fetch failed; surfacing the original list error", ); - // Same era-envelope check as `salvageList` — the raw-wire path bypasses - // the codec that would otherwise enforce it. - if ( - this.isModernEra() && - !ModernResultEnvelopeSchema.safeParse(page).success - ) { + } + // Restore the correlation the re-fetch moved, so a later mark for this + // method does not land on a lenient page that succeeded. + this.restoreResponseCorrelation("tools/list", rejectedResponseId); throw err; } - 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, - schema: toolItemSchemaForEra(this.isModernEra()), - }); - if (rejectedResponseId !== undefined && malformed.length > 0) { - this.dispatchTypedEvent("responseRejected", { - id: rejectedResponseId, - reason: summarizeMalformed(malformed), - }); - } - const nextCursor = nextCursorOf(page); - return { - tools: valid, - ...(nextCursor !== undefined && { nextCursor }), - malformed, - }; } } diff --git a/core/mcp/listSalvage.ts b/core/mcp/listSalvage.ts index 7c5ab1e4..f043cf60 100644 --- a/core/mcp/listSalvage.ts +++ b/core/mcp/listSalvage.ts @@ -46,6 +46,41 @@ export interface MalformedListItem { reason: string; } +/** + * Hard cap on the pages a salvage / scan re-walk will fetch. + * + * The SDK's auto-aggregating `list*()` is capped by + * `ClientOptions.listMaxPages` and THROWS when it is hit, so a partial + * aggregate is never presented as a complete one. The fallback walks in this + * module go around that aggregate, so without their own bound a server whose + * `nextCursor` never converges — but never repeats, so the non-converging guard + * never fires — would keep the walk issuing requests and accumulating entries + * forever. That is reachable from any server the user points the Inspector at, + * which is the whole population here. + * + * The value is passed to the SDK client as `listMaxPages` (see + * `InspectorClient`'s constructor) rather than merely matching its default, so + * the strict path and the fallback are bounded by the SAME number by + * construction and cannot drift if the SDK changes its default. + */ +export const LIST_MAX_PAGES = 64; + +/** + * The error a walk raises when it hits {@link LIST_MAX_PAGES}. + * + * Both callers turn this into "the strict error stands": hitting the cap means + * there are more entries we did not fetch, so returning what we have would + * present a TRUNCATED list as a complete one — the same silent-truncation + * failure {@link rawItemsOf} refuses for a non-array page. That is why this + * aborts rather than `break`ing like the repeated-cursor guard, which stops on + * a page whose entries were all seen already and so loses nothing. + */ +export function listPaginationExceeded(method: string): Error { + return new Error( + `Salvage walk for ${method} exceeded ${LIST_MAX_PAGES} pages without the server's pagination converging`, + ); +} + /** * Whether a failed fetch is the CLIENT refusing a response it received, rather * than the request never producing one.