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 a68b2379..779fdaf6 100644 --- a/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts +++ b/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts @@ -27,6 +27,7 @@ describe("InspectorClient skills methods (#2234)", () => { }; interface SkillsInternals { + protocolEra: string | undefined; client: { request: ( req: { method: string; params: Record }, @@ -146,6 +147,40 @@ describe("InspectorClient skills methods (#2234)", () => { ).rejects.toBeDefined(); }); + 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. + const client = makeClient(); + internals(client).protocolEra = "modern"; + stubRequest(client, { skills: [] }); + await expect(client.listSkills()).rejects.toBeDefined(); + }); + + it("accepts a modern result that carries the envelope", async () => { + const client = makeClient(); + internals(client).protocolEra = "modern"; + stubRequest(client, { + resultType: "complete", + ttlMs: 0, + cacheScope: "public", + skills: [ENTRY], + }); + await expect(client.listSkills()).resolves.toMatchObject({ + skills: [ENTRY], + }); + }); + + it("does NOT require the envelope on a legacy connection", async () => { + // Those are 2026-era attributes; failing a legacy server for their absence + // would reject a conforming server. + const client = makeClient(); + stubRequest(client, { skills: [ENTRY] }); + await expect(client.listSkills()).resolves.toMatchObject({ + skills: [ENTRY], + }); + }); + it("rejects a skills/list result that is not a skills page", async () => { // The explicit result schema is the whole client-side mechanism for a // consumer-owned extension method, so a nonconforming result must fail diff --git a/clients/web/src/test/core/mcp/skills.test.ts b/clients/web/src/test/core/mcp/skills.test.ts index 6bea4241..eb4258d9 100644 --- a/clients/web/src/test/core/mcp/skills.test.ts +++ b/clients/web/src/test/core/mcp/skills.test.ts @@ -378,6 +378,24 @@ describe("checkSkillConformance", () => { expect(issues[0].severity).toBe("error"); }); + it("counts description length in code points, not UTF-16 code units", () => { + // 600 non-BMP characters are 1200 code units. Measuring those would report + // a perfectly valid description as over the 1024-character limit — a + // conforming server failed by an off-by-encoding. + const issues = checkSkillConformance( + entry({ frontmatter: { name: "demo", description: "𝄞".repeat(600) } }), + ); + expect(issues).toEqual([]); + }); + + it("still reports a description over the limit in code points", () => { + const issues = checkSkillConformance( + entry({ frontmatter: { name: "demo", description: "𝄞".repeat(1025) } }), + ); + expect(issues.map((i) => i.code)).toEqual(["malformed-description"]); + expect(issues[0].message).toContain("1025"); + }); + it("accepts a description exactly at the limit", () => { const issues = checkSkillConformance( entry({ frontmatter: { name: "demo", description: "d".repeat(1024) } }), diff --git a/clients/web/src/test/core/mcp/skillsSchemas.test.ts b/clients/web/src/test/core/mcp/skillsSchemas.test.ts index 724c98cc..085e60b8 100644 --- a/clients/web/src/test/core/mcp/skillsSchemas.test.ts +++ b/clients/web/src/test/core/mcp/skillsSchemas.test.ts @@ -3,6 +3,7 @@ import { DYNAMIC_RESOURCES, GetSkillResultSchema, ListSkillsResultSchema, + ModernListSkillsResultSchema, SKILLS_EXTENSION_KEY, SKILLS_GET_METHOD, SKILLS_LIST_METHOD, @@ -87,6 +88,56 @@ describe("ListSkillsResultSchema", () => { }); }); +describe("ModernListSkillsResultSchema", () => { + const envelope = { resultType: "complete", ttlMs: 0, cacheScope: "public" }; + + it("accepts a modern page carrying the base list envelope", () => { + const parsed = ModernListSkillsResultSchema.parse({ + ...envelope, + skills: [ENTRY], + }); + expect(parsed.skills).toHaveLength(1); + }); + + 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. + expect(() => ModernListSkillsResultSchema.parse({ skills: [] })).toThrow(); + expect(() => + ModernListSkillsResultSchema.parse({ + ...envelope, + ttlMs: undefined, + skills: [], + }), + ).toThrow(); + }); + + it("rejects a malformed ttlMs rather than accepting the envelope loosely", () => { + for (const ttlMs of [-1, 0.5]) { + expect(() => + ModernListSkillsResultSchema.parse({ ...envelope, ttlMs, skills: [] }), + ).toThrow(); + } + }); + + it("rejects an unknown cacheScope", () => { + expect(() => + ModernListSkillsResultSchema.parse({ + ...envelope, + cacheScope: "shared", + skills: [], + }), + ).toThrow(); + }); + + it("the LEGACY schema still accepts a page without the envelope", () => { + // Those are 2026-era attributes; a legacy server has no business sending + // them and must not be failed for their absence. + expect(ListSkillsResultSchema.parse({ skills: [] }).skills).toEqual([]); + }); +}); + describe("GetSkillResultSchema", () => { it("unwraps the envelope to the entry", () => { expect(GetSkillResultSchema.parse({ skill: ENTRY })).toEqual(ENTRY); diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 07a9cc6d..70b95eb7 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -147,6 +147,7 @@ import { buildClientExtensions } from "./extensions.js"; import { GetSkillResultSchema, ListSkillsResultSchema, + ModernListSkillsResultSchema, SKILLS_GET_METHOD, SKILLS_LIST_METHOD, type SkillEntry, @@ -5561,11 +5562,20 @@ export class InspectorClient extends InspectorClientEventTarget { // conforming server made to look broken. ...(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. + const resultSchema = this.isModernEra() + ? ModernListSkillsResultSchema + : ListSkillsResultSchema; const response = await this.invokeMcpClient( () => this.client!.request( { method: SKILLS_LIST_METHOD, params }, - ListSkillsResultSchema, + resultSchema, this.getRequestOptions(this.progressTokenOf(metadata)), ), { method: SKILLS_LIST_METHOD }, diff --git a/core/mcp/skills.ts b/core/mcp/skills.ts index 02e3ab6c..aaf4145c 100644 --- a/core/mcp/skills.ts +++ b/core/mcp/skills.ts @@ -71,6 +71,21 @@ const SKILL_NAME_MAX_LENGTH = 64; /** The Agent Skills limit on `frontmatter.description`. */ const SKILL_DESCRIPTION_MAX_LENGTH = 1024; +/** + * Length in Unicode **code points**, not UTF-16 code units. + * + * `String.prototype.length` counts code units, so every non-BMP character + * (emoji, many CJK extension characters) counts twice — a perfectly valid + * 600-character description would be measured as 1200 and reported as + * `malformed-description`. The Agent Skills limit is in characters, and its + * reference validator uses Python's `len()`, which counts code points. Getting + * this wrong fails a conforming server, which is the direction this module + * works hardest to avoid. + */ +function codePointLength(value: string): number { + return [...value].length; +} + /** * What the server declared under `io.modelcontextprotocol/skills`. The only * sub-option SEP-2640 defines is `directoryRead`, which gates @@ -269,7 +284,7 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { message: "frontmatter.name is required but missing or empty.", }); } else if ( - declaredName.length > SKILL_NAME_MAX_LENGTH || + codePointLength(declaredName) > SKILL_NAME_MAX_LENGTH || !SKILL_NAME_PATTERN.test(declaredName) ) { // Reaches here for `" demo "` too: the grammar sees the untrimmed value. @@ -288,11 +303,11 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { severity: "error", message: "frontmatter.description is required but missing or empty.", }); - } else if (rawDescription.length > SKILL_DESCRIPTION_MAX_LENGTH) { + } else if (codePointLength(rawDescription) > SKILL_DESCRIPTION_MAX_LENGTH) { issues.push({ code: "malformed-description", severity: "error", - message: `frontmatter.description is ${rawDescription.length} characters, above the ${SKILL_DESCRIPTION_MAX_LENGTH}-character limit.`, + message: `frontmatter.description is ${codePointLength(rawDescription)} characters, above the ${SKILL_DESCRIPTION_MAX_LENGTH}-character limit.`, }); } if (uriName === undefined) { diff --git a/core/mcp/skillsSchemas.ts b/core/mcp/skillsSchemas.ts index 2ebb7181..bb3a1160 100644 --- a/core/mcp/skillsSchemas.ts +++ b/core/mcp/skillsSchemas.ts @@ -11,17 +11,21 @@ * hatch that modern `tasks/*` needs is deliberately NOT used here — `tasks/*` * are spec names the 2026 codec deleted, which is a different problem. * - * **This module is the whole wire surface.** SEP-2640 is Accepted, so the method - * names and the entry shape are settled, but the skill *format* is delegated to - * the independently-versioned Agent Skills specification and the SEP leaves the - * `skills/get` caching attributes (SEP-2549 `ttlMs` / `cacheScope`) open. Keeping - * every wire type here makes a spec revision a single-file edit (#2234). + * **This module is the wire surface for the two methods the Inspector calls.** + * SEP-2640 is Accepted, so the method names and the entry shape are settled; + * the skill *format* is delegated to the independently-versioned Agent Skills + * specification. Keeping every wire type here makes a spec revision a + * single-file edit (#2234). * - * Schemas are deliberately permissive (`looseObject`, and a `digest` typed as a - * plain string rather than a hex-constrained one) so a non-conforming server is - * *surfaced* rather than rejected — the Inspector is a conformance tool, and a - * malformed digest is a finding to report, not a parse error to swallow. The - * structural checks live in `skills.ts`. + * **Permissive where a defect should be reported; strict where a shape is + * settled.** `looseObject`, and a `digest` typed as a plain string rather than + * a hex-constrained one, so a non-conforming server is *surfaced* by + * `checkSkillConformance` rather than rejected at the parse — a malformed + * digest is a finding, not a parse error to swallow. But a settled shape is + * enforced, because silently normalizing one past the checks would defeat the + * point: `skills/get` requires its `{ skill }` envelope, and a modern + * `skills/list` result requires the base list envelope (see + * `ModernListSkillsResultSchema`). */ import { z } from "zod/v4"; @@ -97,19 +101,8 @@ export const SkillEntrySchema = z.looseObject({ export type SkillEntry = z.infer; /** - * `skills/list` result: a page of entries plus the opaque cursor. - * - * ⚠️ **Whether a modern-era (2026-07-28) result must also carry the SEP-2549 - * caching attributes `ttlMs` / `cacheScope` is unsettled here and deliberately - * not guessed.** #2234's analysis records it as an open point; a review of that - * PR asserted the opposite. Neither reading was checked against the normative - * text, and the two mistakes are not symmetric: leaving the schema permissive - * means a server that omits them is accepted (they pass through untouched when - * sent), while tightening on a wrong reading would *reject* conforming - * responses. `resources/directory/read` was removed from this module for the - * same reason. #2248 settles it against the spec. Note the SDK is no help - * either way — `skills/list` is consumer-owned, so it is absent from the - * cacheable-method registry and nothing stamps or validates these fields. + * `skills/list` result on a **legacy** connection: a page of entries plus the + * opaque cursor, and nothing required beyond that. */ export const ListSkillsResultSchema = z.looseObject({ skills: z.array(SkillEntrySchema), @@ -118,6 +111,35 @@ export const ListSkillsResultSchema = z.looseObject({ export type ListSkillsResult = z.infer; +/** + * `skills/list` result on a **modern** (2026-07-28+) connection: the page plus + * the base list envelope. + * + * SEP-2640's `skills/list` section states: *"In protocol versions 2026-07-28 + * and later, the result also carries … `ttlMs` and `cacheScope`."* The field + * shapes mirror `ModernResultEnvelopeSchema` in `listSalvage.ts`, which is this + * repo's existing statement of a modern result envelope and is already applied + * to modern list results on the salvage path — so the two cannot drift. + * + * The era split is load-bearing rather than defensive. `skills/*` is a + * consumer-owned method, so it is absent from the SDK's cacheable-method + * registry and **nothing else validates this**: without an era-aware schema a + * modern server could answer `{ skills: [] }` and the conformance UI would + * present it as a clean list. The legacy shape stays permissive because these + * are 2026-era attributes a legacy server has no business sending. + * + * ⚠️ Picked by `InspectorClient.listSkills` from the negotiated era — a schema + * cannot know it on its own. + */ +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. + ttlMs: z.int().min(0), + cacheScope: z.enum(["public", "private"]), +}); + /** * The `skills/get` result envelope: the entry wrapped under `skill`. *