From 77ff30bbc940ffdab717f781530eff15a4d8087a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 20:38:10 -0400 Subject: [PATCH] fix: stop the exclusion scan writing the rendered list's report (#1909) Copilot round 10. The scan no longer publishes its malformed entries into the tools/list report at all. Round 9 only stopped it overwriting the aggregate's; the remaining case is unsalvageable for a stronger reason than 'two different requests': - a FAILED strict aggregate runs salvage, which writes the report itself; - a SUCCESSFUL one means every entry parsed, so the rendered list contains no malformed entries. So by the time the scan finds something, either the aggregate already reported it with indices matching the screen, or the aggregate succeeded and what the scan found is provably not in the list being displayed. Publishing could only ever name entries the user cannot see. Nothing is lost: listToolsForScan still marks its own refused response, so the finding surfaces on the Protocol entry for the exchange that carried it, where it is true. The per-page accumulation and raw-entry offset existed only to feed the removed publish, and go with it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E Signed-off-by: cliffhall --- .../inspectorClient-malformed-list.test.ts | 20 +++--- core/mcp/inspectorClient.ts | 65 +++++++------------ 2 files changed, 34 insertions(+), 51 deletions(-) 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 6a8b88cd..64dcafbe 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 @@ -535,10 +535,12 @@ describe("InspectorClient list salvage (#1909)", () => { log.destroy(); }); - it("reports scan-dropped tools at their position in the whole list", async () => { - // The exclusion scan walks pages of its own, so its report has to be - // offset by the RAW entries of PRIOR pages. An unlabeled entry's index is - // all the user has to go on, so a wrong offset points at nothing. + it("indexes a dropped tool against the whole aggregate, across pages", async () => { + // The reported index has to be offset by the RAW entries of PRIOR pages — + // valid and malformed alike, not just the kept ones. An unlabeled entry's + // index is all the user has to go on, so a wrong offset points at nothing. + // This is the aggregate salvage's walk: it owns the `tools/list` report, + // because it is the walk that produced the list being rendered. const server = await startMalformedServer( { tools: [ @@ -610,12 +612,12 @@ describe("InspectorClient list salvage (#1909)", () => { expect(connected.getMalformedListItems()).toEqual([]); }); - it("does not let the scan overwrite the aggregate's report", async () => { + it("keeps the aggregate's report when the scan sees a different list", 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. + // exchanges. The aggregate is the list being RENDERED, so it owns the + // report; the scan never writes it. Here the server answers the scan's + // request with a different malformed shape — which, if the scan published, + // would leave the warning pointing into a list nobody is looking at. const server = await startMalformedServer( { tools: [ diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index d7bb2078..333020aa 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -3579,10 +3579,6 @@ export class InspectorClient extends InspectorClientEventTarget { metadata?: Record, ): Promise { const excluded: ExcludedTool[] = []; - const malformed: MalformedListItem[] = []; - // Raw entries seen so far, across pages — the frame the reported indices - // are relative to (see the mapping below). - let rawOffset = 0; // 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()) { @@ -3597,18 +3593,6 @@ export class InspectorClient extends InspectorClientEventTarget { if (pages >= LIST_MAX_PAGES) throw listPaginationExceeded("tools/list"); pages += 1; const page = await this.listToolsForScan(cursor, metadata); - malformed.push( - ...page.malformed.map((entry) => ({ - ...entry, - // Offset by the RAW entries of PRIOR pages only. `excluded.length` - // would be wrong twice over: it counts just the invalid-header - // tools, and on page one it would also fold in this page's own - // valid count — putting an unlabeled entry, whose index is all the - // user has to go on, at a position that doesn't exist. - index: entry.index + rawOffset, - })), - ); - rawOffset += page.tools.length + page.malformed.length; for (const tool of page.tools) { const scan = scanXMcpHeaderDeclarations(tool.inputSchema); if (!scan.valid) excluded.push({ tool, reason: scan.reason }); @@ -3621,28 +3605,24 @@ export class InspectorClient extends InspectorClientEventTarget { } } while (cursor !== undefined); } - // Report what the scan itself had to drop — but only when the aggregate - // salvage left nothing to say. + // The scan deliberately does NOT publish what it dropped into the + // `tools/list` report. That report is rendered above the Tools list as + // "entries dropped from this list", and the aggregate is what that list + // is — so only the aggregate's salvage may write it. // - // 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. + // The reason is stronger than "these are two different requests". A + // successful strict aggregate means every entry parsed, so the rendered + // list has no malformed entries in it at all; and a FAILED aggregate runs + // salvage, which writes the report itself. So by the time this walk finds + // something, either the aggregate already reported it (with the indices + // that match what is on screen), or the aggregate succeeded and whatever + // this walk just found is provably not in the list being displayed — the + // server changed, or the aggregate came from cache. Publishing here can + // therefore only ever name entries the user cannot see. // - // 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); - } + // The finding is not lost: `listToolsForScan` marks the scan's own refused + // response, so it surfaces on the Protocol entry for the exchange that + // actually carried it, which is where it is true. this.excludedTools = excluded; this.dispatchTypedEvent("excludedToolsChange", excluded); return excluded; @@ -3657,8 +3637,12 @@ export class InspectorClient extends InspectorClientEventTarget { * which would blank the excluded set and leave a tool that vanished for an * invalid header with no explanation at all. That is the same failure #1909 * is about, one level down. So on a decode rejection the page is re-fetched - * leniently and scanned entry by entry; the malformed ones are not "excluded - * tools" and are reported through the salvage channel instead. + * leniently and scanned entry by entry. + * + * The malformed ones are not "excluded tools" and are not returned: they + * describe THIS exchange, not the aggregate the Tools panel renders, so they + * surface as the rejection reason on this response's own Protocol entry and + * go no further (see `refreshExcludedTools`). */ private async listToolsForScan( cursor: string | undefined, @@ -3666,11 +3650,9 @@ export class InspectorClient extends InspectorClientEventTarget { ): Promise<{ tools: Tool[]; nextCursor?: string; - malformed: MalformedListItem[]; }> { try { - const page = await this.listTools(cursor, metadata); - return { ...page, malformed: [] }; + return await this.listTools(cursor, metadata); } catch (err) { if (!isSalvageableRejection(err)) throw err; // Capture before the re-fetch, for the reason `salvageList` documents: @@ -3750,7 +3732,6 @@ export class InspectorClient extends InspectorClientEventTarget { return { tools: valid, ...(nextCursor !== undefined && { nextCursor }), - malformed, }; } catch (fallbackErr) { // The fallback could not explain the rejection — the re-fetch itself