From 754ce5bd95c7123dafd259fa7b99401bb6c82dcf Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 23:11:46 -0400 Subject: [PATCH] fix: address Copilot review round 5 on #2251 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of these reverse round 4, which asked for the opposite. Where the readings conflict I took the permissive one: a wrong tightening rejects conforming servers, a wrong loosening only fails to report something. - skills.ts: stop requiring the `skill:` scheme. SEP-2640 says a server SHOULD use `skill://` and explicitly allows a domain-native scheme, so round 4's requirement handed a conforming `github://…` skill a false `malformed-uri` and skipped its name and root checks. Containment is scheme-independent — it compares against THIS entry's own root — so the traversal fix that requirement came packaged with is unaffected. - skills.ts: the 512-entry and 16 MiB limits are warnings. Both are SHOULD NOTs for a server and MAYs for a host, so calling them errors contradicted the "error = a MUST was broken" contract round 4 established and told authors a permitted skill was invalid. - skills.ts: `skillEntriesMatch` compares canonically — keys sorted, manifest sorted by URI. `JSON.stringify` treated key order and manifest order as differences, though neither carries meaning. - SkillsScreen: a differing `skills/get` is "a different snapshot" (yellow), not a disagreement (red). The SEP defines it as a fresh point-in-time read, so a skill that changed since the listing legitimately differs. - useServerCommands: route `onGetSkill` through the command-scoped OAuth recovery like every other server command. An expired authorization rendered an error and stopped there — no reauthorization, no retry. - test-servers/skills.ts: stamp the modern base result envelope (`resultType` / `ttlMs` / `cacheScope`) on every skills result. The SDK stamps it only for methods in its own codec, and `skills/*` are consumer-owned, so a 2026-era connection was receiving results without it. Unconditional rather than per era: the modern leg builds a server per request, so there is no era to branch on at registration time, and on legacy they are three members no codec inspects. - composable-test-server: remove the `skills.directoryRead` option. It was publicly settable and advertised a method nothing serves, producing the exact false capability the fixture exists to help catch. `skills` is now a plain boolean; the flag returns in phase 3 (#2248) with its handler. Still not changed: whether a modern `skills/list` RESULT SCHEMA must require that envelope. Stamping it server-side is free; requiring it client-side would reject servers on a reading nobody has checked against the normative text. #2248 owns settling it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY Signed-off-by: cliffhall --- .../SkillsScreen/SkillsScreen.test.tsx | 35 ++++++-- .../screens/SkillsScreen/SkillsScreen.tsx | 39 +++++---- .../web/src/hooks/useServerCommands.test.tsx | 51 ++++++++++++ clients/web/src/hooks/useServerCommands.tsx | 20 ++++- clients/web/src/test/core/mcp/skills.test.ts | 38 +++++---- core/mcp/skills.ts | 82 +++++++++++++++---- docs/test-servers.md | 32 +++++--- test-servers/configs/skills-http.json | 8 +- test-servers/src/composable-test-server.ts | 20 +++-- test-servers/src/load-config.ts | 4 +- test-servers/src/skills.ts | 24 +++++- 11 files changed, 272 insertions(+), 81 deletions(-) diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 54ca1a25..19cdb382 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -437,7 +437,7 @@ describe("SkillsScreen", () => { expect(within(issues).getAllByText("duplicate-resource")).toHaveLength(2); }); - it("fetches the selected entry through skills/get and reports agreement", async () => { + it("fetches the selected entry through skills/get and reports a match", async () => { // The acceptance criterion this exists for: `skills/get` is one of the two // methods the extension requires, and a server author's handler is only // exercisable if something actually calls it. @@ -450,13 +450,36 @@ describe("SkillsScreen", () => { ); expect(onGetSkill).toHaveBeenCalledWith(CLEAN_SKILL.uri); expect( - await screen.findByText("skills/get agrees with skills/list"), + await screen.findByText("skills/get matches skills/list"), ).toBeInTheDocument(); }); - it("reports a skills/get entry that disagrees with the listing", async () => { - // Both describe the same skill, so a disagreement is a server bug that - // only a side-by-side fetch can surface. + it("treats key and manifest order as immaterial when matching", async () => { + // The manifest is a set and JSON key order carries no meaning, so a server + // that enumerates either differently is not inconsistent — a + // `JSON.stringify` comparison would have called it one. + const user = userEvent.setup(); + const onGetSkill = vi.fn().mockResolvedValue({ + resources: [...CLEAN_SKILL.resources].reverse(), + frontmatter: { + description: CLEAN_SKILL.frontmatter.description, + name: CLEAN_SKILL.frontmatter.name, + }, + uri: CLEAN_SKILL.uri, + }); + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + await user.click( + screen.getByRole("button", { name: /Fetch with skills\/get/ }), + ); + expect( + await screen.findByText("skills/get matches skills/list"), + ).toBeInTheDocument(); + }); + + it("reports a skills/get entry that differs from the listing", async () => { + // Shown, but not called an error: `skills/get` is a fresh snapshot, so a + // skill that genuinely changed since the listing legitimately differs. const user = userEvent.setup(); const onGetSkill = vi.fn().mockResolvedValue({ ...CLEAN_SKILL, @@ -468,7 +491,7 @@ describe("SkillsScreen", () => { screen.getByRole("button", { name: /Fetch with skills\/get/ }), ); expect( - await screen.findByText("skills/get disagrees with skills/list"), + await screen.findByText("skills/get returned a different snapshot"), ).toBeInTheDocument(); // The fetched entry is rendered beside the verdict so the difference is // inspectable rather than merely asserted. (Its JSON goes through diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 7f801460..b083d5ea 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -23,6 +23,7 @@ import { DYNAMIC_RESOURCES } from "@inspector/core/mcp/skillsSchemas.js"; import { checkSkillConformance, skillDisplayName, + skillEntriesMatch, totalSkillBytes, verifySkillResource, type SkillIssue, @@ -82,13 +83,13 @@ interface PreviewState { /** * The result of the on-demand `skills/get`, plus the manifest it belongs to. - * `agrees` records whether the fetched entry matched the one `skills/list` - * returned — the reason for making the call at all. + * `matches` records whether the fetched entry describes the same skill as the + * one `skills/list` returned — the reason for making the call at all. */ interface FetchedEntryState { key: string | null; entry?: SkillEntry; - agrees?: boolean; + matches?: boolean; message?: string; } @@ -483,12 +484,14 @@ export function SkillsScreen({ // started under so a late answer cannot land under another skill. void onGetSkill(selected.uri) .then((entry) => { - // Compared field-by-field against what `skills/list` advertised. The - // two describe the same skill, so a disagreement is a server bug that - // only shows up when both are fetched. - const agrees = JSON.stringify(entry) === JSON.stringify(selected); + // Compared semantically against what `skills/list` advertised — see + // `skillEntriesMatch` for why a `JSON.stringify` comparison would + // report key order and manifest order as differences. + const matches = skillEntriesMatch(entry, selected); setFetchedEntry((prev) => - prev.key !== null && prev.key !== key ? prev : { key, entry, agrees }, + prev.key !== null && prev.key !== key + ? prev + : { key, entry, matches }, ); }) .catch((err: unknown) => { @@ -767,20 +770,24 @@ export function SkillsScreen({ {fetched?.entry !== undefined && ( - {fetched.agrees - ? "The entry this server returns for this URI is identical to the one it listed." - : "The entry this server returns for this URI differs from the one it listed; both describe the same skill, so one of them is wrong."} + {fetched.matches + ? "The entry this server returns for this URI describes the same skill it listed (compared ignoring key and manifest order)." + : "The entry this server returns for this URI differs from the one it listed. `skills/get` is a fresh snapshot, so this is expected if the skill changed since the list was fetched — and a server inconsistency if it did not."} - {!fetched.agrees && ( + {!fetched.matches && ( { "Client is not connected", ); }); + + it("retries once after a satisfied recovery", async () => { + // Every server command routes through the shared recovery; without it an + // expired authorization would render an error and stop there, with no + // reauthorization and no retry. + const recover = vi.fn().mockResolvedValue(true); + const getSkill = vi + .fn() + .mockRejectedValueOnce(authError()) + .mockResolvedValue({ + uri: "skill://demo/SKILL.md", + frontmatter: {}, + resources: [], + }); + const h = harness({ + client: client({ getSkill }), + activeServerId: "a", + recovery: { handleCommandScopedAuthRecovery: recover }, + }); + await expect( + h.api().onGetSkill("skill://demo/SKILL.md"), + ).resolves.toMatchObject({ uri: "skill://demo/SKILL.md" }); + expect(getSkill).toHaveBeenCalledTimes(2); + }); + + it("rethrows when the recovery was not satisfied", async () => { + const recover = vi.fn().mockResolvedValue(false); + const h = harness({ + client: client({ getSkill: vi.fn().mockRejectedValue(authError()) }), + activeServerId: "a", + recovery: { handleCommandScopedAuthRecovery: recover }, + }); + await expect( + h.api().onGetSkill("skill://demo/SKILL.md"), + ).rejects.toBeInstanceOf(AuthRecoveryRequiredError); + }); + + it("rethrows a non-auth failure untouched", async () => { + const recover = vi.fn(); + const h = harness({ + client: client({ + getSkill: vi.fn().mockRejectedValue(new Error("-32602")), + }), + activeServerId: "a", + recovery: { handleCommandScopedAuthRecovery: recover }, + }); + await expect(h.api().onGetSkill("skill://demo/SKILL.md")).rejects.toThrow( + "-32602", + ); + expect(recover).not.toHaveBeenCalled(); + }); }); describe("onRefreshSkills (#2234)", () => { diff --git a/clients/web/src/hooks/useServerCommands.tsx b/clients/web/src/hooks/useServerCommands.tsx index 73eaa194..1fb7f052 100644 --- a/clients/web/src/hooks/useServerCommands.tsx +++ b/clients/web/src/hooks/useServerCommands.tsx @@ -982,9 +982,25 @@ export function useServerCommands({ const onGetSkill = useCallback( async (uri: string): Promise => { if (!inspectorClient) throw new Error("Client is not connected"); - return inspectorClient.getSkill(uri); + // Routed through the shared recovery like every other server command + // (#2174). Without it an expired authorization renders an error in the + // panel and stops there — no reauthorization, no retry — which is the + // one thing this hook exists to make impossible. + const get = () => inspectorClient.getSkill(uri); + try { + return await get(); + } catch (err) { + if (err instanceof AuthRecoveryRequiredError && activeServerId) { + const satisfied = await handleCommandScopedAuthRecovery(err, { + serverId: activeServerId, + source: "resource", + }); + if (satisfied) return get(); + } + throw err; + } }, - [inspectorClient], + [inspectorClient, activeServerId, handleCommandScopedAuthRecovery], ); const onRefreshSkills = useCallback(() => { diff --git a/clients/web/src/test/core/mcp/skills.test.ts b/clients/web/src/test/core/mcp/skills.test.ts index 4fd5f527..9787d3ba 100644 --- a/clients/web/src/test/core/mcp/skills.test.ts +++ b/clients/web/src/test/core/mcp/skills.test.ts @@ -107,16 +107,18 @@ describe("skillNameFromUri", () => { ); }); + it("reads the name off a domain-native scheme too", () => { + expect(skillNameFromUri("github://acme/repo/data-analysis/SKILL.md")).toBe( + "data-analysis", + ); + }); + it("returns undefined for a URI that does not end in /SKILL.md", () => { expect(skillNameFromUri("skill://demo/other.md")).toBeUndefined(); // The suffix must include the separator: a bare "SKILL.md" has no segment. expect(skillNameFromUri("SKILL.md")).toBeUndefined(); }); - it("returns undefined for a non-skill scheme", () => { - expect(skillNameFromUri("https://demo/SKILL.md")).toBeUndefined(); - }); - it("returns undefined for a relative string", () => { // SEP-2640 requires a full resource URI; treating `demo/SKILL.md` as one // would let a non-conforming entry report a name and pass the path check. @@ -145,12 +147,16 @@ describe("normalizeSkillUri", () => { expect(normalizeSkillUri("demo/SKILL.md")).toBeUndefined(); }); - it("rejects a non-skill scheme", () => { - // Checking only that a URI is hierarchical would let this through and then - // pass the name and root checks — a manifest pointing anywhere on the web, - // reported as conforming. - expect(normalizeSkillUri("https://demo/SKILL.md")).toBeUndefined(); - expect(normalizeSkillUri("file:///demo/SKILL.md")).toBeUndefined(); + it("does not privilege the skill: scheme", () => { + // SEP-2640 only says a server SHOULD use `skill://`, and explicitly allows + // a domain-native scheme — so rejecting one would hand a conforming server + // a false `malformed-uri` and skip its name and root checks. + expect(normalizeSkillUri("github://acme/repo/SKILL.md")).toBe( + "github://acme/repo/SKILL.md", + ); + expect(normalizeSkillUri("https://demo/a/../SKILL.md")).toBe( + "https://demo/SKILL.md", + ); }); it("rejects an opaque-path URI, which the parser does not normalize", () => { @@ -406,10 +412,13 @@ describe("checkSkillConformance", () => { ], }), ); - expect(issues.map((i) => i.code)).toContain("size-limit-exceeded"); + const finding = issues.find((i) => i.code === "size-limit-exceeded"); + expect(finding?.severity).toBe("warning"); }); - it("reports a manifest over the 512-entry limit", () => { + // Both limits are SHOULD NOTs for a server and MAYs for a host, so exceeding + // one makes a skill less portable rather than invalid. + it("reports a manifest over the 512-entry limit as a warning", () => { const resources = [ { uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 }, ...Array.from({ length: SKILL_MAX_RESOURCE_ENTRIES }, (_unused, i) => ({ @@ -419,10 +428,11 @@ describe("checkSkillConformance", () => { })), ]; const issues = checkSkillConformance(entry({ resources })); - expect(issues.map((i) => i.code)).toContain("resource-limit-exceeded"); + const finding = issues.find((i) => i.code === "resource-limit-exceeded"); + expect(finding?.severity).toBe("warning"); }); - it("reports a manifest over the 16 MiB limit", () => { + it("reports a manifest over the 16 MiB limit as a warning", () => { const issues = checkSkillConformance( entry({ resources: [ diff --git a/core/mcp/skills.ts b/core/mcp/skills.ts index 610299fe..2d9346a7 100644 --- a/core/mcp/skills.ts +++ b/core/mcp/skills.ts @@ -52,9 +52,6 @@ export const SKILL_MAX_TOTAL_BYTES = 16 * 1024 * 1024; /** The suffix every skill URI ends with; the segment before it is the name. */ export const SKILL_FILE_SUFFIX = "/SKILL.md"; -/** The URI scheme SEP-2640 defines for skills, as `URL.protocol` spells it. */ -export const SKILL_URI_SCHEME = "skill:"; - /** `sha256:` followed by exactly 64 lowercase hex characters. */ const DIGEST_PATTERN = /^sha256:[0-9a-f]{64}$/; @@ -112,9 +109,13 @@ export function isSkillsExtensionSupported( * rejected too: containment could not be decided on it, and silently accepting * one would reintroduce exactly the hole this function closes. * - * The `skill:` scheme is required. Checking only that the URI is hierarchical - * would let `https://demo/SKILL.md` through and then pass the name and root - * checks — a manifest pointing anywhere on the web, reported as conforming. + * The scheme is deliberately **not** constrained. `skill://` is what SEP-2640 + * recommends and what this repo's fixture serves, but the SEP only says servers + * SHOULD use it and explicitly allows a domain-native scheme (`github://…`), so + * requiring `skill:` would hand a conforming server a false `malformed-uri` and + * skip its name and root checks entirely. Containment is scheme-independent + * anyway: it compares a resource against *this entry's own* root, so an entry + * cannot escape its skill whatever scheme it uses. */ export function normalizeSkillUri(uri: string): string | undefined { let parsed: URL; @@ -123,7 +124,6 @@ export function normalizeSkillUri(uri: string): string | undefined { } catch { return undefined; } - if (parsed.protocol !== SKILL_URI_SCHEME) return undefined; return parsed.pathname.startsWith("/") ? parsed.href : undefined; } @@ -176,10 +176,11 @@ export type SkillIssueCode = | "size-limit-exceeded"; /** - * `error` marks a stated requirement of SEP-2640 that the server broke — - * every MUST, so a manifest reporting "0 errors" really is one the spec - * accepts. `warning` is reserved for what is **legal** yet leaves the - * Inspector unable to verify integrity: `"dynamic"` resources, the case most + * `error` marks a **MUST** of SEP-2640 that the server broke, so a manifest + * reporting "0 errors" really is one the spec accepts. `warning` covers + * everything the spec permits but a consumer still wants told about: the + * `SHOULD NOT`-exceed interoperability limits, and — above all — `"dynamic"` + * resources, which are legal and leave integrity unverifiable, the case most * worth surfacing and the one most easily buried. */ export type SkillIssueSeverity = "error" | "warning"; @@ -226,7 +227,7 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { issues.push({ code: "malformed-uri", severity: "error", - message: `Skill URI must be a "${SKILL_URI_SCHEME}//" URI ending with "${SKILL_FILE_SUFFIX}" and carrying a non-empty path segment before it.`, + message: `Skill URI must be a hierarchical URI ending with "${SKILL_FILE_SUFFIX}" and carrying a non-empty path segment before it.`, }); } else if (declaredName && uriName !== declaredName) { // The one structural invariant the spec states outright: the segment before @@ -250,19 +251,24 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { return issues; } + // Both limits are *interoperability* bounds, not MUSTs: SEP-2640 says a + // server SHOULD NOT exceed them and a host MAY support more. So they are + // warnings — calling a permitted oversized skill an error would contradict + // what `error` means here and tell a server author their skill is invalid + // when it is merely less portable. if (entry.resources.length > SKILL_MAX_RESOURCE_ENTRIES) { issues.push({ code: "resource-limit-exceeded", - severity: "error", - message: `Skill declares ${entry.resources.length} resource entries, above the ${SKILL_MAX_RESOURCE_ENTRIES}-entry limit.`, + severity: "warning", + message: `Skill declares ${entry.resources.length} resource entries, above the ${SKILL_MAX_RESOURCE_ENTRIES}-entry interoperability limit; a host is only required to support up to it.`, }); } const totalBytes = totalSkillBytes(entry.resources); if (totalBytes > SKILL_MAX_TOTAL_BYTES) { issues.push({ code: "size-limit-exceeded", - severity: "error", - message: `Skill resources total ${totalBytes} bytes, above the ${SKILL_MAX_TOTAL_BYTES}-byte (16 MiB) limit.`, + severity: "warning", + message: `Skill resources total ${totalBytes} bytes, above the ${SKILL_MAX_TOTAL_BYTES}-byte (16 MiB) interoperability limit; a host is only required to support up to it.`, }); } @@ -388,6 +394,50 @@ export function totalSkillBytes(resources: readonly SkillResource[]): number { ); } +/** + * Whether a `skills/get` entry describes the same skill as the `skills/list` + * entry alongside it, compared **semantically** rather than byte-for-byte. + * + * Two things a `JSON.stringify` comparison gets wrong here, and both would + * report a conforming server as broken: object key order is not meaningful in + * JSON, and the resource manifest is a *set*, so a server free to enumerate it + * in any order would look inconsistent for reordering it. Both sides are + * canonicalized — keys sorted recursively, manifest entries sorted by URI — + * before they are compared. + * + * A difference is still worth showing, but it is NOT by itself an error: + * SEP-2640 defines `skills/get` as a fresh point-in-time snapshot, so a skill + * that genuinely changed since the listing legitimately differs. The caller + * presents it as "the snapshot moved" and leaves the judgement to the reader. + */ +export function skillEntriesMatch(a: SkillEntry, b: SkillEntry): boolean { + return canonicalJson(a) === canonicalJson(b); +} + +/** Deterministic JSON: object keys sorted recursively, and a skill's manifest + * sorted by URI so its enumeration order carries no meaning. */ +function canonicalJson(value: unknown): string { + return JSON.stringify(canonicalize(value)); +} + +function canonicalize(value: unknown): unknown { + if (Array.isArray(value)) return value.map(canonicalize); + if (value === null || typeof value !== "object") return value; + const entries = Object.entries(value as Record) + .map(([key, member]): [string, unknown] => { + // The manifest is a set; only its contents are meaningful. + if (key === "resources" && Array.isArray(member)) { + const sorted = [...(member as SkillResource[])].sort((x, y) => + String(x?.uri).localeCompare(String(y?.uri)), + ); + return [key, sorted.map(canonicalize)]; + } + return [key, canonicalize(member)]; + }) + .sort(([x], [y]) => x.localeCompare(y)); + return Object.fromEntries(entries); +} + /** Outcome of comparing a fetched file against its advertised digest. */ export type SkillVerificationStatus = | "verified" diff --git a/docs/test-servers.md b/docs/test-servers.md index 3c1a2d22..5d4118de 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -60,13 +60,21 @@ as a missing capability rather than an error. ## Skills (SEP-2640) -`skills-http.json` advertises the `io.modelcontextprotocol/skills` extension -and serves four skills over two `skills/list` pages. It declares the extension -**bare** — `directoryRead` stays off until the fixture actually serves -`resources/directory/read` (phase 3, [#2248](https://github.com/modelcontextprotocol/inspector/issues/2248)), so -Connection Info never reports a sub-option this server would answer `-32601` -for. Set `"skills": { "directoryRead": true }` in a config to exercise the -advertised-flag path. +`skills-http.json` sets `"skills": true` and serves four skills over two +`skills/list` pages. The extension is advertised **bare**: there is deliberately +no `directoryRead` option to turn on, because nothing here serves +`resources/directory/read` and a config that advertised it would produce exactly +the false capability this fixture helps catch — Connection Info reporting a +sub-option supported while the method answers `-32601`. Both come back in +phase 3 ([#2248](https://github.com/modelcontextprotocol/inspector/issues/2248)). + +Every result carries the modern base envelope (`resultType` / `ttlMs` / +`cacheScope`). `skills/*` are consumer-owned, so the SDK stamps nothing for +them; without it a 2026-era connection would receive a result missing the +envelope. It is stamped unconditionally rather than per era — the modern leg +builds a fresh server per request, so there is no era to branch on when the +handlers are registered, and on the legacy leg they are three extra members no +codec inspects. It works on **either era**: `skills/list`, `skills/get` and `resources/directory/read` are consumer-owned extension methods that neither era codec defines, so the SDK's era gate skips them entirely — which is why @@ -82,11 +90,11 @@ the Skills tab runs are untestable without them: | `dynamic-report` | `resources: "dynamic"` — a generated file set, so integrity cannot be verified at all and the tab says so rather than staying silent. | | `wrong-folder` | A URI path segment (`wrong-folder`) that disagrees with `frontmatter.name` (`right-name`), the one structural invariant SEP-2640 states outright. | -Connection Info shows the extension and its `directoryRead` sub-flag; the -Inspector surfaces that flag but does not call `resources/directory/read` yet. -The wire schema for that result is deliberately absent from -`core/mcp/skillsSchemas.ts` too — phase 3 adds it against the normative text -rather than shipping a guess nothing exercises. +Connection Info's **Skills Extension Options** section shows the `directoryRead` +sub-flag — against this fixture, a red ✗. The Inspector surfaces the flag but +does not call `resources/directory/read` yet, and the wire schema for that +result is deliberately absent from `core/mcp/skillsSchemas.ts` too: phase 3 adds +it against the normative text rather than shipping a guess nothing exercises. ## Cancelling a call diff --git a/test-servers/configs/skills-http.json b/test-servers/configs/skills-http.json index 08fa66f1..2b105851 100644 --- a/test-servers/configs/skills-http.json +++ b/test-servers/configs/skills-http.json @@ -3,9 +3,13 @@ "name": "skills", "version": "1.0.0" }, - "tools": [{ "preset": "echo" }], + "tools": [ + { + "preset": "echo" + } + ], "resources": [], - "skills": {}, + "skills": true, "transport": { "type": "streamable-http", "port": 3230 diff --git a/test-servers/src/composable-test-server.ts b/test-servers/src/composable-test-server.ts index 5531af72..eadc45e3 100644 --- a/test-servers/src/composable-test-server.ts +++ b/test-servers/src/composable-test-server.ts @@ -561,8 +561,14 @@ export interface ServerConfig { * Advertise the Skills extension (SEP-2640) and serve `skills/list` / * `skills/get` plus the `skill://` files those entries name. The fixture set * deliberately includes non-conforming skills — see `skills.ts`. + * + * There is deliberately **no `directoryRead` option**. The flag would + * advertise `resources/directory/read`, which nothing here serves, so a + * config could produce exactly the false capability this fixture exists to + * help catch — Connection Info reporting "supported" for a method that + * answers `-32601`. It comes back in phase 3 (#2248) with the handler. */ - skills?: { directoryRead?: boolean }; + skills?: boolean; /** * Advertise the MCP Apps `io.modelcontextprotocol/ui` extension with the * nested `elicitation` setting — the server-side half of the app-rendered @@ -831,17 +837,13 @@ export function createMcpServer(config: ServerConfig): McpServer { }; } - // Skills extension (SEP-2640): a server-declared extension. `directoryRead` - // is opt-in per config and stays OFF in `skills-http.json` until the fixture - // actually serves `resources/directory/read` — advertising a sub-option this - // server would answer `-32601` for would make Connection Info report - // "Supported" for a method that is not (phase 3, #2248). + // Skills extension (SEP-2640): a server-declared extension, advertised bare. + // See `ServerConfig.skills` for why there is no `directoryRead` sub-option + // to turn on. if (config.skills) { capabilities.extensions = { ...(capabilities.extensions ?? {}), - [SKILLS_EXTENSION_KEY]: { - ...(config.skills.directoryRead ? { directoryRead: true } : {}), - }, + [SKILLS_EXTENSION_KEY]: {}, }; // Skill files are fetched through ordinary `resources/read`, so the // resources capability has to be advertised even when the config registers diff --git a/test-servers/src/load-config.ts b/test-servers/src/load-config.ts index 1b136a8c..ffb4dcab 100644 --- a/test-servers/src/load-config.ts +++ b/test-servers/src/load-config.ts @@ -68,8 +68,8 @@ export interface ConfigFile { * `transport.modern`. */ tasksExtension?: boolean; /** Advertise the Skills extension (SEP-2640) and serve its fixture skills. - * `directoryRead` advertises the `resources/directory/read` sub-option. */ - skills?: { directoryRead?: boolean }; + * No `directoryRead` sub-option — see {@link ServerConfig.skills}. */ + skills?: 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/skills.ts b/test-servers/src/skills.ts index 228595eb..49cd3071 100644 --- a/test-servers/src/skills.ts +++ b/test-servers/src/skills.ts @@ -45,6 +45,24 @@ export const SKILLS_EXTENSION_KEY = "io.modelcontextprotocol/skills"; */ export const SKILLS_PAGE_SIZE = 2; +/** + * The modern (2026-07-28) base result envelope, stamped on every skills result. + * + * The SDK stamps this for methods in its own codec, and `skills/*` are + * consumer-owned — so nothing adds it here and a modern connection would + * otherwise receive a result missing `resultType` / `ttlMs` / `cacheScope`. + * Stamped **unconditionally** rather than per era: one `McpServer` config + * serves both legs, the modern leg builds a fresh server per request so there + * is no era to branch on at handler-registration time, and on the legacy leg + * these are three unknown members that a consumer-owned method has no codec to + * reject. Values match `ModernResultEnvelopeSchema` in `core/mcp/listSalvage.ts`. + */ +const MODERN_RESULT_ENVELOPE = { + resultType: "complete", + ttlMs: 0, + cacheScope: "public", +} as const; + /** `sha256:<64 lowercase hex>` over a UTF-8 string, the SEP's digest form. */ function digestOf(text: string): string { return `sha256:${createHash("sha256").update(text, "utf8").digest("hex")}`; @@ -218,6 +236,7 @@ export function listSkillsPage(cursor?: string): Record { const page = FIXTURE_SKILLS.slice(from, from + SKILLS_PAGE_SIZE); const next = from + SKILLS_PAGE_SIZE; return { + ...MODERN_RESULT_ENVELOPE, skills: page.map(toEntry), ...(next < FIXTURE_SKILLS.length ? { nextCursor: String(next) } : {}), }; @@ -238,9 +257,9 @@ export function getSkillEntry(uri: string): Record { `Unknown skill uri: ${uri}`, ); } - // The envelope (`{ skill }`) is the conforming shape, and the only one the + // The `{ skill }` wrapper is the conforming shape, and the only one the // Inspector accepts — see `GetSkillResultSchema`. - return { skill: toEntry(skill) }; + return { ...MODERN_RESULT_ENVELOPE, skill: toEntry(skill) }; } /** The `resources/read` result for a `skill://` file, or `undefined`. */ @@ -250,6 +269,7 @@ export function readSkillFile( const file = FILES_BY_URI.get(uri); if (!file) return undefined; return { + ...MODERN_RESULT_ENVELOPE, contents: [{ uri: file.uri, mimeType: file.mimeType, text: file.text }], }; }