diff --git a/clients/web/src/test/core/mcp/listSalvage.test.ts b/clients/web/src/test/core/mcp/listSalvage.test.ts index a117403b..f547eee0 100644 --- a/clients/web/src/test/core/mcp/listSalvage.test.ts +++ b/clients/web/src/test/core/mcp/listSalvage.test.ts @@ -132,6 +132,20 @@ describe("ModernResultEnvelopeSchema", () => { ).toBe(true); }); + it("requires ttlMs to be a non-negative integer, as the codec does", () => { + // A looser `z.number()` would accept `-1` / `0.5`, so a response with a bad + // envelope AND one bad entry would salvage and hide the envelope violation. + for (const ttlMs of [-1, 0.5]) { + expect( + ModernResultEnvelopeSchema.safeParse({ + resultType: "complete", + ttlMs, + cacheScope: "public", + }).success, + ).toBe(false); + } + }); + it("rejects a modern result missing the envelope the codec would enforce", () => { // The raw-wire salvage path goes around that codec, so a response missing // its cache hints AND carrying a bad entry must not salvage. diff --git a/clients/web/src/test/integration/mcp/listSalvage-era.test.ts b/clients/web/src/test/integration/mcp/listSalvage-era.test.ts new file mode 100644 index 00000000..723b47da --- /dev/null +++ b/clients/web/src/test/integration/mcp/listSalvage-era.test.ts @@ -0,0 +1,214 @@ +import { describe, it, expect, afterEach } from "vitest"; +import { + createServer, + type IncomingMessage, + type Server, + type ServerResponse, +} from "node:http"; +import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; +import { createTransportNode } from "@inspector/core/mcp/node/transport.js"; +import { eraToVersionNegotiation } from "@inspector/core/mcp/types.js"; +import { ToolSchema } from "@modelcontextprotocol/core"; +import { toolItemSchemaForEra } from "@inspector/core/mcp/listSalvage.js"; + +/** + * The era item contract the salvage fallback validates tools against (#1909). + * + * `toolItemSchemaForEra` replicates two rules that live in the SDK's internal + * per-era wire schemas, which are not exported. That is a real cost, and these + * tests are what makes it safe to pay: each one asserts the STRICT path's + * observed behavior on a real connection, so if a future SDK closes or widens a + * divergence, the test fails and the replicated rule gets revisited instead of + * silently rotting. + * + * The failure being prevented: the fallback keeping an entry the strict path + * refused, which the user would then see — and could call — on a connection + * whose era says it isn't valid. + */ + +const ARRAY_OUTPUT_TOOL = { + name: "array_output", + inputSchema: { type: "object" }, + outputSchema: { type: "array" }, +}; + +const EXECUTION_TOOL = { + name: "with_execution", + inputSchema: { type: "object" }, + execution: { taskSupport: "optional" }, +}; + +function startToolsServer( + tools: unknown[], + modern: boolean, +): Promise<{ url: string; stop: () => Promise }> { + const handler = async (req: IncomingMessage, res: ServerResponse) => { + const chunks: Buffer[] = []; + for await (const chunk of req) chunks.push(chunk as Buffer); + const raw = Buffer.concat(chunks).toString(); + if (raw.length === 0) { + res.writeHead(405).end(); + return; + } + const body = JSON.parse(raw) as { id?: unknown; method: string }; + if (body.id === undefined) { + res.writeHead(202).end(); + return; + } + const envelope = modern + ? { resultType: "complete", ttlMs: 0, cacheScope: "public" } + : {}; + const send = (result: object) => { + res.writeHead(200, { "Content-Type": "application/json" }); + res.end( + JSON.stringify({ + jsonrpc: "2.0", + id: body.id, + result: { ...envelope, ...result }, + }), + ); + }; + if (body.method === "initialize") { + send({ + protocolVersion: modern ? "2026-07-28" : "2025-06-18", + capabilities: { tools: {} }, + serverInfo: { name: "era-contract-server", version: "1.0.0" }, + }); + return; + } + if (body.method === "server/discover") { + send({ + supportedVersions: ["2026-07-28"], + capabilities: { tools: {} }, + serverInfo: { name: "era-contract-server", version: "1.0.0" }, + }); + return; + } + if (body.method === "tools/list") { + send({ tools }); + return; + } + send({}); + }; + + const server: Server = createServer((req, res) => { + void handler(req, res); + }); + return new Promise((resolve) => { + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + /* v8 ignore next -- listen() on a fresh server always yields an AddressInfo */ + const port = typeof address === "object" && address ? address.port : 0; + resolve({ + url: `http://127.0.0.1:${port}/mcp`, + stop: () => + new Promise((done) => { + server.close(() => done()); + }), + }); + }); + }); +} + +describe("salvage era item contract (#1909)", () => { + let client: InspectorClient | null = null; + let stop: (() => Promise) | null = null; + + afterEach(async () => { + try { + await client?.disconnect(); + } catch { + // Teardown only. + } + client = null; + await stop?.(); + stop = null; + }); + + async function connect(url: string, era: "legacy" | "modern") { + client = new InspectorClient( + { type: "streamable-http", url }, + { + environment: { transport: createTransportNode }, + versionNegotiation: eraToVersionNegotiation(era), + }, + ); + await client.connect(); + return client; + } + + it("legacy: the era rule is what rejects a non-object outputSchema (pins it)", async () => { + // The rule lives in the SDK's high-level aggregate (the era codec), not in + // the neutral schema the single-page call uses — which is precisely the + // divergence `toolItemSchemaForEra` exists to close. + // + // This tool is the page's ONLY problem, so the observable outcome is + // entirely the SDK's rule talking: the strict aggregate refuses the page, + // the fallback runs, and our contract agrees the tool is not era-valid. If + // a future SDK accepts it, the aggregate succeeds instead and this fails — + // which is the signal to revisit the replicated rule. + const server = await startToolsServer([ARRAY_OUTPUT_TOOL], false); + stop = server.stop; + const connected = await connect(server.url, "legacy"); + + const { tools } = await connected.listAllTools(); + expect(tools).toEqual([]); + expect( + connected.getMalformedListItems().map((entry) => entry.label), + ).toEqual(["array_output"]); + }); + + it("modern: the era rule accepts a tool but strips execution (pins it)", async () => { + // The mirror image: here the strict aggregate SUCCEEDS, so no salvage runs + // and what comes back is the SDK's own normalization. If it ever stops + // stripping, `execution` reappears here and this fails. + const server = await startToolsServer([EXECUTION_TOOL], true); + stop = server.stop; + const connected = await connect(server.url, "modern"); + + const { tools } = await connected.listAllTools(); + expect(tools).toHaveLength(1); + expect(tools[0]).not.toHaveProperty("execution"); + expect(connected.getMalformedListItems()).toEqual([]); + }); + + it("the neutral ToolSchema is looser than both, which is why the contract exists", () => { + expect(ToolSchema.safeParse(ARRAY_OUTPUT_TOOL).success).toBe(true); + const neutral = ToolSchema.safeParse(EXECUTION_TOOL); + expect(neutral.success).toBe(true); + expect(neutral.success && neutral.data).toHaveProperty("execution"); + }); + + it("the era contract matches the strict path on both divergences", () => { + // Legacy: rejected, exactly as the strict parse above rejected it. + expect( + toolItemSchemaForEra(false).safeParse(ARRAY_OUTPUT_TOOL).success, + ).toBe(false); + // Modern: accepted, with `execution` stripped, exactly as above. + const modern = toolItemSchemaForEra(true).safeParse(EXECUTION_TOOL); + expect(modern.success).toBe(true); + expect(modern.success && modern.data).not.toHaveProperty("execution"); + }); + + it("keeps only era-valid tools when salvaging beside a malformed one", async () => { + // The end-to-end shape of the defect: a malformed entry trips the + // fallback, and the array-output tool must NOT ride along on the strength + // of the neutral schema accepting it. + const server = await startToolsServer( + [ + { name: "ok_tool", inputSchema: { type: "object" } }, + ARRAY_OUTPUT_TOOL, + { name: "broken", inputSchema: "not-a-schema" }, + ], + false, + ); + stop = server.stop; + const connected = await connect(server.url, "legacy"); + + const { tools } = await connected.listAllTools(); + expect(tools.map((tool) => tool.name)).toEqual(["ok_tool"]); + expect( + connected.getMalformedListItems().map((entry) => entry.label), + ).toEqual(["array_output", "broken"]); + }); +}); diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 90a6c37d..94a1abd2 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -161,8 +161,8 @@ import { ListPromptsResultSchema, // Per-primitive item schemas — the salvage fallback validates a list's // entries one at a time with these when the whole-result parse rejects - // (#1909, see `listAllSalvaging`). - ToolSchema, + // (#1909, see `listAllSalvaging`). Tools go through `toolItemSchemaForEra` + // instead, since the neutral schema is looser than the negotiated era's. ResourceSchema, ResourceTemplateSchema, PromptSchema, @@ -174,6 +174,7 @@ import { validateToolOutput } from "./toolOutputValidation.js"; import { ModernResultEnvelopeSchema, isSalvageableRejection, + toolItemSchemaForEra, nextCursorOf, lenientListPageSchema, rawItemsOf, @@ -3190,7 +3191,9 @@ export class InspectorClient extends InspectorClientEventTarget { method: "tools/list", itemsKey: "tools", resultSchema: ListToolsResultSchema, - itemSchema: ToolSchema, + // Era-aware: the neutral `ToolSchema` is more permissive than the + // negotiated era's wire schema — see `toolItemSchemaForEra`. + itemSchema: toolItemSchemaForEra(this.isModernEra()), metadata: options?.metadata, finalize: (salvaged) => this.excludeInvalidXMcpHeaderTools(salvaged), aggregate: async () => [ @@ -3656,7 +3659,7 @@ export class InspectorClient extends InspectorClientEventTarget { const { valid, malformed } = salvageListItems({ method: "tools/list", items, - schema: ToolSchema, + schema: toolItemSchemaForEra(this.isModernEra()), }); if (rejectedResponseId !== undefined && malformed.length > 0) { this.dispatchTypedEvent("responseRejected", { diff --git a/core/mcp/listSalvage.ts b/core/mcp/listSalvage.ts index 37aeebbe..7c5ab1e4 100644 --- a/core/mcp/listSalvage.ts +++ b/core/mcp/listSalvage.ts @@ -25,6 +25,8 @@ import { z } from "zod/v4"; import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client"; +import { ToolSchema } from "@modelcontextprotocol/core"; +import type { Tool } from "@modelcontextprotocol/client"; /** * One entry the Inspector dropped from a list result because it failed the @@ -140,7 +142,10 @@ export function nextCursorOf(page: unknown): string | undefined { */ export const ModernResultEnvelopeSchema = z.looseObject({ resultType: z.literal("complete"), - ttlMs: z.number(), + // A non-negative INTEGER, matching the codec: `ttlMs: -1` or `0.5` is an + // envelope violation the strict path rejects, and accepting it here on the + // strength of an unrelated bad entry would hide it. + ttlMs: z.int().min(0), cacheScope: z.enum(["public", "private"]), }); @@ -261,3 +266,40 @@ export function summarizeMalformed(malformed: MalformedListItem[]): string { malformed.length === 1 ? "entry" : "entries" } — ${where}: ${first.reason}${rest}`; } + +/** + * The item contract for `tools/list` on a given era. + * + * The public `ToolSchema` is era-NEUTRAL, and the negotiated era's wire schema + * is stricter in ways the SDK does not export. Validating candidates against + * the neutral schema alone would let the fallback keep an entry the strict path + * refused — an entry the user would then see, and could call, on a connection + * whose era says it isn't valid. The two known divergences (verified against + * SDK 2.0.0 by `listSalvage-era.test.ts`, which fails if either changes): + * + * - **legacy (2025)** requires `outputSchema.type === "object"`; the neutral + * schema accepts any object, so a tool with `outputSchema: { type: "array" }` + * would survive salvage while the strict parse rejects the whole page. + * - **modern (2026)** deletes `execution`, and the strict path STRIPS it. Left + * in place, a salvaged tool would carry a field the era doesn't have, and the + * UI would render capability the connection can't honor. + * + * Replicating rules that live in the SDK's internals is a real cost, taken + * deliberately: the alternative is a fallback that is quietly more permissive + * than the path it stands in for. The tests pin each rule to observed SDK + * behavior, so a divergence that closes (or widens) fails loudly rather than + * rotting. + */ +export function toolItemSchemaForEra(isModern: boolean): z.ZodType { + if (isModern) { + // `execution` is not part of the 2026 wire shape; drop it as the codec does. + return ToolSchema.transform((tool) => { + if (tool.execution === undefined) return tool; + const { execution: _execution, ...rest } = tool; + return rest; + }); + } + return ToolSchema.extend({ + outputSchema: z.looseObject({ type: z.literal("object") }).optional(), + }); +}