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

fix: address Copilot round 2 on the list-salvage path (#1909)

Browse source 
All four findings taken (one visible, three in the suppressed block), plus the
`excludedTools` sibling leak the round-1 reply had deferred:

- `rawItemsOf` read a missing or non-array list member as an empty page, so a
  top-level violation (`{ tools: "invalid" }`) on a later page was discarded
  and the call returned a silently truncated list. It now returns `undefined`
  for "not a list at all", and the walk rethrows the original strict error.

- A failure of the re-walk ITSELF (a malformed `nextCursor`, a connection lost
  mid-walk) escaped in place of the original error, whose response is the one
  `rejectedResponseId` was captured for. The walk is now wrapped: the original
  surfaces, the secondary is logged so it stays diagnosable.

- The exclusion scan's lenient fallback discarded its malformed entries despite
  the comment claiming they were reported. Observable when the aggregate is
  served from cache: the salvage never runs, so nothing else reports them. The
  scan now returns them and `refreshExcludedTools` reports them — only ever
  adding, so a clean scan can't clear what the salvage path just wrote.

- An empty-string cursor is a valid cursor, and `cursor ? {cursor} : {}` dropped
  it, re-requesting page one and duplicating its entries until the
  repeated-cursor guard fired. Both walks now test `!== undefined`.

- `excludedTools` had the same session leak fixed for `malformedListItems` last
  round: cleared only on an explicit `disconnect()`, so the `onerror`-with-no-
  `onclose` route carried the previous server's set into the next connection.
  Cleared on the start-clean path too — safe because every `listAllTools`
  recomputes it, so this can only drop a stale set.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-15 17:52:36 -04:00
commit 1889e04135
5 changed files with 229 additions and 62 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -66,14 +66,16 @@ describe("LenientListPageSchema", () => {
).toBe(false);
});
it("reads an absent or non-array items key as no entries", () => {
expect(rawItemsOf({}, "tools")).toEqual([]);
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: "not-a-list" }) as never,
"tools",
),
rawItemsOf(LenientListPageSchema.parse({ tools: [] }), "tools"),
).toEqual([]);
expect(rawItemsOf({}, "tools")).toBeUndefined();
expect(
rawItemsOf(LenientListPageSchema.parse({ tools: "not-a-list" }), "tools"),
).toBeUndefined();
});
});

22
clients/web/src/test/integration/mcp/inspectorClient-excluded-tools.test.ts
View file

@ -120,6 +120,28 @@ describe("excluded tools (SEP-2243 x-mcp-header)", () => {
expect(excluded.map((x) => x.tool.name)).toEqual(["invalid_header_tool"]);
});
it("does not carry an excluded set into the next connection (#1909)", async () => {
// `resetSessionState()` is the start-clean path, and it is the ONLY one the
// route out that tears down nothing (`onerror` with no `onclose`) reaches.
// A set left behind names tools of the PREVIOUS server — and would persist
// indefinitely against a server that never lists tools again.
const started = await start({});
const connected = await connect(started.url, "modern");
await connected.listAllTools();
expect(connected.getExcludedTools()).toHaveLength(1);
const events: number[] = [];
connected.addEventListener("excludedToolsChange", (e) => {
events.push(e.detail.length);
});
// Reconnecting runs the start-clean reset; assert before anything re-lists.
await connected.disconnect();
await connected.connect();
expect(connected.getExcludedTools()).toEqual([]);
expect(events).toContain(0);
});
it("reports no excluded tools on a legacy connection (no exclusion there)", async () => {
const started = await start(undefined);
const connected = await connect(started.url, "legacy");

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

@ -51,6 +51,8 @@ function startMalformedServer(
url: string;
stop: () => Promise<void>;
calls: string[];
/** Every `cursor` param the server was sent, in order. */
cursors: (string | undefined)[];
/** Swap what the server serves next, to model a server that got fixed. */
setPages: (next: {
resourceTemplates?: ListPage[];
@ -60,10 +62,21 @@ function startMalformedServer(
const calls: string[] = [];
let pages = initialPages;
const cursors: (string | undefined)[] = [];
// Cursor string -> the page index it addresses, learned as pages are served.
// Modeling it this way (rather than parsing the cursor as a number) is what
// makes an EMPTY-STRING cursor meaningful: a client that drops it sends no
// cursor at all, lands back on page one, and duplicates its entries.
const cursorToIndex = new Map<string, number>();
const pageFor = (key: "resourceTemplates" | "tools", cursor?: string) => {
cursors.push(cursor);
const configured = pages[key] ?? [];
const index = cursor === undefined ? 0 : Number(cursor);
return configured[index] ?? { items: [] };
const index = cursor === undefined ? 0 : (cursorToIndex.get(cursor) ?? 0);
const page = configured[index] ?? { items: [] };
if (page.nextCursor !== undefined) {
cursorToIndex.set(page.nextCursor, index + 1);
}
return page;
};
const handler = async (req: IncomingMessage, res: ServerResponse) => {
@ -160,6 +173,7 @@ function startMalformedServer(
resolve({
url: `http://127.0.0.1:${port}/mcp`,
calls,
cursors,
setPages: (next) => {
pages = next;
},
@ -414,6 +428,62 @@ describe("InspectorClient list salvage (#1909)", () => {
]);
});
it("surfaces the ORIGINAL error when the re-walk itself fails", async () => {
// A malformed `nextCursor` fails the lenient page schema too, so the walk
// throws its own error. The original is what the caller's list actually
// failed on — and what `rejectedResponseId` was captured for — so that is
// the one that must surface.
const server = await startMalformedServer({
resourceTemplates: [
{
items: [PHP_EMPTY_ANNOTATIONS, VALID_TEMPLATE],
nextCursor: 42 as unknown as string,
},
],
});
stopServer = server.stop;
const connected = await connectTo(server.url);
await expect(connected.listAllResourceTemplates()).rejects.toThrow(
/annotations/,
);
expect(connected.getMalformedListItems()).toEqual([]);
});
it("keeps the strict error when a page is not a list at all", async () => {
// `{ resourceTemplates: "nope" }` is a top-level violation the per-item
// pass cannot explain; treating it as an empty page would return a
// silently truncated list and discard the error that was right about it.
const server = await startMalformedServer({
resourceTemplates: [{ items: "nope" as unknown as unknown[] }],
});
stopServer = server.stop;
const connected = await connectTo(server.url);
await expect(connected.listAllResourceTemplates()).rejects.toThrow();
expect(connected.getMalformedListItems()).toEqual([]);
});
it("sends an empty-string cursor rather than re-fetching page one", async () => {
// "" is a valid cursor. A truthiness check would drop it, re-request page
// one, and duplicate its entries until the repeated-cursor guard fired.
const server = await startMalformedServer({
resourceTemplates: [
{ items: [PHP_EMPTY_ANNOTATIONS, VALID_TEMPLATE], nextCursor: "" },
{ items: [{ ...VALID_TEMPLATE, name: "page_two" }] },
],
});
stopServer = server.stop;
const connected = await connectTo(server.url);
const { resourceTemplates } = await connected.listAllResourceTemplates();
expect(resourceTemplates.map((t) => t.name)).toEqual([
"full_annotations",
"page_two",
]);
expect(server.cursors).toContain("");
});
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

163
core/mcp/inspectorClient.ts
View file

@ -1616,15 +1616,23 @@ export class InspectorClient extends InspectorClientEventTarget {
// (#1953).
this.outboundRequestMethods.clear();
this.lastAnsweredRequestByMethod.clear();
// Per-session for the same reason: a report names entries of the PREVIOUS
// Per-session for the same reason: both name entries of the PREVIOUS
// server's list. Cleared here as well as in `disconnect()` because the
// route out that tears down nothing (`onerror` with no `onclose`) would
// otherwise carry it into the next connection — indefinitely, if that
// server never lists the method again (#1909).
// otherwise carry them into the next connection — indefinitely, if that
// server never lists the method again. Safe to clear unconditionally at
// connect: `listAllTools` recomputes the excluded set on every aggregate
// and the salvage report on every list, so this can only drop a stale set,
// never a live one (#1909; the `excludedTools` half is the same latent bug
// from #1632, fixed here rather than left as a known sibling).
if (this.malformedListItems.size > 0) {
this.malformedListItems.clear();
this.dispatchTypedEvent("malformedListItemsChange", []);
}
if (this.excludedTools.length > 0) {
this.excludedTools = [];
this.dispatchTypedEvent("excludedToolsChange", []);
}
for (const [, controller] of this.taskInputAbortControllers) {
controller.abort(new Error("Connection ended"));
}
@ -3381,50 +3389,75 @@ export class InspectorClient extends InspectorClientEventTarget {
const malformed: MalformedListItem[] = [];
const seenCursors = new Set<string>();
let cursor: string | undefined;
do {
const effectiveMeta = this.mergeMeta(metadata);
const params = {
...(effectiveMeta ? { _meta: effectiveMeta } : {}),
...(cursor ? { cursor } : {}),
};
// The modern (2026-07-28) codec validates a result against the SPEC
// schema for the method before the caller's schema is consulted, so a
// lenient `client.request` is rejected exactly as the strict one was —
// there is no salvage without going around it. The raw-wire channel that
// exists for the modern `tasks/*` methods does precisely that: the frame
// goes straight through the transport (still logged for the Protocol /
// Network tabs) and only the caller's schema is applied. Legacy keeps the
// ordinary SDK path, which honors request options and `_meta` for us.
const page = await this.invokeMcpClient(
() =>
this.isModernEra()
? this.rawWireRequest(
method,
this.withModernEnvelope(params),
LenientListPageSchema,
)
: this.client!.request(
{ method, params },
LenientListPageSchema,
this.getRequestOptions(metadata?.progressToken),
),
{ method },
);
const salvaged = salvageListItems({
method,
items: rawItemsOf(page, itemsKey),
schema: itemSchema,
startIndex: valid.length + malformed.length,
});
valid.push(...salvaged.valid);
malformed.push(...salvaged.malformed);
cursor = page.nextCursor;
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;
seenCursors.add(cursor);
try {
do {
const effectiveMeta = this.mergeMeta(metadata);
const params = {
...(effectiveMeta ? { _meta: effectiveMeta } : {}),
// `!== undefined`, not truthiness: "" is a valid cursor, and dropping it
// would re-request page one and duplicate its entries until the
// repeated-cursor guard below stopped the walk.
...(cursor !== undefined && { cursor }),
};
// The modern (2026-07-28) codec validates a result against the SPEC
// schema for the method before the caller's schema is consulted, so a
// lenient `client.request` is rejected exactly as the strict one was —
// there is no salvage without going around it. The raw-wire channel that
// exists for the modern `tasks/*` methods does precisely that: the frame
// goes straight through the transport (still logged for the Protocol /
// Network tabs) and only the caller's schema is applied. Legacy keeps the
// ordinary SDK path, which honors request options and `_meta` for us.
const page = await this.invokeMcpClient(
() =>
this.isModernEra()
? this.rawWireRequest(
method,
this.withModernEnvelope(params),
LenientListPageSchema,
)
: this.client!.request(
{ method, params },
LenientListPageSchema,
this.getRequestOptions(metadata?.progressToken),
),
{ method },
);
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
// a silently truncated list and discard the strict error that was right
// about it.
if (items === undefined) throw err;
const salvaged = salvageListItems({
method,
items,
schema: itemSchema,
startIndex: valid.length + malformed.length,
});
valid.push(...salvaged.valid);
malformed.push(...salvaged.malformed);
cursor = page.nextCursor;
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;
seenCursors.add(cursor);
}
} while (cursor !== undefined);
} catch (walkErr) {
// The re-walk itself failed — its own decode rejection (a malformed
// `nextCursor`, an unsalvageable page), or the connection going away
// mid-walk. Either way the ORIGINAL error is the one to surface: it is
// what the caller's list actually failed on, and `rejectedResponseId`
// above was captured for that response, not this one. The secondary
// failure is logged so it stays diagnosable rather than vanishing.
if (walkErr !== err) {
this.logger.warn(
{ err: walkErr, method },
"Salvage re-walk failed; surfacing the original list error",
);
}
} while (cursor !== undefined);
throw err;
}
// Nothing per-item was wrong, so the strict rejection was about something
// this fallback cannot explain — surface it rather than hide it.
@ -3462,6 +3495,7 @@ export class InspectorClient extends InspectorClientEventTarget {
metadata?: Record<string, string>,
): Promise<ExcludedTool[]> {
const excluded: ExcludedTool[] = [];
const malformed: MalformedListItem[] = [];
// 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()) {
@ -3469,6 +3503,13 @@ export class InspectorClient extends InspectorClientEventTarget {
let cursor: string | undefined;
do {
const page = await this.listToolsForScan(cursor, metadata);
malformed.push(
...page.malformed.map((entry) => ({
...entry,
// Index against the aggregate this walk is building, not the page.
index: entry.index + excluded.length + page.tools.length,
})),
);
for (const tool of page.tools) {
const scan = scanXMcpHeaderDeclarations(tool.inputSchema);
if (!scan.valid) excluded.push({ tool, reason: scan.reason });
@ -3481,6 +3522,15 @@ 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) {
this.setMalformedListItems("tools/list", malformed);
}
this.excludedTools = excluded;
this.dispatchTypedEvent("excludedToolsChange", excluded);
return excluded;
@ -3501,15 +3551,21 @@ export class InspectorClient extends InspectorClientEventTarget {
private async listToolsForScan(
cursor: string | undefined,
metadata: Record<string, string> | undefined,
): Promise<{ tools: Tool[]; nextCursor?: string }> {
): Promise<{
tools: Tool[];
nextCursor?: string;
malformed: MalformedListItem[];
}> {
try {
return await this.listTools(cursor, metadata);
const page = await this.listTools(cursor, metadata);
return { ...page, malformed: [] };
} catch (err) {
if (!isClientDecodeRejection(err)) throw err;
const effectiveMeta = this.mergeMeta(metadata);
const params = {
...(effectiveMeta ? { _meta: effectiveMeta } : {}),
...(cursor ? { cursor } : {}),
// See `salvageList`: "" is a valid cursor.
...(cursor !== undefined && { cursor }),
};
const page = this.isModernEra()
? await this.rawWireRequest(
@ -3522,14 +3578,19 @@ export class InspectorClient extends InspectorClientEventTarget {
LenientListPageSchema,
this.getRequestOptions(metadata?.progressToken),
);
const { valid } = salvageListItems({
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: rawItemsOf(page, "tools"),
items,
schema: ToolSchema,
});
return {
tools: valid,
...(page.nextCursor !== undefined && { nextCursor: page.nextCursor }),
malformed,
};
}
}

18
core/mcp/listSalvage.ts
View file

@ -93,10 +93,22 @@ export const LenientListPageSchema = z.looseObject({
export type LenientListPage = z.infer<typeof LenientListPageSchema>;
/** Read the primitive array out of a leniently-parsed page. */
export function rawItemsOf(page: LenientListPage, itemsKey: string): unknown[] {
/**
* Read the primitive array out of a leniently-parsed page.
*
* Returns `undefined` — NOT an empty array — when the member is missing or is
* not an array, because those are different facts. An empty page is a normal
* server answer; a page whose `tools` is the string `"invalid"` is a top-level
* schema violation this fallback cannot explain, and reading it as "no entries"
* would let the caller return a silently truncated list while the strict error
* that was correct about it is discarded.
*/
export function rawItemsOf(
page: LenientListPage,
itemsKey: string,
): unknown[] | undefined {
const value = (page as Record<string, unknown>)[itemsKey];
return Array.isArray(value) ? value : [];
return Array.isArray(value) ? value : undefined;
}
/**