fix: address Copilot round 3 on the list-salvage path (#1909)
The visible review was clean; all five findings were in the suppressed block, two of them real defects. - The exclusion scan's aggregate offset was wrong twice over: `excluded.length` counts only invalid-header tools, and on page one it also folded in the current page's valid count. A reported index therefore pointed at a position that doesn't exist — worst for an unlabeled entry, whose index is all the user has. Replaced with a raw-entry offset advanced by each page's valid + malformed counts. - The scan's own refused response was never marked. It is a distinct exchange from the aggregate's, and the lenient re-fetch moves the method correlation off it, so its Protocol entry rendered as a clean success the client had in fact refused. Marked with the captured id, as `salvageList` already does. - Three double casts in the test fixture are gone: `ListPage` now models raw wire data (`items: unknown`, `nextCursor?: unknown`), which is what a server built to emit spec-forbidden shapes actually serves. Declaring the validated type and casting malformed values through it asserted the opposite of what these tests exist to check. The cursor map narrows to `string` at the one place a cursor addresses a page. 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:
parent
1889e04135
commit
aea3f6d4c2
1 changed files with 110 additions and 10 deletions
|
|
@ -26,9 +26,18 @@ import { eraToVersionNegotiation } from "@inspector/core/mcp/types.js";
|
|||
|
||||
const MODERN_VERSION = "2026-07-28";
|
||||
|
||||
/**
|
||||
* One page exactly as the server will put it on the wire.
|
||||
*
|
||||
* Both fields are `unknown` on purpose: this server exists to serve shapes the
|
||||
* spec forbids (a non-array `items`, a numeric `nextCursor`), so the fixture
|
||||
* models raw wire data rather than the validated type. Declaring the spec type
|
||||
* and casting malformed values through it at each call site would assert the
|
||||
* opposite of what these tests are for.
|
||||
*/
|
||||
interface ListPage {
|
||||
items: unknown[];
|
||||
nextCursor?: string;
|
||||
items: unknown;
|
||||
nextCursor?: unknown;
|
||||
}
|
||||
|
||||
function jsonRpcResult(id: unknown, result: unknown) {
|
||||
|
|
@ -73,7 +82,9 @@ function startMalformedServer(
|
|||
const configured = pages[key] ?? [];
|
||||
const index = cursor === undefined ? 0 : (cursorToIndex.get(cursor) ?? 0);
|
||||
const page = configured[index] ?? { items: [] };
|
||||
if (page.nextCursor !== undefined) {
|
||||
// Only a string cursor can address a page; a malformed one is served
|
||||
// verbatim so the client can reject it, but it maps to nothing.
|
||||
if (typeof page.nextCursor === "string") {
|
||||
cursorToIndex.set(page.nextCursor, index + 1);
|
||||
}
|
||||
return page;
|
||||
|
|
@ -437,7 +448,7 @@ describe("InspectorClient list salvage (#1909)", () => {
|
|||
resourceTemplates: [
|
||||
{
|
||||
items: [PHP_EMPTY_ANNOTATIONS, VALID_TEMPLATE],
|
||||
nextCursor: 42 as unknown as string,
|
||||
nextCursor: 42,
|
||||
},
|
||||
],
|
||||
});
|
||||
|
|
@ -455,7 +466,7 @@ describe("InspectorClient list salvage (#1909)", () => {
|
|||
// 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[] }],
|
||||
resourceTemplates: [{ items: "nope" }],
|
||||
});
|
||||
stopServer = server.stop;
|
||||
const connected = await connectTo(server.url);
|
||||
|
|
@ -484,14 +495,82 @@ describe("InspectorClient list salvage (#1909)", () => {
|
|||
expect(server.cursors).toContain("");
|
||||
});
|
||||
|
||||
it("marks the exclusion scan's own refused response too", async () => {
|
||||
// The scan is a separate exchange from the aggregate's, and its re-fetch
|
||||
// moves the method correlation off it — so without marking it here its
|
||||
// Protocol entry renders as a clean success the client actually refused.
|
||||
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");
|
||||
const log = new MessageLogState(connected);
|
||||
|
||||
await connected.listAllTools();
|
||||
|
||||
const marked = log
|
||||
.getMessages()
|
||||
.filter(
|
||||
(entry) =>
|
||||
"method" in entry.message &&
|
||||
entry.message.method === "tools/list" &&
|
||||
entry.clientError !== undefined,
|
||||
);
|
||||
// Two refused responses: the aggregate's and the scan's.
|
||||
expect(marked).toHaveLength(2);
|
||||
for (const entry of marked) {
|
||||
expect(entry.clientError).toContain("Dropped 1 malformed entry");
|
||||
}
|
||||
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.
|
||||
const server = await startMalformedServer(
|
||||
{
|
||||
tools: [
|
||||
{
|
||||
items: [
|
||||
{ name: "one", inputSchema: { type: "object" } },
|
||||
{ name: "two", inputSchema: { type: "object" } },
|
||||
],
|
||||
nextCursor: "p2",
|
||||
},
|
||||
// Page two, entry one (index 2 of the whole list) is unlabelable.
|
||||
{ items: [42, { name: "four", inputSchema: { type: "object" } }] },
|
||||
],
|
||||
},
|
||||
true,
|
||||
);
|
||||
stopServer = server.stop;
|
||||
const connected = await connectTo(server.url, "modern");
|
||||
|
||||
const { tools } = await connected.listAllTools();
|
||||
expect(tools.map((t) => t.name)).toEqual(["one", "two", "four"]);
|
||||
const reported = connected.getMalformedListItems();
|
||||
expect(reported).toHaveLength(1);
|
||||
expect(reported[0]?.index).toBe(2);
|
||||
expect(reported[0]?.label).toBeUndefined();
|
||||
});
|
||||
|
||||
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
|
||||
// survive rather than being swallowed into a partial list.
|
||||
const server = await startMalformedServer({
|
||||
resourceTemplates: [
|
||||
{ items: [VALID_TEMPLATE], nextCursor: 42 as unknown as string },
|
||||
],
|
||||
resourceTemplates: [{ items: [VALID_TEMPLATE], nextCursor: 42 }],
|
||||
});
|
||||
stopServer = server.stop;
|
||||
const connected = await connectTo(server.url);
|
||||
|
|
|
|||
Loading…
Reference in a new issue