modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix: declare the skills extension in the Inspector's client capabilities (#2373)

Browse source 
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q3RsrgCxVoFPokLB4vAkxi
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-14 08:36:37 -04:00
commit 1d765b8af6
4 changed files with 396 additions and 150 deletions
Download patch file Download diff file Expand all files Collapse all files

61
clients/web/src/test/core/mcp/extensions.test.ts
View file

@ -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", () => {

31
clients/web/src/test/core/mcp/inspectorClient-skills.test.ts
View file

@ -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,
);

79
clients/web/src/test/core/mcp/skillsSchemas.test.ts
View file

@ -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,
);

93
clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts
View file

@ -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<TestServerHttp> {
async function startSkillsServer(
modern: boolean,
requireClientExtension = false,
): Promise<TestServerHttp> {
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<string, boolean>,
): Promise<InspectorClient> {
// 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.