diff --git a/clients/web/src/test/core/mcp/listSalvage.test.ts b/clients/web/src/test/core/mcp/listSalvage.test.ts index f0fd7fdc..a117403b 100644 --- a/clients/web/src/test/core/mcp/listSalvage.test.ts +++ b/clients/web/src/test/core/mcp/listSalvage.test.ts @@ -2,14 +2,18 @@ import { describe, it, expect } from "vitest"; import { z } from "zod/v4"; import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client"; import { - LenientListPageSchema, + ModernResultEnvelopeSchema, describeIssues, isClientDecodeRejection, + isSalvageableRejection, labelForRawItem, + lenientListPageSchema, + nextCursorOf, rawItemsOf, salvageListItems, summarizeMalformed, } from "@inspector/core/mcp/listSalvage.js"; +import { ListToolsResultSchema } from "@modelcontextprotocol/core"; /** * Per-item list salvage (#1909). @@ -48,34 +52,102 @@ describe("isClientDecodeRejection", () => { }); }); -describe("LenientListPageSchema", () => { - it("keeps the primitive array unvalidated while typing the cursor", () => { - const parsed = LenientListPageSchema.parse({ - resourceTemplates: [{ name: "ok" }, { name: 42 }], +describe("lenientListPageSchema", () => { + const ToolsPageSchema = lenientListPageSchema(ListToolsResultSchema, "tools"); + + it("leaves the entries raw while still validating every other field", () => { + // The entries are this module's job to check one at a time; everything + // else is the real schema's, so a second violation can't ride along. + const parsed = ToolsPageSchema.parse({ + tools: [{ name: "ok" }, { name: 42 }], nextCursor: "page-2", }); - expect(parsed.nextCursor).toBe("page-2"); - expect(rawItemsOf(parsed, "resourceTemplates")).toHaveLength(2); + expect(nextCursorOf(parsed)).toBe("page-2"); + expect(rawItemsOf(parsed, "tools")).toHaveLength(2); }); it("rejects a page whose cursor is malformed", () => { // The walk itself depends on `nextCursor`, so a bad one is not salvageable // — the strict error should stand rather than a partial list being kept. expect( - LenientListPageSchema.safeParse({ tools: [], nextCursor: 7 }).success, + ToolsPageSchema.safeParse({ tools: [], nextCursor: 7 }).success, + ).toBe(false); + }); + + it("rejects a top-level violation that rides along with a bad entry", () => { + // A hand-rolled `{ nextCursor? }` schema would accept this, report the bad + // entry, and silently drop the `_meta` violation with the strict error. + expect( + ToolsPageSchema.safeParse({ tools: [{ name: 42 }], _meta: "nope" }) + .success, ).toBe(false); }); 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: [] }), "tools"), - ).toEqual([]); + expect(rawItemsOf(ToolsPageSchema.parse({ tools: [] }), "tools")).toEqual( + [], + ); expect(rawItemsOf({}, "tools")).toBeUndefined(); + expect(rawItemsOf({ tools: "not-a-list" }, "tools")).toBeUndefined(); + expect(rawItemsOf(null, "tools")).toBeUndefined(); + }); + + it("reads no cursor from a non-object page", () => { + expect(nextCursorOf(null)).toBeUndefined(); + expect(nextCursorOf({ nextCursor: 7 })).toBeUndefined(); + }); +}); + +describe("isSalvageableRejection", () => { + it("salvages an invalid result but not an unrecognized result type", () => { + // `UnsupportedResultType` means the codec doesn't know what KIND of frame + // arrived — not a list with a bad entry, so there is nothing to salvage + // and reporting an item-level reason would hide the real failure. expect( - rawItemsOf(LenientListPageSchema.parse({ tools: "not-a-list" }), "tools"), - ).toBeUndefined(); + isSalvageableRejection(new SdkError(SdkErrorCode.InvalidResult, "bad")), + ).toBe(true); + expect( + isSalvageableRejection( + new SdkError(SdkErrorCode.UnsupportedResultType, "unknown"), + ), + ).toBe(false); + // Still marks the Protocol entry, which is why both predicates exist. + expect( + isClientDecodeRejection( + new SdkError(SdkErrorCode.UnsupportedResultType, "unknown"), + ), + ).toBe(true); + }); +}); + +describe("ModernResultEnvelopeSchema", () => { + it("accepts a complete modern result with its cache hints", () => { + expect( + ModernResultEnvelopeSchema.safeParse({ + resultType: "complete", + ttlMs: 0, + cacheScope: "public", + tools: [], + }).success, + ).toBe(true); + }); + + it("rejects a modern result missing the envelope the codec would enforce", () => { + // The raw-wire salvage path goes around that codec, so a response missing + // its cache hints AND carrying a bad entry must not salvage. + expect( + ModernResultEnvelopeSchema.safeParse({ + resultType: "complete", + tools: [], + }).success, + ).toBe(false); + expect( + ModernResultEnvelopeSchema.safeParse({ + resultType: "task", + ttlMs: 0, + cacheScope: "public", + }).success, + ).toBe(false); }); }); 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 e562d6f6..b9b1f8a6 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 @@ -565,6 +565,36 @@ describe("InspectorClient list salvage (#1909)", () => { expect(reported[0]?.label).toBeUndefined(); }); + it("leaves the correlation on the original response when it rethrows", async () => { + // On a rethrow the re-walk has already answered the same method, so the + // caller's own `markResponseRejected` would land on the lenient page that + // succeeded. The correlation is restored so the outer mark still finds the + // response that was actually refused. + const server = await startMalformedServer({ + resourceTemplates: [{ items: [VALID_TEMPLATE], nextCursor: 42 }], + }); + stopServer = server.stop; + const connected = await connectTo(server.url); + const log = new MessageLogState(connected); + + await expect(connected.listAllResourceTemplates()).rejects.toThrow(); + // Simulates what ManagedListState.refresh() does in its catch. + connected.markResponseRejected("resources/templates/list", "outer mark"); + + const calls = log + .getMessages() + .filter( + (entry) => + "method" in entry.message && + entry.message.method === "resources/templates/list", + ); + expect(calls[0]?.clientError).toBe("outer mark"); + for (const later of calls.slice(1)) { + expect(later.clientError).toBeUndefined(); + } + log.destroy(); + }); + 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 df03a016..90a6c37d 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 { - LenientListPageSchema, - isClientDecodeRejection, + ModernResultEnvelopeSchema, + isSalvageableRejection, + nextCursorOf, + lenientListPageSchema, rawItemsOf, salvageListItems, summarizeMalformed, @@ -3187,6 +3189,7 @@ export class InspectorClient extends InspectorClientEventTarget { const tools = await this.listAllSalvaging({ method: "tools/list", itemsKey: "tools", + resultSchema: ListToolsResultSchema, itemSchema: ToolSchema, metadata: options?.metadata, finalize: (salvaged) => this.excludeInvalidXMcpHeaderTools(salvaged), @@ -3254,6 +3257,27 @@ export class InspectorClient extends InspectorClientEventTarget { this.dispatchTypedEvent("responseRejected", { id, reason }); } + /** + * Put the method's response correlation back where it was before a salvage + * re-walk, so a caller that goes on to mark the failure still marks the + * ORIGINAL rejected response. + * + * The re-walk answers the same method, so on any rethrow path the map points + * at a lenient page that succeeded. `ManagedListState.refresh()` catches the + * rethrown error and calls `markResponseRejected(method, ...)`, which would + * then stamp "Rejected by the Inspector" onto that page and leave the real + * one rendering clean — the same misattribution the capture inside + * `salvageList` exists to prevent, just one frame further out. Deleting when + * nothing had answered before makes the outer mark a no-op rather than a lie. + */ + private restoreResponseCorrelation( + method: string, + id: string | number | undefined, + ): void { + if (id === undefined) this.lastAnsweredRequestByMethod.delete(method); + else this.lastAnsweredRequestByMethod.set(method, id); + } + /** Every entry dropped from a list result as malformed, across methods (#1909). */ getMalformedListItems(): MalformedListItem[] { return [...this.malformedListItems.values()].flat(); @@ -3295,6 +3319,7 @@ export class InspectorClient extends InspectorClientEventTarget { method, itemsKey, itemSchema, + resultSchema, aggregate, metadata, finalize, @@ -3302,6 +3327,9 @@ export class InspectorClient extends InspectorClientEventTarget { method: string; itemsKey: string; itemSchema: z.ZodType; + /** The method's real result schema — the salvage page schema is derived + * from it so every field except the entries is still validated. */ + resultSchema: z.ZodObject; aggregate: () => Promise; metadata?: Record; /** @@ -3316,11 +3344,15 @@ export class InspectorClient extends InspectorClientEventTarget { this.setMalformedListItems(method, []); return items; } catch (err) { - if (!isClientDecodeRejection(err)) throw err; + // Deliberately narrower than the Protocol-marking predicate — see + // `isSalvageableRejection`: an unrecognized result KIND is not a list + // with a bad entry in it. + if (!isSalvageableRejection(err)) throw err; const salvaged = await this.salvageList({ method, itemsKey, itemSchema, + resultSchema, metadata, err, }); @@ -3369,15 +3401,18 @@ export class InspectorClient extends InspectorClientEventTarget { method, itemsKey, itemSchema, + resultSchema, metadata, err, }: { method: string; itemsKey: string; itemSchema: z.ZodType; + resultSchema: z.ZodObject; metadata?: Record; err: unknown; }): Promise { + const pageSchema = lenientListPageSchema(resultSchema, itemsKey); // 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 @@ -3413,15 +3448,23 @@ export class InspectorClient extends InspectorClientEventTarget { ? this.rawWireRequest( method, this.withModernEnvelope(params), - LenientListPageSchema, + pageSchema, ) : this.client!.request( { method, params }, - LenientListPageSchema, + pageSchema, this.getRequestOptions(metadata?.progressToken), ), { method }, ); + // The raw-wire path skips the era codec, so the envelope it would have + // enforced is checked here; a violation is not salvageable. + if ( + this.isModernEra() && + !ModernResultEnvelopeSchema.safeParse(page).success + ) { + throw err; + } 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 @@ -3436,7 +3479,7 @@ export class InspectorClient extends InspectorClientEventTarget { }); valid.push(...salvaged.valid); malformed.push(...salvaged.malformed); - cursor = page.nextCursor; + cursor = nextCursorOf(page); 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; @@ -3456,12 +3499,16 @@ export class InspectorClient extends InspectorClientEventTarget { "Salvage re-walk failed; surfacing the original list error", ); } + this.restoreResponseCorrelation(method, rejectedResponseId); throw err; } // Nothing per-item was wrong, so the strict rejection was about something // this fallback cannot explain — surface it rather than hide it. - if (malformed.length === 0) throw err; + if (malformed.length === 0) { + this.restoreResponseCorrelation(method, rejectedResponseId); + throw err; + } if (rejectedResponseId !== undefined) { this.dispatchTypedEvent("responseRejected", { @@ -3568,7 +3615,7 @@ export class InspectorClient extends InspectorClientEventTarget { const page = await this.listTools(cursor, metadata); return { ...page, malformed: [] }; } catch (err) { - if (!isClientDecodeRejection(err)) throw err; + if (!isSalvageableRejection(err)) throw err; // Capture before the re-fetch, for the reason `salvageList` documents: // this refused response is a distinct exchange from the aggregate's, and // the re-fetch below moves the method's correlation off it. Without this @@ -3582,17 +3629,26 @@ export class InspectorClient extends InspectorClientEventTarget { // 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), - LenientListPageSchema, + pageSchema, ) : await this.client!.request( { method: "tools/list", params }, - LenientListPageSchema, + 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`). @@ -3608,9 +3664,10 @@ export class InspectorClient extends InspectorClientEventTarget { reason: summarizeMalformed(malformed), }); } + const nextCursor = nextCursorOf(page); return { tools: valid, - ...(page.nextCursor !== undefined && { nextCursor: page.nextCursor }), + ...(nextCursor !== undefined && { nextCursor }), malformed, }; } @@ -4737,6 +4794,7 @@ export class InspectorClient extends InspectorClientEventTarget { const resources = await this.listAllSalvaging({ method: "resources/list", itemsKey: "resources", + resultSchema: ListResourcesResultSchema, itemSchema: ResourceSchema, metadata: options?.metadata, aggregate: async () => [ @@ -4902,6 +4960,7 @@ export class InspectorClient extends InspectorClientEventTarget { const resourceTemplates = await this.listAllSalvaging({ method: "resources/templates/list", itemsKey: "resourceTemplates", + resultSchema: ListResourceTemplatesResultSchema, itemSchema: ResourceTemplateSchema, metadata: options?.metadata, aggregate: async () => [ @@ -4963,6 +5022,7 @@ export class InspectorClient extends InspectorClientEventTarget { const prompts = await this.listAllSalvaging({ method: "prompts/list", itemsKey: "prompts", + resultSchema: ListPromptsResultSchema, itemSchema: PromptSchema, metadata: options?.metadata, aggregate: async () => [ diff --git a/core/mcp/listSalvage.ts b/core/mcp/listSalvage.ts index 5d12f5da..37aeebbe 100644 --- a/core/mcp/listSalvage.ts +++ b/core/mcp/listSalvage.ts @@ -79,19 +79,70 @@ export function isClientDecodeRejection(err: unknown): boolean { } /** - * A list page parsed with everything except pagination left unvalidated. + * Whether a decode rejection is one the per-item fallback may attempt at all. * - * `looseObject` keeps the unknown keys, so the primitive array (`tools`, - * `resources`, `resourceTemplates`, `prompts`) survives as `unknown` for - * per-item validation. Only `nextCursor` is typed, because the walk itself - * depends on it — if THAT is malformed the page is unusable and the strict - * error should stand. + * Narrower than {@link isClientDecodeRejection} on purpose, and the difference + * is the point: `UnsupportedResultType` says the codec does not recognize what + * KIND of frame arrived, so it is not a list result with a bad entry in it — + * it is not a list result at all. Salvaging it would report an item-level + * reason for a protocol-level failure and hide the real one. Marking the + * Protocol entry rejected still applies to both, which is why the broader + * predicate stays. */ -export const LenientListPageSchema = z.looseObject({ - nextCursor: z.string().optional(), -}); +export function isSalvageableRejection(err: unknown): boolean { + return SdkError.isInstance(err) && err.code === SdkErrorCode.InvalidResult; +} -export type LenientListPage = z.infer; +/** + * The page schema for a salvage re-walk: the method's REAL result schema with + * only its item array relaxed to `unknown[]`. + * + * Deriving it from the spec schema rather than hand-rolling a permissive object + * is what keeps the fallback honest. A hand-rolled `{ nextCursor? }` validates + * pagination and nothing else, so a response carrying a malformed item AND an + * unrelated top-level violation would be accepted, the item reported, and the + * other violation silently dropped along with the strict error that caught it. + * Extending the real schema means every top-level field — `_meta`, `nextCursor`, + * anything a future revision adds — is still validated; only the entries, which + * this whole module exists to check one at a time, are left raw. + */ +export function lenientListPageSchema( + resultSchema: z.ZodObject, + itemsKey: string, +): z.ZodObject { + return resultSchema.extend({ [itemsKey]: z.array(z.unknown()) }); +} + +/** + * Read the pagination cursor off a leniently-parsed page. + * + * The page is typed as a plain object because `extend()` on a runtime-selected + * key loses the shape, so the fields are read through narrowing helpers rather + * than asserted with a cast. The schema has already validated that a present + * `nextCursor` is a string; this narrows the static type to match. + */ +export function nextCursorOf(page: unknown): string | undefined { + if (typeof page !== "object" || page === null) return undefined; + const value = (page as Record).nextCursor; + return typeof value === "string" ? value : undefined; +} + +/** + * The era envelope a modern (2026-07-28) result must carry, checked separately + * because it lives in the SDK's codec rather than in the spec result schema — + * and the modern salvage path deliberately goes around that codec. + * + * Without this, a modern response that is missing its cache hints AND has one + * bad entry would salvage: the entry gets reported and the envelope violation + * disappears. A failure here means "not salvageable", so the strict error + * stands — the safe direction if a future revision changes these rules, since + * the strict path remains the authority on what is valid. + */ +export const ModernResultEnvelopeSchema = z.looseObject({ + resultType: z.literal("complete"), + ttlMs: z.number(), + cacheScope: z.enum(["public", "private"]), +}); /** * Read the primitive array out of a leniently-parsed page. @@ -104,9 +155,10 @@ export type LenientListPage = z.infer; * that was correct about it is discarded. */ export function rawItemsOf( - page: LenientListPage, + page: unknown, itemsKey: string, ): unknown[] | undefined { + if (typeof page !== "object" || page === null) return undefined; const value = (page as Record)[itemsKey]; return Array.isArray(value) ? value : undefined; }