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

fix: bound the salvage walks and mark the scan's refused response (#1909)

Browse source 
Copilot round 6.

Bound both fallback walks with LIST_MAX_PAGES, which is now also passed to
the SDK client as `listMaxPages` so the strict aggregate and its fallback
share one value by construction. A server emitting endlessly UNIQUE cursors
never trips the repeated-cursor guard, so the salvage re-walk and the
SEP-2243 exclusion scan would page and accumulate without bound. Hitting the
cap aborts rather than returning a truncated list as a complete one.

Mark the exclusion scan's refused response on every fallback-failure path.
`salvageList` can rethrow bare because ManagedListState marks from the error
it receives; the scan has no such backstop — listAllTools catches and logs a
failing scan by design — so an unmarked rethrow left a response the client
demonstrably refused rendering as a clean exchange.

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 19:33:27 -04:00
commit 67972adc4e
4 changed files with 245 additions and 44 deletions
Download patch file Download diff file Expand all files Collapse all files

19
clients/web/src/test/core/mcp/listSalvage.test.ts
View file

@ -2,8 +2,10 @@ import { describe, it, expect } from "vitest";
import { z } from "zod/v4";
import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client";
import {
LIST_MAX_PAGES,
ModernResultEnvelopeSchema,
describeIssues,
listPaginationExceeded,
isClientDecodeRejection,
isSalvageableRejection,
labelForRawItem,
@ -317,4 +319,21 @@ describe("summarizeMalformed", () => {
expect(summary).toContain("index 0");
expect(summary).toContain("(+1 more)");
});
describe("the page bound", () => {
it("matches the cap the SDK client is configured with", () => {
// The value is passed to the SDK as `listMaxPages`, so the strict
// aggregate and the fallback walks are bounded by the same number.
expect(LIST_MAX_PAGES).toBe(64);
});
it("names the method and the cap it hit", () => {
// The walk's failure is logged rather than surfaced (the original
// validation error is what the caller sees), so the message is the only
// record of why salvage gave up.
const error = listPaginationExceeded("resources/templates/list");
expect(error.message).toContain("resources/templates/list");
expect(error.message).toContain("64 pages");
});
});
});

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

@ -9,6 +9,7 @@ import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js";
import { MessageLogState } from "@inspector/core/mcp/state/index.js";
import { createTransportNode } from "@inspector/core/mcp/node/transport.js";
import { eraToVersionNegotiation } from "@inspector/core/mcp/types.js";
import { LIST_MAX_PAGES } from "@inspector/core/mcp/listSalvage.js";
/**
* Per-item list salvage against a real connection (#1909).
@ -608,4 +609,91 @@ describe("InspectorClient list salvage (#1909)", () => {
await expect(connected.listAllResourceTemplates()).rejects.toThrow();
expect(connected.getMalformedListItems()).toEqual([]);
});
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
// to stop and would page (and accumulate) without bound. The bound is the
// same `LIST_MAX_PAGES` the SDK aggregate is configured with.
const server = await startMalformedServer({
resourceTemplates: Array.from(
{ length: LIST_MAX_PAGES + 40 },
(_, i) => ({
items: [PHP_EMPTY_ANNOTATIONS],
nextCursor: `cursor-${i}`,
}),
),
});
stopServer = server.stop;
const connected = await connectTo(server.url);
// Truncation is not salvage: with entries still unfetched, returning what
// we have would present a partial list as complete, so the original
// validation error stands.
await expect(connected.listAllResourceTemplates()).rejects.toThrow();
expect(connected.getMalformedListItems()).toEqual([]);
const listCalls = server.calls.filter(
(method) => method === "resources/templates/list",
);
// One strict aggregate attempt (rejected on page one), then a salvage walk
// bounded at exactly the cap. Without the bound this runs to 104 pages —
// and against a truly endless server, forever.
expect(listCalls).toHaveLength(1 + LIST_MAX_PAGES);
});
it("stops the exclusion scan at the page cap too", async () => {
// The SEP-2243 scan walks pages of its own, outside the SDK aggregate's
// cap, so it needs the same bound. Driven directly because a non-converging
// list trips `listAllTools`'s salvage walk first.
const server = await startMalformedServer(
{
tools: Array.from({ length: LIST_MAX_PAGES + 40 }, (_, i) => ({
items: [{ name: `tool_${i}`, inputSchema: { type: "object" } }],
nextCursor: `cursor-${i}`,
})),
},
true,
);
stopServer = server.stop;
const connected = await connectTo(server.url, "modern");
await expect(connected.refreshExcludedTools()).rejects.toThrow(
/exceeded 64 pages/,
);
expect(
server.calls.filter((method) => method === "tools/list"),
).toHaveLength(LIST_MAX_PAGES);
});
it("marks the scan's refused response when its own fallback fails", async () => {
// `tools` is not a list at all, so the scan's lenient re-fetch cannot
// explain the rejection and rethrows. `listAllTools` catches a failing scan
// by design (it must never fail the tools list), so nothing downstream
// would mark the response the client demonstrably refused — it would render
// as a clean exchange.
const server = await startMalformedServer(
{ tools: [{ items: "not-a-list" }] },
true,
);
stopServer = server.stop;
const connected = await connectTo(server.url, "modern");
const log = new MessageLogState(connected);
await expect(connected.refreshExcludedTools()).rejects.toThrow();
const marked = log
.getMessages()
.filter(
(entry) =>
"method" in entry.message &&
entry.message.method === "tools/list" &&
entry.clientError !== undefined,
);
// The mark carries the ORIGINAL decode error: the fallback's own failure is
// a different event from the response being marked.
expect(marked).toHaveLength(1);
expect(marked[0]?.clientError).toBeTruthy();
log.destroy();
});
});

147
core/mcp/inspectorClient.ts
View file

@ -172,8 +172,10 @@ import { AjvJsonSchemaValidator } from "@modelcontextprotocol/client/validators/
import { z } from "zod/v4";
import { validateToolOutput } from "./toolOutputValidation.js";
import {
LIST_MAX_PAGES,
ModernResultEnvelopeSchema,
isSalvageableRejection,
listPaginationExceeded,
toolItemSchemaForEra,
nextCursorOf,
lenientListPageSchema,
@ -688,7 +690,13 @@ export class InspectorClient extends InspectorClientEventTarget {
capabilities?: ClientCapabilities;
versionNegotiation?: VersionNegotiationOptions;
inputRequired?: InputRequiredOptions;
listMaxPages?: number;
} = {
// Bound the SDK's auto-aggregate with the SAME constant the #1909 salvage
// re-walks use, so the strict path and its fallback cannot drift apart.
// This is the SDK's own default today; setting it explicitly is what makes
// the shared value a fact rather than a copied guess.
listMaxPages: LIST_MAX_PAGES,
// Per-server protocol era (SEP §7.8), threaded from config via
// `eraToVersionNegotiation` and defaulted to `{ mode: "legacy" }` in the
// constructor. "legacy" keeps the wire byte-identical to a 2025 client;
@ -3442,8 +3450,15 @@ export class InspectorClient extends InspectorClientEventTarget {
const malformed: MalformedListItem[] = [];
const seenCursors = new Set<string>();
let cursor: string | undefined;
let pages = 0;
try {
do {
// Bounded like the SDK aggregate this stands in for: a server emitting
// endlessly UNIQUE cursors never trips the repeated-cursor guard below,
// so without this the walk would run forever. Aborting surfaces the
// original validation error rather than a truncated list.
if (pages >= LIST_MAX_PAGES) throw listPaginationExceeded(method);
pages += 1;
const effectiveMeta = this.mergeMeta(metadata);
const params = {
...(effectiveMeta ? { _meta: effectiveMeta } : {}),
@ -3569,7 +3584,14 @@ export class InspectorClient extends InspectorClientEventTarget {
if (this.excludesInvalidXMcpHeaderTools()) {
const seenCursors = new Set<string>();
let cursor: string | undefined;
let pages = 0;
do {
// Same bound as `salvageList`, for the same reason: this walk is also
// outside the SDK aggregate's `listMaxPages` cap. Throwing (rather than
// committing a partial set) keeps the caller's "the excluded list may
// be incomplete" warning the honest description of what happened.
if (pages >= LIST_MAX_PAGES) throw listPaginationExceeded("tools/list");
pages += 1;
const page = await this.listToolsForScan(cursor, metadata);
malformed.push(
...page.malformed.map((entry) => ({
@ -3641,53 +3663,90 @@ export class InspectorClient extends InspectorClientEventTarget {
// client refused the result.
const rejectedResponseId =
this.lastAnsweredRequestByMethod.get("tools/list");
const effectiveMeta = this.mergeMeta(metadata);
const params = {
...(effectiveMeta ? { _meta: effectiveMeta } : {}),
// 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),
pageSchema,
)
: await this.client!.request(
{ method: "tools/list", params },
pageSchema,
this.getRequestOptions(metadata?.progressToken),
try {
const effectiveMeta = this.mergeMeta(metadata);
const params = {
...(effectiveMeta ? { _meta: effectiveMeta } : {}),
// 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),
pageSchema,
)
: await this.client!.request(
{ method: "tools/list", params },
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`).
if (items === undefined) throw err;
const { valid, malformed } = salvageListItems({
method: "tools/list",
items,
schema: toolItemSchemaForEra(this.isModernEra()),
});
if (rejectedResponseId !== undefined && malformed.length > 0) {
this.dispatchTypedEvent("responseRejected", {
id: rejectedResponseId,
reason: summarizeMalformed(malformed),
});
}
const nextCursor = nextCursorOf(page);
return {
tools: valid,
...(nextCursor !== undefined && { nextCursor }),
malformed,
};
} catch (fallbackErr) {
// The fallback could not explain the rejection — the re-fetch itself
// failed, or the page carried a violation that is not per-item (a bad
// modern envelope, a `tools` that is not a list).
//
// `salvageList` can simply rethrow here, because ITS caller
// (`ManagedListState.refresh`) marks the Protocol entry from the error
// it receives. This walk has no such backstop: `listAllTools` catches
// and LOGS a failing scan so it can never fail the tools list, so an
// unmarked rethrow leaves the response the client demonstrably refused
// (`isSalvageableRejection` above proves it was an `InvalidResult`)
// rendering as a clean exchange — the exact lie #1953 removes and the
// one this method's own capture above exists to prevent. So mark it
// here, with the ORIGINAL decode error as the reason: the fallback's
// own failure is a different event, and the response being marked is
// the one `err` was raised for.
if (rejectedResponseId !== undefined) {
this.dispatchTypedEvent("responseRejected", {
id: rejectedResponseId,
reason: err instanceof Error ? err.message : String(err),
});
}
if (fallbackErr !== err) {
this.logger.warn(
{ err: fallbackErr },
"Excluded-tools salvage re-fetch failed; surfacing the original list error",
);
// 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
) {
}
// Restore the correlation the re-fetch moved, so a later mark for this
// method does not land on a lenient page that succeeded.
this.restoreResponseCorrelation("tools/list", rejectedResponseId);
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`).
if (items === undefined) throw err;
const { valid, malformed } = salvageListItems({
method: "tools/list",
items,
schema: toolItemSchemaForEra(this.isModernEra()),
});
if (rejectedResponseId !== undefined && malformed.length > 0) {
this.dispatchTypedEvent("responseRejected", {
id: rejectedResponseId,
reason: summarizeMalformed(malformed),
});
}
const nextCursor = nextCursorOf(page);
return {
tools: valid,
...(nextCursor !== undefined && { nextCursor }),
malformed,
};
}
}

35
core/mcp/listSalvage.ts
View file

@ -46,6 +46,41 @@ export interface MalformedListItem {
reason: string;
}
/**
* Hard cap on the pages a salvage / scan re-walk will fetch.
*
* The SDK's auto-aggregating `list*()` is capped by
* `ClientOptions.listMaxPages` and THROWS when it is hit, so a partial
* aggregate is never presented as a complete one. The fallback walks in this
* module go around that aggregate, so without their own bound a server whose
* `nextCursor` never converges — but never repeats, so the non-converging guard
* never fires — would keep the walk issuing requests and accumulating entries
* forever. That is reachable from any server the user points the Inspector at,
* which is the whole population here.
*
* The value is passed to the SDK client as `listMaxPages` (see
* `InspectorClient`'s constructor) rather than merely matching its default, so
* the strict path and the fallback are bounded by the SAME number by
* construction and cannot drift if the SDK changes its default.
*/
export const LIST_MAX_PAGES = 64;
/**
* The error a walk raises when it hits {@link LIST_MAX_PAGES}.
*
* Both callers turn this into "the strict error stands": hitting the cap means
* there are more entries we did not fetch, so returning what we have would
* present a TRUNCATED list as a complete one — the same silent-truncation
* failure {@link rawItemsOf} refuses for a non-array page. That is why this
* aborts rather than `break`ing like the repeated-cursor guard, which stops on
* a page whose entries were all seen already and so loses nothing.
*/
export function listPaginationExceeded(method: string): Error {
return new Error(
`Salvage walk for ${method} exceeded ${LIST_MAX_PAGES} pages without the server's pagination converging`,
);
}
/**
* Whether a failed fetch is the CLIENT refusing a response it received, rather
* than the request never producing one.