From 1d765b8af661a6b6732d4b0f7d90ee7149750404 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 14 Sep 2026 08:36:37 -0400 Subject: [PATCH 1/5] fix: declare the skills extension in the Inspector's client capabilities (#2373) SEP-2133 negotiates an extension from both sides, and a strict server refuses skills/* to a client that did not declare io.modelcontextprotocol/skills. The Inspector called those methods without ever declaring it. Add it to ADVERTISABLE_EXTENSIONS, on by default, which also gives it a Server Settings toggle. Give the composable test server skillsRequireClientExtension and two per-era strict showcase configs, so a strict server is reproducible here. Writing the modern-era test exposed two defects the existing suite hid: its "modern" cases set protocolEra on the transport config, which InspectorClient does not read, so they connected on legacy; and once they really ran modern, every skills result failed, because the modern schemas required resultType, which the SDK codec checks and then lifts off before a caller schema runs. Connect with versionNegotiation and assert the era, and drop the resultType requirement (and the two modern schema variants it was the only difference in). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Q3RsrgCxVoFPokLB4vAkxi Signed-off-by: cliffhall --- .../web/src/test/core/mcp/extensions.test.ts | 61 ++++++++++-- .../core/mcp/inspectorClient-skills.test.ts | 31 +++---- .../src/test/core/mcp/skillsSchemas.test.ts | 79 ++++++---------- .../mcp/inspectorClient-skills.test.ts | 93 ++++++++++++++++--- core/mcp/extensions.ts | 13 +++ core/mcp/inspectorClient.ts | 30 ++---- core/mcp/skillsSchemas.ts | 67 ++++++------- docs/test-servers.md | 33 ++++++- .../configs/skills-strict-legacy-http.json | 18 ++++ .../configs/skills-strict-modern-http.json | 19 ++++ test-servers/src/composable-test-server.ts | 12 ++- test-servers/src/load-config.ts | 3 + test-servers/src/resolve-config.ts | 1 + test-servers/src/skills.ts | 86 ++++++++++++++++- 14 files changed, 396 insertions(+), 150 deletions(-) create mode 100644 test-servers/configs/skills-strict-legacy-http.json create mode 100644 test-servers/configs/skills-strict-modern-http.json diff --git a/clients/web/src/test/core/mcp/extensions.test.ts b/clients/web/src/test/core/mcp/extensions.test.ts index 5a2bbb26..5f25108d 100644 --- a/clients/web/src/test/core/mcp/extensions.test.ts +++ b/clients/web/src/test/core/mcp/extensions.test.ts @@ -7,12 +7,21 @@ import { buildClientExtensions, } from "@inspector/core/mcp/extensions.js"; import { TASKS_EXTENSION_KEY } from "@inspector/core/mcp/modernTaskSchemas.js"; +import { SKILLS_EXTENSION_KEY } from "@inspector/core/mcp/skillsSchemas.js"; // The `ui` extension carries a non-empty advertisement value; the others are // declared with `{}`. Spelled out here so the map assertions stay readable. const UI_ADVERTISEMENT = { mimeTypes: [MCP_APP_MIME_TYPE] }; -describe("extensions (#1738, #1740)", () => { +// Every registry entry switched off, so a test can re-enable exactly the ones +// it is about without restating the whole registry. +const ALL_REGISTRY_OFF = { + [TASKS_EXTENSION_KEY]: false, + [UI_EXTENSION_KEY]: false, + [SKILLS_EXTENSION_KEY]: false, +}; + +describe("extensions (#1738, #1740, #2373)", () => { describe("ADVERTISABLE_EXTENSIONS registry", () => { it("lists the Tasks extension, advertised by default", () => { const tasks = ADVERTISABLE_EXTENSIONS.find( @@ -37,6 +46,19 @@ describe("extensions (#1738, #1740)", () => { expect(typeof MCP_APP_MIME_TYPE).toBe("string"); }); + it("lists the Skills extension, advertised by default with no settings (#2373)", () => { + // SEP-2133 negotiates an extension from both sides, so a server may + // refuse `skills/*` to a client that did not declare it. The Inspector + // calls those methods, so it must declare the extension by default. + const skills = ADVERTISABLE_EXTENSIONS.find( + (e) => e.key === SKILLS_EXTENSION_KEY, + ); + expect(skills).toBeDefined(); + expect(skills?.defaultAdvertised).toBe(true); + expect(skills?.advertisement).toBeUndefined(); + expect(skills?.label).toContain("Skills"); + }); + it("does not list EMA (it follows the auth mode, not a toggle)", () => { expect( ADVERTISABLE_EXTENSIONS.some((e) => e.key === EMA_EXTENSION_KEY), @@ -53,11 +75,12 @@ describe("extensions (#1738, #1740)", () => { }); describe("buildClientExtensions", () => { - it("advertises registry defaults with no overrides (tasks + ui)", () => { + it("advertises registry defaults with no overrides (tasks + ui + skills)", () => { const map = buildClientExtensions({ enterpriseManaged: false }); expect(map).toEqual({ [TASKS_EXTENSION_KEY]: {}, [UI_EXTENSION_KEY]: UI_ADVERTISEMENT, + [SKILLS_EXTENSION_KEY]: {}, }); }); @@ -82,6 +105,7 @@ describe("extensions (#1738, #1740)", () => { expect(map).toEqual({ [TASKS_EXTENSION_KEY]: {}, [UI_EXTENSION_KEY]: UI_ADVERTISEMENT, + [SKILLS_EXTENSION_KEY]: {}, [EMA_EXTENSION_KEY]: {}, }); }); @@ -94,23 +118,39 @@ describe("extensions (#1738, #1740)", () => { it("honors a user override that disables a default-on extension", () => { const map = buildClientExtensions({ enterpriseManaged: false, - advertised: { [TASKS_EXTENSION_KEY]: false, [UI_EXTENSION_KEY]: false }, + advertised: ALL_REGISTRY_OFF, }); expect(map).toEqual({}); }); - it("can disable just the UI extension, keeping Tasks (#1740)", () => { + it("can disable just the UI extension, keeping the others (#1740)", () => { const map = buildClientExtensions({ enterpriseManaged: false, advertised: { [UI_EXTENSION_KEY]: false }, }); - expect(map).toEqual({ [TASKS_EXTENSION_KEY]: {} }); + expect(map).toEqual({ + [TASKS_EXTENSION_KEY]: {}, + [SKILLS_EXTENSION_KEY]: {}, + }); + }); + + it("can disable just the Skills extension, keeping the others (#2373)", () => { + // The Server Settings toggle for skills is how a user checks that a + // server refuses `skills/*` to a client that did not declare it. + const map = buildClientExtensions({ + enterpriseManaged: false, + advertised: { [SKILLS_EXTENSION_KEY]: false }, + }); + expect(map).toEqual({ + [TASKS_EXTENSION_KEY]: {}, + [UI_EXTENSION_KEY]: UI_ADVERTISEMENT, + }); }); it("honors a user override that keeps a default-on extension enabled", () => { const map = buildClientExtensions({ enterpriseManaged: false, - advertised: { [TASKS_EXTENSION_KEY]: true, [UI_EXTENSION_KEY]: false }, + advertised: { ...ALL_REGISTRY_OFF, [TASKS_EXTENSION_KEY]: true }, }); expect(map).toEqual({ [TASKS_EXTENSION_KEY]: {} }); }); @@ -134,13 +174,14 @@ describe("extensions (#1738, #1740)", () => { expect(map).toEqual({ [TASKS_EXTENSION_KEY]: {}, [UI_EXTENSION_KEY]: UI_ADVERTISEMENT, + [SKILLS_EXTENSION_KEY]: {}, }); }); it("layers EMA on even when all registry entries are disabled", () => { const map = buildClientExtensions({ enterpriseManaged: true, - advertised: { [TASKS_EXTENSION_KEY]: false, [UI_EXTENSION_KEY]: false }, + advertised: ALL_REGISTRY_OFF, }); expect(map).toEqual({ [EMA_EXTENSION_KEY]: {} }); }); @@ -163,7 +204,11 @@ describe("extensions (#1738, #1740)", () => { }); // A nested setting, NOT a second extension — the contract is explicit // that no new extension id is introduced. - expect(Object.keys(map)).toEqual([TASKS_EXTENSION_KEY, UI_EXTENSION_KEY]); + expect(Object.keys(map)).toEqual([ + TASKS_EXTENSION_KEY, + UI_EXTENSION_KEY, + SKILLS_EXTENSION_KEY, + ]); }); it("advertises nothing when the UI extension itself is turned off", () => { diff --git a/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts b/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts index a254db94..c8215362 100644 --- a/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts +++ b/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts @@ -159,10 +159,12 @@ describe("InspectorClient skills methods (#2234)", () => { }); it("accepts a modern result that carries the envelope", async () => { + // No `resultType` in the stub: the SDK codec lifts it off before the + // client's schema runs, so this is the shape a real modern page arrives + // in (#2373). const client = makeClient(); internals(client).protocolEra = "modern"; stubRequest(client, { - resultType: "complete", ttlMs: 0, cacheScope: "public", skills: [ENTRY], @@ -317,14 +319,18 @@ describe("InspectorClient skills methods (#2234)", () => { }); }); - it("requires resultType on a modern connection", async () => { + it("accepts a modern result as the SDK codec delivers it, without resultType (#2373)", async () => { + // The codec checks `resultType` and lifts it off before this schema + // runs, so a stub returning the lifted shape is what a real modern + // connection hands the client. Requiring it here failed every conforming + // server; the live check is in the integration suite. const client = makeClient(); internals(client).protocolEra = "modern"; - stubRequest(client, { resources: [] }); + stubRequest(client, { resources: [CHILD] }); declareSkills(client, true); await expect( client.readResourceDirectory("skill://demo"), - ).rejects.toBeDefined(); + ).resolves.toMatchObject({ resources: [CHILD] }); }); it("accepts a legacy result without resultType", async () => { @@ -388,21 +394,14 @@ describe("InspectorClient skills methods (#2234)", () => { }); }); - it("requires resultType on a modern skills/get", async () => { - // Base-protocol (SEP-2322) and present in SEP-2640's own example, unlike - // the caching attributes the SEP leaves open. + it("accepts a modern skills/get as the SDK codec delivers it, without resultType (#2373)", async () => { + // `resultType` is base-protocol (SEP-2322), so the codec enforces it and + // lifts it off before this schema runs — requiring it here rejected every + // conforming modern server. The caching attributes SEP-2640 leaves open + // stay optional too. const client = makeClient(); internals(client).protocolEra = "modern"; stubRequest(client, { skill: ENTRY }); - await expect( - client.getSkill("skill://demo/SKILL.md"), - ).rejects.toBeDefined(); - }); - - it("accepts a modern skills/get without the caching attributes", async () => { - const client = makeClient(); - internals(client).protocolEra = "modern"; - stubRequest(client, { skill: ENTRY, resultType: "complete" }); await expect(client.getSkill("skill://demo/SKILL.md")).resolves.toEqual( ENTRY, ); diff --git a/clients/web/src/test/core/mcp/skillsSchemas.test.ts b/clients/web/src/test/core/mcp/skillsSchemas.test.ts index 53f991e6..bbe78184 100644 --- a/clients/web/src/test/core/mcp/skillsSchemas.test.ts +++ b/clients/web/src/test/core/mcp/skillsSchemas.test.ts @@ -2,12 +2,10 @@ import { describe, it, expect } from "vitest"; import { DIRECTORY_MIME_TYPE, DirectoryReadResultSchema, - ModernDirectoryReadResultSchema, RESOURCES_DIRECTORY_READ_METHOD, DYNAMIC_RESOURCES, GetSkillEnvelopeSchema, GetSkillResultSchema, - ModernGetSkillEnvelopeSchema, ListSkillsResultSchema, ModernListSkillsResultSchema, SKILLS_EXTENSION_KEY, @@ -95,9 +93,11 @@ describe("ListSkillsResultSchema", () => { }); describe("ModernListSkillsResultSchema", () => { - const envelope = { resultType: "complete", ttlMs: 0, cacheScope: "public" }; + // What the schema actually receives on a modern connection: the SDK codec + // has already checked `resultType` and lifted it off the result (#2373). + const envelope = { ttlMs: 0, cacheScope: "public" }; - it("accepts a modern page carrying the base list envelope", () => { + it("accepts a modern page carrying the caching attributes", () => { const parsed = ModernListSkillsResultSchema.parse({ ...envelope, skills: [ENTRY], @@ -105,10 +105,20 @@ describe("ModernListSkillsResultSchema", () => { expect(parsed.skills).toHaveLength(1); }); + it("does not require resultType, which the SDK codec lifts before this schema runs (#2373)", () => { + // Requiring it rejected every conforming modern page: the codec checks + // `resultType` itself, then deletes it. An integration test proves the + // lift against a real connection; this pins the schema's side of it. + expect( + ModernListSkillsResultSchema.safeParse({ ...envelope, skills: [] }) + .success, + ).toBe(true); + }); + it("rejects a modern page that omits the caching attributes", () => { // The whole reason for the era split: `skills/*` is consumer-owned, so the - // SDK codec validates none of it, and `{ skills: [] }` would otherwise - // reach the conformance UI as a clean list. + // SDK codec checks none of the caching attributes, and `{ skills: [] }` + // would otherwise reach the conformance UI as a clean list. expect(() => ModernListSkillsResultSchema.parse({ skills: [] })).toThrow(); expect(() => ModernListSkillsResultSchema.parse({ @@ -195,9 +205,6 @@ describe("directory read schemas (#2248)", () => { ], }; expect(DirectoryReadResultSchema.safeParse(example).success).toBe(true); - expect(ModernDirectoryReadResultSchema.safeParse(example).success).toBe( - true, - ); }); it("accepts an empty directory", () => { @@ -230,39 +237,24 @@ describe("directory read schemas (#2248)", () => { ).toBe(false); }); - it("requires resultType on the modern variant only", () => { - const legacyShape = { resources: [CHILD] }; - expect(DirectoryReadResultSchema.safeParse(legacyShape).success).toBe(true); - expect(ModernDirectoryReadResultSchema.safeParse(legacyShape).success).toBe( - false, - ); - }); - - it("does NOT require the caching attributes on the modern variant", () => { + it("does NOT require the caching attributes, unlike a modern skills/list page", () => { // The deliberate asymmetry with `ModernListSkillsResultSchema`: SEP-2640 // states `ttlMs`/`cacheScope` for a modern `skills/list` and says nothing // of the kind for this method, whose only worked example omits them. // Requiring them would fail a server that matched the spec's own example. - expect( - ModernDirectoryReadResultSchema.safeParse({ - resultType: "complete", - resources: [], - }).success, - ).toBe(true); - expect( - ModernListSkillsResultSchema.safeParse({ - resultType: "complete", - skills: [], - }).success, - ).toBe(false); + expect(DirectoryReadResultSchema.safeParse({ resources: [] }).success).toBe( + true, + ); + expect(ModernListSkillsResultSchema.safeParse({ skills: [] }).success).toBe( + false, + ); }); it("still accepts the caching attributes when a server sends them", () => { // Permitted, not mandated — a schema is not the place to reject an extra // member the spec leaves open. expect( - ModernDirectoryReadResultSchema.safeParse({ - resultType: "complete", + DirectoryReadResultSchema.safeParse({ resources: [], ttlMs: 60, cacheScope: "public", @@ -286,25 +278,10 @@ describe("GetSkillResultSchema caching attributes (#2248)", () => { ).toBe(true); }); - it("requires resultType on the modern envelope, but still not the caching fields", () => { - // "Left open" covers `ttlMs` / `cacheScope` and only those. `resultType` is - // base-protocol (SEP-2322) and appears in SEP-2640's own `skills/get` - // example, so leaving it optional here while requiring it of - // `resources/directory/read` was an inconsistency in this module rather - // than a distinction the spec draws (Copilot). - expect( - ModernGetSkillEnvelopeSchema.safeParse({ skill: ENTRY }).success, - ).toBe(false); - expect( - ModernGetSkillEnvelopeSchema.safeParse({ - skill: ENTRY, - resultType: "complete", - }).success, - ).toBe(true); - }); - - it("keeps the legacy envelope permissive about resultType", () => { - // A 2026-era member a legacy server has no business sending. + it("serves both eras with one envelope schema (#2373)", () => { + // The modern variant used to add a required `resultType`, which the SDK + // codec lifts before any caller schema runs — so it could never pass on a + // real modern connection. With that gone the two eras want the same shape. expect(GetSkillEnvelopeSchema.safeParse({ skill: ENTRY }).success).toBe( true, ); 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 7bbf03c2..d6c4f11e 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, afterEach } from "vitest"; 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 { getSkillsExtension } from "@inspector/core/mcp/skills.js"; import { ManagedSkillsState } from "@inspector/core/mcp/state/managedSkillsState.js"; import { @@ -55,7 +56,10 @@ describe("Skills extension over a real transport (#2234)", () => { } }); - async function startSkillsServer(modern: boolean): Promise { + async function startSkillsServer( + modern: boolean, + requireClientExtension = false, + ): Promise { const started = createTestServerHttp({ serverInfo: createTestServerInfo("skills-integration", "1.0.0"), // An ordinary resource alongside the skills, so the fixture's @@ -69,6 +73,7 @@ describe("Skills extension over a real transport (#2234)", () => { }, ], skills: true, + ...(requireClientExtension && { skillsRequireClientExtension: true }), ...(modern && { modern: {} }), }); await started.start(); @@ -79,20 +84,85 @@ describe("Skills extension over a real transport (#2234)", () => { async function connect( url: string, modern: boolean, + advertisedExtensions?: Record, ): Promise { + // The era is chosen by `versionNegotiation`. This helper used to set + // `protocolEra` on the transport config instead, which `InspectorClient` + // does not read — so every "modern" case below connected on legacy and + // passed without exercising the modern leg at all (#2373). The assertion + // after connect is what keeps that from recurring silently. const connected = new InspectorClient( + { type: "streamable-http", url }, { - type: "streamable-http", - url, - ...(modern && { protocolEra: "modern" as const }), + environment: { transport: createTransportNode }, + versionNegotiation: eraToVersionNegotiation( + modern ? "modern" : "legacy", + ), + advertisedExtensions, }, - { environment: { transport: createTransportNode } }, ); await connected.connect(); client = connected; + expect(connected.getProtocolEra()).toBe(modern ? "modern" : "legacy"); return connected; } + /** + * #2373: SEP-2133 negotiates an extension from both sides, and a strict + * server refuses `skills/*` to a client that did not declare + * `io.modelcontextprotocol/skills` itself. The Inspector never declared it, + * and every test above passed anyway because the default fixture serves any + * client. These run against the strict fixture on both eras — the legacy leg + * reads the declaration from `initialize`, the modern leg from each + * request's `_meta` envelope, so each is a separate path to prove. + */ + for (const modern of [false, true]) { + const era = modern ? "modern" : "legacy"; + + describe(`against a server that requires the client's declaration (${era})`, () => { + it("is served, because the Inspector declares the extension by default", async () => { + const started = await startSkillsServer(modern, true); + const connected = await connect(started.url, modern); + const first = await connected.listSkills(); + expect(first.skills).toHaveLength(2); + const entry = await connected.getSkill( + "skill://data-analysis/SKILL.md", + ); + expect(entry.frontmatter.name).toBe("data-analysis"); + const page = await connected.readResourceDirectory( + "skill://data-analysis", + ); + expect(page.resources).toHaveLength(1); + }); + + it("is refused once the declaration is turned off", async () => { + // The negative control: without it the test above could pass against + // a fixture that stopped checking. It is also the Server Settings + // toggle's whole purpose — reproducing a strict server's refusal. + const started = await startSkillsServer(modern, true); + 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( + 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/); + // A skill file is an ordinary resource and stays readable. + const read = await connected.readResource( + "skill://data-analysis/reference.md", + ); + expect(read.result.contents[0].uri).toBe( + "skill://data-analysis/reference.md", + ); + }); + }); + } + for (const modern of [false, true]) { const era = modern ? "modern" : "legacy"; @@ -114,12 +184,13 @@ describe("Skills extension over a real transport (#2234)", () => { const first = await connected.listSkills(); // On the modern leg this call resolving is itself the envelope - // assertion: `listSkills` selects `ModernListSkillsResultSchema` from - // the negotiated era, and that schema rejects a page without - // `resultType` / `ttlMs` / `cacheScope`. It cannot be asserted on the - // returned value — `listSkills` narrows its result to the two fields - // below — so a modern page missing the envelope surfaces here as a - // rejection rather than as a missing property. + // assertion, in two halves: the SDK codec rejects a result without + // `resultType` (and lifts it off), and `listSkills` then selects + // `ModernListSkillsResultSchema`, which rejects a page without + // `ttlMs` / `cacheScope`. It cannot be asserted on the returned value + // — `listSkills` narrows its result to the two fields below — so a + // modern page missing the envelope surfaces here as a rejection + // rather than as a missing property. // // The fixture pages at two over eight skills, so a client that stops // here sees a quarter of the catalog. diff --git a/core/mcp/extensions.ts b/core/mcp/extensions.ts index 140d6323..eab7ae53 100644 --- a/core/mcp/extensions.ts +++ b/core/mcp/extensions.ts @@ -1,6 +1,7 @@ import type { ClientCapabilities } from "@modelcontextprotocol/client"; import { RESOURCE_MIME_TYPE } from "@modelcontextprotocol/ext-apps/app-bridge"; import { TASKS_EXTENSION_KEY } from "./modernTaskSchemas.js"; +import { SKILLS_EXTENSION_KEY } from "./skillsSchemas.js"; /** * Extension identifier for SEP-2350 enterprise-managed authorization. Advertised @@ -91,6 +92,18 @@ export const ADVERTISABLE_EXTENSIONS: readonly AdvertisableExtension[] = [ defaultAdvertised: true, advertisement: { mimeTypes: [MCP_APP_MIME_TYPE] }, }, + { + key: SKILLS_EXTENSION_KEY, + label: "Skills (io.modelcontextprotocol/skills)", + // The Skills extension (SEP-2640). SEP-2133 negotiates an extension from + // both sides, so a server may refuse `skills/list`, `skills/get` and + // `resources/directory/read` to a client that did not declare it — and the + // Inspector calls all three once the server declares its half. Advertised + // by default for that reason; turning it off is how to check a server's + // refusal path. Declared with no settings: SEP-2640 defines none for the + // client side. (#2373) + defaultAdvertised: true, + }, ]; export interface BuildClientExtensionsInput { diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 72f3c192..d3c6aacb 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -150,8 +150,6 @@ import { DirectoryReadResultSchema, GetSkillEnvelopeSchema, ListSkillsResultSchema, - ModernGetSkillEnvelopeSchema, - ModernDirectoryReadResultSchema, ModernListSkillsResultSchema, RESOURCES_DIRECTORY_READ_METHOD, SKILLS_EXTENSION_KEY, @@ -5796,21 +5794,16 @@ export class InspectorClient extends InspectorClientEventTarget { uri, ...(effectiveMeta ? { _meta: effectiveMeta } : {}), }; - // Era-aware for the same reason `skills/list` is: the method is - // consumer-owned, so no SDK codec stamps or checks its envelope. The modern - // variant requires `resultType` — a base-protocol member SEP-2322 puts on - // every modern result — and still not the caching attributes, which - // SEP-2640 leaves open. The envelope is returned whole; `getSkill` - // unwraps. - const resultSchema = this.isModernEra() - ? ModernGetSkillEnvelopeSchema - : GetSkillEnvelopeSchema; + // One schema for both eras (#2373): on a modern connection the SDK codec + // has already enforced `resultType` and lifted it off, and the caching + // attributes are left open by SEP-2640, so there is nothing era-specific + // left to require. The envelope is returned whole; `getSkill` unwraps. try { return await this.invokeMcpClient( () => this.client!.request( { method: SKILLS_GET_METHOD, params }, - resultSchema, + GetSkillEnvelopeSchema, this.getRequestOptions(this.progressTokenOf(metadata)), ), { method: SKILLS_GET_METHOD }, @@ -5872,19 +5865,16 @@ export class InspectorClient extends InspectorClientEventTarget { // for page one. ...(cursor !== undefined ? { cursor } : {}), }; - // Era-aware for the same reason `skills/list` is — the method is - // consumer-owned, so no SDK codec stamps or checks its envelope. The modern - // variant requires only `resultType`; see the schema for why it stops - // short of the caching attributes that `skills/list` requires. - const resultSchema = this.isModernEra() - ? ModernDirectoryReadResultSchema - : DirectoryReadResultSchema; + // One schema for both eras (#2373): the SDK codec enforces and lifts + // `resultType` on a modern connection, and SEP-2640 requires no caching + // attributes of this method — see `skillsSchemas.ts` for why that differs + // from `skills/list`. try { return await this.invokeMcpClient( () => this.client!.request( { method: RESOURCES_DIRECTORY_READ_METHOD, params }, - resultSchema, + DirectoryReadResultSchema, this.getRequestOptions(this.progressTokenOf(metadata)), ), { method: RESOURCES_DIRECTORY_READ_METHOD }, diff --git a/core/mcp/skillsSchemas.ts b/core/mcp/skillsSchemas.ts index 79e76341..ed21db36 100644 --- a/core/mcp/skillsSchemas.ts +++ b/core/mcp/skillsSchemas.ts @@ -131,9 +131,18 @@ export type ListSkillsResult = z.infer; * * ⚠️ Picked by `InspectorClient.listSkills` from the negotiated era — a schema * cannot know it on its own. + * + * ⚠️ **No `resultType` member, and that is not an omission (#2373).** The + * SDK's 2026-07-28 codec checks `resultType` on every result — missing is + * `InvalidResult`, anything but `"complete"` is `UnsupportedResultType` — and + * then **deletes it** before the result reaches the caller's schema. Requiring + * it here therefore rejected every conforming modern page, and did so unseen + * only because the integration suite's "modern" cases were connecting on + * legacy. The caching attributes are different: the codec neither checks nor + * lifts them for a consumer-owned method, so this schema is still the only + * thing that does. */ export const ModernListSkillsResultSchema = ListSkillsResultSchema.extend({ - resultType: z.literal("complete"), // A non-negative INTEGER, matching the codec: `ttlMs: -1` or `0.5` is an // envelope violation, and accepting it here would hide exactly the kind of // defect this schema exists to surface. @@ -183,21 +192,6 @@ export const GetSkillEnvelopeSchema = z.looseObject({ */ export type GetSkillEnvelope = z.infer; -/** - * `skills/get` on a **modern** (2026-07-28+) connection: the envelope plus - * `resultType`, and deliberately still not the caching attributes. - * - * The "left open" quote above covers `ttlMs` / `cacheScope` and **only** those. - * `resultType` is a different thing: SEP-2322 makes it a member of every modern - * result, and SEP-2640's own `skills/get` example carries - * `"resultType": "complete"`. Leaving it optional here while requiring it of - * `resources/directory/read` was an inconsistency in this module rather than a - * distinction the spec draws (Copilot). - */ -export const ModernGetSkillEnvelopeSchema = GetSkillEnvelopeSchema.extend({ - resultType: z.literal("complete"), -}); - /** * `skills/get` result, unwrapped to the entry it carries. * @@ -252,29 +246,28 @@ export const DirectoryReadResultSchema = z.looseObject({ export type DirectoryReadResult = z.infer; -/** - * `resources/directory/read` result on a **modern** (2026-07-28+) connection: - * the page plus `resultType`, and deliberately **not** `ttlMs` / `cacheScope`. +/* + * There is deliberately **no modern variant** of the `resources/directory/read` + * or `skills/get` schemas, though `skills/list` has one (#2373). * - * That asymmetry with {@link ModernListSkillsResultSchema} is the one judgement - * call in this module, so it is written down rather than left to be re-derived: + * On a modern (2026-07-28+) connection the SDK codec already enforces + * `resultType` — the base-protocol member SEP-2322 puts on every modern result + * — and lifts it off before any caller schema runs. What remains is the + * caching attributes, and only `skills/list` is required to carry those: * - * - For `skills/list` the SEP states the requirement outright — *"In protocol - * versions 2026-07-28 and later, the result also carries … `ttlMs` and - * `cacheScope`"* — so requiring them is quoting the spec. + * - For `skills/list` the SEP states it outright — *"In protocol versions + * 2026-07-28 and later, the result also carries … `ttlMs` and + * `cacheScope`"* — so {@link ModernListSkillsResultSchema} requiring them is + * quoting the spec. * - For `resources/directory/read` it states **nothing of the kind**, and its - * one worked example of the result carries `resultType: "complete"` and no - * caching attributes at all. Requiring them here would fail a server that - * matched the SEP's own example, which is the failure direction this module - * works hardest to avoid. + * one worked example carries `resultType: "complete"` and no caching + * attributes at all. Requiring them would fail a server that matched the + * SEP's own example, the failure direction this module works hardest to + * avoid. + * - For `skills/get` the SEP leaves the question open in as many words (see + * {@link GetSkillEnvelopeSchema}). * - * `resultType` is required because it is the base protocol's, not this - * extension's: SEP-2322 makes it a member of every modern result, the SEP's - * example carries it, and `skills/*` being consumer-owned means the SDK codec - * validates none of it — so if this schema does not, nothing does. - * - * ⚠️ Picked by `InspectorClient.readResourceDirectory` from the negotiated era. + * So both eras parse those two results with one schema each. An earlier + * revision required `resultType` on "modern" variants of both; since the codec + * lifts it, they could never pass on a real modern connection. */ -export const ModernDirectoryReadResultSchema = DirectoryReadResultSchema.extend( - { resultType: z.literal("complete") }, -); diff --git a/docs/test-servers.md b/docs/test-servers.md index 833b25c8..2b65a999 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -62,6 +62,7 @@ as a missing capability rather than an error. | `tasks-{legacy,modern}-http.json` **(era per file)** | Tasks, both eras | [#1631](https://github.com/modelcontextprotocol/inspector/issues/1631) | | `cancellation-modern-http.json` **(modern era)** | Cancelling a call by closing its response stream | [#2140](https://github.com/modelcontextprotocol/inspector/issues/2140) | | `skills-http.json` **(either era)** | Skills tab: `skills/list`, `resources/directory/read`, digest verification, the frontmatter cross-check, and the non-conforming cases | [#2234](https://github.com/modelcontextprotocol/inspector/issues/2234), [#2248](https://github.com/modelcontextprotocol/inspector/issues/2248) | +| `skills-strict-{legacy,modern}-http.json` **(era per file)** | The same skills, served only to a client that **declared** `io.modelcontextprotocol/skills` itself (SEP-2133) | [#2373](https://github.com/modelcontextprotocol/inspector/issues/2373) | ## Skills (SEP-2640) @@ -77,6 +78,32 @@ handler cannot reach it. To exercise the *undeclared* case, connect to any config **without** `"skills"`, where the Inspector must refuse to send the call locally rather than letting the server answer it. +⚠️ **`skills-http.json` serves any client, including one that never declared the +extension.** SEP-2133 negotiates an extension from both sides, and a strict +server refuses `skills/*` to a client that did not declare +`io.modelcontextprotocol/skills` itself. The Inspector shipped without that +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 +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, +and is refused however it is configured — so there is one file per era rather +than one for both: + +1. Connect — the Inspector declares the Skills extension by default, so the + Skills tab lists the same eight skills. The client's declaration is visible + in the `initialize` request (Legacy) or in each request's + `io.modelcontextprotocol/clientCapabilities` `_meta` entry (Modern). +2. Open **Server Settings → Advertised Extensions**, uncheck + **Skills (io.modelcontextprotocol/skills)**, and reconnect. +3. `skills/list` now fails with `-32601` — the refusal a strict server sends. + **Both `skills/*` results carry the full modern base envelope** (`resultType` / `ttlMs` / `cacheScope`). They are consumer-owned methods, so the SDK stamps nothing for them; without it a 2026-era connection would receive a result @@ -91,8 +118,10 @@ words and says nothing of the kind for this method, whose one worked example carries `resultType` and nothing else. A fixture sending more than the SEP shows would make a client that wrongly *required* them look correct, which is the opposite of what a conformance fixture is for — so `readDirectoryPage` stops -where the spec does, and `ModernDirectoryReadResultSchema` requires exactly as -much. +where the spec does. On the client, the SDK's modern codec enforces +`resultType` and lifts it off before the Inspector's own schema runs, so +`DirectoryReadResultSchema` serves both eras and requires no caching attributes +([#2373](https://github.com/modelcontextprotocol/inspector/issues/2373)). It works on **either era**: `skills/list`, `skills/get` and `resources/directory/read` are consumer-owned extension methods that neither diff --git a/test-servers/configs/skills-strict-legacy-http.json b/test-servers/configs/skills-strict-legacy-http.json new file mode 100644 index 00000000..acad504b --- /dev/null +++ b/test-servers/configs/skills-strict-legacy-http.json @@ -0,0 +1,18 @@ +{ + "serverInfo": { + "name": "skills-strict-legacy", + "version": "1.0.0" + }, + "tools": [ + { + "preset": "echo" + } + ], + "resources": [], + "skills": true, + "skillsRequireClientExtension": true, + "transport": { + "type": "streamable-http", + "port": 3232 + } +} diff --git a/test-servers/configs/skills-strict-modern-http.json b/test-servers/configs/skills-strict-modern-http.json new file mode 100644 index 00000000..29d63df2 --- /dev/null +++ b/test-servers/configs/skills-strict-modern-http.json @@ -0,0 +1,19 @@ +{ + "serverInfo": { + "name": "skills-strict-modern", + "version": "1.0.0" + }, + "tools": [ + { + "preset": "echo" + } + ], + "resources": [], + "skills": true, + "skillsRequireClientExtension": true, + "transport": { + "type": "streamable-http", + "port": 3233, + "modern": true + } +} diff --git a/test-servers/src/composable-test-server.ts b/test-servers/src/composable-test-server.ts index 2c094fc9..fef18353 100644 --- a/test-servers/src/composable-test-server.ts +++ b/test-servers/src/composable-test-server.ts @@ -614,6 +614,14 @@ export interface ServerConfig { * refuse to send the call locally. */ skills?: boolean; + /** + * With {@link ServerConfig.skills}, answer `skills/list`, `skills/get` and + * `resources/directory/read` with `-32601` unless the client declared + * `io.modelcontextprotocol/skills` in its own capabilities — a strict + * SEP-2133 server (#2373). Off by default so the existing skills fixtures + * keep serving any client. Ignored without `skills`. + */ + skillsRequireClientExtension?: boolean; /** * Advertise the MCP Apps `io.modelcontextprotocol/ui` extension with the * nested `elicitation` setting — the server-side half of the app-rendered @@ -1675,7 +1683,9 @@ export function createMcpServer(config: ServerConfig): McpServer { // `skill://` half of resources/read. Wired after the SDK's own handlers so // the resources/read wrapper can delegate non-skill URIs to them. if (config.skills) { - wireSkillsHandlers(mcpServer); + wireSkillsHandlers(mcpServer, { + requireClientExtension: config.skillsRequireClientExtension, + }); } // Extension-gated tools (#1739): start each gated tool disabled, then enable diff --git a/test-servers/src/load-config.ts b/test-servers/src/load-config.ts index 865fdaab..f5a64dbf 100644 --- a/test-servers/src/load-config.ts +++ b/test-servers/src/load-config.ts @@ -87,6 +87,9 @@ export interface ConfigFile { /** Advertise the Skills extension (SEP-2640) and serve its fixture skills, * including `directoryRead` — see {@link ServerConfig.skills}. */ skills?: boolean; + /** With `skills`, refuse the skills methods to a client that did not declare + * the extension — see {@link ServerConfig.skillsRequireClientExtension}. */ + skillsRequireClientExtension?: boolean; /** Advertise the MCP Apps `io.modelcontextprotocol/ui` extension with the nested * `elicitation` setting — the server half of app-rendered form elicitation * (#1854). Pair with the `app_choose_option` tool + `choose_option_app` resource. */ diff --git a/test-servers/src/resolve-config.ts b/test-servers/src/resolve-config.ts index 52eaca8e..e832cbda 100644 --- a/test-servers/src/resolve-config.ts +++ b/test-servers/src/resolve-config.ts @@ -91,6 +91,7 @@ export function resolveConfig(config: ConfigFile): ServerConfig { tasks: config.tasks, tasksExtension: config.tasksExtension, skills: config.skills, + skillsRequireClientExtension: config.skillsRequireClientExtension, appElicitation: config.appElicitation, maxPageSize: config.maxPageSize, emptyStringCursor: config.emptyStringCursor, diff --git a/test-servers/src/skills.ts b/test-servers/src/skills.ts index 95b1c468..4ef3a9db 100644 --- a/test-servers/src/skills.ts +++ b/test-servers/src/skills.ts @@ -59,6 +59,7 @@ import { createHash } from "node:crypto"; import * as z from "zod/v4"; import { + CLIENT_CAPABILITIES_META_KEY, ProtocolError, ProtocolErrorCode, type McpServer, @@ -698,29 +699,106 @@ const DirectoryReadResultShape = z.object({ nextCursor: z.string().optional(), }); +export interface WireSkillsOptions { + /** + * Refuse the extension's own methods to a client that did not declare + * `io.modelcontextprotocol/skills` in its capabilities (#2373). SEP-2133 + * negotiates an extension from both sides, and a strict server enforces the + * client's half; without this the fixture served every client, which is how + * an Inspector that never declared the extension passed against it. + */ + requireClientExtension?: boolean; +} + +/** + * The request's `_meta` envelope as a handler receives it. The SDK types the + * envelope's keys as an empty object, so it is read as a plain record and each + * value is narrowed where it is used. + */ +type ReceivedEnvelope = Record | undefined; + +function hasSkillsExtension(capabilities: unknown): boolean { + if (typeof capabilities !== "object" || capabilities === null) return false; + const extensions = (capabilities as { extensions?: unknown }).extensions; + return ( + typeof extensions === "object" && + extensions !== null && + SKILLS_EXTENSION_KEY in extensions + ); +} + +/** + * Throw `-32601` when the client did not declare the Skills extension. + * + * The two eras keep the declaration in different places. A modern + * (2026-07-28) request carries it in its own `_meta` envelope, which the SDK + * hands the handler as `ctx.mcpReq.envelope` — read first, because that is the + * per-request source of truth the SDK points handlers at. A legacy connection + * has no envelope, so the value `initialize` declared is the fallback. + * + * ⚠️ That fallback only exists on a **stateful** legacy connection. A legacy + * client reaching a server started with `modern` is served statelessly — a + * fresh instance per request that never saw `initialize` — so it has no + * declaration to read and is refused. That is why the strict showcase configs + * come one per era. + */ +function assertClientDeclaredSkills( + mcpServer: McpServer, + method: string, + envelope: ReceivedEnvelope, +): void { + 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`, + ); + } +} + /** * Wire `skills/list`, `skills/get` and the `skill://` half of `resources/read` * onto an `McpServer`. */ -export function wireSkillsHandlers(mcpServer: McpServer): void { +export function wireSkillsHandlers( + mcpServer: McpServer, + options: WireSkillsOptions = {}, +): void { const lowLevel = mcpServer.server; + // Only the extension's own methods are gated. A `skill://` file is read + // through ordinary `resources/read`, a core method the client needs no + // extension to call. + const gate = (method: string, envelope: ReceivedEnvelope): void => { + if (options.requireClientExtension) { + assertClientDeclaredSkills(mcpServer, method, envelope); + } + }; lowLevel.setRequestHandler( "skills/list", { params: ListSkillsParamsSchema, result: ListSkillsResultShape }, - async (params) => listSkillsPage(params.cursor), + async (params, ctx) => { + gate("skills/list", ctx.mcpReq.envelope); + return listSkillsPage(params.cursor); + }, ); lowLevel.setRequestHandler( "skills/get", { params: GetSkillParamsSchema, result: GetSkillResultShape }, - async (params) => getSkillEntry(params.uri), + async (params, ctx) => { + gate("skills/get", ctx.mcpReq.envelope); + return getSkillEntry(params.uri); + }, ); lowLevel.setRequestHandler( "resources/directory/read", { params: DirectoryReadParamsSchema, result: DirectoryReadResultShape }, - async (params) => { + async (params, ctx) => { + gate("resources/directory/read", ctx.mcpReq.envelope); const page = readDirectoryPage(params.uri, params.cursor); // `-32602` for both "no such URI" and "exists but is not a directory", // which is what SEP-2640 specifies — the same code `resources/read` uses From 07411047ccb33853eea25b3ade13a92d2bfb5c56 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 14 Sep 2026 08:54:06 -0400 Subject: [PATCH 2/5] fix: answer a modern strict refusal with -32021 and cover the configs (#2374 review) SEP-2575 gives a modern request that needs an undeclared capability its own error, -32021 MissingRequiredClientCapability with data.requiredCapabilities; -32601 there claims the method does not exist. The strict fixture now throws MissingRequiredClientCapabilityError on the modern leg and keeps -32601 on legacy, which has no such code. The negative-control test now asserts the code and data per era, and a new integration test resolves both checked-in strict configs through loadConfig/resolveConfig, so a misspelled key or a dropped mapping fails instead of leaving the manual fixtures permissive. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Q3RsrgCxVoFPokLB4vAkxi Signed-off-by: cliffhall --- .../mcp/inspectorClient-skills.test.ts | 33 ++++-- .../mcp/skills-strict-configs.test.ts | 100 ++++++++++++++++++ docs/test-servers.md | 11 +- test-servers/src/skills.ts | 22 ++-- 4 files changed, 149 insertions(+), 17 deletions(-) create mode 100644 clients/web/src/test/integration/mcp/skills-strict-configs.test.ts 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); } /** From 224384971142847c83acf6faf3cb96d50452d3b0 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 14 Sep 2026 09:07:58 -0400 Subject: [PATCH 3/5] docs: bring comments and docs in line with -32021 and the Skills registry entry (#2374 review) Round 2 found text the PR's own changes made stale: the fixture's option doc, handler summary and repro step still promised -32601 on modern; the core skills.ts header still said Skills must never be in ADVERTISABLE_EXTENSIONS; the listSkills and getSkillResult comments still said the codec validates none of a skills result, or that the result arrives as the server sent it; and two tests and a docs step still described Tasks as the only registry extension. The backfill test also now asserts the Skills declaration is on by default. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Q3RsrgCxVoFPokLB4vAkxi Signed-off-by: cliffhall --- .../core/mcp/inspectorClient-skills.test.ts | 5 +++-- ...spectorClient-advertised-extensions.test.ts | 5 +++-- .../inspectorClient-coverage-backfill.test.ts | 8 +++++--- core/mcp/inspectorClient.ts | 18 +++++++++++------- core/mcp/skills.ts | 15 +++++++++------ core/mcp/skillsSchemas.ts | 5 ++++- docs/test-servers.md | 6 ++++-- test-servers/src/composable-test-server.ts | 11 +++++++---- test-servers/src/skills.ts | 4 +++- 9 files changed, 49 insertions(+), 28 deletions(-) diff --git a/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts b/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts index c8215362..602d501b 100644 --- a/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts +++ b/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts @@ -150,8 +150,9 @@ describe("InspectorClient skills methods (#2234)", () => { it("requires the modern list envelope on a modern connection", async () => { // SEP-2640: "In protocol versions 2026-07-28 and later, the result also - // carries … `ttlMs` and `cacheScope`." Nothing else validates it — - // `skills/*` is consumer-owned, so the SDK codec never sees it. + // carries … `ttlMs` and `cacheScope`." The SDK codec checks and removes + // `resultType` on every modern result, but for a consumer-owned method it + // checks neither caching attribute — so this schema is what does. const client = makeClient(); internals(client).protocolEra = "modern"; stubRequest(client, { skills: [] }); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-advertised-extensions.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-advertised-extensions.test.ts index 97ca51c4..fc7f2c44 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-advertised-extensions.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-advertised-extensions.test.ts @@ -117,8 +117,9 @@ describe("advertised-extension gating (#1739)", () => { it("omits the gated tool when the extension is not advertised", async () => { const started = await startGatedServer(); - // Disable the only registry extension → the client sends no extensions, - // so the server never enables the gated tool. + // Disable the Tasks extension → the client still sends the other registry + // extensions (MCP Apps UI, Skills), but not the one this tool is gated on, + // so the server never enables it. const connected = await connect(started.url, { [TASKS_EXTENSION_KEY]: false, }); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-coverage-backfill.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-coverage-backfill.test.ts index 18ff9258..afbdff41 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-coverage-backfill.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-coverage-backfill.test.ts @@ -880,10 +880,11 @@ describe("InspectorClient coverage backfill", () => { }); describe("constructor capability branches and createReceiverTask options", () => { - it("advertises only the tasks extension when no other capabilities are set", async () => { + it("still advertises the registry extensions when no other capabilities are set", async () => { // sample:false + elicit:false + no roots + no receiverTasks → the only - // advertised capability is the always-on Tasks extension (#1631), so - // `capabilities` is non-empty and is always attached. + // advertised capabilities are the default-on registry extensions (Tasks + // since #1631, Skills since #2373), so `capabilities` is non-empty and + // is always attached. client = new InspectorClient( { type: "stdio", @@ -901,6 +902,7 @@ describe("InspectorClient coverage backfill", () => { expect(caps.elicitation).toBeUndefined(); expect(caps.roots).toBeUndefined(); expect(caps.extensions?.["io.modelcontextprotocol/tasks"]).toBeDefined(); + expect(caps.extensions?.["io.modelcontextprotocol/skills"]).toBeDefined(); await client.connect(); expect(client.getStatus()).toBe("connected"); }); diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index d3c6aacb..04293238 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -5739,11 +5739,13 @@ export class InspectorClient extends InspectorClientEventTarget { ...(cursor !== undefined ? { cursor } : {}), }; // Era-aware: a modern (2026-07-28+) `skills/list` result also carries the - // base list envelope (`resultType` / `ttlMs` / `cacheScope`). `skills/*` is - // consumer-owned, so the SDK codec validates none of it — without picking - // the schema here a modern server could answer `{ skills: [] }` and the - // conformance UI would show a clean list. Legacy stays permissive: those - // are 2026-era attributes. + // base list envelope (`resultType` / `ttlMs` / `cacheScope`). The two halves + // are checked in different places: the SDK codec checks `resultType` on + // every modern result and removes it before this schema runs (#2373), but + // `skills/*` is consumer-owned, so it checks neither caching attribute — + // without picking the schema here a modern server could answer + // `{ skills: [] }` and the conformance UI would show a clean list. Legacy + // stays permissive: those are 2026-era attributes. const resultSchema = this.isModernEra() ? ModernListSkillsResultSchema : ListSkillsResultSchema; @@ -5773,8 +5775,10 @@ export class InspectorClient extends InspectorClientEventTarget { } /** - * `skills/get` as the server sent it — the `{ skill }` envelope **and any - * other members it carried**. + * `skills/get` as the SDK decoded it — the `{ skill }` envelope **and any + * other members the server sent**, such as the caching attributes. On a + * modern connection `resultType` is not among them: the codec checks it and + * removes it before the result gets here (#2373). * * Separate from {@link getSkill} because the callers differ: the UIs want the * entry, while the CLI prints the result and must not reshape it. SEP-2640 diff --git a/core/mcp/skills.ts b/core/mcp/skills.ts index fb7b9113..cacb8f9f 100644 --- a/core/mcp/skills.ts +++ b/core/mcp/skills.ts @@ -11,12 +11,15 @@ * shared by every client, and produce a structured finding list rather than a * boolean — a report is useful, "invalid" is not. * - * ⚠️ Skills is a **server-declared** extension, read off the connecting server's - * `capabilities.extensions`. It deliberately does NOT belong in - * `ADVERTISABLE_EXTENSIONS` (`core/mcp/extensions.ts`), which is the catalog of - * extensions the *Inspector* advertises and the user toggles in Server Settings. - * The precedent is `appElicitation.ts`, which reads the server side the same - * way; getting it backwards would put a meaningless toggle in Server Settings. + * ⚠️ Skills is negotiated from **both** sides (SEP-2133), and the two halves + * live in different places. This module reads the *server's* declaration off + * the connecting server's `capabilities.extensions` — that is what gates the + * Skills screen. The *Inspector's* own declaration is a separate entry in + * `ADVERTISABLE_EXTENSIONS` (`core/mcp/extensions.ts`): a strict server refuses + * `skills/*` to a client that did not declare it, and turning that toggle off + * in Server Settings is how to check a server's refusal path (#2373). An + * earlier revision of this comment said Skills must never be in that registry; + * that was the missing client declaration #2373 fixed. * * The Inspector is an inspector, not a host: a `resources/read` of a `SKILL.md` * is explicitly not a load and confers no standing, so none of the SEP's host diff --git a/core/mcp/skillsSchemas.ts b/core/mcp/skillsSchemas.ts index ed21db36..a91cb2ed 100644 --- a/core/mcp/skillsSchemas.ts +++ b/core/mcp/skillsSchemas.ts @@ -181,7 +181,10 @@ export const GetSkillEnvelopeSchema = z.looseObject({ }); /** - * The `skills/get` result as the server sent it, envelope and all. + * The `skills/get` result envelope as the SDK decoded it: the `skill` wrapper + * plus any caching attributes the server sent. On a modern connection + * `resultType` is not in it — the codec checks it and removes it before this + * schema runs (#2373). * * Exported alongside the unwrapping schema below because the two callers want * different things: the UIs want the entry, while the CLI's job is to print diff --git a/docs/test-servers.md b/docs/test-servers.md index 87718044..e40057bd 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -105,7 +105,9 @@ than one for both: `io.modelcontextprotocol/clientCapabilities` `_meta` entry (Modern). 2. Open **Server Settings → Advertised Extensions**, uncheck **Skills (io.modelcontextprotocol/skills)**, and reconnect. -3. `skills/list` now fails with `-32601` — the refusal a strict server sends. +3. `skills/list` now fails — with `-32021` MissingRequiredClientCapability on + the modern file and `-32601` on the legacy one, the refusal a strict server + sends in each era. **Both `skills/*` results carry the full modern base envelope** (`resultType` / `ttlMs` / `cacheScope`). They are consumer-owned methods, so the SDK stamps @@ -528,7 +530,7 @@ Requiredness is a property of the **expression**, not the variable: RFC 6570 dro 1. Connect — the Inspector advertises the Tasks extension by default, so the Tools list shows both `echo` and `get_weather`. 2. Open **Server Settings → Advertised Extensions**, uncheck **Tasks (io.modelcontextprotocol/tasks)**, and reconnect. -3. The client now advertises no extensions, the server never enables `get_weather`, and the Tools list shows only `echo`. +3. The client no longer advertises the Tasks extension (it still advertises the others, such as MCP Apps UI and Skills), the server never enables `get_weather`, and the Tools list shows only `echo`. This is the debugging knob for a server legitimately changing tool registration based on what the client advertises. Legacy stateful leg only — the modern per-request leg has no persistent `oninitialized`. diff --git a/test-servers/src/composable-test-server.ts b/test-servers/src/composable-test-server.ts index fef18353..013e9624 100644 --- a/test-servers/src/composable-test-server.ts +++ b/test-servers/src/composable-test-server.ts @@ -615,11 +615,14 @@ export interface ServerConfig { */ skills?: boolean; /** - * With {@link ServerConfig.skills}, answer `skills/list`, `skills/get` and - * `resources/directory/read` with `-32601` unless the client declared + * With {@link ServerConfig.skills}, refuse `skills/list`, `skills/get` and + * `resources/directory/read` unless the client declared * `io.modelcontextprotocol/skills` in its own capabilities — a strict - * SEP-2133 server (#2373). Off by default so the existing skills fixtures - * keep serving any client. Ignored without `skills`. + * SEP-2133 server (#2373). A modern request is refused with `-32021` + * MissingRequiredClientCapability carrying `data.requiredCapabilities`; a + * legacy one, whose era has no such code, with `-32601`. Off by default so + * the existing skills fixtures keep serving any client. Ignored without + * `skills`. */ skillsRequireClientExtension?: boolean; /** diff --git a/test-servers/src/skills.ts b/test-servers/src/skills.ts index b65a3b8a..eddc7c98 100644 --- a/test-servers/src/skills.ts +++ b/test-servers/src/skills.ts @@ -729,7 +729,9 @@ function hasSkillsExtension(capabilities: unknown): boolean { } /** - * Throw `-32601` when the client did not declare the Skills extension. + * Refuse the request when the client did not declare the Skills extension: + * `-32021` MissingRequiredClientCapability for a modern request, `-32601` for + * a legacy one. * * The two eras keep the declaration in different places. A modern * (2026-07-28) request carries it in its own `_meta` envelope, which the SDK From e53f6a1bf085785a15859720dae0aefe84469e54 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 14 Sep 2026 09:12:08 -0400 Subject: [PATCH 4/5] test: await each strict refusal before starting the next (#2374) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The negative control started all three refused calls up front and awaited them in turn, so the later rejections sat unhandled while the first was awaited. Vitest reports that as an unhandled error and fails the run even with every assertion passing — which is how local:gate went red on a green suite. Start each call only when it is awaited. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Q3RsrgCxVoFPokLB4vAkxi Signed-off-by: cliffhall --- .../integration/mcp/inspectorClient-skills.test.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) 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 48cea122..fb7dc821 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts @@ -158,13 +158,16 @@ describe("Skills extension over a real transport (#2234)", () => { }, } : { code: -32601 }; + // Thunks, not promises: starting all three up front leaves the later + // rejections unhandled while the first is awaited, which fails the + // run even though every assertion passes. const refusals = [ - connected.listSkills(), - connected.getSkill("skill://data-analysis/SKILL.md"), - connected.readResourceDirectory("skill://data-analysis"), + () => connected.listSkills(), + () => connected.getSkill("skill://data-analysis/SKILL.md"), + () => connected.readResourceDirectory("skill://data-analysis"), ]; for (const refusal of refusals) { - await expect(refusal).rejects.toMatchObject({ + await expect(refusal()).rejects.toMatchObject({ ...expected, message: expect.stringMatching( /requires the client to declare io\.modelcontextprotocol\/skills/, From f3e0edd25730f1a65287976afa52f01ed0d8a387 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 14 Sep 2026 09:25:16 -0400 Subject: [PATCH 5/5] docs: describe DirectoryReadResultSchema as the shape both eras share (#2374 review) Its doc comment still called it the legacy result, though both eras now parse with it once the codec has removed resultType. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Q3RsrgCxVoFPokLB4vAkxi Signed-off-by: cliffhall --- core/mcp/skillsSchemas.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/core/mcp/skillsSchemas.ts b/core/mcp/skillsSchemas.ts index a91cb2ed..2904c408 100644 --- a/core/mcp/skillsSchemas.ts +++ b/core/mcp/skillsSchemas.ts @@ -235,8 +235,10 @@ export type GetSkillResult = SkillEntry; export const DirectoryChildSchema = ResourceSchema; /** - * `resources/directory/read` result on a **legacy** connection: the directory's - * direct children plus the opaque cursor. + * `resources/directory/read` result on **either** era: the directory's direct + * children plus the opaque cursor. On a modern connection this is the shape + * after the SDK codec has checked and removed `resultType`, which is why no + * modern variant exists — see the note below (#2373). * * `looseObject`, matching every other result schema here: a server that also * sends the caching attributes is not wrong for doing so, and a schema is not