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

feat: require the modern list envelope on skills/list (review round 14)

Browse source 
The review finally quoted the normative text, which is what I asked for
across five rounds before declining: SEP-2640's `skills/list` section says
"In protocol versions 2026-07-28 and later, the result also carries …
`ttlMs` and `cacheScope`." So the era-aware validation is in.

- skillsSchemas: `ModernListSkillsResultSchema` = the page plus the base
  list envelope, with field shapes mirroring `ModernResultEnvelopeSchema`
  in `listSalvage.ts` so this repo's two statements of a modern envelope
  cannot drift. `InspectorClient.listSkills` picks it from the negotiated
  era; the legacy shape stays permissive, because those are 2026-era
  attributes a legacy server has no business sending.
  Nothing else catches this: `skills/*` is consumer-owned, so it is absent
  from the SDK's cacheable-method registry and a modern server answering
  `{ skills: [] }` was reaching the conformance UI as a clean list.
- skills.ts: count `frontmatter.description` (and `name`) length in Unicode
  CODE POINTS. `String.length` counts UTF-16 code units, so 600 non-BMP
  characters measured as 1200 and a perfectly valid description was
  reported `malformed-description` — a conforming server failed by an
  off-by-encoding, which is the direction this module works hardest to
  avoid. The Agent Skills reference validator uses Python `len()`.
- #2248 narrowed: the `skills/list` half is settled here; what remains is
  whether `skills/get` carries the same attributes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-05 02:01:18 -04:00
commit 11ac134975
3 changed files with 178 additions and 27 deletions
Download patch file Download diff file Expand all files Collapse all files

12
core/mcp/inspectorClient.ts
View file

@ -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 },

21
core/mcp/skills.ts
View file

@ -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) {

68
core/mcp/skillsSchemas.ts
View file

@ -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<typeof SkillEntrySchema>;
/**
* `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<typeof ListSkillsResultSchema>;
/**
* `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`.
*