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 c9224ff1..6a8b88cd 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 @@ -610,6 +610,107 @@ describe("InspectorClient list salvage (#1909)", () => { expect(connected.getMalformedListItems()).toEqual([]); }); + it("does not let the scan overwrite the aggregate's report", async () => { + // Both walks run on a modern connection and they are separate, uncached + // exchanges. The aggregate is the list being RENDERED, so its report owns + // the indices; the scan must not replace them with its own — the server can + // answer the second request differently, and then the warning would point + // into a list nobody is looking at. + const server = await startMalformedServer( + { + tools: [ + { + items: [ + { name: "ok_tool", inputSchema: { type: "object" } }, + { name: "broken", inputSchema: "not-a-schema" }, + ], + }, + ], + }, + true, + ); + stopServer = server.stop; + const connected = await connectTo(server.url, "modern"); + + // Serve a DIFFERENT malformed shape to the scan's walk, which runs after + // the aggregate — so an overwrite is visible as a changed index/label. + let swapped = false; + const original = connected.listTools.bind(connected); + connected.listTools = async (cursor?: string, meta?: object) => { + if (!swapped) { + swapped = true; + server.setPages({ + tools: [ + { + items: [ + { name: "a", inputSchema: { type: "object" } }, + { name: "b", inputSchema: { type: "object" } }, + { name: "later_break", inputSchema: 42 }, + ], + }, + ], + }); + } + return original(cursor, meta as Record | undefined); + }; + + await connected.listAllTools(); + + // The aggregate's finding, at the aggregate's index — not the scan's. + expect(connected.getMalformedListItems()).toEqual([ + { + method: "tools/list", + index: 1, + label: "broken", + reason: expect.stringMatching(/^inputSchema/), + }, + ]); + }); + + it("marks the scan's refused response even when the re-fetch is clean", async () => { + // The list can change between the strict page and the lenient re-fetch. A + // conforming re-fetch says nothing about the response that was refused, so + // leaving it unmarked would render a rejected exchange as a clean success. + const server = await startMalformedServer( + { tools: [{ items: [{ name: "broken", inputSchema: 42 }] }] }, + true, + ); + stopServer = server.stop; + const connected = await connectTo(server.url, "modern"); + const log = new MessageLogState(connected); + + // Let the strict call fail, then serve a conforming list to every later + // request — so the scan's re-fetch finds nothing per-item wrong. + const original = connected.listTools.bind(connected); + connected.listTools = async (cursor?: string, meta?: object) => { + try { + return await original( + cursor, + meta as Record | undefined, + ); + } finally { + server.setPages({ + tools: [ + { items: [{ name: "fixed", inputSchema: { type: "object" } }] }, + ], + }); + } + }; + + await connected.refreshExcludedTools(); + + const marked = log + .getMessages() + .filter( + (entry) => + "method" in entry.message && + entry.message.method === "tools/list" && + entry.clientError !== undefined, + ); + expect(marked).toHaveLength(1); + log.destroy(); + }); + 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 diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index cc7f0b65..d7bb2078 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -3178,7 +3178,11 @@ export class InspectorClient extends InspectorClientEventTarget { const effectiveMeta = this.mergeMeta(metadata); const params: ListToolsRequest["params"] = { ...(effectiveMeta ? { _meta: effectiveMeta } : {}), - ...(cursor ? { cursor } : {}), + // `!== undefined`, not truthiness: "" is a valid cursor. Dropping it asks + // for page one again — and `listToolsForScan` pairs this call with a + // fallback that already preserves it, so the two legs of one scan page + // would otherwise disagree about which page they fetched. + ...(cursor !== undefined && { cursor }), }; const response = await this.invokeMcpClient(() => this.client!.request( @@ -3617,13 +3621,26 @@ 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) { + // Report what the scan itself had to drop — but only when the aggregate + // salvage left nothing to say. + // + // The aggregate OWNS this report, because the aggregate is the list the + // user is looking at. This walk is a second, deliberately un-cached + // exchange, so its entries need not be the rendered ones: the server can + // change between the two requests, and a cache-served aggregate is a + // different list version entirely. Overwriting an aggregate report with + // this one would then replace accurate indices with indices into a list + // nobody is seeing. + // + // It still publishes when the aggregate said nothing, which is the case + // this exists for: a cache-served aggregate succeeds without ever running + // salvage, so without this a malformed tool would be dropped from the scan + // with no explanation anywhere. `setMalformedListItems` is not called at + // all when empty, so a clean scan cannot clear what the aggregate wrote. + if ( + malformed.length > 0 && + this.malformedListItems.get("tools/list") === undefined + ) { this.setMalformedListItems("tools/list", malformed); } this.excludedTools = excluded; @@ -3674,17 +3691,26 @@ export class InspectorClient extends InspectorClientEventTarget { 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), - ); + // Through `invokeMcpClient`, like the strict `listTools` above and like + // `salvageList`'s walk: this re-fetch can meet an auth challenge of its + // own, and outside that wrapper the recovery never runs. Its failure is + // then swallowed by `listAllTools`'s best-effort scan catch, leaving a + // stale excluded-tools set and no sign of why. + const page = await this.invokeMcpClient( + () => + this.isModernEra() + ? this.rawWireRequest( + "tools/list", + this.withModernEnvelope(params), + pageSchema, + ) + : this.client!.request( + { method: "tools/list", params }, + pageSchema, + this.getRequestOptions(metadata?.progressToken), + ), + { method: "tools/list" }, + ); // Same era-envelope check as `salvageList` — the raw-wire path bypasses // the codec that would otherwise enforce it. if ( @@ -3702,10 +3728,22 @@ export class InspectorClient extends InspectorClientEventTarget { items, schema: toolItemSchemaForEra(this.isModernEra()), }); - if (rejectedResponseId !== undefined && malformed.length > 0) { + // Mark regardless of what the re-fetch found. The list can change + // between the two requests, so a conforming re-fetch is possible — and + // it says nothing about the response that was actually refused. Leaving + // that one unmarked because the SECOND request came back clean would + // render a rejected exchange as a clean success, which is the lie + // #1953 removes. With nothing per-item to name, the original decode + // error is the reason. + if (rejectedResponseId !== undefined) { this.dispatchTypedEvent("responseRejected", { id: rejectedResponseId, - reason: summarizeMalformed(malformed), + reason: + malformed.length > 0 + ? summarizeMalformed(malformed) + : err instanceof Error + ? err.message + : String(err), }); } const nextCursor = nextCursorOf(page);