Repository navigation
feat: Skills extension (SEP-2640) for server and client - #3047
Open
KKonstantinov wants to merge 48 commits into
Open
KKonstantinov wants to merge 48 commits into
KKonstantinov wants to merge 48 commits into
Conversation
…, server handlers) Adds the first phase of first-class Skills extension support behind new `/ext/skills` subpath exports. Root barrels are unchanged. - `@modelcontextprotocol/core/ext/skills` — shared, runtime-neutral schemas, inferred types, wire constants and `skillsCapabilityOf()`. - `@modelcontextprotocol/client/ext/skills` — `listSkills()`, `getSkill()` and `getSkillsCapability()`, gated on the server advertising both `io.modelcontextprotocol/skills` and `resources`. - `@modelcontextprotocol/server/ext/skills` — `installSkills()` declares the extension capability and serves `skills/list` / `skills/get` from caller-provided skill definitions. The skills result schemas deliberately carry no `resultType` member: the 2026-07-28 era codec validates that discriminator on decode and consumes it before any caller-supplied result schema runs, so a schema that re-declares it can never match (#2789). Regression tests pin this end to end. Metadata surface only — filesystem discovery, digest-verified resource reads and `resources/directory/read` follow separately. Refs #2798 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`packageTopologyPins` asserts each published package's export-map keys exactly, so the new `./ext/skills` subpath on core, client and server trips it by design. Per docs/behavior-surface-pins.md the pin is updated in the same PR rather than loosened, with a note on each entry recording why the subpath is public and why it stays off the root barrel. No behavior change: this updates the expectation only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lity fix(skills): default cache hints and preserve legacy wire responses
Loose object schemas throughout, _meta on Skill, resources/directory/read params and result schemas with caching fields, SKILL_LIMITS in place of the schema-level 512-entry cap. Co-authored-by: tobi-oye <tobiloba5000@gmail.com>
Built-in YAML-subset reader for SKILL.md frontmatter (the subset the C# SDK reads, YAML 1.2 core-schema typing, fail-closed on anchors, aliases, tags, duplicate keys and deep nesting), with a differential test against the yaml library (devDependency only).
A ':' before whitespace, a flow indicator or the end of the line is an
indicator in flow collections, and a flow key that starts with '[' or '{' is
a complex key; both are now rejected instead of read as plain text. Plain
flow scalars stop at ',[]{}' and ' #'.
Runtime-neutral UTF-8, base64 and SHA-256 helpers; skill URI rules (segment encoding, canonical form, containment, reference resolution, one code-unit order); a MIME table; and entry validation and building shared by the client and server. Every tsconfig that compiles core-internal sources maps @modelcontextprotocol/core/ext/skills.
isWithin rejects segments that decode to a '/' or NUL, and skillRootUri rejects roots that end in '/' (an empty segment). Tests restore the escapes and exercise the resource sort.
One reusable, extension-shaped object per catalog: attach(mcp) declares io.modelcontextprotocol/skills, registers a skill:// resource template and the skills/list, skills/get and resources/directory/read handlers. Entries from custom sources are validated; caching fields only on 2026-07-28. Replaces installSkills from #2818. Co-authored-by: Matt Carey <mcarey@cloudflare.com>
A SkillFiles adapter (list and read over relative paths) with an in-memory implementation, and buildCatalogSnapshot: it walks the adapter, builds and checks every skill (invalid ones skipped with a warning, or collected under strict), and assembles the entries, file metadata, directories and listing a catalog serves. Metadata only; file contents are never held.
…memorySkillFiles A symlinked SKILL.md marks its folder as a skill, so it is skipped and reported like any other special file; special files outside every skill are reported too. A name that cannot be encoded as a URI skips its skill with a warning instead of aborting the build. memorySkillFiles reads paths directly and re-indexes only when the record's keys change.
memorySkillFiles re-reads its record once per walk (on list(''), where every
load and reload() starts) instead of on every call, and the catalog build
indexes the walked tree once instead of rescanning it per skill and per
directory. Output and ordering are unchanged; 10,000 files build in about
0.1 s instead of several seconds.
SkillsExtension.load(files) builds a SkillCatalog through a two-method SkillFiles adapter (memorySkillFiles for in-code files). It finds every SKILL.md, hashes the files, checks the SEP and Agent Skills rules, skips invalid skills with a warning (or fails under strict), lists only SKILL.md in resources/list, pages with position cursors and re-checks every read against the published digest. reload() is serialized and all-or-nothing, and sends resources/list_changed when the listing changes.
A connected server whose transport rejects resources/list_changed now produces a warning instead of an unhandled rejection. Adds a same-size edit test so the digest check on reads is exercised.
loadSkills('./skills') and nodeSkillFiles(root) at a Node-only subpath, so
ext/skills itself stays free of Node built-ins (pinned by barrelClean).
Symlinks are never followed: a skill containing one is skipped with a warning.
barrelClean follows relative from- and side-effect imports with a single walker and pins the root entry and ext/skills against every node: import. Adds a real-filesystem test for a symlinked skill folder and narrows the nodeSkillFiles doc to what O_NOFOLLOW guarantees.
new SkillsClient(client) reports supported and directoryRead, lists every page (bounded by listMaxPages, stopping at a repeated cursor) with invalid entries and every copy of a repeated URI returned in invalid (or thrown under strict), gets one entry and reads skill directories. Replaces listSkills and getSkill from #2818. Co-authored-by: Matt Carey <mcarey@cloudflare.com>
new SkillsClient(client) lists (every page, invalid and duplicate entries reported in invalid, or thrown under strict), gets and reads skills. read(skill, ref) resolves ref against the skill root, refuses unlisted files before any request, checks size, digest and SKILL.md frontmatter, and fetches once more with cacheMode 'refresh' when a cached copy fails. Replaces listSkills/getSkill from #2818. Co-authored-by: Matt Carey <mcarey@cloudflare.com>
…s by file key encodeSegment now emits each path segment the way WHATWG URL serialises it (always encoding % / ? # and \), so a host that resolves "references/Q&A.md" gets the published URI. SkillsClient.read matches refs to the listing by a normalised file key (fragment dropped, segments re-encoded) so servers that encode differently still verify, refuses unparsable refs as unlisted, and decides "own SKILL.md" by the same key.
…signs joinUri encodes the segment that lands in the authority with encodeURIComponent, so grouping folders and prefixes such as me@home or acme:prod keep publishing. '^' is always encoded, whatever the runtime's URL parser does. skillFileKey decodes each valid escape on its own, so names with a stray '%' match by their plain names.
A %XX that does not start a valid UTF-8 sequence is keyed as %25 plus the original two characters, exactly as encodeSegment publishes such a name, so plain-name reads of files like "10%Dec.md" match their listing.
…C errors A 2026-07-28 Streamable HTTP server answers an unimplemented method with HTTP 404 and a -32601 JSON-RPC body. The transport now surfaces that body in-band, as it already did for 400, so callers get ProtocolError(-32601) instead of an opaque SdkHttpError. Legacy-era exchanges are unchanged.
End to end through public import paths: loadSkills on a temp folder, SkillsClient verifying every file, over createMcpHandler (one server per request) and stdio, on 2025-11-25 and 2026-07-28. The wire suite pins resources/list, resources/read, resources/templates/list, skills/* and the error codes, including app-resource precedence, listResources and baseUri; the SEP-3371 middleware version must pass it unchanged. Co-authored-by: Matt Carey <mcarey@cloudflare.com>
Bumps @modelcontextprotocol/conformance to 0.2.0-alpha.12. The everything server serves a SkillsExtension catalog (built from #2797's fixture idea with the SDK's own API); the everything client loads skills through SkillsClient for the verify-digest, verify-size, verify-frontmatter and no-prefetch scenarios. Co-authored-by: tobi-oye <tobiloba5000@gmail.com>
Without --spec-version the conformance runner runs extension scenarios at 2026-07-28 for servers and 2025-11-25 for clients. Two forced legs cover the other version on each side, and CI runs them.
The server serves the skills/ folder next to it with loadSkills (prefix "examples"); the client lists the catalog, loads git-workflow with verified reads, follows a reference, browses the refunds folder and catches a verification failure, over stdio and HTTP on both protocol eras.
docs/servers/skills.md and docs/clients/skills.md are synced from executed companions under examples/guides. The skills changeset credits every author explicitly and replaces #2818's; a separate patch changeset covers the client's 2026-07-28 method-not-found handling, and the protocol-versions era table now lists HTTP 400/404.
…wording The client guide documents list()'s invalid entries, the strict option and explaining declines; the server guide mentions prefix and pageSize and says precisely what strict rejects. Changeset wording matches the code, each snippet imports only what it uses, and the companions assert the claims the pages quote.
The frontmatter parser trimmed with String.prototype.trim, which strips characters YAML 1.2 keeps as content (U+00A0, U+3000, U+2028, U+FEFF, ...). It now trims only spaces and tabs, and fails closed on a BOM before the first key, so it never reads a value differently from a YAML 1.2 parser.
SkillCatalogError carries the mcpBrand like the SDK's other errors, so instanceof works across bundle copies, and both packages' brand-conformance tests now walk the ext/skills entries.
…r extension - core-internal exposes its skills internals at ./ext/skills instead of its root barrel, so neither package's root entry loads the parser or imports @modelcontextprotocol/core/ext/skills (barrelClean pins both). - SkillCatalog works where WeakRef is missing (Workers before 2025-05-05). - reload() reports listChanged, and the docs say to call the handler's notify when serving through createMcpHandler. - attach() rejects a second skills extension on the same server before changing anything, and registers the capability last. - The attach hook is package-internal, out of the public declarations. - Catalog entries are frozen, so editing them cannot change what is served. - nodeSkillFiles refuses paths that resolve outside its root, on Windows too.
…le once - The description (1-1024) and compatibility (1-500) limits count code points, as the Agent Skills reference validator does; .length counted UTF-16 units, so 600 emoji were rejected and a valid skill was skipped or filed as invalid. - A tab after a block scalar's content indent is content, as in YAML; the tab check ran on every line before parsing. Tabs still fail closed where a line is read as structure and inside a block scalar's indent, blank and comment lines included (those were read as empty or as ending the scalar). - validateSkillEntry refuses resource URIs with a query or fragment and finds duplicates by skillFileKey, so Q&A.md and Q%26A.md, or c.md and c.md#x, can no longer list one file twice (SEP-2640: "each exactly once"). - sha256Digest hashes a view over the input's buffer instead of a copy; only a SharedArrayBuffer view is copied, since WebCrypto rejects those. - Comments cut to one line and JSDoc dropped from non-exported helpers.
…file once - attach() refuses a server that already handles skills/list, skills/get or, for a source that reads directories, resources/directory/read, with McpServer's own check and message, before changing anything. It used to replace the user's handler silently. - The "all or nothing" comment now says only what is true: registerResource stores the template before checking the resources/* handlers, so a low-level resources/read handler still leaves an inert template behind. - baseUri is canonicalised like request URIs; a mixed-case host or explicit default port made every read not-found and dropped every listed resource. A baseUri that does not parse now throws a TypeError. - Under strict, the over-limit problem says to raise limits instead of telling the user to set strict: true. - The catalog build hashes each file when it reads it and keeps only SKILL.md until it is parsed. A per-build cache and deepest-first order read and hash each file once, not once per enclosing skill; warnings now come out deepest skill first. A single large file is still read in full: checking its size first needs a size in the SkillFiles listing. - The tracked-server set, which reload() walks to send list_changed, prunes again only once it doubles past the survivors, not on every attach. - Comments cut to one line and JSDoc dropped from non-public code.
…s paging - A __proto__ object in an entry's frontmatter became the parsed object's prototype through Zod's catchall, so a host read fields such as allowed-tools that SKILL.md lacks while verification, comparing own keys, reported a match. The shared frontmatter schema refuses it on both sides, and verification compares every field a reader sees. - read() requires a content item for the requested URI, compared in canonical form, instead of falling back to the first item; a dynamic skill could return another resource's bytes as the requested file. - read() refetches with cacheMode 'refresh' only when the failing copy came from the response cache, as SEP-2640 requires, instead of on every verification failure. The changeset and docs say so. - SkillsClient shares Client's page walk and strict-capabilities flag through a package-internal hook set in Client's static block, instead of reading privates by name, copying the page cap and running its own paginator that stopped silently on any repeated cursor. - read() validates host-supplied entries and throws a TypeError before any request; the exact-spelling-first lookup is gone, as a valid entry lists each file once.
…its own PR This reverts commit 96a6d1e, with its patch changeset and era-table row. The fix is unrelated to skills and changes how a stable transport reports errors, so it gets its own review. The skills wire suite now expects the current behaviour on 2026-07-28: HTTP 404 with a -32601 body, surfaced as SdkHttpError.
🦋 Changeset detectedLatest commit: a7da98d The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Beyond the inline findings, two things that looked suspicious were checked and ruled out: nested skills being folded into the parent skill's file list (catalogBuild.ts:137-146) is deliberate and matches the spec's directory model, and the ttlMs: 0 / cacheScope: 'private' defaults filled in when no cacheHint is given (extension.ts:100) are the spec's own defaults, not a stricter choice by this SDK.
Extended reasoning...
The change adds the SEP-2640 Skills extension as new ext/skills subpaths across core, core-internal, server and client, plus a client.ts refactor exposing internal paging and readResource seams; it serves files from disk via a skill:// resource template, so path containment and digest verification are the sensitive surface. Seven verified findings (including a half-configured server on failed attach and a possible crash when onWarning throws after reload) rule out approval; the ruled-out note above records what else was examined.
3 optional notes from this repository's REVIEW.md or CLAUDE.md checks were not posted as comments, over this review's limit for such notes; they are on this commit's check card.
Findings marked 🟡 are optional suggestions and need no follow-up push.
CodeQL (js/polynomial-redos) flagged /\/+$/ on baseUri input in joinUri: it takes quadratic time on a long run of '/'. trimTrailingSlashes does the same with a loop, and SkillsExtension's baseUri handling uses it too.
Multi-line SKILL.md and markdown strings in the docs companions, test fixtures, conformance fixture and integration suites read as the files they stand for. Byte-identical; parser corpora, the BOM/CRLF fixture and single-line strings stay escaped.
Contributor
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 7 findings from earlier reviews are still open above.
Still open from earlier reviews (7):
- 🔴
packages/client/src/ext/skills/skillsClient.ts:107—Hosts reading a dynamic skill from a third-party server get resources/read not-found (or InvalidResult) for files whose… - Also unresolved: 6 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
mimeTypeFor read the extension off a plain object literal, so a file named notes.__proto__ was published with mimeType {} and x.constructor with a function, and clients rejected the read. Inherited names now fall back to application/octet-stream.
…ification When a list_changed send failed and onWarning threw, the throw escaped: on a synchronous send failure it rejected reload() and skipped the remaining servers, and on an asynchronous one it became an unhandled rejection. The warning call is now guarded, as the comment on #notify already promised.
For a dynamic skill, read() requested the normalised key rather than the resolved ref, turning Q%26A.md into Q&A.md. RFC 3986 treats a reserved character and its percent-encoding as different URIs, so a server that keys files by exact URI answered not-found. read() now requests the resolved URI (fragment dropped) and matches the returned contents by key, so a reply that spells the file differently is still accepted. Listed files keep requesting the entry's spelling.
…alid to keep valid entries list() returned the valid entries and moved invalid ones into invalid unless the caller opted into strict, the only list verb in the SDK that tolerated a spec-breaking peer by default. It now throws an SdkError naming every invalid entry, as listTools and listResources reject a malformed result and as the Go and C# skills clients do; skipInvalid: true keeps the valid entries and returns the rest in invalid. The strict option is replaced by skipInvalid.
The client compares an entry's frontmatter with its own YAML 1.2 core-schema parse, as SEP-2640 requires field by field, so a server whose YAML parser types values differently (YAML 1.1 reads yes as true) fails verification. The client guide and the parseFrontmatter JSDoc now say so and point at parseFrontmatter.
SKILL_URI_SCHEME, SKILL_MANIFEST_FILENAME and SKILLS_META_PREFIX were exported from three ext/skills subpaths, yet nothing in the SDK read them; the code spells the values inline, and SKILL_URI_SCHEME ('skill:') did not even match the 'skill://' the code uses. The extension has not shipped, so they go before they become API.
The repository keeps inline comments to one line; the reasoning (skills on the root barrel put the frontmatter parser in every package's root module graph) is in 7ee8d91's message.
Contributor
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline note, I re-read the fix commits since my last review (own-key MIME lookup in mime.ts, the try/catch around the warn sink in SkillCatalog#notifyFailed, dynamic-skill reads now requesting the resolved URI with contents matched by skillFileKey, and strict-by-default list() with skipInvalid) and found no regressions in them.
Extended reasoning...
The push since the last review changes 15 files (134 insertions, 58 deletions), mostly in packages/client/src/ext/skills/skillsClient.ts, packages/core-internal/src/ext/skills/mime.ts, packages/server/src/ext/skills/extension.ts and docs/clients/skills.md, plus tests for each. It touches no auth, crypto or injection surface; the SHA-256 digest verification path in the client is unchanged by this push. The PR remains a large multi-package feature with one earlier inline thread still open, so it is not an approve candidate; this note only records that the fix commits were checked.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
parseFrontmatter received the whole SKILL.md, so a host following the guide and passing a YAML parser got a multi-document error, and every read() failed frontmatter verification; the client exposes no splitter to work around it. The client now splits the file itself (BOM and CRLF removed) and passes the YAML block, so parseFrontmatter: yaml => YAML.parse(yaml) works as the guide describes.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the Skills extension (SEP-2640,
io.modelcontextprotocol/skills) to the server and client SDKs, behind newext/skillssubpaths. A server publishes a folder of skills withloadSkills('./skills')andattach(server). A client lists skills and reads their files with size, digest and frontmatter checks. Package root exports are unchanged.Motivation and Context
SEP-2640 is an accepted extension that lets servers ship agent skills over MCP: a
SKILL.mdplus its supporting files, served as resources and discovered throughskills/list,skills/getandresources/directory/read.This PR implements it in the TypeScript SDK:
skills/list,skills/getandresources/directory/read, and expose every file through askill://resource template.strict: true. A file is served only while its bytes match the published size and digest, andreload()swaps the whole catalog at once.c1aa87e.How Has This Been Tested?
Run at 8afe260. The client, integration and client conformance runs were repeated after the transport revert (62fee61):
0.2.0-alpha.12:Breaking Changes
None. The skills API is new and lives only in the
ext/skillssubpaths, and the package roots load no skills code.Types of changes
Checklist
Additional context
Design decisions:
list()is strict by default. When a server'sskills/listresponse contains an entry that fails validation, or two entries that share a URI,SkillsClient.list()throws anSdkErrornaming every such entry. A host that would rather keep the valid skills creates the client withnew SkillsClient(client, { skipInvalid: true }).list()then returns them and reports each rejected entry in aninvalidarray, with its position in the listing, its URI if it has one, and the reason it was rejected.yaml. It adds about 5 KB gzipped, only in the skills subpaths, and can be swapped foryamlif you prefer the dependency.resources/listshows only each skill'sSKILL.mdfor built-in catalogs, which the spec allows. The full file list is in each skill's entry, so listing every file would only makeresources/listlonger.@experimental. The API is shaped to move onto SEP-3371'sextensions: [skills]without a breaking change.Credits: