modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix: let the aggregate own the tools/list report, and harden the scan (#1909)

Browse source 
Copilot round 9.

The exclusion scan is a separate, uncached tools/list exchange, so its entries
need not be the ones rendered — the server can answer differently, and a
cache-served aggregate is a different list version. It was overwriting the
aggregate's report wholesale, replacing accurate indices with indices into a
list nobody is looking at. The aggregate now owns the report; the scan
publishes only when the aggregate left nothing, which is the cache-served case
that made it publish at all (round 2).

A conforming re-fetch no longer swallows the mark: the list can change between
the strict page and the lenient one, and a clean second response says nothing
about the first, which was refused. It is marked with the original decode
error when there is nothing per-item to name.

The scan's fallback re-fetch now goes through invokeMcpClient, like the strict
call beside it and like salvageList's walk, so an auth challenge on that
request can still be recovered instead of being swallowed by listAllTools's
best-effort scan catch.

Also fixes listTools dropping an empty-string cursor (truthiness rather than
!== undefined) — its own fallback leg already preserved it, so the two legs of
one scan page disagreed about which page they had fetched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDXCSrnSBV7vZfxycG5Z6E
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-15 20:23:59 -04:00
commit efd0bf2b7c
2 changed files with 160 additions and 21 deletions
Download patch file Download diff file Expand all files Collapse all files

101
clients/web/src/test/integration/mcp/inspectorClient-malformed-list.test.ts
View file

@ -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<string, string> | 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<string, string> | 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

80
core/mcp/inspectorClient.ts
View file

@ -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);