From 1d765b8af661a6b6732d4b0f7d90ee7149750404 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 14 Sep 2026 08:36:37 -0400 Subject: [PATCH] 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