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