From 573b14e14e80e831254f52f43aa5b75eabf7995d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 18:22:10 -0400 Subject: [PATCH] fix: address Copilot round 4 on the list-salvage path (#1909) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings, both about the fallback accepting more than it should. - The lenient page schema validated pagination and nothing else, so a response carrying a malformed entry AND an unrelated top-level violation salvaged: the entry was reported and the other violation vanished with the strict error that caught it. The page schema is now DERIVED from the method's real result schema with only its item array relaxed (`lenientListPageSchema`), so every other field — `_meta`, `nextCursor`, anything a future revision adds — is still validated. The modern era envelope the raw-wire path bypasses (`resultType`, `ttlMs`, `cacheScope`) is checked explicitly for the same reason. Salvage is also narrowed to `InvalidResult`: `UnsupportedResultType` means the codec doesn't recognize what KIND of frame arrived, so it isn't a list with a bad entry and reporting an item-level reason would hide a protocol failure. Marking the Protocol entry still applies to both, hence two predicates. - On every rethrow path the re-walk had already answered the method, so the caller's own `markResponseRejected` (in `ManagedListState.refresh`) landed on the lenient page that succeeded and left the refused response clean. The correlation is restored before rethrowing — deleted when nothing had answered before, so the outer mark is a no-op rather than a lie. `nextCursorOf` / `rawItemsOf` narrow the page's fields instead of casting: the runtime-keyed `extend()` loses the shape, and asserting it back would be exactly the unjustified cast the last round removed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 Signed-off-by: cliffhall --- .../web/src/test/core/mcp/listSalvage.test.ts | 102 +++++++++++++++--- .../inspectorClient-malformed-list.test.ts | 30 ++++++ core/mcp/inspectorClient.ts | 82 ++++++++++++-- core/mcp/listSalvage.ts | 74 +++++++++++-- 4 files changed, 251 insertions(+), 37 deletions(-) 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; }