diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 909625ac..e4842135 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -127,10 +127,15 @@ export interface SkillsScreenProps { /** Fetch one skill file's contents via `resources/read`, on demand. */ onReadSkillFile: (uri: string) => Promise; /** - * Re-fetch the selected entry through `skills/get` (SEP-2640). Distinct from - * the entry `skills/list` already returned, and the point of exercising it is - * that the two must agree: a server whose `skills/get` disagrees with its own - * listing is broken in a way only a side-by-side fetch can show. + * Re-fetch the selected entry through `skills/get` (SEP-2640) — the + * extension's second required method, which nothing else in the app calls. + * + * It is a **fresh point-in-time snapshot**, so a conforming result may + * legitimately differ from an older listing, and the screen presents a + * difference as an updated snapshot rather than a fault. What it does treat + * as a fault is the fetched entry being non-conforming in its own right, or + * answering for a different URI than the one requested — neither of which a + * fresh read excuses. */ onGetSkill: (uri: string) => Promise; } diff --git a/clients/web/src/test/core/mcp/skills.test.ts b/clients/web/src/test/core/mcp/skills.test.ts index eb4258d9..f4ea62e2 100644 --- a/clients/web/src/test/core/mcp/skills.test.ts +++ b/clients/web/src/test/core/mcp/skills.test.ts @@ -88,10 +88,16 @@ describe("getSkillsExtension", () => { ).toBeUndefined(); }); - it("treats a non-object declaration as declared with no sub-options", () => { - expect(getSkillsExtension(caps({ [SKILLS_EXTENSION_KEY]: true }))).toEqual({ - directoryRead: false, - }); + it("rejects a non-object declaration", () => { + // SEP-2133 declares an extension as an object of sub-options, so a + // primitive is not a declaration — and treating one as support would show + // the Skills tab and send `skills/list` to a server that never claimed to + // serve it. Same parsing as the UI extension in `appElicitation.ts`. + for (const declared of [true, false, "skills", 1]) { + expect( + getSkillsExtension(caps({ [SKILLS_EXTENSION_KEY]: declared })), + ).toBeUndefined(); + } }); it("isSkillsExtensionSupported mirrors presence", () => { diff --git a/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts new file mode 100644 index 00000000..3b53d6a3 --- /dev/null +++ b/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts @@ -0,0 +1,183 @@ +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 { getSkillsExtension } from "@inspector/core/mcp/skills.js"; +import { ManagedSkillsState } from "@inspector/core/mcp/state/managedSkillsState.js"; +import { + createTestServerHttp, + type TestServerHttp, + createTestServerInfo, +} from "@modelcontextprotocol/inspector-test-server"; + +/** + * Live coverage of the Skills extension (SEP-2640, #2234) over a real + * transport against the real fixture. + * + * Everything else that covers this feature stubs the seam it is about: the + * client unit tests replace `client.request`, the screen tests mock the + * callbacks, and the store tests use a fake client. That leaves precisely the + * integration-sensitive claims unguarded — that `skills/list` and `skills/get` + * can be served through the SDK's **public** `setRequestHandler` for a + * consumer-owned method, that the fixture's `resources/read` wrapper answers + * `skill://` URIs while leaving other URIs to the SDK, that the cursor walk + * actually pages, and that all of it works on **both** protocol eras. Each of + * those is an assertion about the SDK's behavior, so only a real connection + * can check it. + * + * The era coverage is the point of the parameterization: `skills/*` are in + * neither era codec, which is *why* one fixture is expected to serve both + * legs — and that expectation had no test until this one. + */ +describe("Skills extension over a real transport (#2234)", () => { + let client: InspectorClient | null = null; + const servers: TestServerHttp[] = []; + + afterEach(async () => { + if (client) { + try { + await client.disconnect(); + } catch { + // ignore + } + client = null; + } + while (servers.length) { + const s = servers.pop(); + try { + await s?.stop(); + } catch { + // ignore + } + } + }); + + async function startSkillsServer(modern: boolean): Promise { + const started = createTestServerHttp({ + serverInfo: createTestServerInfo("skills-integration", "1.0.0"), + // An ordinary resource alongside the skills, so the fixture's + // `resources/read` wrapper is proven to DELEGATE rather than swallow. + resources: [ + { + name: "plain", + uri: "foobar://plain", + mimeType: "text/plain", + text: "plain", + }, + ], + skills: true, + ...(modern && { modern: {} }), + }); + await started.start(); + servers.push(started); + return started; + } + + async function connect( + url: string, + modern: boolean, + ): Promise { + const connected = new InspectorClient( + { + type: "streamable-http", + url, + ...(modern && { protocolEra: "modern" as const }), + }, + { environment: { transport: createTransportNode } }, + ); + await connected.connect(); + client = connected; + return connected; + } + + for (const modern of [false, true]) { + const era = modern ? "modern" : "legacy"; + + describe(`on the ${era} era`, () => { + it("advertises the extension in its capabilities", async () => { + const started = await startSkillsServer(modern); + const connected = await connect(started.url, modern); + // Bare, per the fixture: no `directoryRead` until phase 3 serves it. + expect(getSkillsExtension(connected.getCapabilities())).toEqual({ + directoryRead: false, + }); + }); + + it("serves skills/list as a paged walk", async () => { + const started = await startSkillsServer(modern); + const connected = await connect(started.url, modern); + + 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. + // + // The fixture pages at two, so a client that stops here sees half. + expect(first.skills).toHaveLength(2); + expect(first.nextCursor).toBeDefined(); + + const second = await connected.listSkills(first.nextCursor); + expect(second.skills).toHaveLength(2); + expect(second.nextCursor).toBeUndefined(); + }); + + it("walks every page through the managed store", async () => { + const started = await startSkillsServer(modern); + const connected = await connect(started.url, modern); + const store = new ManagedSkillsState(connected); + try { + const skills = await store.refresh(); + expect(skills.map((s) => s.frontmatter.name)).toEqual([ + "data-analysis", + "tampered-notes", + "dynamic-report", + "right-name", + ]); + expect(store.getPagination()).toEqual({ pageCount: 2 }); + } finally { + store.destroy(); + } + }); + + it("serves skills/get for one entry", async () => { + const started = await startSkillsServer(modern); + const connected = await connect(started.url, modern); + const entry = await connected.getSkill( + "skill://data-analysis/SKILL.md", + ); + expect(entry.frontmatter.name).toBe("data-analysis"); + expect(Array.isArray(entry.resources)).toBe(true); + }); + + it("answers -32602 for an unknown skill uri", async () => { + const started = await startSkillsServer(modern); + const connected = await connect(started.url, modern); + await expect( + connected.getSkill("skill://nope/SKILL.md"), + ).rejects.toThrow(/Unknown skill uri/); + }); + + it("reads a skill file through resources/read", async () => { + const started = await startSkillsServer(modern); + const connected = await connect(started.url, modern); + const read = await connected.readResource( + "skill://data-analysis/reference.md", + ); + const block = read.result.contents[0]; + expect(block.uri).toBe("skill://data-analysis/reference.md"); + expect("text" in block && block.text).toContain("Column rules"); + }); + + it("still serves an ordinary resource — the wrapper delegates", async () => { + // The one thing the `resources/read` wrap must not break. + const started = await startSkillsServer(modern); + const connected = await connect(started.url, modern); + const read = await connected.readResource("foobar://plain"); + expect(read.result.contents[0].uri).toBe("foobar://plain"); + }); + }); + } +}); diff --git a/core/mcp/skills.ts b/core/mcp/skills.ts index aaf4145c..5d488374 100644 --- a/core/mcp/skills.ts +++ b/core/mcp/skills.ts @@ -109,9 +109,13 @@ export function getSkillsExtension( capabilities: ServerCapabilities | undefined, ): SkillsExtensionSupport | undefined { const declared = capabilities?.extensions?.[SKILLS_EXTENSION_KEY]; - if (declared === undefined || declared === null) return undefined; + // Must be an OBJECT. SEP-2133 declares an extension as an object of + // sub-options, so a primitive (`false`, `"skills"`) is not a declaration — + // and treating one as support would show the Skills tab and send + // `skills/list` to a server that never claimed to serve it. Matches how + // `appElicitation.ts` parses the UI extension. + if (typeof declared !== "object" || declared === null) return undefined; const directoryRead = - typeof declared === "object" && (declared as { directoryRead?: unknown }).directoryRead === true; return { directoryRead }; }