From 721d31426d80adabd0ec9843597944e8b610df97 Mon Sep 17 00:00:00 2001 From: olaservo Date: Tue, 20 Oct 2026 05:58:11 -0700 Subject: [PATCH] fix(skills): require ttlMs and cacheScope on modern resources/directory/read results (#2740) The stable ext-skills spec (ext-skills#157) makes ReadResourceDirectoryResult a CacheableResult, so a modern (2026-07-28+) result must carry ttlMs and cacheScope, as skills/list and skills/get already do. The Inspector parsed both eras with one permissive schema and reported a modern server omitting them as clean; the test server's fixture sent resultType alone and was itself non-conformant on the modern leg. - core/mcp/skillsSchemas.ts: add ModernDirectoryReadResultSchema and replace the "no modern variant" note with the new citation. - core/mcp/inspectorClient.ts: readResourceDirectory picks the schema from the negotiated era, as listSkills and getSkillResult do. - test-servers/src/skills.ts: readDirectoryPage and DirectoryReadResultShape send and type the full modern envelope. - Tests mirror the #2404 coverage for skills/get: schema unit cases, client era cases, and an integration case on both eras. - docs: test-servers.md and the roadmap no longer describe the old asymmetry. Co-Authored-By: Claude Fable 5.1 Signed-off-by: olaservo --- .../core/mcp/inspectorClient-skills.test.ts | 46 +++++++++ .../src/test/core/mcp/skillsSchemas.test.ts | 99 ++++++++++++++++--- .../mcp/inspectorClient-skills.test.ts | 12 +++ core/mcp/inspectorClient.ts | 17 +++- core/mcp/skillsSchemas.ts | 74 ++++++++------ docs/inspector-roadmap-2026-h2.md | 2 +- docs/test-servers.md | 20 ++-- test-servers/src/skills.ts | 25 +++-- 8 files changed, 218 insertions(+), 77 deletions(-) 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 bb94a54674..77eb4f0cae 100644 --- a/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts +++ b/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts @@ -327,6 +327,52 @@ describe("InspectorClient skills methods (#2234)", () => { // server; the live check is in the integration suite. const client = makeClient(); internals(client).protocolEra = "modern"; + stubRequest(client, { + resources: [CHILD], + ttlMs: 0, + cacheScope: "public", + }); + declareSkills(client, true); + await expect( + client.readResourceDirectory("skill://demo"), + ).resolves.toMatchObject({ resources: [CHILD] }); + }); + + it("requires the caching attributes on a modern resources/directory/read (#2740)", async () => { + // The stable ext-skills spec makes `ReadResourceDirectoryResult` a + // `CacheableResult`. The SDK codec checks neither attribute for a + // consumer-owned method, so without the modern schema this server would + // read as conforming. + const client = makeClient(); + internals(client).protocolEra = "modern"; + stubRequest(client, { resources: [CHILD] }); + declareSkills(client, true); + await expect( + client.readResourceDirectory("skill://demo"), + ).rejects.toBeDefined(); + }); + + it("returns the modern caching attributes whole — the CLI prints the result as received", async () => { + const client = makeClient(); + internals(client).protocolEra = "modern"; + stubRequest(client, { + resources: [CHILD], + ttlMs: 5, + cacheScope: "private", + }); + declareSkills(client, true); + await expect( + client.readResourceDirectory("skill://demo"), + ).resolves.toEqual({ + resources: [CHILD], + ttlMs: 5, + cacheScope: "private", + }); + }); + + it("does NOT require the caching attributes on a legacy connection", async () => { + // 2026-era attributes a legacy server has no business sending. + const client = makeClient(); stubRequest(client, { resources: [CHILD] }); declareSkills(client, true); await expect( diff --git a/clients/web/src/test/core/mcp/skillsSchemas.test.ts b/clients/web/src/test/core/mcp/skillsSchemas.test.ts index a9bb3c4800..b6940910a7 100644 --- a/clients/web/src/test/core/mcp/skillsSchemas.test.ts +++ b/clients/web/src/test/core/mcp/skillsSchemas.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest"; import { DIRECTORY_MIME_TYPE, DirectoryReadResultSchema, + ModernDirectoryReadResultSchema, RESOURCES_DIRECTORY_READ_METHOD, DYNAMIC_RESOURCES, GetSkillEnvelopeSchema, @@ -237,30 +238,98 @@ describe("directory read schemas (#2248)", () => { DirectoryReadResultSchema.safeParse({ resources: "nope" }).success, ).toBe(false); }); +}); + +describe("resources/directory/read caching attributes (#2740)", () => { + const CHILD = { + uri: "skill://demo/templates/invoice.md", + name: "invoice.md", + mimeType: "text/markdown", + }; + const CACHE = { ttlMs: 0, cacheScope: "public" } as const; - 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. + it("the LEGACY schema accepts a result with the caching attributes and one without", () => { + // They are 2026-era attributes: a legacy server must not be failed for + // their absence, nor for sending them. expect(DirectoryReadResultSchema.safeParse({ resources: [] }).success).toBe( true, ); - expect(ModernListSkillsResultSchema.safeParse({ skills: [] }).success).toBe( - false, - ); + expect( + DirectoryReadResultSchema.safeParse({ resources: [], ...CACHE }).success, + ).toBe(true); }); - 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. + it("the MODERN schema requires ttlMs and cacheScope", () => { + // The stable ext-skills spec: `ReadResourceDirectoryResult` extends + // `CacheableResult`, so both are REQUIRED, as on `resources/list`. expect( - DirectoryReadResultSchema.safeParse({ + ModernDirectoryReadResultSchema.safeParse({ resources: [] }).success, + ).toBe(false); + expect( + ModernDirectoryReadResultSchema.safeParse({ resources: [], ttlMs: 0 }) + .success, + ).toBe(false); + expect( + ModernDirectoryReadResultSchema.safeParse({ resources: [], - ttlMs: 60, - cacheScope: "public", + cacheScope: "private", }).success, - ).toBe(true); + ).toBe(false); + }); + + it("the MODERN schema accepts the spec's directory-read example as the SDK codec delivers it (#2373)", () => { + // The stable spec's examples carry `ttlMs: 300000, cacheScope: "public"`. + // No `resultType`: the codec lifts it before any caller schema runs. + const parsed = ModernDirectoryReadResultSchema.parse({ + resources: [CHILD], + nextCursor: "7", + ttlMs: 300_000, + cacheScope: "public", + }); + expect(parsed.resources).toEqual([CHILD]); + expect(parsed.nextCursor).toBe("7"); + expect(parsed.ttlMs).toBe(300_000); + expect(parsed.cacheScope).toBe("public"); + }); + + it.each([-1, 0.5])( + "the MODERN schema rejects ttlMs %s — not a non-negative integer", + (ttlMs) => { + expect( + ModernDirectoryReadResultSchema.safeParse({ + resources: [], + ttlMs, + cacheScope: "public", + }).success, + ).toBe(false); + }, + ); + + it("the MODERN schema rejects a cacheScope outside public/private", () => { + expect( + ModernDirectoryReadResultSchema.safeParse({ + resources: [], + ttlMs: 0, + cacheScope: "shared", + }).success, + ).toBe(false); + }); + + it("the MODERN schema still requires the resources page", () => { + expect( + ModernDirectoryReadResultSchema.safeParse({ ...CACHE }).success, + ).toBe(false); + }); + + it("requires the attributes on every modern skills result alike", () => { + // The three modern schemas now agree; the asymmetry the earlier revision + // documented is gone. + expect(ModernListSkillsResultSchema.safeParse({ skills: [] }).success).toBe( + false, + ); + expect( + ModernDirectoryReadResultSchema.safeParse({ resources: [] }).success, + ).toBe(false); }); }); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts index e8322365a8..aeab38bd5e 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts @@ -326,6 +326,18 @@ describe("Skills extension over a real transport (#2234)", () => { ]); }); + it("carries the caching attributes on resources/directory/read (#2740)", async () => { + const started = await startSkillsServer(modern); + const connected = await connect(started.url, modern); + // On the modern leg `readResourceDirectory` selects + // `ModernDirectoryReadResultSchema`, so this resolving is itself the + // requirement; the fixture stamps both eras alike. + const page = await connected.readResourceDirectory( + "skill://data-analysis", + ); + expect(page).toMatchObject({ ttlMs: 0, cacheScope: "public" }); + }); + it("lists a dynamic skill's files, which is what the method is for", async () => { // `dynamic-report` advertises no manifest, so a directory read is the // only way its files are discoverable at all — the case SEP-2640 says diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 5d535036a5..5c918cd464 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -182,6 +182,7 @@ import { import { buildClientExtensions } from "./extensions.js"; import { DirectoryReadResultSchema, + ModernDirectoryReadResultSchema, GetSkillEnvelopeSchema, ModernGetSkillEnvelopeSchema, ListSkillsResultSchema, @@ -5056,16 +5057,22 @@ export class InspectorClient extends InspectorClientEventTarget { // for page one. ...(cursor !== undefined ? { cursor } : {}), }; - // One schema for both eras (#2373): the SDK codec enforces and lifts - // `resultType` on a modern connection, and SEP-2640 requires no caching - // attributes of this method — see `skillsSchemas.ts` for why that differs - // from `skills/list`. + // Era-aware, like `listSkills` and `getSkillResult` (#2740): the stable + // ext-skills spec makes `ReadResourceDirectoryResult` a `CacheableResult`, + // so a modern result must carry `ttlMs` / `cacheScope`. The SDK codec + // checks and lifts `resultType` (#2373) but, for a consumer-owned method, + // neither caching attribute — so this schema is the only thing that does. + // Legacy stays permissive. The result is returned whole: the CLI prints it + // as received, so the attributes reach its output. + const resultSchema = this.isModernEra() + ? ModernDirectoryReadResultSchema + : DirectoryReadResultSchema; try { return await this.invokeMcpClient( () => this.client!.request( { method: RESOURCES_DIRECTORY_READ_METHOD, params }, - DirectoryReadResultSchema, + resultSchema, this.getRequestOptions(this.progressTokenOf(metadata)), ), { method: RESOURCES_DIRECTORY_READ_METHOD }, diff --git a/core/mcp/skillsSchemas.ts b/core/mcp/skillsSchemas.ts index e0f2f36f5d..e915f91007 100644 --- a/core/mcp/skillsSchemas.ts +++ b/core/mcp/skillsSchemas.ts @@ -24,8 +24,9 @@ * 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` or `skills/get` result requires the caching attributes (see - * `ModernListSkillsResultSchema` and `ModernGetSkillEnvelopeSchema`). + * `skills/list`, `skills/get` or `resources/directory/read` result requires + * the caching attributes (see `ModernListSkillsResultSchema`, + * `ModernGetSkillEnvelopeSchema` and `ModernDirectoryReadResultSchema`). */ import { z } from "zod/v4"; @@ -244,46 +245,55 @@ export type GetSkillResult = SkillEntry; export const DirectoryChildSchema = ResourceSchema; /** - * `resources/directory/read` result on **either** era: the directory's direct - * children plus the opaque cursor. On a modern connection this is the shape - * after the SDK codec has checked and removed `resultType`, which is why no - * modern variant exists — see the note below (#2373). + * `resources/directory/read` result on a **legacy** connection: the directory's + * direct children plus the opaque cursor, and nothing required beyond that. * - * `looseObject`, matching every other result schema here: a server that also - * sends the caching attributes is not wrong for doing so, and a schema is not - * the place to reject an extra member. + * Its modern counterpart is {@link ModernDirectoryReadResultSchema}. The + * caching attributes are 2026-era, so the legacy shape stays permissive, + * exactly as {@link ListSkillsResultSchema} and {@link GetSkillEnvelopeSchema} + * do: `looseObject`, so a legacy server that also sends them is not failed for + * doing so. */ export const DirectoryReadResultSchema = z.looseObject({ resources: z.array(DirectoryChildSchema), nextCursor: z.string().optional(), }); +/** + * The result as the SDK decoded it, on either era. Typed from the legacy + * schema, whose caching attributes are optional: the modern schema only + * narrows them to required, so either era's result fits — the same reason + * {@link GetSkillEnvelope} is typed from the legacy envelope. + */ export type DirectoryReadResult = z.infer; -/* - * There is deliberately **no modern variant** of the `resources/directory/read` - * schema, though `skills/list` and `skills/get` each have one (#2373, #2404). +/** + * `resources/directory/read` result on a **modern** (2026-07-28+) connection: + * the page plus `CacheableResult`'s required `ttlMs` / `cacheScope` (#2740). * - * On a modern (2026-07-28+) connection the SDK codec already enforces - * `resultType` — the base-protocol member SEP-2322 puts on every modern result - * — and lifts it off before any caller schema runs. What remains is the - * caching attributes, and only the two `skills/*` results are required to - * carry those: + * SEP-2640 as accepted said nothing about caching attributes on this method, + * and its one worked example carried `resultType` alone — so an earlier + * revision of this module deliberately had no modern variant here, on the + * reading that requiring more than the SEP's own example would fail a + * conforming server. The stable ext-skills specification settles it the way + * it settled `skills/get` (#2404): *"`ReadResourceDirectoryResult` extends + * `CacheableResult`, so `ttlMs` and `cacheScope` are **REQUIRED**, as they + * are on `resources/list`, and carry the same semantics. They describe the + * freshness of the directory listing, not of the held entry"* (ext-skills#157, + * recorded in its `docs/decisions.md`), and its directory-read examples now + * carry `"ttlMs": 300000, "cacheScope": "public"`. * - * - For `skills/list` the SEP states it outright — *"In protocol versions - * 2026-07-28 and later, the result also carries … `ttlMs` and - * `cacheScope`"* — so {@link ModernListSkillsResultSchema} requiring them is - * quoting the spec. - * - For `skills/get` the stable ext-skills specification states that - * `GetSkillResult` extends `CacheableResult`, so - * {@link ModernGetSkillEnvelopeSchema} requires them too. - * - For `resources/directory/read` it states **nothing of the kind**, and its - * one worked example carries `resultType: "complete"` and no caching - * attributes at all. Requiring them would fail a server that matched the - * SEP's own example, the failure direction this module works hardest to - * avoid. + * Same field shapes as {@link ModernListSkillsResultSchema}, and the same + * reason it exists: this is a consumer-owned method, so the SDK codec checks + * and lifts `resultType` (#2373) but checks neither caching attribute, and + * without this a modern server omitting them would be reported as clean. * - * So both eras parse that result with one schema. An earlier revision required - * `resultType` on "modern" variants; since the codec lifts it, they could never - * pass on a real modern connection. + * ⚠️ Picked by `InspectorClient.readResourceDirectory` from the negotiated + * era — a schema cannot know it on its own. */ +export const ModernDirectoryReadResultSchema = DirectoryReadResultSchema.extend( + { + ttlMs: z.int().min(0), + cacheScope: z.enum(["public", "private"]), + }, +); diff --git a/docs/inspector-roadmap-2026-h2.md b/docs/inspector-roadmap-2026-h2.md index 2d0aee9230..3e4998c541 100644 --- a/docs/inspector-roadmap-2026-h2.md +++ b/docs/inspector-roadmap-2026-h2.md @@ -194,7 +194,7 @@ where the Inspector is thinnest over the SDK. Watch closely. | Feature | Confidence | Notes | | ------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| **Cache hint display** — `ttlMs` / `cacheScope` on the SEP-2549 surfaces (`tools/list`, `prompts/list`, `resources/list`, `resources/templates/list`, `resources/read`), plus modern (2026-07-28+) `skills/list` and `skills/get`, which the stable ext-skills spec requires to carry both fields (both validated as required on modern connections since [#2404](https://github.com/modelcontextprotocol/inspector/issues/2404)); legacy results do not require the fields but are shown when a server sends them, with freshness countdown and "stale" marking | 🟢 | SEP-2549 is Final. The runtime parses the hints everywhere and honors them through the SDK cache for the four `*/list` methods; `resources/read`, `skills/list` and `skills/get` go through plain requests that validate but do not honor them, so this item includes that plumbing as well as the display. | +| **Cache hint display** — `ttlMs` / `cacheScope` on the SEP-2549 surfaces (`tools/list`, `prompts/list`, `resources/list`, `resources/templates/list`, `resources/read`), plus modern (2026-07-28+) `skills/list`, `skills/get` and `resources/directory/read`, which the stable ext-skills spec requires to carry both fields (validated as required on modern connections since [#2404](https://github.com/modelcontextprotocol/inspector/issues/2404) and [#2740](https://github.com/modelcontextprotocol/inspector/issues/2740)); legacy results do not require the fields but are shown when a server sends them, with freshness countdown and "stale" marking | 🟢 | SEP-2549 is Final. The runtime parses the hints everywhere and honors them through the SDK cache for the four `*/list` methods; `resources/read`, `skills/list`, `skills/get` and `resources/directory/read` go through plain requests that validate but do not honor them, so this item includes that plumbing as well as the display. | | **Cache behavior observations** — note a re-fetch of a still-fresh result, and a list that changed inside its declared TTL, as diagnostics rather than errors | 🟢 | Inspector-shaped: nobody else observes both the hint and the reality. `ttlMs` is a freshness hint, so both are compliant. | | **Stateful-tool workflow investigation** — how to help a user carry an SEP-2567-style handle from one tool result into the next call | 🟡 | Replaces the first draft's "session lifecycle lane". The protocol has no concept of a handle (it is ordinary tool data), so a generic view would be inference; investigate before designing. | | **ETag support** — send `If-None-Match`, show 304s and version changes | 🔴 | Watch until a SEP reaches Draft with an SDK impl. | diff --git a/docs/test-servers.md b/docs/test-servers.md index 1b073f760f..d3c7bea80b 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -120,16 +120,16 @@ 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. -⚠️ **`resources/directory/read` deliberately carries `resultType` alone.** -SEP-2640 states the caching attributes for a modern `skills/list` in as many -words and says nothing of the kind for this method, whose one worked example -carries `resultType` and nothing else. A fixture sending more than the SEP shows -would make a client that wrongly *required* them look correct, which is the -opposite of what a conformance fixture is for — so `readDirectoryPage` stops -where the spec does. On the client, the SDK's modern codec enforces -`resultType` and lifts it off before the Inspector's own schema runs, so -`DirectoryReadResultSchema` serves both eras and requires no caching attributes -([#2373](https://github.com/modelcontextprotocol/inspector/issues/2373)). +`resources/directory/read` carries the same full envelope. SEP-2640 as accepted +showed `resultType` alone for this method, and the fixture once stopped where +the SEP did; the stable ext-skills spec makes `ReadResourceDirectoryResult` a +`CacheableResult`, so `ttlMs` / `cacheScope` are required there too +([#2740](https://github.com/modelcontextprotocol/inspector/issues/2740)). On +the client, the SDK's modern codec enforces `resultType` and lifts it off +before the Inspector's own schema runs +([#2373](https://github.com/modelcontextprotocol/inspector/issues/2373)), and +`ModernDirectoryReadResultSchema` requires the two caching attributes on a +modern connection, as the `skills/list` and `skills/get` schemas do. It works on **either era**: `skills/list`, `skills/get` and `resources/directory/read` are consumer-owned extension methods that neither diff --git a/test-servers/src/skills.ts b/test-servers/src/skills.ts index eddc7c988c..e9228e163e 100644 --- a/test-servers/src/skills.ts +++ b/test-servers/src/skills.ts @@ -600,13 +600,11 @@ export function readDirectoryPage( const from = Number.isFinite(start) && start > 0 ? start : 0; const next = from + DIRECTORY_PAGE_SIZE; return { - // `resultType` ONLY — deliberately not the full `MODERN_RESULT_ENVELOPE` - // the two `skills/*` results carry. SEP-2640 requires `ttlMs`/`cacheScope` - // of a modern `skills/list` in as many words and says nothing of the kind - // here, and its one worked example of a directory result carries - // `resultType` alone. A fixture that sent more than the SEP shows would - // make a client that wrongly required them look correct. - resultType: MODERN_RESULT_ENVELOPE.resultType, + // The full envelope, as the two `skills/*` results carry. SEP-2640 as + // accepted showed `resultType` alone here, so this fixture once sent only + // that; the stable ext-skills spec makes `ReadResourceDirectoryResult` a + // `CacheableResult`, with `ttlMs`/`cacheScope` REQUIRED (#2740). + ...MODERN_RESULT_ENVELOPE, resources: children.slice(from, next), ...(next < children.length ? { nextCursor: String(next) } : {}), }; @@ -681,15 +679,14 @@ const GetSkillResultShape = z.object({ }); /** - * The `resources/directory/read` result. Carries `resultType` from the modern - * envelope but **not** `ttlMs` / `cacheScope`: SEP-2640 states those for - * `skills/list` and says nothing about them here, and its one worked example of - * a directory result omits them. The fixture matches the SEP's example so a - * client that requires more than the spec asks for fails against it — which is - * the whole point of a conformance fixture. + * The `resources/directory/read` result: the full modern envelope, like the + * two `skills/*` results. The stable ext-skills spec makes + * `ReadResourceDirectoryResult` a `CacheableResult`, so `ttlMs` / + * `cacheScope` are required and a fixture sending `resultType` alone would be + * non-conformant on the modern leg (#2740). */ const DirectoryReadResultShape = z.object({ - resultType: ModernEnvelopeShape.resultType, + ...ModernEnvelopeShape, resources: z.array( z.object({ uri: z.string(),