diff --git a/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts index d6c4f11e..48cea122 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts @@ -143,15 +143,34 @@ describe("Skills extension over a real transport (#2234)", () => { const connected = await connect(started.url, modern, { "io.modelcontextprotocol/skills": false, }); - await expect(connected.listSkills()).rejects.toThrow( - /requires the client to declare io\.modelcontextprotocol\/skills/, - ); - await expect( + // The wire code is the assertion, not just the message: the two eras + // name this refusal differently, and a fixture sliding back to a + // semantically different error must fail here. Modern is SEP-2575's + // `-32021` MissingRequiredClientCapability listing the missing + // extension; legacy has no such code and stays `-32601`. + const expected = modern + ? { + code: -32021, + data: { + requiredCapabilities: { + extensions: { "io.modelcontextprotocol/skills": {} }, + }, + }, + } + : { code: -32601 }; + const refusals = [ + connected.listSkills(), connected.getSkill("skill://data-analysis/SKILL.md"), - ).rejects.toThrow(/requires the client to declare/); - await expect( connected.readResourceDirectory("skill://data-analysis"), - ).rejects.toThrow(/requires the client to declare/); + ]; + for (const refusal of refusals) { + await expect(refusal).rejects.toMatchObject({ + ...expected, + message: expect.stringMatching( + /requires the client to declare io\.modelcontextprotocol\/skills/, + ), + }); + } // A skill file is an ordinary resource and stays readable. const read = await connected.readResource( "skill://data-analysis/reference.md", diff --git a/clients/web/src/test/integration/mcp/skills-strict-configs.test.ts b/clients/web/src/test/integration/mcp/skills-strict-configs.test.ts new file mode 100644 index 00000000..8902371a --- /dev/null +++ b/clients/web/src/test/integration/mcp/skills-strict-configs.test.ts @@ -0,0 +1,100 @@ +import { describe, it, expect, afterEach } from "vitest"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +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 { + createTestServerHttp, + type TestServerHttp, + loadConfig, + resolveConfig, +} from "@modelcontextprotocol/inspector-test-server"; + +/** + * Live coverage of the two checked-in strict Skills configs (#2373) — the + * documented manual reproduction of a server that enforces the client's half + * of SEP-2133 negotiation. + * + * `inspectorClient-skills.test.ts` exercises the fixture behavior by passing + * `skillsRequireClientExtension` straight to `createTestServerHttp`, so it + * never touches the JSON-to-`ServerConfig` mapping. A misspelled key in either + * file, or a `resolveConfig` that stopped threading the flag, would leave the + * manual fixtures quietly permissive while that suite stayed green. These + * tests **resolve the checked-in files** and assert the refusal they exist + * for, so that class fails here instead. + */ +const configsDir = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "../../../../../../test-servers/configs", +); + +const CONFIGS = [ + { file: "skills-strict-legacy-http.json", era: "legacy" as const }, + { file: "skills-strict-modern-http.json", era: "modern" as const }, +]; + +describe("strict Skills showcase configs (#2373)", () => { + let client: InspectorClient | null = null; + let server: TestServerHttp | null = null; + + afterEach(async () => { + if (client) { + try { + await client.disconnect(); + } catch { + // ignore + } + client = null; + } + if (server) { + try { + await server.stop(); + } catch { + // ignore + } + server = null; + } + }); + + for (const { file, era } of CONFIGS) { + describe(file, () => { + const configPath = path.join(configsDir, file); + + it("resolves with the strict flag, skills, and its era intact", () => { + const resolved = resolveConfig(loadConfig(configPath)); + expect(resolved.skills).toBe(true); + expect(resolved.skillsRequireClientExtension).toBe(true); + // One file per era is the documented contract: a legacy client + // reaching a modern server is served statelessly and cannot be + // checked, so the legacy file must not turn the modern leg on. + expect(resolved.modern !== undefined).toBe(era === "modern"); + }); + + it("refuses skills/list to a client that did not declare the extension", async () => { + // The harness picks the port rather than the config's fixed one, so + // this cannot collide with a showcase server someone runs by hand. + const resolved = resolveConfig(loadConfig(configPath)); + const started = createTestServerHttp({ ...resolved, port: undefined }); + await started.start(); + server = started; + + const connected = new InspectorClient( + { type: "streamable-http", url: started.url }, + { + environment: { transport: createTransportNode }, + versionNegotiation: eraToVersionNegotiation(era), + advertisedExtensions: { "io.modelcontextprotocol/skills": false }, + }, + ); + await connected.connect(); + client = connected; + expect(connected.getProtocolEra()).toBe(era); + + await expect(connected.listSkills()).rejects.toMatchObject({ + code: era === "modern" ? -32021 : -32601, + }); + }); + }); + } +}); diff --git a/docs/test-servers.md b/docs/test-servers.md index 2b65a999..87718044 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -86,10 +86,13 @@ declaration and passed against this fixture regardless ([#2373](https://github.com/modelcontextprotocol/inspector/issues/2373)). `skills-strict-legacy-http.json` (port 3232) and `skills-strict-modern-http.json` (port 3233) add -`"skillsRequireClientExtension": true`, which answers `skills/list`, -`skills/get` and `resources/directory/read` with `-32601` unless the client -declared it (skill files still come through ordinary `resources/read`, which -needs no extension). ⚠️ **Connect each with its own era.** A modern request +`"skillsRequireClientExtension": true`, which refuses `skills/list`, +`skills/get` and `resources/directory/read` unless the client declared it +(skill files still come through ordinary `resources/read`, which needs no +extension). The refusal differs by era: a **modern** request gets SEP-2575's +`-32021` MissingRequiredClientCapability (HTTP 400) with the Skills extension +under `data.requiredCapabilities`; a **legacy** one, whose era has no such code, +gets `-32601`. ⚠️ **Connect each with its own era.** A modern request carries the declaration in its `_meta` envelope; a legacy one relies on what `initialize` declared, which only a stateful legacy server keeps. A legacy client reaching the modern file is served statelessly, holds no declaration, diff --git a/test-servers/src/skills.ts b/test-servers/src/skills.ts index 4ef3a9db..b65a3b8a 100644 --- a/test-servers/src/skills.ts +++ b/test-servers/src/skills.ts @@ -60,6 +60,7 @@ import { createHash } from "node:crypto"; import * as z from "zod/v4"; import { CLIENT_CAPABILITIES_META_KEY, + MissingRequiredClientCapabilityError, ProtocolError, ProtocolErrorCode, type McpServer, @@ -747,15 +748,24 @@ function assertClientDeclaredSkills( method: string, envelope: ReceivedEnvelope, ): void { + const modernCapabilities = envelope?.[CLIENT_CAPABILITIES_META_KEY]; const capabilities = - envelope?.[CLIENT_CAPABILITIES_META_KEY] ?? - mcpServer.server.getClientCapabilities(); - if (!hasSkillsExtension(capabilities)) { - throw new ProtocolError( - ProtocolErrorCode.MethodNotFound, - `${method} requires the client to declare ${SKILLS_EXTENSION_KEY} in its capabilities`, + modernCapabilities ?? mcpServer.server.getClientCapabilities(); + if (hasSkillsExtension(capabilities)) return; + const message = `${method} requires the client to declare ${SKILLS_EXTENSION_KEY} in its capabilities`; + // The two eras name this refusal differently. SEP-2575 gives a modern + // request that needs an undeclared capability its own code, `-32021` + // MissingRequiredClientCapability (HTTP 400), carrying the missing + // capabilities in `data.requiredCapabilities` so the client can see what to + // declare. `-32601` there would claim the method does not exist. The legacy + // era has no such code, so a legacy refusal stays `-32601`. + if (modernCapabilities !== undefined) { + throw new MissingRequiredClientCapabilityError( + { requiredCapabilities: { extensions: { [SKILLS_EXTENSION_KEY]: {} } } }, + message, ); } + throw new ProtocolError(ProtocolErrorCode.MethodNotFound, message); } /**