From e3d8c63da5747cb53c40087af39fe8b3bc7ed743 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Sun, 4 Oct 2026 22:23:49 +0900 Subject: [PATCH] Add the Skills extension (SEP-2640) ## Motivation and Context Issue #568 asks for the Skills extension (SEP-2640, Final): a server publishes Agent Skills, folders holding a `SKILL.md` with YAML frontmatter plus supporting files, through two methods, `skills/list` and `skills/get`, whose entries repeat the frontmatter and list every file with its SHA-256 digest and size, and a host reads the files through `resources/read` and verifies each one against the entry before loading it. Server side, `MCP::Server.new(skills:)` takes the catalog and advertises `io.modelcontextprotocol/skills`, with `resources` required alongside it as the SEP demands. `MCP::Skill.from_directory` covers the common case: every directory under a root that holds a `SKILL.md` becomes a skill, the frontmatter is read, digests and sizes are computed, a nested skill is published on its own and listed by the enclosing skill as supporting files (its manifest must be complete), and the default `resources/read` serves the files, answering `-32602` for an unserved URI under a skill. `resources/list` carries each skill's `SKILL.md` with the frontmatter `name` and `description`, and `skills/list` and `skills/get` carry the cache hints, both as the specification asks. `MCP::Skill.new` builds a skill in code and checks at definition time what a host may reject (URI grammar, `name` equal to the last path segment, entries under the root with the skill's own `SKILL.md` among them, the SEP limits). A skill defined locally is also held to the Agent Skills rules the specification requires of a served skill and the Python SDK enforces: a `name` of 1 to 64 lowercase letters, digits, and single hyphens, and a `description` of at most 1024 characters. An entry a client receives is not, as Agent Skills advises clients. `Server#skills_handlers(list:, get:)` serves catalogs that depend on the caller, the shape the Python SDK's `Skills(list_skills=, get_skill=)` converged on and the one a Rails user asked for on #568; both handlers are required because a declaring server must answer `skills/get` for every skill it serves. Handlers do not combine with a registry that holds skills: a registered skill, its `SKILL.md` entry in `resources/list`, and its files are served to every caller, so `skills_handlers` refuses such a server, and a catalog that hides a skill from some callers serves the files under the same rule, with `MCP::Skill.read_file`. The files of a nested skill belong to the enclosing skill as well, so such a catalog hides a nested skill only by withholding both, which the documentation spells out. What the handlers answer is never stamped `cacheScope: "public"`, but `"private"` where the server-wide `cache_scope` would make it so: a handler answers per caller and, returning skills, has no result of its own to carry a scope in. Client side, `MCP::Client#list_skills`, `#get_skill`, and `#read_skill_resource` are admitted only after a handshake of the current connection advertised the extension and `resources`, as the SEP asks of clients and as the Python and TypeScript clients do. `read_skill_resource` verifies size, digest, and (for a `SKILL.md`) the frontmatter field by field, withholds content that does not match, and latches the skill so no further read of it is sent on this connection; that is what keeps a host from loading part of a tampered skill and what the `sep-2640-client-verify-*` conformance scenarios check. The SEP and the Python SDK name their thin, unverified wrapper `read_skill_uri`; the verified read gets its own name so the two never mean different things, and `MCP::Skill.verify_resource` and `verify_frontmatter` give hosts that read raw the same checks. Frontmatter is read under rules stricter than YAML so every library reads it the same way: one document, no tags, aliases, or merge keys, String keys, JSON-representable values, and no plain scalar whose YAML 1.1 reading (Psych's) differs from its YAML 1.2 reading (the JavaScript and Go libraries'). Files of directory-backed skills are looked up by exact URI in a map built at construction, opened without following a symbolic link in the last path component (hard links are refused), and refused when their inode, size, link count, or digest changed since registration. The advertised capabilities and the dispatch gate are computed from the configured state, so a server never advertises what it does not serve. `resources/directory/read`, which the specification leaves optional, is not served in this release; declaring `directoryRead` is refused so it can be added later without a change in meaning. `skills/list` and `skills/get` join the protocol request methods that are not run without an id. The entry's `_meta`, which the stable text declares on `Skill` and the Python and TypeScript entries carry, is passed through on both sides: `MCP::Skill.new(meta:)` puts it on the `skills/list` and `skills/get` entries, and a client entry exposes what the server sent as `#meta`, under the JSON rules of the frontmatter and with no key acted on, so an annotation a catalog attaches reaches the host whichever SDK is on the other side. Strings JSON cannot encode (bytes invalid in their labeled encoding, or a binary String with a byte beyond ASCII, which `valid_encoding?` alone lets through) are refused in the frontmatter and `_meta` at definition time as well: json 3.1 cannot generate the response carrying them, while older json still generates a binary String holding valid UTF-8 bytes, recent versions with a deprecation warning announcing that change. Frontmatter read from a file was already held to valid UTF-8. Closes #568. ## How Has This Been Tested? test/mcp/skill_test.rb, test/mcp/skill/resource_entry_test.rb, test/mcp/skill/frontmatter_test.rb, test/mcp/skill/directory_test.rb, test/mcp/server_skills_test.rb, test/mcp/client_skills_test.rb, test/mcp/methods_test.rb, and the `sep-2640-client-*` drivers in conformance/client.rb; the pending `sep-2640-skills-enumeration` and `-manifest` server scenarios pass when run by name against conformance/server.rb, and `-directory` skips because `directoryRead` is not declared. Against the library at cc886567, `skills/list` answers `-32601` and `MCP::Client#read_resource` hands back a tampered `SKILL.md` without complaint. The server test also checks that neither skills method runs without a request id, however the skills were configured. The `_meta` of an entry is checked in the skill, server, and client tests, on the wire and as `#meta`. skill_test.rb requires digest itself: alone, it computed digests before anything autoloaded `MCP::Skill` and failed under most seeds. Without the encoding check, `MCP::Skill.new` accepted a binary string with a byte beyond ASCII and `JSON.generate` then failed on `to_h`. ## Breaking Changes None for servers that neither configure skills nor declare the extension by hand: `Server#capabilities=` now also accepts an `MCP::Server::Capabilities` builder and raises `ArgumentError` when the Hash declares the Skills extension without skills being configured, and `Server#define_resource` and `#resources=` refuse a URI that a directory-backed skill serves. `MCP::Client#connect` keeps its signature and return value. The server test helper takes the request params positionally only, since Ruby 2.7 reads a trailing Hash as keywords when the method also takes one. --- conformance/client.rb | 12 + conformance/server.rb | 11 + conformance/skills/pdf-processing/SKILL.md | 8 + .../skills/pdf-processing/references/FORMS.md | 3 + docs/_client/index.md | 1 + docs/_extensions/index.md | 1 + docs/_extensions/skills.md | 199 ++++ docs/index.md | 2 +- lib/mcp.rb | 1 + lib/mcp/client.rb | 29 +- lib/mcp/client/skill_errors.rb | 45 + lib/mcp/client/skills.rb | 430 ++++++++ lib/mcp/methods.rb | 5 + lib/mcp/server.rb | 352 ++++++- lib/mcp/skill.rb | 522 ++++++++++ lib/mcp/skill/directory.rb | 316 ++++++ lib/mcp/skill/frontmatter.rb | 262 +++++ lib/mcp/skill/resource_entry.rb | 109 ++ test/mcp/client_skills_test.rb | 933 ++++++++++++++++++ test/mcp/methods_test.rb | 10 + test/mcp/server_skills_test.rb | 542 ++++++++++ test/mcp/skill/directory_test.rb | 475 +++++++++ test/mcp/skill/frontmatter_test.rb | 174 ++++ test/mcp/skill/resource_entry_test.rb | 76 ++ test/mcp/skill_test.rb | 378 +++++++ 25 files changed, 4880 insertions(+), 16 deletions(-) create mode 100644 conformance/skills/pdf-processing/SKILL.md create mode 100644 conformance/skills/pdf-processing/references/FORMS.md create mode 100644 docs/_extensions/skills.md create mode 100644 lib/mcp/client/skill_errors.rb create mode 100644 lib/mcp/client/skills.rb create mode 100644 lib/mcp/skill.rb create mode 100644 lib/mcp/skill/directory.rb create mode 100644 lib/mcp/skill/frontmatter.rb create mode 100644 lib/mcp/skill/resource_entry.rb create mode 100644 test/mcp/client_skills_test.rb create mode 100644 test/mcp/server_skills_test.rb create mode 100644 test/mcp/skill/directory_test.rb create mode 100644 test/mcp/skill/frontmatter_test.rb create mode 100644 test/mcp/skill/resource_entry_test.rb create mode 100644 test/mcp/skill_test.rb diff --git a/conformance/client.rb b/conformance/client.rb index b568e2ec..2da5e62a 100644 --- a/conformance/client.rb +++ b/conformance/client.rb @@ -297,9 +297,21 @@ def build_provider_for(scenario, context) abort("No tool exposed by conformance server for #{scenario}") unless tool client.call_tool(tool: tool, arguments: {}) end + when "sep-2640-client-no-prefetch" + # SEP-2640: assembling a registry reads only the listing; the check is that no skill file is fetched. + client.list_skills + when "sep-2640-client-verify-digest", "sep-2640-client-verify-size", "sep-2640-client-verify-frontmatter" + # SEP-2640: the server tampers with `SKILL.md` on the wire. The verified read raises and latches the skill, + # so the supporting file below is never requested, which is how the harness detects the rejection. + client.list_skills + client.read_skill_resource(uri: "skill://pdf-processing/SKILL.md") + client.read_skill_resource(uri: "skill://pdf-processing/references/FORMS.md") else abort("Unknown or unsupported scenario: #{scenario}") end +rescue MCP::Client::SkillVerificationError => e + warn("#{scenario}: client rejected the skill content as expected (#{e.reason})") + exit rescue MCP::Client::OAuth::Flow::AuthorizationError, MCP::Client::RequestHandlerError, Faraday::Error => e warn("#{scenario}: client ended in the expected terminal failure (#{e.class}: #{e.message})") exit diff --git a/conformance/server.rb b/conformance/server.rb index a3f781dd..77139848 100644 --- a/conformance/server.rb +++ b/conformance/server.rb @@ -947,6 +947,7 @@ def build_server ], resources: resources, resource_templates: resource_templates, + skills: skills, # A per-boot random key is enough for conformance: every MRTR round trip completes # within one server process, and sealing keeps the fixture's `requestState` values # tamper-evident without any tool-level verification code. @@ -995,6 +996,12 @@ def resource_templates ] end + # SEP-2640: the skill directories under conformance/skills, so the `sep-2640-skills-*` server scenarios have + # a catalog to enumerate; the files are served through `MCP::Skill.read_file` in the read handler below. + def skills + MCP::Skill.from_directory(File.expand_path("skills", __dir__)) + end + def configure_handlers(server) server.logging_message_notification = MCP::LoggingMessageNotification.new(level: "debug") server.server_context = server @@ -1008,6 +1015,10 @@ def configure_resources_read_handler(server) server.resources_read_handler do |params| uri = params[:uri].to_s + # A custom reader replaces the default one, skill files included (SEP-2640). + skill_file = MCP::Skill.read_file(server.skills, uri) + next skill_file if skill_file + case uri when "test://static-text" [ diff --git a/conformance/skills/pdf-processing/SKILL.md b/conformance/skills/pdf-processing/SKILL.md new file mode 100644 index 00000000..0cfa402f --- /dev/null +++ b/conformance/skills/pdf-processing/SKILL.md @@ -0,0 +1,8 @@ +--- +name: pdf-processing +description: Extract PDF text, fill forms, merge files. Use when handling PDFs. +--- + +# PDF processing + +Use the forms reference in `references/FORMS.md` when a task involves filling a PDF form. diff --git a/conformance/skills/pdf-processing/references/FORMS.md b/conformance/skills/pdf-processing/references/FORMS.md new file mode 100644 index 00000000..33560d23 --- /dev/null +++ b/conformance/skills/pdf-processing/references/FORMS.md @@ -0,0 +1,3 @@ +# Forms + +Fill AcroForm fields by name; flatten the document before delivery. diff --git a/docs/_client/index.md b/docs/_client/index.md index 2e3077c9..c43b4b5e 100644 --- a/docs/_client/index.md +++ b/docs/_client/index.md @@ -24,6 +24,7 @@ This class supports: - Resource listing via the `resources/list` method (`MCP::Client#resources`) - Resource template listing via the `resources/templates/list` method (`MCP::Client#resource_templates`) - Resource reading via the `resources/read` method (`MCP::Client#read_resource`) +- Skills (SEP-2640) via the `skills/list` and `skills/get` methods and digest-verified reads (`MCP::Client#list_skills`, `#get_skill`, `#read_skill_resource`), see [Skills](/extensions/skills/) - Prompt listing via the `prompts/list` method (`MCP::Client#prompts`) - Prompt retrieval via the `prompts/get` method (`MCP::Client#get_prompt`) - Completion requests via the `completion/complete` method (`MCP::Client#complete`); see [Completion](/server/completion/) diff --git a/docs/_extensions/index.md b/docs/_extensions/index.md index 5b52c0fd..49c9dac8 100644 --- a/docs/_extensions/index.md +++ b/docs/_extensions/index.md @@ -15,3 +15,4 @@ as described on [Capability Extensions](/extensions/capability-extensions/). The extensions this SDK ships support for: - [MCP Apps](/extensions/mcp-apps/) (SEP-1865) - interactive HTML user interfaces rendered by the host for tool results +- [Skills](/extensions/skills/) (SEP-2640) - Agent Skills served over MCP resources, with digest-verified reads on the client diff --git a/docs/_extensions/skills.md b/docs/_extensions/skills.md new file mode 100644 index 00000000..9c141c60 --- /dev/null +++ b/docs/_extensions/skills.md @@ -0,0 +1,199 @@ +--- +layout: default +title: Skills +nav_order: 4 +--- + +# Skills + +Skills (SEP-2640) is a Final extension (negotiated via [Capability Extensions](/extensions/capability-extensions/)) that lets a server +publish Agent Skills: folders holding a `SKILL.md` with YAML frontmatter plus supporting files, served as ordinary MCP resources +under `skill://` URIs. The extension adds two methods, `skills/list` and `skills/get`, whose entries name a skill's `SKILL.md`, +repeat its frontmatter, and list its files with SHA-256 digests and sizes, so a host can approve a skill from the listing and +verify every file it later fetches. See the [Skills specification](https://github.com/modelcontextprotocol/ext-skills/blob/main/specification/stable/skills.mdx), +the stable text behind SEP-2640. + +## Serving Skills + +The common case is a directory of skill folders. `MCP::Skill.from_directory` reads the frontmatter, computes the digests and +sizes, and the server serves `skills/list`, `skills/get`, and `resources/read` for every file: + +```ruby +# skills/ +# pdf-processing/ +# SKILL.md <- frontmatter `name` must equal the directory name +# references/FORMS.md +server = MCP::Server.new(name: "catalog", skills: MCP::Skill.from_directory("skills")) +``` + +Passing `skills:` advertises `io.modelcontextprotocol/skills` under `capabilities.extensions`; `resources`, which the SEP requires +alongside it, is part of the default capabilities. An empty Array serves an empty catalog; `nil` (the default) serves no skills, +and `skills/list` then answers `-32601`. `MCP::Skill.capability` exists for a hand-built `MCP::Server::Capabilities`; such +a declaration must be paired with a non-`nil` `skills:` in the same `MCP::Server.new` call. + +Skills assembled in code are `MCP::Skill` values whose files the application serves itself: + +```ruby +skill_md = File.read("git-workflow/SKILL.md") +skill = MCP::Skill.new( + uri: "skill://git-workflow/SKILL.md", + frontmatter: { name: "git-workflow", description: "Branching and commit conventions. Use when preparing a commit or a pull request." }, + resources: [MCP::Skill::ResourceEntry.from_content(uri: "skill://git-workflow/SKILL.md", content: skill_md)], +) +server = MCP::Server.new(name: "catalog", skills: [skill]) +server.define_resource( + uri: "skill://git-workflow/SKILL.md", + name: "git-workflow", # a SKILL.md resource takes its name and description from the frontmatter + description: "Branching and commit conventions. Use when preparing a commit or a pull request.", + mime_type: "text/markdown", +) { skill_md } +``` + +Every URI a skill lists must be served, or a client reading it fails closed. `MCP::Skill.new` checks what a host may reject: +the URI shape (`skill:///SKILL.md`, segments limited to letters, digits, `-`, `.`, `_`, and `~`), `name` equal to the last +path segment and following the Agent Skills naming rules (1 to 64 lowercase letters, digits, and single hyphens between them), +a `description` of 1 to 1024 characters, JSON-representable frontmatter values, entries under the skill root with +the skill's own `SKILL.md` among them, and the SEP limits of 512 entries and 16 MiB. `resources: "dynamic"` declares a skill +whose files are generated per request and carry no digests. `meta:` is the entry's `_meta`, +passed through under the same JSON rules as the frontmatter and given no meaning by the SDK; +the `io.modelcontextprotocol.skills/` key prefix is reserved for the extension, +so a server's own annotations use its own reverse-domain prefix. +A skill nested in another skill's directory is a skill of its own +and, as the specification requires of a complete manifest, its files (its `SKILL.md` included) are supporting files of +the enclosing skill as well: `from_directory` lists them in both entries, and a hand-built enclosing skill lists them itself. + +With the default `resources/list` handler, the listing carries the `SKILL.md` of every registered skill with `name` and +`description` taken from the frontmatter and `mimeType` `text/markdown`, as the specification asks of a `SKILL.md` resource, +unless the application registered a resource at that URI itself (as the example above does in order to serve the file). + +{: .important } +> Files of a directory-backed skill are looked up by exact URI in a map built at construction, so request data never reaches +> the filesystem. Dotfiles, symbolic links, FIFOs, and devices are skipped, a hard link is refused, and every other regular file +> is published, editor backups included. The tree is trusted as the operator's: keep it writable only by the operator while +> skills are built and served. Each file is read at request time and refused with `-32603` when its inode, size, or digest +> no longer matches what was registered, or when it has gained a hard link. A read of a URI under such a skill that names +> none of its files answers `-32602`, as the specification requires, so resource templates are shadowed for the whole subtree +> under the skill root; URIs outside every skill root keep the server's usual behavior. A resource registered with the URI of +> a skill file is refused. `resources_read_handler` +> replaces the default reader, skill files included; delegate with `MCP::Skill.read_file(server.skills, params[:uri]) || +> your_contents`, which returns the `contents` Array or `nil`, and answer `-32602` yourself for a `nil` under a skill root. + +### Catalogs That Depend on the Caller + +A catalog that depends on the caller is served from handlers; both are required because a server declaring the extension +must answer `skills/get` for every skill it serves, listed or not: + +```ruby +server.skills_handlers( + # Array; the server paginates + list: ->(params, server_context:) { skills_for(server_context) }, + # MCP::Skill, or nil for -32602 + get: ->(params, server_context:) { skills_for(server_context).find { |skill| skill.uri == params[:uri] } }, +) +server.resources_read_handler do |params, server_context:| + MCP::Skill.read_file(skills_for(server_context), params[:uri]) || other_contents(params[:uri]) +end +``` + +Handlers may receive `server_context:` when they declare the keyword. A result outside this contract +(not an `MCP::Skill`, a duplicate URI, a skill for another URI) is answered with `-32603`. +`skills/list` and `skills/get` carry the `ttlMs`/`cacheScope` cache hints the way `resources/list` and `resources/read` do. + +{: .important } +> A server serves skills from the `skills:` registry or from handlers, never both: `skills_handlers` raises `ArgumentError` +> while the registry holds skills, because a registered skill, its `SKILL.md` entry in `resources/list`, and its files +> are served to every caller. + +A catalog that hides a skill from some callers applies its rule everywhere itself: + +- `server_context` is the value the server was built with, plus the `_meta` of the request under `:_meta`, + so the catalog needs a server built for each caller from the authenticated request + (see [Server Context](/server/server-context/)). +- Do not reassign the `server_context` of a server that concurrent requests share, + and do not decide from `_meta`, which the client chooses. +- Filter `get` as well as `list`, and the reads, as the `resources_read_handler` above does. + `MCP::Skill.read_file` serves the files of the directory-backed skills among those it is given + and returns `nil` for any other URI. +- A handler that answers `-32602` for an unserved URI under a skill root, as the specification asks, + judges that against the caller's skills too, so that a hidden skill answers exactly like a URI that does not exist. +- What the handlers answer is never marked `cacheScope: "public"`: + where the `cache_scope` of the server would make it so, the result says `"private"`. +- The reads are not covered: on a server configured with `cache_scope: "public"`, + return `{ contents: contents, cacheScope: "private" }` from the `resources_read_handler` for files that depend on the caller. +- Leaving a nested skill out of a caller's catalog does not hide it from a caller who is given the enclosing skill: + the enclosing skill lists the nested files as its own, with their URIs, digests, and sizes, as a complete manifest must, + and serves them. +- Keep a skill that some callers must not see out of the directory of a skill they may see, + or withhold the enclosing skill from them as well. + +## Reading Skills + +```ruby +page = client.list_skills # ListSkillsResult(skills:, next_cursor:, meta:, ttl_ms:, cache_scope:) +entry = client.get_skill(uri: "skill://pdf-processing/SKILL.md").skill +entry.meta # the entry's _meta as a frozen Hash, or nil +result = client.read_skill_resource(uri: entry.uri, skill: entry) +result.bytes # the verified bytes, frozen +client.read_skill_resource(uri: "skill://pdf-processing/references/FORMS.md", skill: entry).bytes +client.skills_capability # the extension settings, or nil +``` + +Nothing is read ahead of need: the specification forbids fetching a skill's files on connection, on listing, or at approval, +so call `read_skill_resource` for `SKILL.md` when the skill is loaded and for a supporting file when it is used. +A relative reference in a `SKILL.md` resolves against the directory of that `SKILL.md`, which for a nested skill is its own root +and not the enclosing one; `read_skill_resource` takes the resulting URI. + +`read_skill_resource` fetches a file through `resources/read` and checks it against the entry remembered from `list_skills` or +`get_skill` (or passed as `skill:`): exactly one content item for the URI, the size before anything is decoded, the SHA-256 +digest, and for a `SKILL.md` the frontmatter compared field by field with the entry. A mismatch raises +`MCP::Client::SkillVerificationError` (`reason` `:contents`, `:size`, `:digest`, `:parse`, or `:frontmatter`), withholds +the content, and latches the skill: no further read of it is sent on this connection, even after a new listing, so reading +a latched skill again takes a new connection. A supporting file is read only after the skill's `SKILL.md` verified; +without `skill:` it resolves to the most specific remembered skill that contains it, never to an enclosing one, +and a `SKILL.md` URI resolves only to its own entry, never as a supporting file of an enclosing skill. +The specification resolves a read against the entry the host loaded the skill from, so a host acting on a skill +keeps that entry at least until the skill's `SKILL.md` leaves the model's context and passes it as `skill:`; +when an enclosing skill lists the file too and no `skill:` is given, the read is refused as `:ambiguous` rather than guessed. +Calls refused before anything is sent raise `MCP::Client::SkillReadRefusedError` with `reason` `:not_advertised` +(no handshake of this connection advertised the extension and `resources`), `:connection_changed`, `:latched`, +`:unknown_skill` (pass `skill:` or call `get_skill` first; also the answer for a URI longer than 8 KiB or deeper than +128 segments, which this SDK does not resolve), `:ambiguous`, `:dynamic`, `:not_in_manifest`, or `:skill_md_unverified`; +later releases may add reasons, so treat an unknown one as a refusal. +For a skill the host is acting on, `:not_in_manifest` is what the specification calls a read of an unlisted file, +a verification failure equivalent to a digest mismatch: no content is fetched, the SDK latches nothing +(the URI may come from model output), and a host that takes it as a sign that the skill changed fetches the entry again +with `get_skill`. Both classes inherit from `MCP::Client::SkillError`, and their messages show URIs escaped to ASCII, +since a server chooses them. `meta:` and `cancellation:` work as on the other methods. + +Entries are remembered per client and bound to the connection: `connect` forgets them, and a `connect` that performs no new +handshake (the transport is still connected) keeps the latches. A listing or `get_skill` that replaces an entry drops its +verified status, so supporting reads of that skill are refused until its `SKILL.md` is read again, and the specification asks +the host to treat a changed `resources` set as revoking any approval bound to the old one. `skills_capability` returning `nil` +means not ready or not advertised on the current connection, never that skill files may be read raw. + +Hosts that load a skill with `read_resource` instead own the obligations above: stop loading the skill on any raise and +verify `SKILL.md` before any other file. `MCP::Skill.verify_resource(skill, uri, bytes)` checks size and digest (pass `text.b` +for a text item, the decoded `blob` otherwise) and `MCP::Skill.verify_frontmatter(skill, text)` compares the frontmatter; a static +`SKILL.md` needs both, and a `"dynamic"` skill can only be checked with the second. + +{: .important } +> The frontmatter of a skill entry is untrusted input, and so is its `_meta`. The SDK compares the frontmatter and passes both +> through; it never acts on a field. +> The SEP requires hosts to ignore or approval-gate `allowed-tools`, hooks, and scripts in skills that arrive over MCP, and to treat +> `name` and `description` as display text. `mime_type` on a read result is the server's unverified display metadata. These are +> the obligations the SDK can state here; the Security Considerations of the specification list the rest a host must implement +> itself: tagging content with its originating server, binding resource reads to that server, resolving skill names per origin, +> binding persisted approval to the entry's `resources` set, and isolating any disk cache from filesystem-skill discovery. + +{: .note } +> Frontmatter is read under rules stricter than YAML so that every YAML library reads it the same way: one document without +> directives or document markers, no tags, aliases, or merge keys, keys that are untagged strings, values JSON can carry, and no +> plain scalar whose YAML 1.1 reading differs from its YAML 1.2 reading (`yes`, `no`, `on`, `off`, `010`, `1:20`, `1_000`, +> `0o10`, and the like): quote such values. The NEL, LINE SEPARATOR, and PARAGRAPH SEPARATOR characters (line breaks in +> YAML 1.1 only) and integers outside the range from -(2^53 - 1) to 2^53 - 1 (which JavaScript readers round) are refused +> as well. A leading UTF-8 BOM is skipped for extraction while digests cover it. The block is limited to 64 KiB by this SDK, +> not by the protocol. Hosts must accept skills up to the 512-entry and 16 MiB limits and may accept larger ones, so +> the client accepts entries of any size; `MCP::Skill#total_size` and `#resources` are the hooks for a host that declines +> large skills. + +`resources/directory/read` and the `directoryRead` setting are not served in this release; declaring `directoryRead` is refused. diff --git a/docs/index.md b/docs/index.md index 5682317c..1d1ed3a8 100644 --- a/docs/index.md +++ b/docs/index.md @@ -112,7 +112,7 @@ For comprehensive documentation, see: - [Protocol Versions](/protocol-versions/) - supported versions, the era model, and client negotiation - [Building Servers](server/) - transports, discovery, tools, prompts, resources, server-to-client requests, multi round-trip requests, notifications, protocol utilities, configuration, and OAuth 2.1 authorization - [Building Clients](client/) - transports, lifecycle negotiation, multi round-trip requests, and OAuth 2.1 authorization -- [Extensions](/extensions/) - capability extensions and MCP Apps +- [Extensions](/extensions/) - capability extensions, MCP Apps, and Skills ## API Documentation diff --git a/lib/mcp.rb b/lib/mcp.rb index 2213f231..99c083e4 100644 --- a/lib/mcp.rb +++ b/lib/mcp.rb @@ -24,6 +24,7 @@ module MCP autoload :ResultType, "mcp/result_type" autoload :Server, "mcp/server" autoload :ServerSession, "mcp/server_session" + autoload :Skill, "mcp/skill" autoload :Tool, "mcp/tool" autoload :TraceContext, "mcp/trace_context" diff --git a/lib/mcp/client.rb b/lib/mcp/client.rb index 7c9d8b93..b6f1df1d 100644 --- a/lib/mcp/client.rb +++ b/lib/mcp/client.rb @@ -7,6 +7,8 @@ require_relative "client/stdio" require_relative "client/http" require_relative "client/paginated_result" +require_relative "client/skill_errors" +require_relative "client/skills" require_relative "client/tool" require_relative "result_type" @@ -114,6 +116,9 @@ def initialize(message, request, original_error: nil) STATE_ONLY_BACKOFF_INITIAL_SECONDS = 0.05 STATE_ONLY_BACKOFF_CAP_SECONDS = 0.25 + # The Skills extension methods (SEP-2640): `list_skills`, `get_skill`, `read_skill_resource`, `skills_capability`. + include Skills + # Initializes a new MCP::Client instance. # # @param transport [Object] The transport object to use for communication with the server. @@ -144,6 +149,8 @@ def initialize(transport:, input_required_max_rounds: DEFAULT_INPUT_REQUIRED_MAX # a SEP-2322 `input_required` result. @input_required_handlers = {} @input_required_max_rounds = input_required_max_rounds + # Skills extension state (SEP-2640), bound to the connection by `connect`; see `Client::Skills`. + @skills_state = Skills::State.new end # The user may want to access additional transport-specific methods/attributes @@ -217,7 +224,15 @@ def connect(client_info: nil, protocol_version: nil, capabilities: {}, mode: nil effective_mode = resolve_connect_mode(mode, protocol_version) - if effective_mode == :legacy + # Skills state (SEP-2640) follows the connection: a handshake about to run starts a new generation, its + # result is recorded only if no newer handshake started meanwhile, and a failure clears readiness under + # the same condition, so a stale completion or failure changes nothing. The bundled transports answer + # a repeated `connect` with their cached handshake result without any I/O, so when the transport is + # connected and Skills is ready nothing is reset: the latches of a live connection stay put. + generation = @skills_state.begin_handshake unless skills_ready_on_connected_transport? + completed = false + + result = if effective_mode == :legacy transport.connect( client_info: client_info, protocol_version: protocol_version, @@ -231,6 +246,11 @@ def connect(client_info: nil, protocol_version: nil, capabilities: {}, mode: nil mode: effective_mode, ) end + completed = true + @skills_state.handshake_completed(generation, result) if generation + result + ensure + @skills_state.handshake_failed(generation) if generation && !completed end # Sends `server/discover` (MCP 2026-07-28, SEP-2575): sessionless capability discovery @@ -677,6 +697,13 @@ def exclude_invalid_x_mcp_header?(tool) true end + # Whether a `connect` would only hand back the transport's cached handshake: the transport reports itself + # connected and Skills already recorded a handshake of this connection. A transport without connection + # state always starts a new generation. + def skills_ready_on_connected_transport? + transport.respond_to?(:connected?) && transport.connected? && @skills_state.ready? + end + # Resolves the effective SEP-2575 lifecycle mode for `connect`: # # - An explicit `protocol_version` from a legacy generation pins the legacy handshake without a probe, diff --git a/lib/mcp/client/skill_errors.rb b/lib/mcp/client/skill_errors.rb new file mode 100644 index 00000000..fd4d9827 --- /dev/null +++ b/lib/mcp/client/skill_errors.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +module MCP + class Client + # Base class of the errors the Skills extension (SEP-2640) raises on the client side, so a host can + # rescue all of them at once. Messages carry request and skill URIs only, never content. + class SkillError < StandardError; end + + # A `skills/list` or `skills/get` result the server returned is not a conformant skill entry + # (wrong shape, a malformed digest, a resource outside the skill root, a duplicate URI within one page, + # or a `skills/get` result for another URI). Nothing is remembered from such a result. + class SkillEntryError < SkillError; end + + # The content a `resources/read` returned does not match the skill's manifest: `reason` is one of + # `:contents` (not exactly one item for the requested URI with exactly one of `text` or `blob`, + # an undecodable `blob`, or an item nested too deeply to copy), `:size`, `:digest`, `:parse` (the `SKILL.md` + # frontmatter cannot be read), or `:frontmatter` (it differs from the entry). The skill is latched for + # the life of the connection. + class SkillVerificationError < SkillError + attr_reader :uri, :skill_uri, :reason + + def initialize(message, uri:, skill_uri:, reason:) + super(message) + @uri = uri + @skill_uri = skill_uri + @reason = reason + end + end + + # A Skills call was refused before it was sent, or its completion was discarded because the connection + # changed. `reason` is one of `:not_advertised`, `:connection_changed`, `:latched`, `:unknown_skill`, + # `:dynamic`, `:not_in_manifest`, or `:skill_md_unverified`; later releases may add reasons, so treat + # an unknown one as a refusal too. Nothing is latched by a refusal. + class SkillReadRefusedError < SkillError + attr_reader :uri, :skill_uri, :reason + + def initialize(message, reason:, uri: nil, skill_uri: nil) + super(message) + @reason = reason + @uri = uri + @skill_uri = skill_uri + end + end + end +end diff --git a/lib/mcp/client/skills.rb b/lib/mcp/client/skills.rb new file mode 100644 index 00000000..0dc7439b --- /dev/null +++ b/lib/mcp/client/skills.rb @@ -0,0 +1,430 @@ +# frozen_string_literal: true + +require "set" +require "digest" + +module MCP + class Client + # One page of `skills/list`: `skills` is a frozen Array of `MCP::Skill`; `next_cursor`, `meta`, `ttl_ms`, and + # `cache_scope` mirror the response members. Frozen on construction. + ListSkillsResult = Struct.new(:skills, :next_cursor, :meta, :ttl_ms, :cache_scope, keyword_init: true) do + def initialize(**kwargs) + super(**kwargs) + freeze + end + end + + # The entry `skills/get` returned for exactly the requested URI. Frozen on construction. + GetSkillResult = Struct.new(:skill, :meta, :ttl_ms, :cache_scope, keyword_init: true) do + def initialize(**kwargs) + super(**kwargs) + freeze + end + end + + # A skill file read and verified by `read_skill_resource`: `bytes` is the verified content as a frozen + # binary-encoded String (the decoded `blob`, or the bytes of the `text`), `mime_type` is the server's + # unverified display metadata (never a reason to pick an interpreter), `contents` is the raw frozen + # `contents` Array, `skill` is the entry object the content was checked against, and `verified` is true: + # the digest, size, and (for a `SKILL.md`) frontmatter matched. Every value is independent of the objects + # it was built from, so mutating a source cannot change the result. + SkillReadResult = Struct.new(:uri, :bytes, :mime_type, :contents, :skill, :verified, keyword_init: true) do + def initialize(**kwargs) + super(**kwargs) + freeze + end + end + + # Client side of the Skills extension (SEP-2640), mixed into `MCP::Client`: `list_skills`, `get_skill`, + # `read_skill_resource`, and `skills_capability`. + # + # Every call is admitted only after a handshake of the current connection advertised both the extension and + # `resources`, as the SEP asks of clients. Entries the server returned are remembered per client instance, + # bound to the connection: `connect` starts a new generation and forgets them. `read_skill_resource` checks + # the content against the entry it resolved (size, SHA-256, and the frontmatter of a `SKILL.md`), withholds + # content that does not match, and latches the skill so that no later read of it is sent on this connection, + # which is what keeps a host from loading part of a tampered skill. `read_resource` stays the raw, + # unverified way to fetch a skill file; `MCP::Skill.verify_resource` and `MCP::Skill.verify_frontmatter` + # exist for hosts that go that way and then own the latching and ordering obligations themselves. + module Skills + # Fetches one page of the skills the server serves. + # + # @raise [SkillReadRefusedError] `:not_advertised` before any request is sent, `:connection_changed` when + # `connect` ran while the request was in flight (the page is discarded). + # @raise [SkillEntryError] When the page is not a conformant `skills/list` result. + def list_skills(cursor: nil, meta: nil, cancellation: nil) + generation = @skills_state.admit(connected: connected?) + params = cursor ? { cursor: cursor } : nil + response = request(method: Methods::SKILLS_LIST, params: params, meta: meta, cancellation: cancellation) + @skills_state.ensure_current!(generation) + + result = response.is_a?(Hash) ? response["result"] : nil + raise SkillEntryError, "Response validation failed: missing or invalid `result`" unless result.is_a?(Hash) + + entries = result["skills"] + raise SkillEntryError, "Response validation failed: `skills` is not an Array" unless entries.is_a?(Array) + + skills = entries.map { |entry| parse_skill_entry(entry) } + uris = skills.map(&:uri) + raise SkillEntryError, "Response validation failed: a skill URI is listed twice in one page" unless uris.uniq.size == uris.size + + @skills_state.install(generation, skills) + ListSkillsResult.new( + skills: skills.freeze, + next_cursor: result["nextCursor"], + meta: result["_meta"], + ttl_ms: result["ttlMs"], + cache_scope: result["cacheScope"], + ) + end + + # Fetches the entry of one skill by the URI of its `SKILL.md`, listed or not. + # + # @raise [SkillReadRefusedError] `:not_advertised` or `:connection_changed`, as for `list_skills`. + # @raise [SkillEntryError] When the result is not a conformant entry for exactly `uri`. + def get_skill(uri:, meta: nil, cancellation: nil) + check_skill_uri!(uri) + + generation = @skills_state.admit(connected: connected?) + response = request(method: Methods::SKILLS_GET, params: { uri: uri }, meta: meta, cancellation: cancellation) + @skills_state.ensure_current!(generation) + + result = response.is_a?(Hash) ? response["result"] : nil + raise SkillEntryError, "Response validation failed: missing or invalid `result`" unless result.is_a?(Hash) + + skill = parse_skill_entry(result["skill"]) + raise SkillEntryError, "Response validation failed: skills/get answered for #{skill.uri.dump} instead of #{uri.dump}" unless skill.uri == uri + + @skills_state.install(generation, [skill]) + GetSkillResult.new(skill: skill, meta: result["_meta"], ttl_ms: result["ttlMs"], cache_scope: result["cacheScope"]) + end + + # Reads one file of a skill through `resources/read` and verifies it against the skill's entry before + # returning it. The entry is `skill:` when given (for this read only), else the most specific remembered + # skill that contains `uri`, never an enclosing one: when an enclosing skill lists `uri` as well the read is + # refused as `:ambiguous`, since only the host knows which skill it is acting on, and a `SKILL.md` URI + # resolves only to its own entry. A supporting file is read only after the skill's `SKILL.md` verified on + # this instance. + # + # @raise [SkillReadRefusedError] Before anything is sent: `:not_advertised`, `:latched`, `:unknown_skill` (also + # for a URI longer than 8 KiB or deeper than 128 segments, an SDK limit), `:ambiguous` (an enclosing + # remembered skill lists `uri` too; pass `skill:` for the skill being acted on), `:dynamic` (no digests; use + # `read_resource` with `MCP::Skill.verify_frontmatter`), `:not_in_manifest`, `:skill_md_unverified`; on + # completion, `:connection_changed` or `:latched`. + # @raise [SkillVerificationError] When the content does not match; the skill is latched for the connection. + def read_skill_resource(uri:, skill: nil, meta: nil, cancellation: nil) + check_skill_uri!(uri) + raise ArgumentError, "The value of skill must be an MCP::Skill or nil (got #{skill.class})." unless skill.nil? || skill.is_a?(Skill) + + # A frozen copy, so a caller mutating its String during the read cannot change the URI that was verified. + uri = -uri + unless Skill::Uri.resolvable?(uri) + # Refused before the ancestor roots are computed, which would cost time and memory quadratic in + # the length of a URI that may come from model output; the message leaves the URI out for that reason. + @skills_state.admit(connected: connected?) + raise SkillReadRefusedError.new( + "Skills call refused (unknown_skill): the URI is longer than #{Skill::Uri::MAX_RESOLVED_BYTES} bytes or deeper " \ + "than #{Skill::Uri::MAX_RESOLVED_SEGMENTS} segments, beyond what the client resolves", + reason: :unknown_skill, + uri: uri, + ) + end + + candidates = Skill::Uri.candidate_skill_uris(uri) + generation, entry, resource_entry = @skills_state.admit_read(uri, candidates, skill, connected: connected?) + response = request(method: Methods::RESOURCES_READ, params: { uri: uri }, meta: meta, cancellation: cancellation) + outcome = verify_skill_contents(entry, resource_entry, uri, response) + bytes, mime_type, contents = @skills_state.complete_read(generation, uri, candidates, entry, explicit: !skill.nil?, outcome: outcome) + + SkillReadResult.new(uri: uri, bytes: bytes, mime_type: mime_type, contents: contents, skill: entry, verified: true) + end + + # The Skills settings the current connection's handshake advertised, as a frozen Hash, or nil when + # a Skills call would be refused with `:not_advertised`: not ready, not connected, or not advertised. + # nil never means "read skill files raw instead". + def skills_capability + @skills_state.skills_capability(connected: connected?) + end + + private + + def check_skill_uri!(uri) + raise ArgumentError, "The value of uri must be a String (got #{uri.class})." unless uri.is_a?(String) + return if uri.valid_encoding? && uri.encoding.ascii_compatible? + + raise ArgumentError, "The value of uri must be validly encoded in an ASCII-compatible encoding." + end + + def parse_skill_entry(entry) + Skill.from_entry(entry) + rescue ArgumentError => e + raise SkillEntryError, "Skill entry rejected: #{e.message}" + end + + # Pure verification, run outside the state lock: exactly one content item for `uri` with exactly one of + # `text` or `blob`, the size before decoding, the digest, and for the skill's own `SKILL.md` the frontmatter. + # Returns `[:ok, bytes, mime_type, contents]` or `[:failed, error]`. + def verify_skill_contents(entry, resource_entry, uri, response) + result = response.is_a?(Hash) ? response["result"] : nil + contents = result.is_a?(Hash) ? result["contents"] : nil + item = contents.is_a?(Array) && contents.size == 1 ? contents.first : nil + unless item.is_a?(Hash) && item["uri"] == uri + return skill_verification_failure(:contents, uri, entry, "did not come back as exactly one content item") + end + + text = item["text"] + blob = item["blob"] + unless (text.is_a?(String) && blob.nil?) || (blob.is_a?(String) && text.nil?) + return skill_verification_failure(:contents, uri, entry, "did not carry exactly one of text or blob") + end + + expected_size = resource_entry.size + if text + bytes = text.b + return skill_verification_failure(:size, uri, entry, "has a different size than listed") unless bytes.bytesize == expected_size + else + # Checked before decoding so the manifest's size bounds the work: this is the base64 length of exactly `size` bytes. + return skill_verification_failure(:size, uri, entry, "has a different size than listed") unless blob.length <= ((expected_size + 2) / 3) * 4 + + bytes = begin + blob.unpack1("m0") + rescue ArgumentError + return skill_verification_failure(:contents, uri, entry, "did not carry valid base64") + end + return skill_verification_failure(:size, uri, entry, "has a different size than listed") unless bytes.bytesize == expected_size + end + + unless "sha256:#{Digest::SHA256.hexdigest(bytes)}" == resource_entry.digest + return skill_verification_failure(:digest, uri, entry, "has a different digest than listed") + end + + if uri == entry.uri + begin + Skill.verify_frontmatter(entry, bytes.dup.force_encoding(Encoding::UTF_8)) + rescue SkillVerificationError => e + return [:failed, e] + end + end + + copy = begin + Skill::Json.deep_freeze(Skill::Json.deep_dup(contents)) + rescue ArgumentError + return skill_verification_failure(:contents, uri, entry, "came back nested too deeply to be the file listed") + end + + mime_type = item["mimeType"] + [:ok, bytes.freeze, mime_type.is_a?(String) ? mime_type.dup.freeze : nil, copy] + end + + def skill_verification_failure(reason, uri, entry, detail) + # URIs a server chose are shown with `dump`, which escapes everything outside printable ASCII (`inspect` + # leaves bidirectional overrides as they are), so they cannot forge or reorder lines in a host's log. + message = "Resource #{uri.dump} #{detail} by #{entry.uri.dump}" + [:failed, SkillVerificationError.new(message, uri: uri, skill_uri: entry.uri, reason: reason)] + end + + # Per-instance Skills state, bound to the connection. Every public method takes the one Mutex and none of + # them calls another inside it (Ruby's Mutex is not reentrant); hashing, decoding, and parsing happen + # outside, in the client methods above. No lock is ever held across I/O. + class State + def initialize + @mutex = Mutex.new + @generation = 0 + @ready = false + @recorded = nil + @index = {} + @verified = {}.compare_by_identity + @latched = Set.new + end + + # A new connection is about to be established: forget everything and return the new generation. + def begin_handshake + @mutex.synchronize do + @generation += 1 + @ready = false + @recorded = nil + @index = {} + @verified = {}.compare_by_identity + @latched = Set.new + @generation + end + end + + # The handshake that started `generation` completed with `result`, the wire Hash `transport.connect` + # returned; records a frozen copy of its capabilities, unless a newer handshake started meanwhile. + # Capabilities nested too deeply to copy are recorded as none, so `connect` succeeds as it does without + # Skills and every Skills call is refused. + def handshake_completed(generation, result) + capabilities = read_key(result, "capabilities") + copy = begin + capabilities.is_a?(Hash) ? Skill::Json.deep_freeze(Skill::Json.deep_dup(capabilities)) : nil + rescue ArgumentError + nil + end + @mutex.synchronize do + next unless generation == @generation + + @ready = true + @recorded = copy + end + end + + # The handshake that started `generation` raised; a stale failure changes nothing. + def handshake_failed(generation) + @mutex.synchronize do + next unless generation == @generation + + @ready = false + @recorded = nil + end + end + + def ready? + @mutex.synchronize { @ready } + end + + def admit(connected:) + @mutex.synchronize do + admit_or_refuse!(connected) + @generation + end + end + + def ensure_current!(generation) + @mutex.synchronize do + refuse!(:connection_changed, nil, nil, "the connection changed while the request was in flight") unless generation == @generation + end + end + + # Resolves and checks a read before it is sent; returns the generation, the entry, and its resource entry. + def admit_read(uri, candidates, explicit, connected:) + @mutex.synchronize do + admit_or_refuse!(connected) + latched = candidates.find { |candidate| @latched.include?(candidate) } + refuse!(:latched, uri, latched, "#{latched.dump} failed verification on this connection") if latched + + entry = resolve_entry(uri, candidates, explicit) + refuse!(:dynamic, uri, entry.uri, "#{entry.uri.dump} is dynamic and lists no digests") if entry.dynamic? + + resource_entry = entry.resources.find { |candidate| candidate.uri == uri } + refuse!(:not_in_manifest, uri, entry.uri, "#{uri.dump} is not listed by #{entry.uri.dump}") unless resource_entry + if uri != entry.uri && !@verified.key?(entry) + refuse!(:skill_md_unverified, uri, entry.uri, "#{entry.uri.dump} has not been read and verified on this connection yet") + end + + [@generation, entry, resource_entry] + end + end + + # Remembers entries a catalog call returned; a replaced entry object loses its verified status. + def install(generation, skills) + @mutex.synchronize do + refuse!(:connection_changed, nil, nil, "the connection changed while the request was in flight") unless generation == @generation + + skills.each do |skill| + previous = @index[skill.uri] + @verified.delete(previous) if previous && !previous.equal?(skill) + @index[skill.uri] = skill + end + end + end + + # Settles a read: discards a stale or newly latched completion, latches the skill on a failure, and + # grants the entry object verified status after its own `SKILL.md` verified (an automatic read only + # while the object is still current; an explicit read for the object the caller supplied). + def complete_read(generation, uri, candidates, entry, explicit:, outcome:) + @mutex.synchronize do + refuse!(:connection_changed, uri, entry.uri, "the connection changed while the request was in flight") unless generation == @generation + latched = candidates.find { |candidate| @latched.include?(candidate) } + refuse!(:latched, uri, latched, "#{latched.dump} failed verification on this connection") if latched + + status, *values = outcome + if status == :failed + @latched << entry.uri + raise values.first + end + + if uri == entry.uri && (explicit || @index[entry.uri].equal?(entry)) + @verified[entry] = true + end + values + end + end + + def skills_capability(connected:) + @mutex.synchronize do + next unless admitted?(connected) + + read_key(read_key(@recorded, "extensions"), Skill::EXTENSION_ID) + end + end + + private + + def admit_or_refuse!(connected) + return if admitted?(connected) + + refuse!(:not_advertised, nil, nil, "no handshake of this connection advertised the skills extension and resources") + end + + def admitted?(connected) + return false unless @ready && connected && @recorded.is_a?(Hash) + + settings = read_key(read_key(@recorded, "extensions"), Skill::EXTENSION_ID) + settings.is_a?(Hash) && read_key(@recorded, "resources").is_a?(Hash) + end + + def resolve_entry(uri, candidates, explicit) + if explicit + if Skill::Uri.manifest_uri?(uri) && uri != explicit.uri + refuse!(:unknown_skill, uri, explicit.uri, "a SKILL.md URI resolves only to its own skill") + end + return explicit + end + + if Skill::Uri.manifest_uri?(uri) + entry = @index[uri] + refuse!(:unknown_skill, uri, nil, "#{uri.dump} has not been listed or fetched on this connection; pass skill: or call get_skill") unless entry + return entry + end + + remembered = candidates.reverse_each.map { |candidate| @index[candidate] }.compact + refuse!(:unknown_skill, uri, nil, "no remembered skill contains #{uri.dump}; pass skill: or call get_skill") if remembered.empty? + + # Only the most specific remembered skill containing `uri` is resolved automatically. The specification + # resolves a read against the entry the host loaded the skill from, and only the host knows which skill + # it is acting on, so an enclosing skill that lists the file (it lists its nested skills' files too) is + # reached with `skill:` alone: never because the nested entry is dynamic or leaves the file out. + entry = remembered.first + return entry if entry.dynamic? + + enclosing = remembered.drop(1).select do |candidate| + !candidate.dynamic? && candidate.resources.any? { |resource| resource.uri == uri } + end + unless enclosing.empty? + refuse!( + :ambiguous, + uri, + entry.uri, + "#{uri.dump} is also listed by #{enclosing.map { |candidate| candidate.uri.dump }.join(" and ")}; " \ + "pass skill: for the skill being acted on", + ) + end + + entry + end + + def read_key(hash, key) + return unless hash.is_a?(Hash) + + value = hash[key] + value.nil? ? hash[key.to_sym] : value + end + + def refuse!(reason, uri, skill_uri, detail) + raise SkillReadRefusedError.new("Skills call refused (#{reason}): #{detail}", reason: reason, uri: uri, skill_uri: skill_uri) + end + end + end + end +end diff --git a/lib/mcp/methods.rb b/lib/mcp/methods.rb index 60af7084..12a62e89 100644 --- a/lib/mcp/methods.rb +++ b/lib/mcp/methods.rb @@ -22,6 +22,11 @@ module Methods RESOURCES_SUBSCRIBE = "resources/subscribe" RESOURCES_UNSUBSCRIBE = "resources/unsubscribe" + # Skills extension (SEP-2640): registered only when a server configures skills, + # so an unconfigured server answers `-32601` rather than a capability error. + SKILLS_LIST = "skills/list" + SKILLS_GET = "skills/get" + TOOLS_CALL = "tools/call" TOOLS_LIST = "tools/list" diff --git a/lib/mcp/server.rb b/lib/mcp/server.rb index 3ef08c09..9987ceb6 100644 --- a/lib/mcp/server.rb +++ b/lib/mcp/server.rb @@ -180,16 +180,31 @@ class ValidationError < StandardError; end Methods::RESOURCES_LIST, Methods::RESOURCES_TEMPLATES_LIST, Methods::RESOURCES_READ, + # Both Skills results extend `CacheableResult`, so both carry the hints at 2026-07-28 (SEP-2640). + Methods::SKILLS_LIST, + Methods::SKILLS_GET, ].freeze - attr_accessor :description, :name, :title, :version, :website_url, :instructions, :tools, :prompts, :resource_templates, :server_context, :configuration, :capabilities, :transport, :logging_message_notification - attr_reader :icons, :resources, :page_size, :client_capabilities, :ttl_ms, :cache_scope, :request_state_security + attr_accessor :description, :name, :title, :version, :website_url, :instructions, :tools, :prompts, :resource_templates, :server_context, :configuration, :transport, :logging_message_notification + attr_reader :icons, :resources, :page_size, :client_capabilities, :ttl_ms, :cache_scope, :request_state_security, :capabilities + # The skills registered with `skills:` as a frozen Array (an empty one when only `skills_handlers` configured + # the extension), or nil when the server serves no skills. + attr_reader :skills # Replaces the icons advertised in `serverInfo`; a Hash is converted through `Icon.from`. def icons=(value) @icons = Icon.from_list(value) end + # Replaces the capabilities (a Hash or an `MCP::Server::Capabilities` builder). The Skills extension rules + # are checked before the value is taken, as at construction: the extension may be declared only when skills + # are configured, `resources` must be declared alongside it, and `directoryRead` is not served. + def capabilities=(value) + candidate = normalize_capabilities(value) + validate_skills_capabilities!(candidate) + @capabilities = candidate + end + def initialize( description: nil, icons: [], @@ -202,6 +217,7 @@ def initialize( prompts: [], resources: [], resource_templates: [], + skills: nil, server_context: nil, configuration: nil, capabilities: nil, @@ -224,6 +240,17 @@ def initialize( @prompts = prompts.to_h { |p| [p.name_value, p] } @resources = resources @resource_templates = resource_templates + # Skills (SEP-2640): `nil`, whether omitted or supplied, means the extension is not served; an empty Array + # serves an empty catalog. The file map of directory-backed skills is built once here and never changes. + @skills_configured = !skills.nil? + @skills = skills.nil? ? nil : normalize_skills!(skills) + @skills_index = (@skills || []).to_h { |skill| [skill.uri, skill] } + @skill_files = Skill.file_map(@skills || []) + @skill_file_roots = Skill.file_roots(@skills || []) + @skills_list_handler = nil + @skills_get_handler = nil + @skills_methods_registered = false + ensure_no_skill_file_collision!(resources.map(&:uri)) @resource_index = index_resources_by_uri(resources) @resources_list_handler = nil @server_context = server_context @@ -240,15 +267,12 @@ def initialize( @configuration = MCP.configuration.merge(configuration) @client = nil @client_protocol_version = nil + # Accept either a plain Hash or an `MCP::Server::Capabilities` builder, normalized before `validate!` + # so the Skills extension rules see the final Hash. + @capabilities = normalize_capabilities(capabilities) validate! - # Accept either a plain Hash or an `MCP::Server::Capabilities` builder. - @capabilities = if capabilities.is_a?(Capabilities) - capabilities.to_h - else - capabilities || default_capabilities - end @client_capabilities = nil @logging_message_notification = nil @@ -274,6 +298,7 @@ def initialize( # The protocol's own request methods, as opposed to the notifications above and to custom methods # registered later: these need a request id to be run. @protocol_request_methods = @handlers.keys.reject { |name| Methods.notification?(name) }.freeze + register_skills_handlers! if @skills_configured @transport = transport end @@ -332,6 +357,7 @@ def define_resource(uri: nil, name: nil, title: nil, description: nil, icons: [] mime_type: mime_type, annotations: annotations, size: size, meta: meta, &block ) + ensure_no_skill_file_collision!([resource.uri]) @resources << resource @resource_index[resource.uri] = resource @@ -350,6 +376,7 @@ def define_resource_template(uri_template: nil, name: nil, title: nil, descripti end def resources=(resources) + ensure_no_skill_file_collision!(resources.map(&:uri)) @resources = resources @resource_index = index_resources_by_uri(resources) end @@ -468,6 +495,48 @@ def resources_read_handler(&block) @handlers[Methods::RESOURCES_READ] = block end + # Serves the Skills extension (SEP-2640) from per-request handlers instead of the `skills:` registry, for + # catalogs that depend on the caller. Both handlers are required because a server declaring the extension + # MUST answer `skills/get` for every skill it serves, listed or not. + # + # `list` receives the request params (and `server_context:` when it declares the keyword) and returns + # the whole collection of `MCP::Skill` in a stable order; the server paginates it. `get` returns the `MCP::Skill` + # for `params[:uri]`, or nil for `-32602`. A result that is not what these contracts describe is answered + # with `-32603`. + # + # A server serves skills from the `skills:` registry or from handlers, never both: a registered skill, + # its `SKILL.md` entry in `resources/list`, and its files are served to every caller, which a catalog that + # hides a skill from some callers must not inherit, so handlers are refused while the registry holds skills. + # With handlers the application serves the files itself and applies the same rule there, for example from + # `resources_read_handler` with `MCP::Skill.read_file(skills_for(server_context), params[:uri])`. + # A skill nested in the directory of another is served through the enclosing skill as well, whose manifest + # lists its files, so hiding it from a caller means withholding the enclosing skill too. + # What the handlers answer is never stamped `cacheScope: "public"`: where the `cache_scope` of the server + # would make it so, the result says `"private"`. + # Every check runs before anything is assigned, so a failure leaves the previous configuration in place. + def skills_handlers(list:, get:) + unless list.respond_to?(:call) && get.respond_to?(:call) + raise ArgumentError, "skills_handlers requires callable list: and get: handlers." + end + if @skills && !@skills.empty? + raise ArgumentError, "skills_handlers cannot be combined with skills in the skills: registry, which are served " \ + "to every caller, files included. Return the skills from the handlers and serve their files yourself." + end + + validate_skills_capabilities!(@capabilities, configured: true) + unless @skills_methods_registered + [Methods::SKILLS_LIST, Methods::SKILLS_GET].each do |method_name| + raise MethodAlreadyDefinedError, method_name if @handlers.key?(method_name) + end + end + + @skills_list_handler = list + @skills_get_handler = get + @skills ||= [].freeze + @skills_configured = true + register_skills_handlers! + end + # Sets a custom handler for `completion/complete` requests. # The block receives the parsed request params and should return completion values. # @@ -574,6 +643,7 @@ def build_sampling_params( def validate! validate_tool_name! + validate_skills_capabilities!(@capabilities) # NOTE: The draft protocol version is the next version after 2025-11-25. if @configuration.protocol_version <= "2025-06-18" @@ -683,7 +753,8 @@ def handle_request(request, method, session: nil, related_request_id: nil, auth_ end begin - Methods.ensure_capability!(method, capabilities) + # The same snapshot `initialize` and `server/discover` advertise, so dispatch cannot disagree with it. + Methods.ensure_capability!(method, effective_capabilities) rescue Methods::MissingRequiredCapabilityError => e # A method of a capability the server does not declare is one the server does not implement, so it is answered as Method not found, # which the TypeScript and Python servers return for a handler they never registered, and which the Streamable HTTP transport maps to @@ -1254,18 +1325,20 @@ def discover(_request) # promise delivery over `subscriptions/listen` streams, so they are stripped when the transport does not serve that RPC # (e.g. stdio), matching the Python SDK's era-aware capability derivation. def discover_capabilities - return capabilities if @transport.respond_to?(:serves_subscriptions_listen?) && @transport.serves_subscriptions_listen? + advertised = effective_capabilities + return advertised if @transport.respond_to?(:serves_subscriptions_listen?) && @transport.serves_subscriptions_listen? - without_notification_flags(capabilities) + without_notification_flags(advertised) end # Capabilities as advertised by `initialize`. A transport that delivers no notifications to its sessions # (stateless Streamable HTTP) cannot keep the promise the `listChanged` and `subscribe` flags make, so they are # left out, as `server/discover` leaves them out when `subscriptions/listen` is not served. def handshake_capabilities - return capabilities unless @transport.respond_to?(:delivers_notifications?) && !@transport.delivers_notifications? + advertised = effective_capabilities + return advertised unless @transport.respond_to?(:delivers_notifications?) && !@transport.delivers_notifications? - without_notification_flags(capabilities) + without_notification_flags(advertised) end def without_notification_flags(capabilities) @@ -1417,7 +1490,7 @@ def list_resources(request, server_context: nil) resources = if @resources_list_handler invoke_resources_list_handler(request, server_context) else - @resources + @resources + skill_manifest_resources end page = paginate(resources, cursor: cursor_from(request), page_size: @page_size, request: request, &:to_h) @@ -1425,6 +1498,17 @@ def list_resources(request, server_context: nil) apply_cache_metadata({ resources: page[:items], nextCursor: page[:next_cursor] }.compact) end + # The `SKILL.md` of every registered skill as a resource entry, with `name` and `description` taken from + # the frontmatter and `text/markdown`, as the Skills extension asks of a `SKILL.md` resource; a resource + # the application registered at that URI takes its place. + def skill_manifest_resources + return [] if @skills.nil? || @skills.empty? + + @skills.reject { |skill| @resource_index.key?(skill.uri) }.map do |skill| + Resource.new(uri: skill.uri, name: skill.name, description: skill.frontmatter["description"], mime_type: "text/markdown") + end + end + # Calls the `resources_list_handler` block, forwarding `server_context:` only when the block opts in # by declaring the keyword (the same rule `dispatch_optional_context_handler` applies). def invoke_resources_list_handler(request, server_context) @@ -1435,6 +1519,229 @@ def invoke_resources_list_handler(request, server_context) end end + # `skills/list` (SEP-2640): the registry, or the `list` handler's collection, paginated like `resources/list`. + def list_skills(request, server_context: nil) + skills = if @skills_list_handler + result = invoke_skills_handler(@skills_list_handler, request, server_context) + problem = skills_problem(result) + raise invalid_skills_result(request, problem) if problem + + result + else + @skills + end + + page = paginate(skills, cursor: cursor_from(request), page_size: @page_size, request: request, &:to_h) + + apply_skills_cache_metadata({ skills: page[:items], nextCursor: page[:next_cursor] }.compact, @skills_list_handler) + end + + # `skills/get` (SEP-2640): answers for every skill the server serves, listed or not. The URI shape is checked + # before any handler runs; a `get` handler's result must be the skill for exactly that URI. + def get_skill(request, server_context: nil) + # Absent, null, or Array params reach here through the generic JSON-RPC validation. + uri = request.is_a?(Hash) ? request[:uri] : nil + unless Skill::Uri.manifest_uri?(uri) + raise RequestHandlerError.new( + "Invalid params: uri is not a skill SKILL.md URI", + request, + error_type: :invalid_params, + error_code: JsonRpcHandler::ErrorCode::INVALID_PARAMS, + error_data: { uri: uri }, + ) + end + + skill = if @skills_get_handler + result = invoke_skills_handler(@skills_get_handler, request, server_context) + unless result.nil? + raise invalid_skills_result(request, "skills/get returned #{result.class}") unless result.is_a?(Skill) + raise invalid_skills_result(request, "skills/get returned #{result.uri} for #{uri}") unless result.uri == uri + + problem = skills_problem([result]) + raise invalid_skills_result(request, problem) if problem + end + result + else + @skills_index[uri] + end + + if skill.nil? + raise RequestHandlerError.new( + "Skill not found: #{uri}", + request, + error_type: :invalid_params, + error_code: JsonRpcHandler::ErrorCode::INVALID_PARAMS, + error_data: { uri: uri }, + ) + end + + apply_skills_cache_metadata({ skill: skill.to_h }, @skills_get_handler) + end + + # A handler answers per caller and, returning skills, has no result of its own to carry a scope in, + # so what it answered is never marked for shared caches, whatever scope the server was configured with. + def apply_skills_cache_metadata(result, handler) + stamped = apply_cache_metadata(result) + return stamped unless handler && stamped[:cacheScope] == "public" + + stamped.merge(cacheScope: "private") + end + + def invoke_skills_handler(handler, request, server_context) + if handler_declares_server_context?(handler) + handler.call(request, server_context: server_context) + else + handler.call(request) + end + end + + # The wire sees one fixed message; what was wrong goes to the exception reporter only. + def invalid_skills_result(request, problem) + RequestHandlerError.new( + "Skills handler returned an invalid result", + request, + error_type: :internal_error, + error_code: JsonRpcHandler::ErrorCode::INTERNAL_ERROR, + original_error: StandardError.new(problem), + ) + end + + # Describes the first problem in a set of skills served together, or returns nil: every element + # an `MCP::Skill` and URIs unique. A skill may list any file under its own root, a nested skill's files + # included, because the specification wants the enclosing manifest complete. + def skills_problem(skills) + unless skills.is_a?(Array) && skills.all? { |skill| skill.is_a?(Skill) } + return "skills must be an Array of MCP::Skill (got #{skills.class})" + end + + registered = {} + skills.each do |skill| + return "skill #{skill.uri} is listed twice" if registered.key?(skill.uri) + + registered[skill.uri] = true + end + + nil + end + + def normalize_skills!(skills) + problem = skills_problem(skills) + raise ArgumentError, "skills: #{problem}." if problem + + skills.dup.freeze + end + + def register_skills_handlers! + return if @skills_methods_registered + + [Methods::SKILLS_LIST, Methods::SKILLS_GET].each do |method_name| + raise MethodAlreadyDefinedError, method_name if @handlers.key?(method_name) + end + @handlers[Methods::SKILLS_LIST] = method(:list_skills) + @handlers[Methods::SKILLS_GET] = method(:get_skill) + # Request methods of the extension, so like the protocol's own they are not run without an id; added here + # rather than at construction because `skills_handlers` registers them later. + @protocol_request_methods = (@protocol_request_methods + [Methods::SKILLS_LIST, Methods::SKILLS_GET]).freeze + @skills_methods_registered = true + end + + # A file of a directory-backed skill is served ahead of any resource with the same URI, so registering such + # a resource would be silently shadowed; it is refused before anything is committed. + def ensure_no_skill_file_collision!(uris) + uris.each do |uri| + next unless @skill_files.key?(uri) + + raise ArgumentError, "Resource URI #{uri} is a file of a directory-backed skill and cannot be registered as a resource." + end + end + + def normalize_capabilities(value) + case value + when Capabilities then value.to_h + when nil then default_capabilities + when Hash then value + else raise ArgumentError, "capabilities must be a Hash or an MCP::Server::Capabilities (got #{value.class})." + end + end + + # The capabilities a client sees in `initialize` and `server/discover`, and that dispatch gates on. The Skills + # entry is computed from the configured state rather than read from the stored Hash, which the public reader + # exposes for in-place mutation: when skills are configured the extension is advertised with `directoryRead` + # removed (nothing serves it) and `resources` is ensured, when they are not it is absent. Only the nested + # structures being modified are copied; the stored Hash is left as it is. + def effective_capabilities + base = @capabilities + extensions = declared_extensions(base) + declared_keys = skills_extension_keys(extensions) + unless @skills_configured + return base if declared_keys.empty? + + stripped = extensions.reject { |key, _| declared_keys.include?(key) } + snapshot = base.dup + snapshot.delete(:extensions) + snapshot.delete("extensions") + snapshot[:extensions] = stripped unless stripped.empty? + return snapshot + end + + settings = declared_keys.map { |key| extensions[key] }.find { |value| value.is_a?(Hash) } || {} + settings = settings.reject { |key, _| key.to_s == "directoryRead" } + others = extensions.reject { |key, _| declared_keys.include?(key) } + snapshot = base.dup + snapshot.delete("extensions") + snapshot[:extensions] = others.merge(Skill::EXTENSION_ID => settings) + # One `resources` key, so a second spelling cannot serialize after it and undo the declaration. + resources = capability_member(snapshot, :resources) + snapshot.delete("resources") + snapshot[:resources] = resources.is_a?(Hash) ? resources : {} + snapshot + end + + # The Skills extension rules for an explicitly supplied capabilities Hash (SEP-2640): the extension is declared + # only when skills are configured, its settings are a Hash without `directoryRead`, and `resources` is declared + # alongside it. Both key spellings are read, because JSON serialization erases the difference between + # `:extensions` and `"extensions"` or between a Symbol and a String extension identifier. + # Checked at construction, on `capabilities=`, and when handlers are installed. + def validate_skills_capabilities!(caps, configured: @skills_configured) + extensions = declared_extensions(caps) + declared_keys = skills_extension_keys(extensions) + + if !declared_keys.empty? && !configured + raise ArgumentError, "capabilities declare #{Skill::EXTENSION_ID} but no skills are configured; " \ + "pass skills: (an empty Array serves an empty catalog) or call skills_handlers." + end + + declared_keys.each do |key| + settings = extensions[key] + unless settings.is_a?(Hash) + raise ArgumentError, "The #{Skill::EXTENSION_ID} capability must be a Hash (got #{settings.class})." + end + if settings.any? { |name, value| name.to_s == "directoryRead" && value } + raise ArgumentError, "directoryRead is not served by this release; do not declare it under #{Skill::EXTENSION_ID}." + end + end + + if configured && !capability_member(caps, :resources).is_a?(Hash) + raise ArgumentError, "SEP-2640 requires the resources capability alongside #{Skill::EXTENSION_ID}; declare resources too." + end + end + + def capability_member(caps, name) + value = caps[name] + value.nil? ? caps[name.to_s] : value + end + + # The `extensions` declarations under both key spellings merged into one Hash (empty when there are none), + # so a Skills declaration is found whichever spelling holds it and the other extensions survive the rewrite. + def declared_extensions(caps) + [caps[:extensions], caps["extensions"]].grep(Hash).reduce({}, :merge) + end + + # Every key of an `extensions` Hash that spells the Skills extension identifier, as a String or a Symbol. + def skills_extension_keys(extensions) + extensions.keys.select { |key| key.to_s == Skill::EXTENSION_ID } + end + # Default `resources/read` handler: routes to class-based resources and resource templates. # Fully replaced when `resources_read_handler` is set. When no class-based resource or template is registered, # unknown URIs keep the historical no-op `[]` response instead of raising. @@ -1442,11 +1749,28 @@ def read_resource(request, server_context: nil) uri = request[:uri] add_instrumentation_data(resource_uri: uri) + # Files of directory-backed skills are looked up by exact string equality in a map built at construction, + # so request data never reaches the filesystem; a miss falls through to the routing below. + record = @skill_files[uri] + return Skill.read_record(record, request) if record + resource = @resource_index[uri] if resource.is_a?(Class) return normalize_resource_contents(call_resource_contents(resource, server_context)) end + # A URI under a directory-backed skill that names none of its files: the manifest is complete, so the file + # is not served, and the Skills extension asks for `-32602` where the base reader keeps its `[]`. + if resource.nil? && uri.is_a?(String) && @skill_file_roots.any? { |root| Skill::Uri.under?(uri, root) } + raise RequestHandlerError.new( + "Skill file not found: #{uri}", + request, + error_type: :invalid_params, + error_code: JsonRpcHandler::ErrorCode::INVALID_PARAMS, + error_data: { uri: uri }, + ) + end + @resource_templates.each do |template| next unless template.is_a?(Class) diff --git a/lib/mcp/skill.rb b/lib/mcp/skill.rb new file mode 100644 index 00000000..9ea03c1d --- /dev/null +++ b/lib/mcp/skill.rb @@ -0,0 +1,522 @@ +# frozen_string_literal: true + +require "digest" +require_relative "skill/resource_entry" +require_relative "skill/frontmatter" +require_relative "skill/directory" + +module MCP + # A skill as the Skills extension describes it (SEP-2640, `io.modelcontextprotocol/skills`): a `skill://` + # URI naming its `SKILL.md`, the frontmatter of that file kept verbatim, either the list of its files + # with their digests and sizes or the String `"dynamic"` for content generated per request, and the `_meta` + # of the entry when it has one, passed through under the JSON rules of the frontmatter with no key acted on + # (keys of the extension itself use the `io.modelcontextprotocol.skills/` prefix). + # + # Every check runs at construction, as `MCP::Icon` does, so a manifest a host may reject fails on + # the server at definition time: the URI grammar, the frontmatter (`name` and `description`, `name` equal to + # the last segment of the skill path, JSON-representable values), and the resource entries (under + # the skill root, unique, the skill's own `SKILL.md` among them). A skill defined locally is also held to + # the unreserved URI character set, the Agent Skills rules for `name` and `description`, and the SEP limits + # (512 entries, 16 MiB); entries a client receives from a server are checked structurally only, because + # a host MUST accept skills up to the limits and MAY accept larger ones, and the Agent Skills guidance for + # clients is to load a skill whose name breaks a rule anyway. The files of a nested skill, its `SKILL.md` + # included, are supporting files of the enclosing skill and appear in both manifests, as the specification + # requires of a complete manifest. + # + # Instances are frozen value objects. Equality is identity: the client binds verified state to the exact + # entry object it checked. + # + # https://github.com/modelcontextprotocol/ext-skills/blob/main/specification/stable/skills.mdx + class Skill + EXTENSION_ID = "io.modelcontextprotocol/skills" + # SEP-2640 limits per skill: servers SHOULD NOT serve a skill beyond them, hosts MUST accept up to them. + MAX_RESOURCES = 512 + MAX_TOTAL_SIZE = 16 * 1024 * 1024 + # The Agent Skills rules SEP-2640 requires of the last segment of a skill path, its `name`: lowercase + # letters, digits, and single hyphens that neither lead nor trail, 64 characters at most. `description` + # holds at most 1024 characters. + NAME_PATTERN = /\A[a-z0-9]+(?:-[a-z0-9]+)*\z/.freeze + MAX_NAME_LENGTH = 64 + MAX_DESCRIPTION_LENGTH = 1024 + # The `resources` value of a skill whose files are generated per request and carry no digests. + DYNAMIC = "dynamic" + + # `skill://` URI rules shared by the server and the client. URIs are plain Strings: nothing is decoded, + # normalized, or joined, and `URI.parse` is never used. + # + # @api private + module Uri + extend self + + SCHEME = "skill://" + MANIFEST = "SKILL.md" + # RFC 3986 unreserved characters, the only ones a locally defined URI may use in a segment. + LOCAL_SEGMENT = /\A[A-Za-z0-9._~-]+\z/.freeze + # SDK limits (not protocol rules) on a URI the client resolves against its remembered skills. Resolution + # considers every ancestor root, so without a bound one read would cost time and memory quadratic in + # the length of a URI that may come from untrusted model output. + MAX_RESOLVED_BYTES = 8 * 1024 + MAX_RESOLVED_SEGMENTS = 128 + + # Whether `uri` is within the limits above; counts separators instead of splitting, so the check is linear. + def resolvable?(uri) + uri.bytesize <= MAX_RESOLVED_BYTES && uri.count("/") <= MAX_RESOLVED_SEGMENTS + 1 + end + + # The path segments of a `skill://` URI, or nil when it is not structurally valid: a String in + # an ASCII-compatible encoding with at least one segment, none empty, none `.` or `..`. + def segments(uri) + return unless uri.is_a?(String) && uri.encoding.ascii_compatible? && uri.start_with?(SCHEME) + + parts = uri[SCHEME.length..-1].split("/", -1) + return if parts.empty? || parts.any? { |part| part.empty? || part == "." || part == ".." } + + parts + end + + def structural?(uri) + !segments(uri).nil? + end + + # The segments before `SKILL.md`, or nil when the URI does not name a `SKILL.md`. + def skill_path(uri) + parts = segments(uri) + return unless parts && parts.length >= 2 && parts.last == MANIFEST + + parts[0...-1] + end + + def manifest_uri?(uri) + !skill_path(uri).nil? + end + + def local_segments?(parts) + parts.all? { |part| LOCAL_SEGMENT.match?(part) } + end + + def root(skill_path) + "#{SCHEME}#{skill_path.join("/")}" + end + + # Whether `uri` lies under `root` at a segment boundary: `skill://ab/x` is not under `skill://a`. + def under?(uri, root) + uri.start_with?("#{root}/") + end + + # The `SKILL.md` URIs of every skill whose root could contain `uri`, least specific first; for + # a `SKILL.md` URI the last candidate is the URI itself. + def candidate_skill_uris(uri) + parts = segments(uri) + return [] unless parts && parts.length >= 2 + + directories = parts[0...-1] + (1..directories.length).map { |count| "#{root(directories.first(count))}/#{MANIFEST}" } + end + end + + # JSON-value helpers for frontmatter and capability copies. + # + # @api private + module Json + extend self + + # The largest integer every JSON reader represents exactly (RFC 7493); beyond it JavaScript loses digits, + # so two hosts would disagree about one frontmatter. + MAX_SAFE_INTEGER = (2**53) - 1 + # The default nesting limit of `JSON.parse`, which the SDK's transports apply to a whole message. Restated + # here so that a value from a transport parsing without one is refused instead of exhausting the stack. + MAX_DEPTH = 100 + + # A deep copy with String keys holding only JSON-representable values (String that JSON can encode, Integer within + # the safe range, finite Float, true, false, nil, Array, Hash); refuses cycles, nesting beyond `MAX_DEPTH`, and keys + # that collide once stringified. + def normalize(value, path = {}.compare_by_identity) + case value + when String then normalize_string(value) + when TrueClass, FalseClass, NilClass then value + when Integer + raise ArgumentError, "must hold integers within 2**53" unless value.abs <= MAX_SAFE_INTEGER + + value + when Float + raise ArgumentError, "must hold finite numbers" unless value.finite? + + value + when Array + raise ArgumentError, "must not be cyclic" if path.key?(value) + + refuse_depth!(path.size) + path[value] = true + result = value.map { |member| normalize(member, path) } + path.delete(value) + result + when Hash + raise ArgumentError, "must not be cyclic" if path.key?(value) + + refuse_depth!(path.size) + path[value] = true + result = {} + value.each do |key, member| + name = case key + when String then normalize_string(key) + when Symbol then normalize_string(key.to_s) + else raise ArgumentError, "keys must be Strings or Symbols (got #{key.class})" + end + raise ArgumentError, "keys collide after stringification (#{name.dump})" if result.key?(name) + + result[name] = normalize(member, path) + end + path.delete(value) + result + else + raise ArgumentError, "must be JSON-representable (got #{value.class})" + end + end + + def deep_freeze(value) + case value + when Hash + value.each_value { |member| deep_freeze(member) } + when Array + value.each { |member| deep_freeze(member) } + end + value.freeze + end + + # A deep copy of a value a transport returned; refuses nesting beyond `MAX_DEPTH` with an `ArgumentError`. + def deep_dup(value, depth = 0) + case value + when Hash + refuse_depth!(depth) + value.each_with_object({}) { |(key, member), copy| copy[deep_dup(key, depth + 1)] = deep_dup(member, depth + 1) } + when Array + refuse_depth!(depth) + value.map { |member| deep_dup(member, depth + 1) } + when String then value.dup + else value + end + end + + # Structural equality with Integer and Float kept distinct, Hash keys compared as sets, and Arrays in order. + def equal?(expected, actual) + case expected + when Hash + return false unless actual.is_a?(Hash) && expected.size == actual.size + + expected.all? { |key, member| actual.key?(key) && equal?(member, actual[key]) } + when Array + return false unless actual.is_a?(Array) && expected.size == actual.size + + expected.each_with_index.all? { |member, index| equal?(member, actual[index]) } + when Float + actual.is_a?(Float) && (expected == actual || (expected.nan? && actual.nan?)) + when Integer + actual.is_a?(Integer) && expected == actual + else + expected.class.equal?(actual.class) && expected == actual + end + end + + private + + def refuse_depth!(depth) + raise ArgumentError, "must not nest deeper than #{MAX_DEPTH} levels" if depth >= MAX_DEPTH + end + + def normalize_string(value) + raise ArgumentError, "must hold strings JSON can encode" unless utf8_encodable?(value) + + value.dup + end + + # JSON carries UTF-8. `encode` hands a String already labeled UTF-8 back unchecked, so that case asks + # `valid_encoding?` instead; a binary String with a byte beyond ASCII fails the conversion. + def utf8_encodable?(value) + return value.valid_encoding? if value.encoding == Encoding::UTF_8 + + value.encode(Encoding::UTF_8) + true + rescue EncodingError + false + end + end + + private_constant :Frontmatter + + class << self + # The `capabilities.extensions` fragment advertising the extension, for a hand-built + # `MCP::Server::Capabilities`; a server configured with `skills:` adds it itself. + def capability + { EXTENSION_ID => {} } + end + + # Builds a skill from an entry a server returned (String or Symbol keys), under the structural rules only. + # + # @api private + def from_entry(entry) + raise ArgumentError, "A skill entry must be a Hash (got #{entry.class})." unless entry.is_a?(Hash) + + skill = allocate + skill.__send__( + :initialize_members, + read_key(entry, "uri"), + read_key(entry, "frontmatter"), + read_key(entry, "resources"), + meta: read_key(entry, "_meta"), + local: false, + file_records: nil, + ) + skill + end + + # Builds a locally defined skill that also carries the records `from_directory` serves files from. + # + # @api private + def with_file_records(uri:, frontmatter:, resources:, file_records:, meta: nil) + skill = allocate + skill.__send__(:initialize_members, uri, frontmatter, resources, meta: meta, local: true, file_records: file_records) + skill + end + + # Checks fetched bytes against the entry for `uri` in `skill`'s manifest (size, then SHA-256), for hosts + # that read skill files through `MCP::Client#read_resource` rather than `#read_skill_resource`. Pass + # exactly the bytes received: `text.b` for a `text` item, the decoded `blob` otherwise. Raises + # `MCP::Client::SkillReadRefusedError` when the skill is dynamic or does not list `uri`, and + # `MCP::Client::SkillVerificationError` on a mismatch. Does not compare frontmatter: a `SKILL.md` + # also needs `verify_frontmatter`. + def verify_resource(skill, uri, bytes) + check_skill_and_uri!(skill, uri) + raise ArgumentError, "The value of bytes must be a String (got #{bytes.class})." unless bytes.is_a?(String) + + if skill.dynamic? + raise Client::SkillReadRefusedError.new( + "Skill #{skill.uri.dump} is dynamic and lists no digests to verify #{uri.dump} against", + reason: :dynamic, uri: uri, skill_uri: skill.uri, + ) + end + + entry = skill.resources.find { |candidate| candidate.uri == uri } + unless entry + raise Client::SkillReadRefusedError.new( + "Resource #{uri.dump} is not listed by #{skill.uri.dump}", + reason: :not_in_manifest, uri: uri, skill_uri: skill.uri, + ) + end + + unless bytes.bytesize == entry.size + raise Client::SkillVerificationError.new( + "Resource #{uri.dump} does not have the size listed by #{skill.uri.dump}", + uri: uri, skill_uri: skill.uri, reason: :size, + ) + end + + unless "sha256:#{Digest::SHA256.hexdigest(bytes)}" == entry.digest + raise Client::SkillVerificationError.new( + "Resource #{uri.dump} does not have the digest listed by #{skill.uri.dump}", + uri: uri, skill_uri: skill.uri, reason: :digest, + ) + end + + true + end + + # Compares the frontmatter of a fetched `SKILL.md` (its text) with `skill`'s entry field by field, + # the host-side MUST of SEP-2640, which `verify_resource` does not cover. Raises + # `MCP::Client::SkillVerificationError` with reason `:parse` or `:frontmatter`. + def verify_frontmatter(skill, text) + raise ArgumentError, "The value of skill must be an MCP::Skill (got #{skill.class})." unless skill.is_a?(Skill) + raise ArgumentError, "The value of text must be a String (got #{text.class})." unless text.is_a?(String) + + actual = begin + Frontmatter.load(text) + rescue Frontmatter::Error => e + raise Client::SkillVerificationError.new( + "The frontmatter of #{skill.uri.dump} cannot be read", + uri: skill.uri, skill_uri: skill.uri, reason: e.reason, + ) + end + + unless Json.equal?(skill.frontmatter, actual) + raise Client::SkillVerificationError.new( + "The frontmatter of #{skill.uri.dump} differs from its entry", + uri: skill.uri, skill_uri: skill.uri, reason: :frontmatter, + ) + end + + true + end + + # The file records of directory-backed skills keyed by URI, for the server's default resource reader. + # A nested skill and its enclosing skill share the records of the nested files, so the same record + # under the same URI is fine; two different records for one URI are not. + # + # @api private + def file_map(skills) + map = {} + skills.each do |skill| + records = skill.__send__(:file_records) + next unless records + + records.each do |uri, record| + existing = map[uri] + next if existing == record + raise ArgumentError, "The skill file #{uri} is served by more than one skill from different files." if existing + + map[uri] = record + end + end + map.freeze + end + + # The roots (`skill://`) of the directory-backed skills among `skills`, for the server to tell + # a read of an unserved file under one of them from an ordinary unknown resource. + # + # @api private + def file_roots(skills) + skills.select { |skill| skill.__send__(:file_records) }.map { |skill| Uri.root(skill.skill_path) }.uniq.freeze + end + + private + + def read_key(hash, key) + value = hash[key] + value.nil? ? hash[key.to_sym] : value + end + + def check_skill_and_uri!(skill, uri) + raise ArgumentError, "The value of skill must be an MCP::Skill (got #{skill.class})." unless skill.is_a?(Skill) + raise ArgumentError, "The value of uri must be a String (got #{uri.class})." unless uri.is_a?(String) + end + end + + attr_reader :uri, :skill_path, :name, :frontmatter, :resources + + # The `_meta` of the entry as a frozen Hash with String keys, or nil when the entry carries none. + attr_reader :meta + + def initialize(uri:, frontmatter:, resources:, meta: nil) + initialize_members(uri, frontmatter, resources, meta: meta, local: true, file_records: nil) + end + + def dynamic? + @resources == DYNAMIC + end + + # The sum of the listed sizes, or nil for a dynamic skill; the hook for a host that declines large skills. + def total_size + @total_size + end + + def to_h + entry = { uri: uri, frontmatter: frontmatter, resources: dynamic? ? DYNAMIC : resources.map(&:to_h) } + entry[:_meta] = meta if meta + entry + end + + private + + def file_records + @file_records + end + + def initialize_members(uri, frontmatter, resources, meta:, local:, file_records:) + path = Uri.skill_path(uri) + unless path + raise ArgumentError, "The value of uri must be a skill:///SKILL.md URI with non-empty path segments." + end + if local && !Uri.local_segments?(path) + raise ArgumentError, "The value of uri may only use letters, digits, -, ., _, and ~ in its segments." + end + + normalized = begin + Json.normalize(frontmatter) + rescue ArgumentError => e + raise ArgumentError, "The value of frontmatter #{e.message}." + end + unless normalized.is_a?(Hash) + raise ArgumentError, "The value of frontmatter must be a Hash (got #{frontmatter.class})." + end + ["name", "description"].each do |field| + unless normalized[field].is_a?(String) && !normalized[field].empty? + raise ArgumentError, "The value of frontmatter must hold a non-empty String under #{field}." + end + end + unless normalized["name"] == path.last + raise ArgumentError, "The frontmatter name must equal the last segment of the skill path." + end + check_agent_skills_rules!(normalized) if local + + @uri = -uri + @skill_path = path.map { |segment| -segment }.freeze + @frontmatter = Json.deep_freeze(normalized) + @name = @frontmatter["name"] + @resources = normalize_resources(resources, local) + @total_size = dynamic? ? nil : @resources.sum(&:size) + @meta = normalize_meta(meta) + @file_records = file_records + freeze + end + + # The length is looked at first, so the pattern never runs over a long name. + def check_agent_skills_rules!(frontmatter) + name = frontmatter["name"] + unless name.length <= MAX_NAME_LENGTH && NAME_PATTERN.match?(name) + raise ArgumentError, "The frontmatter name must be 1 to #{MAX_NAME_LENGTH} lowercase letters, digits, " \ + "and single hyphens between them." + end + return if frontmatter["description"].length <= MAX_DESCRIPTION_LENGTH + + raise ArgumentError, "The frontmatter description must hold at most #{MAX_DESCRIPTION_LENGTH} characters." + end + + def normalize_meta(meta) + return if meta.nil? + + normalized = begin + Json.normalize(meta) + rescue ArgumentError => e + raise ArgumentError, "The value of meta #{e.message}." + end + raise ArgumentError, "The value of meta must be a Hash (got #{meta.class})." unless normalized.is_a?(Hash) + + Json.deep_freeze(normalized) + end + + def normalize_resources(resources, local) + return DYNAMIC if resources == DYNAMIC + + unless resources.is_a?(Array) + raise ArgumentError, "The value of resources must be \"dynamic\" or an Array of entries (got #{resources.class})." + end + + root = Uri.root(@skill_path) + seen = {} + entries = resources.each_with_index.map do |value, index| + entry = begin + # A remote entry may carry members this SDK does not know; a local definition may not. + local ? ResourceEntry.from(value) : ResourceEntry.from_wire(value) + rescue ArgumentError => e + raise ArgumentError, "resources[#{index}]: #{e.message}" + end + raise ArgumentError, "resources[#{index}]: The entry uri must lie under #{root.dump}." unless Uri.under?(entry.uri, root) + if local && !Uri.local_segments?(Uri.segments(entry.uri)) + raise ArgumentError, "resources[#{index}]: The entry uri may only use letters, digits, -, ., _, and ~ in its segments." + end + raise ArgumentError, "resources[#{index}]: The entry uri is listed twice." if seen.key?(entry.uri) + + seen[entry.uri] = true + entry + end + raise ArgumentError, "The value of resources must list the skill's own SKILL.md." unless seen.key?(@uri) + + if local + raise ArgumentError, "The value of resources must hold at most #{MAX_RESOURCES} entries." if entries.size > MAX_RESOURCES + if entries.sum(&:size) > MAX_TOTAL_SIZE + raise ArgumentError, "The value of resources must list at most #{MAX_TOTAL_SIZE} bytes in total." + end + end + + entries.freeze + end + end +end diff --git a/lib/mcp/skill/directory.rb b/lib/mcp/skill/directory.rb new file mode 100644 index 00000000..9c96dafe --- /dev/null +++ b/lib/mcp/skill/directory.rb @@ -0,0 +1,316 @@ +# frozen_string_literal: true + +require "digest" + +module MCP + class Skill + # Directory-backed skills (SEP-2640): `from_directory` turns every directory under a root that holds + # a `SKILL.md` into a skill whose files the server's default `resources/read` serves, with the digests and + # sizes computed here. + # + # Rules fixed for the first release: regular files only (symbolic links, FIFOs, sockets, and devices are + # skipped, hard links refused), dotfiles skipped, everything else published (editor backups included), every directory and file + # name limited to the unreserved URI characters, a directory holding its own `SKILL.md` a skill of its own + # whose files the enclosing skill lists as supporting files too, files read at request time and refused + # with one generic error when they changed since registration. + class << self + # Scans `root` and returns the skills found under it (an Array of `MCP::Skill`, ordered by URI); every + # problem is an `ArgumentError` naming the path. `root` is resolved once with `File.realpath`, so + # a symbolic link to the directory is fine; a `SKILL.md` directly under `root`, or no skill at all, is + # refused, because `root` is the container of skill directories, not a skill. + # + # The tree is trusted as the operator's: it must be writable only by the operator while skills are built + # and served. The checks on each file narrow what a concurrent change to the tree can do; they do not + # confine a writer who can change it. + def from_directory(root) + root = root.to_path if root.respond_to?(:to_path) + raise ArgumentError, "The skills root must be a path String (got #{root.class})." unless root.is_a?(String) + + real_root = begin + File.realpath(root) + rescue SystemCallError => e + raise ArgumentError, "The skills root #{root} cannot be resolved (#{e.class})." + end + raise ArgumentError, "The skills root #{real_root} is not a directory." unless File.directory?(real_root) + if File.exist?(File.join(real_root, Uri::MANIFEST)) + raise ArgumentError, "The skills root #{real_root} holds a SKILL.md itself; pass the directory that contains the skill directories." + end + + skills = [] + scan_directory(real_root, [], skills) + raise ArgumentError, "No directory holding a SKILL.md was found under #{real_root}." if skills.empty? + + skills.sort_by(&:uri) + end + + # Serves one file of a directory-backed skill for a custom `resources_read_handler`, which replaces + # the default reader and with it the serving of skill files: returns the `contents` Array holding one item, + # or nil when `uri` is not a file of any of `skills`. A file that changed since registration raises + # the same `-32603` the default reader answers. The files of a nested skill are files of the enclosing skill + # as well, so a caller given the enclosing skill reads them whether or not the nested one is among `skills`. + def read_file(skills, uri) + record = file_map(skills)[uri] + return unless record + + [read_record_item(record)] + rescue FileError => e + raise file_changed_error(nil, e) + end + + # The default reader's path for a file record found in the server's map. + # + # @api private + def read_record(record, request) + [read_record_item(record)] + rescue FileError => e + raise file_changed_error(request, e) + end + + private + + # Internal: a file could not be read as registered; the message names the path and is reported only. + class FileError < StandardError; end + + # Frozen through, so a custom reader handed `read_file`'s item cannot alter the URI later responses carry. + FileRecord = Struct.new(:uri, :path, :size, :digest, :dev, :ino, :text, keyword_init: true) do + def initialize(**kwargs) + super(**kwargs) + each { |member| member.freeze if member.is_a?(String) } + freeze + end + end + + MIME_TYPES = { ".md" => "text/markdown" }.freeze + + def scan_directory(directory, segments, skills) + children_of(directory).each do |name| + next if name.start_with?(".") + + path = File.join(directory, name) + next unless lstat_of(path).directory? + + check_name!(name, path) + child_segments = segments + [name] + if skill_directory?(path) + skills << collect_skill(path, child_segments, skills) + else + scan_directory(path, child_segments, skills) + end + end + end + + def skill_directory?(path) + File.lstat(File.join(path, Uri::MANIFEST)).file? + rescue SystemCallError + false + end + + def check_name!(name, path) + return if Uri::LOCAL_SEGMENT.match?(name) + + raise ArgumentError, "The name of #{path} may only use letters, digits, -, ., _, and ~, so that it can be a skill:// URI segment." + end + + # Directory listing and `lstat` in a stable order; a filesystem error is reported as an `ArgumentError` + # like every other construction problem, since the input is the operator's own tree. + def children_of(directory) + Dir.children(directory).sort + rescue SystemCallError => e + raise ArgumentError, "The directory #{directory} cannot be listed (#{e.class})." + end + + def lstat_of(path) + File.lstat(path) + rescue SystemCallError => e + raise ArgumentError, "The entry #{path} cannot be examined (#{e.class})." + end + + # Builds the skill rooted at `directory`: walks its files, reads the frontmatter from the same bytes + # the manifest's digest was computed over, and attaches the file records. + def collect_skill(directory, segments, skills) + root_uri = Uri.root(segments) + manifest_uri = "#{root_uri}/#{Uri::MANIFEST}" + walk = { records: {}, manifest_uri: manifest_uri, manifest_bytes: nil, budget: MAX_TOTAL_SIZE } + walk_skill_files(directory, segments, walk, skills) + + records = walk[:records].sort_by { |uri, _record| uri }.to_h + manifest_bytes = walk[:manifest_bytes] + # `skill_directory?` saw the file, so only a change to the tree during the scan leaves it unread. + raise ArgumentError, "The SKILL.md of #{directory} disappeared while the directory was being scanned." if manifest_bytes.nil? + + frontmatter = begin + Frontmatter.load(manifest_bytes) + rescue Frontmatter::Error => e + raise ArgumentError, "The frontmatter of #{records.fetch(manifest_uri).path} cannot be read (#{e.reason}); see the frontmatter rules in the Skills documentation." + end + + entries = records.values.map { |record| ResourceEntry.new(uri: record.uri, digest: record.digest, size: record.size) } + begin + with_file_records(uri: manifest_uri, frontmatter: frontmatter, resources: entries, file_records: records.freeze) + rescue ArgumentError => e + raise ArgumentError, "#{directory}: #{e.message}" + end + end + + def walk_skill_files(directory, segments, walk, skills) + children_of(directory).each do |name| + next if name.start_with?(".") + + path = File.join(directory, name) + stat = lstat_of(path) + next unless stat.directory? || stat.file? + + check_name!(name, path) + child_segments = segments + [name] + if stat.directory? + if skill_directory?(path) + # A nested skill is published on its own, and its files are supporting files of the enclosing + # skill as well: the records are shared, so both manifests carry the same digests and sizes. + nested = collect_skill(path, child_segments, skills) + skills << nested + adopt_records(nested.__send__(:file_records), walk) + else + walk_skill_files(path, child_segments, walk, skills) + end + else + uri = Uri.root(child_segments) + record, bytes = capture_file(path, uri, walk) + walk[:records][uri] = record + walk[:manifest_bytes] = bytes if uri == walk[:manifest_uri] + end + end + end + + # Adds a nested skill's records to the enclosing skill's walk; they count against its size budget like + # its own files, since the specification counts the limits over every listed entry. + def adopt_records(records, walk) + records.each_value do |record| + if record.size > walk[:budget] + raise ArgumentError, "#{record.path} takes the enclosing skill past the #{MAX_TOTAL_SIZE} byte limit of SEP-2640" + end + + walk[:budget] -= record.size + walk[:records][record.uri] = record + end + end + + # Hashes one file and returns its record with the bytes it hashed. The skill's remaining size budget is + # checked against `fstat` before anything is read, so an oversized file is refused without loading it, and + # the read itself stops one byte past the budget, so a file growing after that check is never held whole. + def capture_file(path, uri, walk) + # The paths come from the resolved root, so `realpath` must return the path unchanged; an intermediate + # directory swapped for a symbolic link after `lstat` breaks that. Checked before and after the open, + # so passing takes two swaps inside that window instead of one. + raise FileError, "#{path} is not the file its path names" unless resolved_path?(path) + + open_regular_file(path) do |io, stat| + # A hard link reaches a file outside the tree the way a symbolic link would, so it is refused rather than + # skipped, which tells the operator that a file is missing from the manifest. The handle must also be + # the file its path names now. + raise FileError, "#{path} has more than one hard link" if stat.nlink > 1 + + current = File.stat(path) + unless resolved_path?(path) && current.dev == stat.dev && current.ino == stat.ino + raise FileError, "#{path} is not the file its path names" + end + + if stat.size > walk[:budget] + raise FileError, "#{path} takes the skill past the #{MAX_TOTAL_SIZE} byte limit of SEP-2640" + end + + bytes = io.read(walk[:budget] + 1) || "".b + if bytes.bytesize > walk[:budget] + raise FileError, "#{path} takes the skill past the #{MAX_TOTAL_SIZE} byte limit of SEP-2640" + end + + walk[:budget] -= bytes.bytesize + record = FileRecord.new( + uri: uri, + path: path, + size: bytes.bytesize, + digest: "sha256:#{Digest::SHA256.hexdigest(bytes)}", + dev: stat.dev, + ino: stat.ino, + text: bytes.dup.force_encoding(Encoding::UTF_8).valid_encoding?, + ) + [record, bytes] + end + rescue FileError => e + raise ArgumentError, e.message + end + + def resolved_path?(path) + File.realpath(path) == path + rescue SystemCallError + false + end + + # Opens a regular file for reading without following a symbolic link in its last component, and hands + # the handle and its `fstat` to the block; everything the block reads comes from the handle it checked. + # Where the platform lacks `NOFOLLOW`, an `lstat` before the open refuses a symbolic link instead; + # the `fstat` comparison and the digest over the served bytes still close the race. + def open_regular_file(path) + if !nofollow_supported? && File.lstat(path).symlink? + raise FileError, "#{path} is a symbolic link" + end + + File.open(path, open_flags) do |io| + io.binmode + stat = io.stat + raise FileError, "#{path} is not a regular file" unless stat.file? + + yield io, stat + end + rescue SystemCallError, IOError => e + raise FileError, "#{path} cannot be read (#{e.class})" + end + + def open_flags + flags = File::RDONLY | File::NONBLOCK + flags |= File::NOFOLLOW if nofollow_supported? + flags + end + + def nofollow_supported? + File::Constants.const_defined?(:NOFOLLOW) + end + + # Reads a registered file for serving: the handle must still be the recorded inode with the recorded size + # and no additional hard link (`nlink` is compared the way registration does, so a filesystem that reports 0 + # is served), and the bytes read (at most one more than recorded, so growth shows) must still have + # the recorded digest. + def read_record_item(record) + open_regular_file(record.path) do |io, stat| + unless stat.size == record.size && stat.dev == record.dev && stat.ino == record.ino && stat.nlink <= 1 + raise FileError, "#{record.path} changed since registration (inode, size, or link count)" + end + + data = io.read(record.size + 1) || "".b + unless data.bytesize == record.size && "sha256:#{Digest::SHA256.hexdigest(data)}" == record.digest + raise FileError, "#{record.path} changed since registration (content)" + end + + item = { uri: record.uri } + mime_type = MIME_TYPES[File.extname(record.path)] + item[:mimeType] = mime_type if mime_type + if record.text + item[:text] = data.force_encoding(Encoding::UTF_8) + else + item[:blob] = [data].pack("m0") + end + item + end + end + + def file_changed_error(request, cause) + Server::RequestHandlerError.new( + "Skill file changed since registration", + request, + error_type: :internal_error, + error_code: JsonRpcHandler::ErrorCode::INTERNAL_ERROR, + original_error: cause, + ) + end + end + end +end diff --git a/lib/mcp/skill/frontmatter.rb b/lib/mcp/skill/frontmatter.rb new file mode 100644 index 00000000..cbfc806c --- /dev/null +++ b/lib/mcp/skill/frontmatter.rb @@ -0,0 +1,262 @@ +# frozen_string_literal: true + +require "psych" + +module MCP + class Skill + # Reads and compares the YAML frontmatter of a `SKILL.md`, the same way on the server (where + # `from_directory` publishes it) and on the client (where a host compares what it fetched with the entry + # it holds, a SEP-2640 MUST). The rules are stricter than YAML so that every YAML library reads the block + # the same way: one document, no directives or document markers, no tags, no aliases or merge keys, keys + # that are untagged Strings, and no plain scalar whose YAML 1.1 reading (Psych's) differs from its + # YAML 1.2 core-schema reading (the JavaScript and Go libraries'); such a value is written quoted instead. + # Values must be JSON-representable because the entry travels as JSON. + module Frontmatter + extend self + + # Raised internally with `reason` `:parse` (the block cannot be read under these rules) or `:frontmatter` + # (it reads fine but differs from the entry); callers translate it into their own error types. + class Error < StandardError + attr_reader :reason + + def initialize(reason, message = "frontmatter #{reason}") + super(message) + @reason = reason + end + end + + BOM = "\xEF\xBB\xBF".b.freeze + DELIMITER = "---" + # NEL, LINE SEPARATOR, and PARAGRAPH SEPARATOR: line breaks in YAML 1.1 (Psych) but not in YAML 1.2. + UNICODE_BREAKS = Regexp.union([0x85, 0x2028, 0x2029].map { |codepoint| codepoint.chr(Encoding::UTF_8) }).freeze + MERGE_KEY = "<<" + # An SDK loading limit for the frontmatter block, not a protocol rule: the file itself may be 16 MiB. + MAX_BLOCK_BYTES = 64 * 1024 + MAX_DEPTH = 64 + + # Resolves plain scalars by the YAML 1.2 core schema, to compare with Psych's YAML 1.1 resolution. + module CoreSchema + extend self + + NULL = /\A(?:null|Null|NULL|~)?\z/.freeze + TRUE = /\A(?:true|True|TRUE)\z/.freeze + FALSE = /\A(?:false|False|FALSE)\z/.freeze + INT10 = /\A[-+]?[0-9]+\z/.freeze + INT8 = /\A0o[0-7]+\z/.freeze + INT16 = /\A0x[0-9a-fA-F]+\z/.freeze + FLOAT = /\A[-+]?(?:\.[0-9]+|[0-9]+(?:\.[0-9]*)?)(?:[eE][-+]?[0-9]+)?\z/.freeze + INF = /\A[-+]?\.(?:inf|Inf|INF)\z/.freeze + NAN = /\A\.(?:nan|NaN|NAN)\z/.freeze + + def resolve(text) + case text + when NULL then nil + when TRUE then true + when FALSE then false + when INT10 then Integer(text, 10) + when INT8 then text[2..-1].to_i(8) + when INT16 then text[2..-1].to_i(16) + when INF then text.start_with?("-") ? -Float::INFINITY : Float::INFINITY + when NAN then Float::NAN + # Ruby's Float() rejects a trailing dot, which the core schema allows. + when FLOAT then Float(text.sub(/\.(?=[eE]|\z)/, ".0")) + else text + end + end + + # Whether Psych's reading of a plain scalar differs in type or value from the core-schema reading. + # A scanner failure counts as a difference, because the other side would not fail the same way. + def divergent?(scanner, text) + psych = scanner.tokenize(text) + core = resolve(text) + return true unless psych.class.equal?(core.class) + return false if core.is_a?(Float) && core.nan? && psych.nan? + + psych != core + rescue StandardError + true + end + end + + # Builds the AST as `Psych.parse_stream` does and stops the parser at the first collection nested beyond + # `MAX_DEPTH`. The scan in libyaml costs the square of the flow nesting, so a block within the size limit + # could otherwise take seconds before `check_nodes!` ever saw its depth. + class BoundedTreeBuilder < Psych::TreeBuilder + def initialize + super + @depth = 0 + end + + def start_sequence(*) + descend + super + end + + def start_mapping(*) + descend + super + end + + def end_sequence + @depth -= 1 + super + end + + def end_mapping + @depth -= 1 + super + end + + private + + def descend + @depth += 1 + raise Error.new(:parse) if @depth > MAX_DEPTH + end + end + private_constant :BoundedTreeBuilder + + # Extracts and parses the frontmatter of a `SKILL.md` given as bytes; returns a Hash with String keys. + def load(bytes) + parse(extract(bytes)) + end + + # The text between the opening `---` line and the next `---` line. A leading UTF-8 BOM is skipped + # here only; digests always cover the raw bytes. Lines end with `\n` or `\r\n`. + def extract(bytes) + data = bytes.b + data = data.byteslice(BOM.bytesize..-1) if data.start_with?(BOM) + text = data.force_encoding(Encoding::UTF_8) + raise Error.new(:parse) unless text.valid_encoding? + + lines = text.each_line + first = begin + lines.next + rescue StopIteration + raise Error.new(:parse) + end + raise Error.new(:parse) unless delimiter_line?(first) && first.end_with?("\n") && !foreign_break?(first) + + block = +"" + closed = false + # `loop` ends when `next` raises StopIteration at the end of the text; `each` would restart from + # the first line instead of continuing after it. + loop do + line = lines.next + raise Error.new(:parse) if foreign_break?(line) + + if delimiter_line?(line) + closed = true + break + end + + block << line + raise Error.new(:parse) if block.bytesize > MAX_BLOCK_BYTES + end + raise Error.new(:parse) unless closed + + block + end + + # Parses an extracted block under the rules above. + def parse(block) + document = single_document(block) + check_nodes!(document.root) + + value = begin + Psych.safe_load(block, permitted_classes: [], permitted_symbols: [], aliases: false) + rescue Psych::Exception + raise Error.new(:parse) + end + raise Error.new(:parse) unless value.is_a?(Hash) + + begin + Json.normalize(value) + rescue ArgumentError + raise Error.new(:parse) + end + end + + # Whether two normalized frontmatter Hashes are equal, with Integer and Float kept distinct. + def equal?(expected, actual) + Json.equal?(expected, actual) + end + + private + + def delimiter_line?(line) + line.chomp == DELIMITER + end + + # A line break the delimiter scan above does not see but Psych does: a carriage return that is not part of + # a `\r\n` ending, or one of the YAML 1.1-only separators. The scan and Psych must never disagree, and neither + # must Psych and a YAML 1.2 reader. Only an exact `\r\n` suffix is taken off before looking (`chomp` would + # also strip a lone `\r` or the `\r` of `\r\r\n`). + def foreign_break?(line) + line.delete_suffix("\r\n").include?("\r") || line.match?(UNICODE_BREAKS) + end + + # `Psych.parse` and `safe_load` read only the first document, so a `...` or a second `---` inside + # the block would hide content from every check; the stream must hold exactly one implicit document. + def single_document(block) + stream = begin + parser = Psych::Parser.new(BoundedTreeBuilder.new) + parser.parse(block) + parser.handler.root + rescue Psych::Exception + raise Error.new(:parse) + end + documents = stream.children + raise Error.new(:parse) unless documents.size == 1 + + document = documents.first + version = document.version + unless (version.nil? || version.empty?) && document.tag_directives.empty? && + document.implicit && document.implicit_end && document.root.is_a?(Psych::Nodes::Mapping) + raise Error.new(:parse) + end + + document + end + + # Walks the AST iteratively, before `safe_load` ever builds Ruby objects from it. + def check_nodes!(root) + scanner = Psych::ScalarScanner.new(Psych::ClassLoader.new) + stack = [[root, 1]] + until stack.empty? + node, depth = stack.pop + raise Error.new(:parse) if depth > MAX_DEPTH + raise Error.new(:parse) if node.respond_to?(:tag) && node.tag + + case node + when Psych::Nodes::Scalar + raise Error.new(:parse) if node.plain && CoreSchema.divergent?(scanner, node.value) + when Psych::Nodes::Sequence + node.children.each { |child| stack.push([child, depth + 1]) } + when Psych::Nodes::Mapping + check_mapping!(node, depth, stack, scanner) + else + raise Error.new(:parse) + end + end + end + + def check_mapping!(node, depth, stack, scanner) + keys = {} + node.children.each_slice(2) do |key, value| + raise Error.new(:parse) unless key.is_a?(Psych::Nodes::Scalar) && key.tag.nil? + + text = key.value + if key.plain + raise Error.new(:parse) if CoreSchema.divergent?(scanner, text) + raise Error.new(:parse) unless CoreSchema.resolve(text).is_a?(String) + end + raise Error.new(:parse) if text == MERGE_KEY || keys.key?(text) + + keys[text] = true + stack.push([value, depth + 1]) + end + end + end + end +end diff --git a/lib/mcp/skill/resource_entry.rb b/lib/mcp/skill/resource_entry.rb new file mode 100644 index 00000000..9217df4e --- /dev/null +++ b/lib/mcp/skill/resource_entry.rb @@ -0,0 +1,109 @@ +# frozen_string_literal: true + +require "digest" + +module MCP + class Skill + # One file of a skill as its manifest lists it: `{ uri, digest, size }` (SEP-2640). The `digest` is + # `sha256:` followed by 64 lowercase hex characters over the file's raw bytes, `size` is the byte length. + # Only the shape is checked here; whether the URI lies under the skill's root is checked by `MCP::Skill`, + # which knows the root. + class ResourceEntry + DIGEST_PATTERN = /\Asha256:[0-9a-f]{64}\z/.freeze + + # The keys `from` accepts in a Hash, as Symbols or Strings. + HASH_KEYWORDS = { "uri" => :uri, "digest" => :digest, "size" => :size }.freeze + private_constant :HASH_KEYWORDS + + class << self + # Returns `value` when it is already a `ResourceEntry`, builds one from a Hash, and refuses anything else. + def from(value) + return value if value.is_a?(ResourceEntry) + + unless value.is_a?(Hash) + raise ArgumentError, "A skill resource entry must be an MCP::Skill::ResourceEntry or a Hash (got #{value.class})." + end + + keywords = {} + value.each do |key, member| + unless key.is_a?(Symbol) || key.is_a?(String) + raise ArgumentError, "A skill resource entry Hash key must be a Symbol or a String (got #{key.class})." + end + + keyword = HASH_KEYWORDS[key.to_s] + unless keyword + raise ArgumentError, "A skill resource entry Hash may only hold uri, digest, and size (got #{key.inspect})." + end + raise ArgumentError, "A skill resource entry Hash gives #{keyword} twice." if keywords.key?(keyword) + + keywords[keyword] = member + end + + new(**keywords) + end + + # Builds an entry from a server's wire Hash (String or Symbol keys), reading `uri`, `digest`, and `size` and + # ignoring any other member a server may add; `from` refuses unknown members for local definitions instead. + # + # @api private + def from_wire(value) + return value if value.is_a?(ResourceEntry) + raise ArgumentError, "A skill resource entry must be a Hash (got #{value.class})." unless value.is_a?(Hash) + + new(uri: wire_member(value, "uri"), digest: wire_member(value, "digest"), size: wire_member(value, "size")) + end + + # Builds the entry for a file from the exact bytes the resource will serve, so an author never formats + # a digest by hand: pass `File.binread` output for a binary file and UTF-8 text for a text file. + def from_content(uri:, content:) + unless content.is_a?(String) + raise ArgumentError, "The value of content must be a String (got #{content.class})." + end + + new(uri: uri, digest: "sha256:#{Digest::SHA256.hexdigest(content.b)}", size: content.bytesize) + end + + private + + def wire_member(hash, key) + value = hash[key] + value.nil? ? hash[key.to_sym] : value + end + end + + attr_reader :uri, :digest, :size + + def initialize(uri:, digest:, size:) + unless uri.is_a?(String) && Uri.structural?(uri) + raise ArgumentError, "The value of uri must be a skill:// URI with non-empty path segments." + end + + unless digest.is_a?(String) && digest.encoding.ascii_compatible? && DIGEST_PATTERN.match?(digest) + raise ArgumentError, "The value of digest must be sha256: followed by 64 lowercase hex characters." + end + + unless size.is_a?(Integer) && size >= 0 + raise ArgumentError, "The value of size must be a non-negative Integer (got #{size.class})." + end + + @uri = -uri + @digest = -digest + @size = size + freeze + end + + def to_h + { uri: uri, digest: digest, size: size } + end + + def ==(other) + other.is_a?(ResourceEntry) && to_h == other.to_h + end + alias_method :eql?, :== + + def hash + to_h.hash + end + end + end +end diff --git a/test/mcp/client_skills_test.rb b/test/mcp/client_skills_test.rb new file mode 100644 index 00000000..88d6af3e --- /dev/null +++ b/test/mcp/client_skills_test.rb @@ -0,0 +1,933 @@ +# frozen_string_literal: true + +require "test_helper" +require "digest" + +module MCP + class ClientSkillsTest < ActiveSupport::TestCase + SKILL_MD = "---\nname: pdf-processing\ndescription: Extract PDFs\n---\n\nBody.\n" + FORMS = "Forms\n" + LOGO = "\x89PNG\r\n\x1a\n\xFF".b + SKILL_URI = "skill://pdf-processing/SKILL.md" + FORMS_URI = "skill://pdf-processing/references/FORMS.md" + LOGO_URI = "skill://pdf-processing/logo.png" + + def digest(content) + "sha256:#{Digest::SHA256.hexdigest(content)}" + end + + def wire_entry(uri, content) + { "uri" => uri, "digest" => digest(content), "size" => content.bytesize } + end + + def pdf_entry(skill_md: SKILL_MD) + { + "uri" => SKILL_URI, + "frontmatter" => { "name" => "pdf-processing", "description" => "Extract PDFs" }, + "resources" => [wire_entry(SKILL_URI, skill_md), wire_entry(FORMS_URI, FORMS), wire_entry(LOGO_URI, LOGO)], + } + end + + # A transport with a canned catalog and file store. `connect` returns the wire Hash a legacy handshake + # returns and may block on a Queue; `send_request` may run a hook first, so a test can act mid-flight. + class FakeTransport + attr_accessor :connected, :capabilities, :skills, :files, :gate, :hook, :requests, :get_answer + + def initialize(skills, files) + @connected = false + @capabilities = { "resources" => {}, "extensions" => { Skill::EXTENSION_ID => {} } } + @skills = skills + @files = files + @gate = nil + @hook = nil + @requests = [] + @get_answer = nil + end + + def connect(client_info: nil, protocol_version: nil, capabilities: {}) + outcome = @gate&.pop + raise outcome if outcome.is_a?(Exception) + + @connected = true + { "protocolVersion" => "2025-11-25", "capabilities" => @capabilities, "serverInfo" => { "name" => "fake", "version" => "1" } } + end + + def connected? + @connected + end + + def send_request(request:) + @requests << request + hook = @hook + @hook = nil + hook&.call(request) + + case request[:method] + when "skills/list" then { "result" => { "skills" => @skills } } + when "skills/get" + skill = @get_answer || @skills.find { |candidate| candidate["uri"] == request[:params][:uri] } + skill ? { "result" => { "skill" => skill } } : { "error" => { "code" => -32602, "message" => "Skill not found" } } + when "resources/read" + uri = request[:params][:uri] + item = @files[uri] + { "result" => { "contents" => item ? [{ "uri" => uri, "mimeType" => "text/markdown" }.merge(item)] : [] } } + end + end + end + + def files + { SKILL_URI => { "text" => SKILL_MD.dup }, FORMS_URI => { "text" => FORMS.dup }, LOGO_URI => { "blob" => [LOGO].pack("m0") } } + end + + def connected_client(skills: [pdf_entry], files: self.files) + transport = FakeTransport.new(skills, files) + client = Client.new(transport: transport) + client.connect + [client, transport] + end + + def assert_refused(reason, &block) + error = assert_raises(Client::SkillReadRefusedError, &block) + assert_equal reason, error.reason + error + end + + def assert_verification_failure(reason, &block) + error = assert_raises(Client::SkillVerificationError, &block) + assert_equal reason, error.reason + error + end + + test "skills calls are refused until a handshake of this connection advertised the extension and resources" do + transport = FakeTransport.new([pdf_entry], files) + client = Client.new(transport: transport) + assert_refused(:not_advertised) { client.list_skills } + assert_refused(:not_advertised) { client.get_skill(uri: SKILL_URI) } + assert_refused(:not_advertised) { client.read_skill_resource(uri: SKILL_URI) } + assert_nil client.skills_capability + assert_empty transport.requests + + transport.capabilities = { "resources" => {} } + client.connect + assert_refused(:not_advertised) { client.list_skills } + + transport.connected = false + transport.capabilities = { "extensions" => { Skill::EXTENSION_ID => {} } } + client.connect + assert_refused(:not_advertised) { client.list_skills } + + transport.connected = false + transport.capabilities = { "resources" => {}, "extensions" => { Skill::EXTENSION_ID => "yes" } } + client.connect + assert_refused(:not_advertised) { client.list_skills } + + transport.connected = false + transport.capabilities = { "resources" => {}, "extensions" => { Skill::EXTENSION_ID => {} } } + client.connect + assert_equal({}, client.skills_capability) + assert_predicate client.skills_capability, :frozen? + assert_equal 1, client.list_skills.skills.size + end + + test "a transport that reports itself disconnected refuses skills calls until the next connect" do + client, transport = connected_client + transport.connected = false + + assert_refused(:not_advertised) { client.list_skills } + assert_nil client.skills_capability + client.connect + assert_equal 1, client.list_skills.skills.size + end + + test "a transport without connect never becomes ready" do + transport = mock + transport.stubs(:respond_to?).with(:connect).returns(false) + transport.stubs(:respond_to?).with(:connected?).returns(false) + client = Client.new(transport: transport) + + assert_nil client.connect + assert_refused(:not_advertised) { client.list_skills } + end + + test "list_skills returns a frozen page of entries and remembers them" do + git = { "uri" => "skill://git/SKILL.md", "frontmatter" => { "name" => "git", "description" => "Git" }, "resources" => "dynamic" } + client, transport = connected_client(skills: [pdf_entry, git]) + page = client.list_skills + + assert_kind_of Client::ListSkillsResult, page + assert_predicate page, :frozen? + assert_predicate page.skills, :frozen? + assert_equal [SKILL_URI, "skill://git/SKILL.md"], page.skills.map(&:uri) + assert_kind_of Skill, page.skills.first + assert_nil page.next_cursor + assert_equal "skills/list", transport.requests.last[:method] + refute transport.requests.last.key?(:params) + + client.list_skills(cursor: "1", meta: { "x" => 1 }) + assert_equal({ cursor: "1", _meta: { "x" => 1 } }, transport.requests.last[:params]) + end + + test "list_skills and get_skill validate the result and remember nothing from a malformed one" do + client, transport = connected_client + + transport.skills = [pdf_entry.merge("resources" => [wire_entry(FORMS_URI, FORMS)])] + assert_raises(Client::SkillEntryError) { client.list_skills } + transport.skills = [pdf_entry, pdf_entry] + error = assert_raises(Client::SkillEntryError) { client.list_skills } + assert_match(/listed twice/, error.message) + transport.skills = "nope" + assert_raises(Client::SkillEntryError) { client.list_skills } + assert_refused(:unknown_skill) { client.read_skill_resource(uri: SKILL_URI) } + + transport.skills = [pdf_entry] + got = client.get_skill(uri: SKILL_URI) + assert_kind_of Client::GetSkillResult, got + assert_predicate got, :frozen? + assert_equal SKILL_URI, got.skill.uri + + transport.get_answer = { "uri" => "skill://other/SKILL.md", "frontmatter" => { "name" => "other", "description" => "d" }, "resources" => "dynamic" } + error = assert_raises(Client::SkillEntryError) { client.get_skill(uri: SKILL_URI) } + assert_includes error.message, 'answered for "skill://other/SKILL.md" instead of "skill://pdf-processing/SKILL.md"' + transport.get_answer = nil + + assert_raises(Client::ServerError) { client.get_skill(uri: "skill://nope/SKILL.md") } + assert_raises(ArgumentError) { client.get_skill(uri: 42) } + end + + test "remote entries beyond the limits are accepted and expose their size" do + resources = Array.new(600) { |index| { "uri" => "skill://big/f#{index}", "digest" => digest("x"), "size" => 100_000 } } + [wire_entry("skill://big/SKILL.md", "x")] + client, _transport = connected_client(skills: [{ "uri" => "skill://big/SKILL.md", "frontmatter" => { "name" => "big", "description" => "d" }, "resources" => resources }]) + skill = client.list_skills.skills.first + + assert_equal 601, skill.resources.size + assert_equal 60_000_001, skill.total_size + end + + test "read_skill_resource verifies the SKILL.md first, then supporting files, and returns independent frozen values" do + client, transport = connected_client + assert_refused(:unknown_skill) { client.read_skill_resource(uri: FORMS_URI) } + page = client.list_skills + assert_refused(:skill_md_unverified) { client.read_skill_resource(uri: FORMS_URI) } + assert_equal 1, transport.requests.size, "refusals send nothing" + + result = client.read_skill_resource(uri: SKILL_URI) + assert_kind_of Client::SkillReadResult, result + assert_predicate result, :frozen? + assert_equal SKILL_URI, result.uri + assert_equal SKILL_MD.b, result.bytes + assert_equal Encoding::BINARY, result.bytes.encoding + assert_predicate result.bytes, :frozen? + assert_equal "text/markdown", result.mime_type + assert_same page.skills.first, result.skill + assert result.verified + assert_predicate result.contents, :frozen? + assert_predicate result.contents.first, :frozen? + assert_equal [{ "uri" => SKILL_URI, "mimeType" => "text/markdown", "text" => SKILL_MD }], result.contents + + transport.files[SKILL_URI]["text"] << "mutated after the read" + assert_equal SKILL_MD, result.contents.first["text"] + + assert_equal FORMS.b, client.read_skill_resource(uri: FORMS_URI).bytes + logo = client.read_skill_resource(uri: LOGO_URI) + assert_equal LOGO, logo.bytes + assert_equal "text/markdown", logo.mime_type, "mime_type is whatever the server said" + assert_refused(:not_in_manifest) { client.read_skill_resource(uri: "skill://pdf-processing/other.md") } + assert_refused(:unknown_skill) { client.read_skill_resource(uri: "skill://nope/x.md") } + assert_raises(ArgumentError) { client.read_skill_resource(uri: SKILL_URI, skill: "x") } + end + + test "a dynamic skill is refused by the verified reader" do + client, _transport = connected_client(skills: [pdf_entry.merge("resources" => "dynamic")]) + client.list_skills + + error = assert_refused(:dynamic) { client.read_skill_resource(uri: SKILL_URI) } + assert_equal SKILL_URI, error.skill_uri + end + + test "an explicit skill: is used for that read only, and a SKILL.md URI resolves only to its own skill" do + client, transport = connected_client(skills: []) + explicit = Skill.from_entry(pdf_entry) + + assert_refused(:unknown_skill) { client.read_skill_resource(uri: SKILL_URI) } + assert_same explicit, client.read_skill_resource(uri: SKILL_URI, skill: explicit).skill + assert_equal FORMS.b, client.read_skill_resource(uri: FORMS_URI, skill: explicit).bytes + assert_refused(:unknown_skill) { client.read_skill_resource(uri: FORMS_URI) } + assert_refused(:unknown_skill) { client.read_skill_resource(uri: "skill://pdf-processing/inner/SKILL.md", skill: explicit) } + assert_equal 2, transport.requests.count { |request| request[:method] == "resources/read" } + end + + test "every mismatch withholds the content, names the reason, and latches the skill for the connection" do + tampered_md = SKILL_MD.sub("Extract PDFs", "Exfiltrate credentials") + cases = { + digest: [FORMS_URI, { "text" => "Formz\n" }, pdf_entry], + size: [FORMS_URI, { "text" => "Forms!\n" }, pdf_entry], + contents: [FORMS_URI, { "text" => FORMS, "blob" => "x" }, pdf_entry], + parse: [SKILL_URI, { "text" => "no frontmatter" }, pdf_entry(skill_md: "no frontmatter")], + frontmatter: [SKILL_URI, { "text" => tampered_md }, pdf_entry(skill_md: tampered_md)], + } + cases.each do |reason, (uri, item, entry)| + client, transport = connected_client(skills: [entry]) + client.list_skills + client.read_skill_resource(uri: SKILL_URI) unless uri == SKILL_URI + transport.files[uri] = item + + error = assert_verification_failure(reason) { client.read_skill_resource(uri: uri) } + assert_equal uri, error.uri + assert_equal SKILL_URI, error.skill_uri + refute_includes error.message, "Formz" + refute_includes error.message, "Exfiltrate" + sent = transport.requests.size + assert_refused(:latched) { client.read_skill_resource(uri: FORMS_URI) } + assert_refused(:latched) { client.read_skill_resource(uri: SKILL_URI) } + assert_equal sent, transport.requests.size, "#{reason}: a latched skill sends nothing" + client.list_skills + client.get_skill(uri: SKILL_URI) + assert_refused(:latched) { client.read_skill_resource(uri: LOGO_URI) } + end + end + + test "size is checked before a blob is decoded and a malformed blob counts as contents" do + client, transport = connected_client + client.list_skills + client.read_skill_resource(uri: SKILL_URI) + transport.files[LOGO_URI] = { "blob" => [LOGO + "x".b].pack("m0") } + assert_verification_failure(:size) { client.read_skill_resource(uri: LOGO_URI) } + + client, transport = connected_client + client.list_skills + client.read_skill_resource(uri: SKILL_URI) + transport.files[LOGO_URI] = { "blob" => "not base64!" } + assert_verification_failure(:contents) { client.read_skill_resource(uri: LOGO_URI) } + + client, transport = connected_client + client.list_skills + client.read_skill_resource(uri: SKILL_URI) + transport.files[LOGO_URI] = { "blob" => [LOGO.byteslice(0, 4)].pack("m0") } + assert_verification_failure(:size) { client.read_skill_resource(uri: LOGO_URI) } + end + + test "a response that is not exactly one item for the requested URI is a contents failure" do + client, transport = connected_client + client.list_skills + transport.files.delete(SKILL_URI) + assert_verification_failure(:contents) { client.read_skill_resource(uri: SKILL_URI) } + + client, transport = connected_client + client.list_skills + transport.define_singleton_method(:send_request) do |request:| + if request[:method] == "resources/read" + { "result" => { "contents" => [{ "uri" => "skill://other/SKILL.md", "text" => SKILL_MD }] } } + else + super(request: request) + end + end + assert_verification_failure(:contents) { client.read_skill_resource(uri: SKILL_URI) } + + client, transport = connected_client + client.list_skills + transport.define_singleton_method(:send_request) do |request:| + if request[:method] == "resources/read" + { "result" => { "contents" => [{ "uri" => SKILL_URI, "text" => SKILL_MD }, { "uri" => SKILL_URI, "text" => SKILL_MD }] } } + else + super(request: request) + end + end + assert_verification_failure(:contents) { client.read_skill_resource(uri: SKILL_URI) } + end + + test "latches survive a connect that performs no new handshake and are cleared by a real reconnect" do + client, transport = connected_client + client.list_skills + transport.files[SKILL_URI] = { "text" => SKILL_MD.sub("Body.", "Evil!") } + assert_verification_failure(:digest) { client.read_skill_resource(uri: SKILL_URI) } + + client.connect + assert_refused(:latched) { client.read_skill_resource(uri: SKILL_URI) } + + transport.connected = false + transport.files[SKILL_URI] = { "text" => SKILL_MD } + client.connect + assert_refused(:unknown_skill) { client.read_skill_resource(uri: SKILL_URI) } + client.list_skills + assert client.read_skill_resource(uri: SKILL_URI).verified + end + + test "a latched ancestor refuses an unknown descendant before unknown_skill, and an enclosing skill is reached only through skill:" do + outer_md = "---\nname: outer\ndescription: Outer\n---\n" + inner_md = "---\nname: inner\ndescription: Inner\n---\n" + outer = { + "uri" => "skill://acme/outer/SKILL.md", + "frontmatter" => { "name" => "outer", "description" => "Outer" }, + "resources" => [ + wire_entry("skill://acme/outer/SKILL.md", outer_md), + wire_entry("skill://acme/outer/inner/notes.txt", "notes\n"), + wire_entry("skill://acme/outer/inner/SKILL.md", inner_md), + ], + } + inner = { + "uri" => "skill://acme/outer/inner/SKILL.md", + "frontmatter" => { "name" => "inner", "description" => "Inner" }, + "resources" => [wire_entry("skill://acme/outer/inner/SKILL.md", inner_md)], + } + store = { + "skill://acme/outer/SKILL.md" => { "text" => outer_md.sub("Outer", "Changed") }, + "skill://acme/outer/inner/notes.txt" => { "text" => "notes\n" }, + "skill://acme/outer/inner/SKILL.md" => { "text" => inner_md }, + } + honest_outer = outer.merge("resources" => [wire_entry("skill://acme/outer/SKILL.md", outer_md.sub("Outer", "Changed")), *outer["resources"][1..-1]]) + + # Only the outer skill is remembered: the inner SKILL.md never resolves as the outer skill's file, and + # a latched ancestor refuses its descendants before anything else is considered. + client, _transport = connected_client(skills: [honest_outer], files: store) + client.list_skills + assert_refused(:unknown_skill) { client.read_skill_resource(uri: "skill://acme/outer/inner/SKILL.md") } + assert_verification_failure(:frontmatter) { client.read_skill_resource(uri: "skill://acme/outer/SKILL.md") } + assert_refused(:latched) { client.read_skill_resource(uri: "skill://acme/outer/inner/notes.txt") } + assert_refused(:latched) { client.read_skill_resource(uri: "skill://acme/outer/inner/SKILL.md") } + + # Both remembered, in either order: a file the nested skill leaves out is never verified against the enclosing + # manifest automatically, whatever verified, because the nested entry is the most specific one; a host acting + # on the enclosing skill names it. The nested SKILL.md resolves only to the nested entry. + honest_store = store.merge("skill://acme/outer/SKILL.md" => { "text" => outer_md }) + [[outer, inner], [inner, outer]].each do |order| + client, transport = connected_client(skills: order, files: honest_store) + outer_entry = client.list_skills.skills.find { |skill| skill.uri == outer["uri"] } + assert_refused(:ambiguous) { client.read_skill_resource(uri: "skill://acme/outer/inner/notes.txt") } + assert_refused(:skill_md_unverified) { client.read_skill_resource(uri: "skill://acme/outer/inner/notes.txt", skill: outer_entry) } + assert client.read_skill_resource(uri: "skill://acme/outer/SKILL.md").verified + error = assert_refused(:ambiguous) { client.read_skill_resource(uri: "skill://acme/outer/inner/notes.txt") } + assert_equal inner["uri"], error.skill_uri + result = client.read_skill_resource(uri: "skill://acme/outer/inner/notes.txt", skill: outer_entry) + assert_equal "notes\n".b, result.bytes + assert_equal outer["uri"], result.skill.uri + assert client.read_skill_resource(uri: "skill://acme/outer/inner/SKILL.md").verified + assert_equal 3, transport.requests.count { |request| request[:method] == "resources/read" } + end + + # A dynamic nested skill is the most specific entry too: the automatic path answers for it and never falls + # back to the enclosing manifest that lists the file. + dynamic_inner = inner.merge("resources" => "dynamic") + client, transport = connected_client(skills: [outer, dynamic_inner], files: honest_store) + outer_entry = client.list_skills.skills.find { |skill| skill.uri == outer["uri"] } + assert client.read_skill_resource(uri: "skill://acme/outer/SKILL.md").verified + assert_refused(:dynamic) { client.read_skill_resource(uri: "skill://acme/outer/inner/notes.txt") } + assert_equal 1, transport.requests.count { |request| request[:method] == "resources/read" } + assert_equal "notes\n".b, client.read_skill_resource(uri: "skill://acme/outer/inner/notes.txt", skill: outer_entry).bytes + + # A file both skills list is never picked between automatically, whatever verified: only the host knows which + # skill it is acting on, so it names that entry with skill:, and the file is verified against that manifest. + inner_listing_notes = inner.merge("resources" => [*inner["resources"], wire_entry("skill://acme/outer/inner/notes.txt", "notes\n")]) + client, _transport = connected_client(skills: [outer, inner_listing_notes], files: honest_store) + entries = client.list_skills.skills.to_h { |skill| [skill.uri, skill] } + assert client.read_skill_resource(uri: "skill://acme/outer/SKILL.md").verified + assert client.read_skill_resource(uri: "skill://acme/outer/inner/SKILL.md").verified + error = assert_refused(:ambiguous) { client.read_skill_resource(uri: "skill://acme/outer/inner/notes.txt") } + assert_equal "skill://acme/outer/inner/SKILL.md", error.skill_uri + assert_equal outer["uri"], client.read_skill_resource(uri: "skill://acme/outer/inner/notes.txt", skill: entries[outer["uri"]]).skill.uri + assert_equal inner["uri"], client.read_skill_resource(uri: "skill://acme/outer/inner/notes.txt", skill: entries[inner["uri"]]).skill.uri + + # A nested skill that fails verification is latched, which blocks the shared file even through the verified + # enclosing entry named explicitly. + tampered_inner = honest_store.merge("skill://acme/outer/inner/SKILL.md" => { "text" => inner_md.sub("Inner", "Changed") }) + client, _transport = connected_client(skills: [outer, inner_listing_notes], files: tampered_inner) + outer_entry = client.list_skills.skills.find { |skill| skill.uri == outer["uri"] } + assert client.read_skill_resource(uri: "skill://acme/outer/SKILL.md").verified + assert_raises(Client::SkillVerificationError) { client.read_skill_resource(uri: "skill://acme/outer/inner/SKILL.md") } + assert_refused(:latched) { client.read_skill_resource(uri: "skill://acme/outer/inner/notes.txt", skill: outer_entry) } + end + + test "a refreshed catalog does not let a verified enclosing skill stand in for a nested skill that changed" do + outer_md = "---\nname: outer\ndescription: Outer\n---\n" + inner_md = "---\nname: inner\ndescription: Inner\n---\n" + inner_md_v2 = inner_md.sub("Inner", "Innex") + notes_uri = "skill://acme/outer/inner/notes.txt" + catalog = lambda do |manifest, notes| + description = manifest == inner_md ? "Inner" : "Innex" + [ + { + "uri" => "skill://acme/outer/SKILL.md", + "frontmatter" => { "name" => "outer", "description" => "Outer" }, + "resources" => [wire_entry("skill://acme/outer/SKILL.md", outer_md), wire_entry("skill://acme/outer/inner/SKILL.md", manifest), wire_entry(notes_uri, notes)], + }, + { + "uri" => "skill://acme/outer/inner/SKILL.md", + "frontmatter" => { "name" => "inner", "description" => description }, + "resources" => [wire_entry("skill://acme/outer/inner/SKILL.md", manifest), wire_entry(notes_uri, notes)], + }, + ] + end + files = lambda do |manifest, notes| + { "skill://acme/outer/SKILL.md" => { "text" => outer_md }, "skill://acme/outer/inner/SKILL.md" => { "text" => manifest }, notes_uri => { "text" => notes } } + end + client, transport = connected_client(skills: catalog.call(inner_md, "v1\n"), files: files.call(inner_md, "v1\n")) + held = client.list_skills.skills.find { |skill| skill.uri == "skill://acme/outer/inner/SKILL.md" } + assert client.read_skill_resource(uri: "skill://acme/outer/SKILL.md").verified + assert client.read_skill_resource(uri: "skill://acme/outer/inner/SKILL.md").verified + assert_equal "v1\n".b, client.read_skill_resource(uri: notes_uri, skill: held).bytes + + # The server changes the nested skill in both manifests at once, the host refreshes the catalog and re-reads + # only the enclosing SKILL.md: the changed file is never served as verified through the enclosing entry, and + # the held entry fails closed until its own SKILL.md is read again, which then fails and latches the skill. + transport.skills = catalog.call(inner_md_v2, "v2\n") + transport.files = files.call(inner_md_v2, "v2\n") + client.list_skills + assert client.read_skill_resource(uri: "skill://acme/outer/SKILL.md").verified + assert_refused(:ambiguous) { client.read_skill_resource(uri: notes_uri) } + assert_refused(:skill_md_unverified) { client.read_skill_resource(uri: notes_uri, skill: held) } + assert_verification_failure(:digest) { client.read_skill_resource(uri: "skill://acme/outer/inner/SKILL.md", skill: held) } + assert_refused(:latched) { client.read_skill_resource(uri: notes_uri, skill: held) } + end + + test "two remembered skills listing one file are refused as ambiguous whatever their digests say" do + outer_md = "---\nname: outer\ndescription: Outer\n---\n" + inner_md = "---\nname: inner\ndescription: Inner\n---\n" + notes_uri = "skill://acme/outer/inner/notes.txt" + outer = { + "uri" => "skill://acme/outer/SKILL.md", + "frontmatter" => { "name" => "outer", "description" => "Outer" }, + "resources" => [wire_entry("skill://acme/outer/SKILL.md", outer_md), wire_entry(notes_uri, "notes\n")], + } + inner = { + "uri" => "skill://acme/outer/inner/SKILL.md", + "frontmatter" => { "name" => "inner", "description" => "Inner" }, + "resources" => [wire_entry("skill://acme/outer/inner/SKILL.md", inner_md), wire_entry(notes_uri, "other\n")], + } + store = { + "skill://acme/outer/SKILL.md" => { "text" => outer_md }, + "skill://acme/outer/inner/SKILL.md" => { "text" => inner_md }, + notes_uri => { "text" => "notes\n" }, + } + client, transport = connected_client(skills: [outer, inner], files: store) + outer_entry = client.list_skills.skills.find { |skill| skill.uri == outer["uri"] } + client.read_skill_resource(uri: outer["uri"]) + + error = assert_refused(:ambiguous) { client.read_skill_resource(uri: notes_uri) } + assert_equal notes_uri, error.uri + assert_equal 1, transport.requests.count { |request| request[:method] == "resources/read" } + # The host names the skill it is acting on, and the file is verified against that manifest alone. + assert_equal "notes\n".b, client.read_skill_resource(uri: notes_uri, skill: outer_entry).bytes + + # Agreeing digests change nothing: the choice is the host's either way. + inner_agreeing = inner.merge("resources" => [wire_entry("skill://acme/outer/inner/SKILL.md", inner_md), wire_entry(notes_uri, "notes\n")]) + client, _transport = connected_client(skills: [outer, inner_agreeing], files: store) + client.list_skills + client.read_skill_resource(uri: outer["uri"]) + assert_refused(:ambiguous) { client.read_skill_resource(uri: notes_uri) } + end + + test "with three nested levels the most specific skill answers and every enclosing lister is named" do + deep = "skill://a/b/c/deep.txt" + entry_for = lambda do |path, name| + manifest = "---\nname: #{name}\ndescription: #{name.upcase}\n---\n" + { + "uri" => "skill://#{path}/SKILL.md", + "frontmatter" => { "name" => name, "description" => name.upcase }, + "resources" => [wire_entry("skill://#{path}/SKILL.md", manifest), wire_entry(deep, "d\n")], + } + end + skills = [entry_for.call("a", "a"), entry_for.call("a/b", "b"), entry_for.call("a/b/c", "c")] + store = skills.to_h { |skill| [skill["uri"], { "text" => "---\nname: #{skill["frontmatter"]["name"]}\ndescription: #{skill["frontmatter"]["description"]}\n---\n" }] } + store[deep] = { "text" => "d\n" } + client, transport = connected_client(skills: skills, files: store) + entries = client.list_skills.skills.to_h { |skill| [skill.uri, skill] } + + error = assert_refused(:ambiguous) { client.read_skill_resource(uri: deep) } + assert_equal "skill://a/b/c/SKILL.md", error.skill_uri + assert_includes error.message, '"skill://a/b/SKILL.md" and "skill://a/SKILL.md"' + assert_empty transport.requests.select { |request| request[:method] == "resources/read" } + + client.read_skill_resource(uri: "skill://a/b/c/SKILL.md") + assert_equal "d\n".b, client.read_skill_resource(uri: deep, skill: entries["skill://a/b/c/SKILL.md"]).bytes + end + + test "a URI longer or deeper than the client resolves is refused at once, before the ancestor roots are computed" do + client, transport = connected_client + client.list_skills + deep = "skill://#{Array.new(20_000, "a").join("/")}/x" + long = "skill://pdf-processing/#{"x" * (Skill::Uri::MAX_RESOLVED_BYTES + 1)}" + + started = Process.clock_gettime(Process::CLOCK_MONOTONIC) + [deep, long].each do |uri| + error = assert_refused(:unknown_skill) { client.read_skill_resource(uri: uri) } + assert_operator error.message.bytesize, :<, 200, "the message leaves the URI out" + end + assert_operator Process.clock_gettime(Process::CLOCK_MONOTONIC) - started, :<, 1 + assert_empty transport.requests.select { |request| request[:method] == "resources/read" } + + # The advertisement check keeps its precedence, and a URI at the limits takes the ordinary path. + idle = Client.new(transport: FakeTransport.new([pdf_entry], files)) + assert_refused(:not_advertised) { idle.read_skill_resource(uri: deep) } + at_limit = "skill://#{Array.new(Skill::Uri::MAX_RESOLVED_SEGMENTS, "a").join("/")}" + assert_refused(:unknown_skill) { client.read_skill_resource(uri: at_limit) } + assert_raises(ArgumentError) { client.read_skill_resource(uri: "skill://pdf-processing/\xFF") } + assert_raises(ArgumentError) { client.get_skill(uri: "skill://pdf-processing/\xFF/SKILL.md") } + end + + test "a URI in an encoding that is not ASCII-compatible is refused as an argument and as an entry" do + wide = SKILL_URI.encode("UTF-16LE") + client, transport = connected_client(skills: [pdf_entry.merge("uri" => wide)]) + + assert_raises(Client::SkillEntryError) { client.list_skills } + assert_raises(ArgumentError) { client.read_skill_resource(uri: wide) } + assert_raises(ArgumentError) { client.get_skill(uri: wide) } + assert_equal ["skills/list"], transport.requests.map { |request| request[:method] } + end + + test "server-chosen URIs appear escaped in error messages" do + # ESC and LF, then RIGHT-TO-LEFT OVERRIDE, ZERO WIDTH SPACE, and NEL, which `inspect` would leave as they are + # (NEL on Ruby 2.7 only). + name = "a\e[31m\nFORGED LOG LINE#{[0x202E, 0x200B, 0x85].pack("U*")}" + uri = "skill://#{name}/SKILL.md" + entry = { "uri" => uri, "frontmatter" => { "name" => name, "description" => "d" }, "resources" => [wire_entry(uri, "listed")] } + client, transport = connected_client(skills: [entry], files: { uri => { "text" => "tampered" } }) + client.list_skills + transport.get_answer = entry + + errors = [ + assert_refused(:not_in_manifest) { client.read_skill_resource(uri: "skill://#{name}/other.md") }, + assert_refused(:unknown_skill) { client.read_skill_resource(uri: "skill://#{name}/nested/SKILL.md") }, + assert_raises(Client::SkillEntryError) { client.get_skill(uri: "skill://#{name}/nested/SKILL.md") }, + assert_raises(Client::SkillVerificationError) { client.read_skill_resource(uri: uri) }, + assert_refused(:latched) { client.read_skill_resource(uri: uri) }, + ] + errors.each do |error| + assert_predicate error.message, :ascii_only?, error.class.name + refute_match(/[\n\e]/, error.message) + end + assert_equal uri, errors[3].uri + end + + test "the segment boundary keeps skill://ab apart from skill://a" do + a_md = "---\nname: a\ndescription: A\n---\n" + a = { "uri" => "skill://a/SKILL.md", "frontmatter" => { "name" => "a", "description" => "A" }, "resources" => [wire_entry("skill://a/SKILL.md", a_md)] } + client, _transport = connected_client(skills: [a], files: { "skill://a/SKILL.md" => { "text" => a_md } }) + client.list_skills + + assert_refused(:unknown_skill) { client.read_skill_resource(uri: "skill://ab/x") } + end + + test "a replaced entry does not inherit verified status, and an in-flight automatic read cannot restore it" do + client, transport = connected_client + first = client.list_skills.skills.first + client.read_skill_resource(uri: SKILL_URI) + assert_equal FORMS.b, client.read_skill_resource(uri: FORMS_URI).bytes + + second = client.list_skills.skills.first + refute_same first, second + assert_refused(:skill_md_unverified) { client.read_skill_resource(uri: FORMS_URI) } + + # The catalog is replaced while the SKILL.md read is in flight: the read completes against the old object. + transport.hook = ->(_request) { client.list_skills } + result = client.read_skill_resource(uri: SKILL_URI) + assert_same second, result.skill + assert_refused(:skill_md_unverified) { client.read_skill_resource(uri: FORMS_URI) } + + # An explicit read verifies the exact object the caller supplied, even after replacement. + assert client.read_skill_resource(uri: SKILL_URI, skill: second).verified + assert_equal FORMS.b, client.read_skill_resource(uri: FORMS_URI, skill: second).bytes + + # An explicit read spanning a replacement still marks the supplied object; the new index object stays unverified. + third = client.list_skills.skills.first + transport.hook = ->(_request) { client.list_skills } + assert_same third, client.read_skill_resource(uri: SKILL_URI, skill: third).skill + assert_equal FORMS.b, client.read_skill_resource(uri: FORMS_URI, skill: third).bytes + assert_refused(:skill_md_unverified) { client.read_skill_resource(uri: FORMS_URI) } + end + + test "a connect during an in-flight call discards its result as connection_changed without latching" do + client, transport = connected_client + transport.hook = ->(_request) { + transport.connected = false + client.connect } + assert_refused(:connection_changed) { client.list_skills } + + client.list_skills + client.read_skill_resource(uri: SKILL_URI) + transport.files[FORMS_URI] = { "text" => "Formz\n" } + transport.hook = ->(_request) { + transport.connected = false + client.connect } + assert_refused(:connection_changed) { client.read_skill_resource(uri: FORMS_URI) } + client.list_skills + transport.files[FORMS_URI] = { "text" => FORMS } + assert client.read_skill_resource(uri: SKILL_URI).verified, "nothing was latched by the stale failure" + + transport.hook = ->(_request) { + transport.connected = false + client.connect } + transport.skills = "garbage" + assert_refused(:connection_changed) { client.list_skills } + end + + test "overlapping handshakes: a stale completion or failure changes nothing" do + transport = FakeTransport.new([pdf_entry], files) + client = Client.new(transport: transport) + + first_gate = Queue.new + transport.gate = first_gate + blocked = Thread.new { client.connect } + blocked.report_on_exception = false + sleep(0.01) until blocked.status == "sleep" + transport.gate = nil + client.connect + assert_equal 1, client.list_skills.skills.size + + first_gate << RuntimeError.new("handshake A failed late") + error = assert_raises(RuntimeError) { blocked.value } + assert_equal "handshake A failed late", error.message + assert_equal 1, client.list_skills.skills.size, "the failure of the older attempt did not clear readiness" + + transport.connected = false + second_gate = Queue.new + transport.gate = second_gate + blocked = Thread.new { client.connect } + blocked.report_on_exception = false + sleep(0.01) until blocked.status == "sleep" + transport.gate = nil + client.connect + transport.capabilities = { "resources" => {} } + second_gate << :go + blocked.join(5) + assert_equal 1, client.list_skills.skills.size, "the older completion did not replace the recorded capabilities" + end + + test "readiness comes from the handshake result on the modern lifecycle too" do + transport = Class.new do + attr_reader :connected + + def initialize + @connected = false + end + + def connect(client_info: nil, protocol_version: nil, capabilities: {}, mode: :legacy) + @connected = true + { "supportedVersions" => ["2026-07-28"], "capabilities" => { "resources" => {}, "extensions" => { MCP::Skill::EXTENSION_ID => {} } } } + end + + def connected? + @connected + end + + def send_request(request:) + { "result" => { "skills" => [] } } + end + end.new + client = Client.new(transport: transport) + client.connect(mode: :modern) + + assert_equal({}, client.skills_capability) + assert_equal [], client.list_skills.skills + end + + test "a failed handshake leaves the client refusing skills calls" do + transport = FakeTransport.new([pdf_entry], files) + client = Client.new(transport: transport) + transport.gate = Queue.new + transport.gate << RuntimeError.new("boom") + assert_raises(RuntimeError) { client.connect } + + assert_refused(:not_advertised) { client.list_skills } + assert_nil client.skills_capability + end + + test "a malformed read response is a contents failure that latches, whatever its shape" do + [{ "result" => false }, { "result" => [] }, { "result" => { "contents" => "text" } }, { "result" => { "contents" => nil } }].each do |response| + client, transport = connected_client + client.list_skills + transport.define_singleton_method(:send_request) do |request:| + request[:method] == "resources/read" ? response : super(request: request) + end + + assert_verification_failure(:contents) { client.read_skill_resource(uri: SKILL_URI) } + assert_refused(:latched) { client.read_skill_resource(uri: SKILL_URI) } + end + end + + test "list_skills and get_skill refuse a response that is not a Hash or whose result is not a Hash" do + [nil, "nope", { "result" => "nope" }].each do |response| + client, transport = connected_client + transport.define_singleton_method(:send_request) { |request:| response } + + assert_raises(Client::SkillEntryError, response.inspect) { client.list_skills } + assert_raises(Client::SkillEntryError, response.inspect) { client.get_skill(uri: SKILL_URI) } + end + end + + test "values nested deeper than JSON.parse reads by default are refused rather than exhausting the stack" do + # Only a transport that parses without a nesting limit can deliver these. + deep = 100_000.times.inject("leaf") { |value, _| [value] } + + entry = pdf_entry.merge("frontmatter" => { "name" => "pdf-processing", "description" => "Extract PDFs", "metadata" => deep }) + client, _transport = connected_client(skills: [entry]) + error = assert_raises(Client::SkillEntryError) { client.list_skills } + assert_includes error.message, "must not nest deeper than 100 levels" + assert_raises(Client::SkillEntryError) { client.get_skill(uri: SKILL_URI) } + + client, _transport = connected_client(files: files.merge(SKILL_URI => { "text" => SKILL_MD.dup, "_meta" => { "deep" => deep } })) + client.list_skills + assert_verification_failure(:contents) { client.read_skill_resource(uri: SKILL_URI) } + assert_refused(:latched) { client.read_skill_resource(uri: SKILL_URI) } + + # The handshake itself succeeds, as it does for a client that never uses Skills. + transport = FakeTransport.new([pdf_entry], files) + transport.capabilities = transport.capabilities.merge("experimental" => { "deep" => deep }) + client = Client.new(transport: transport) + assert_equal "fake", client.connect.dig("serverInfo", "name") + assert_nil client.skills_capability + assert_refused(:not_advertised) { client.list_skills } + end + + test "an explicit skill cannot claim a URI outside its root, and a URI that is not structurally valid is unknown" do + client, transport = connected_client(skills: []) + explicit = Skill.from_entry(pdf_entry) + + assert_refused(:not_in_manifest) { client.read_skill_resource(uri: "skill://other/file.md", skill: explicit) } + assert_refused(:unknown_skill) { client.read_skill_resource(uri: "skill://pdf-processing/../x") } + assert_refused(:unknown_skill) { client.read_skill_resource(uri: "skill://pdf-processing/references/FORMS.md/") } + assert_empty transport.requests.select { |request| request[:method] == "resources/read" } + end + + test "the result's uri is an independent frozen copy of the argument" do + client, _transport = connected_client + client.list_skills + argument = SKILL_URI.dup + result = client.read_skill_resource(uri: argument) + argument << "/tampered" + + assert_equal SKILL_URI, result.uri + assert_predicate result.uri, :frozen? + end + + test "a stale get_skill completion is refused as connection_changed" do + client, transport = connected_client + transport.hook = ->(_request) { + transport.connected = false + client.connect } + + assert_refused(:connection_changed) { client.get_skill(uri: SKILL_URI) } + end + + test "a read completing after another read latched the skill is discarded" do + client, transport = connected_client + client.list_skills + client.read_skill_resource(uri: SKILL_URI) + transport.files[LOGO_URI] = { "blob" => "AAAA" } + transport.hook = ->(_request) { assert_verification_failure(:size) { client.read_skill_resource(uri: LOGO_URI) } } + + assert_refused(:latched) { client.read_skill_resource(uri: FORMS_URI) } + end + + test "concurrent reads and reconnects neither deadlock nor raise anything but Skills errors" do + client, transport = connected_client + client.list_skills + client.read_skill_resource(uri: SKILL_URI) + unexpected = Queue.new + readers = Array.new(4) do + Thread.new do + 50.times do + client.read_skill_resource(uri: FORMS_URI) + rescue Client::SkillError + nil + rescue StandardError => e + unexpected << e + end + end + end + reconnector = Thread.new do + 5.times do + transport.connected = false + client.connect + client.list_skills + client.read_skill_resource(uri: SKILL_URI) + rescue Client::SkillError + nil + rescue StandardError => e + unexpected << e + end + end + + (readers + [reconnector]).each { |thread| assert thread.join(10), "a thread did not finish" } + assert_equal 0, unexpected.size, unexpected.size.times.map { unexpected.pop.inspect }.join("; ") + end + + test "list_skills and get_skill carry the cursor, the cache hints, and _meta of the result" do + client, transport = connected_client + transport.define_singleton_method(:send_request) do |request:| + case request[:method] + when "skills/list" + { "result" => { "skills" => @skills, "nextCursor" => "page-2", "ttlMs" => 5000, "cacheScope" => "private", "_meta" => { "a" => 1 } } } + when "skills/get" + { "result" => { "skill" => @skills.first, "ttlMs" => 60_000, "cacheScope" => "public" } } + end + end + + page = client.list_skills + assert_equal "page-2", page.next_cursor + assert_equal 5000, page.ttl_ms + assert_equal "private", page.cache_scope + assert_equal({ "a" => 1 }, page.meta) + + got = client.get_skill(uri: SKILL_URI) + assert_equal SKILL_URI, got.skill.uri + assert_equal 60_000, got.ttl_ms + assert_equal "public", got.cache_scope + assert_nil got.meta + end + + test "list_skills and get_skill expose the _meta of an entry as a frozen Hash and refuse an entry whose _meta is not an object" do + annotated = pdf_entry.merge("_meta" => { "com.example/origin" => "catalog" }) + client, transport = connected_client(skills: [annotated]) + + page = client.list_skills + assert_equal({ "com.example/origin" => "catalog" }, page.skills.first.meta) + assert_predicate page.skills.first.meta, :frozen? + assert_nil page.meta + got = client.get_skill(uri: SKILL_URI) + assert_equal({ "com.example/origin" => "catalog" }, got.skill.meta) + assert_nil got.meta + assert_nil connected_client.first.list_skills.skills.first.meta + + transport.skills = [pdf_entry.merge("_meta" => "origin=catalog")] + error = assert_raises(Client::SkillEntryError) { client.list_skills } + assert_equal "Skill entry rejected: The value of meta must be a Hash (got String).", error.message + assert_raises(Client::SkillEntryError) { client.get_skill(uri: SKILL_URI) } + assert_equal({ "com.example/origin" => "catalog" }, client.read_skill_resource(uri: SKILL_URI).skill.meta) + end + + test "skills calls are refused while a reconnect handshake is in flight, even on a connected transport" do + client, transport = connected_client + client.list_skills + transport.connected = false + gate = Queue.new + transport.gate = gate + blocked = Thread.new { client.connect } + blocked.report_on_exception = false + begin + deadline = Process.clock_gettime(Process::CLOCK_MONOTONIC) + 5 + until blocked.status == "sleep" + flunk("the handshake did not block") if Process.clock_gettime(Process::CLOCK_MONOTONIC) > deadline + sleep(0.01) + end + # The transport already reports connected, so only the missing readiness can refuse the calls. + transport.connected = true + + assert_refused(:not_advertised) { client.list_skills } + assert_refused(:not_advertised) { client.read_skill_resource(uri: SKILL_URI) } + assert_nil client.skills_capability + ensure + gate << :go + blocked.join(5) + end + assert_equal 1, client.list_skills.skills.size + end + + test "verify helpers serve a raw read path" do + client, _transport = connected_client + skill = client.list_skills.skills.first + contents = client.read_resource(uri: FORMS_URI) + + assert Skill.verify_resource(skill, FORMS_URI, contents.first["text"].b) + assert Skill.verify_frontmatter(skill, client.read_resource(uri: SKILL_URI).first["text"]) + end + end +end diff --git a/test/mcp/methods_test.rb b/test/mcp/methods_test.rb index c6ae11a5..8373d8d2 100644 --- a/test/mcp/methods_test.rb +++ b/test/mcp/methods_test.rb @@ -66,6 +66,16 @@ def ensure_capability_does_not_raise_for(method, capabilities: {}) ensure_capability_raises_error_for Methods::COMPLETION_COMPLETE, required_capability_name: "completions" + # Skills (SEP-2640): the server registers these handlers only when skills are configured, so an unconfigured + # server answers `-32601` and the capability gate has nothing to add. + ensure_capability_does_not_raise_for Methods::SKILLS_LIST + ensure_capability_does_not_raise_for Methods::SKILLS_GET + + test "names the SEP-2640 methods" do + assert_equal "skills/list", Methods::SKILLS_LIST + assert_equal "skills/get", Methods::SKILLS_GET + end + # Client methods and notifications ensure_capability_does_not_raise_for Methods::NOTIFICATIONS_INITIALIZED diff --git a/test/mcp/server_skills_test.rb b/test/mcp/server_skills_test.rb new file mode 100644 index 00000000..f55b1ddd --- /dev/null +++ b/test/mcp/server_skills_test.rb @@ -0,0 +1,542 @@ +# frozen_string_literal: true + +require "test_helper" +require "fileutils" +require "tmpdir" + +module MCP + class ServerSkillsTest < ActiveSupport::TestCase + include InitializeParamsTestHelper + include InstrumentationTestHelper + + SKILL_MD = "---\nname: pdf-processing\ndescription: Extract PDFs\n---\n\nBody.\n" + + def entry(uri, content) + Skill::ResourceEntry.from_content(uri: uri, content: content) + end + + def pdf_skill + Skill.new( + uri: "skill://pdf-processing/SKILL.md", + frontmatter: { name: "pdf-processing", description: "Extract PDFs" }, + resources: [entry("skill://pdf-processing/SKILL.md", SKILL_MD), entry("skill://pdf-processing/references/FORMS.md", "Forms\n")], + ) + end + + def git_skill + Skill.new(uri: "skill://git/SKILL.md", frontmatter: { name: "git", description: "Git" }, resources: "dynamic") + end + + # No keyword next to the positional params Hash: Ruby 2.7 would read the Hash as the keywords. + def request(method, params = nil) + body = { jsonrpc: "2.0", id: 1, method: method } + body[:params] = params if params + body + end + + def modern_request(method, params = {}) + request(method, params.merge(_meta: { + "io.modelcontextprotocol/protocolVersion": "2026-07-28", + "io.modelcontextprotocol/clientInfo": { name: "modern_client", version: "2.0" }, + "io.modelcontextprotocol/clientCapabilities": {}, + })) + end + + def initialize_capabilities(server) + server.handle(request("initialize", initialize_params)).dig(:result, :capabilities) + end + + test "a server without skills answers -32601 and advertises no extension" do + server = Server.new(name: "plain") + + response = server.handle(request("skills/list")) + assert_equal JsonRpcHandler::ErrorCode::METHOD_NOT_FOUND, response[:error][:code] + assert_equal "skills/list", response[:error][:data] + assert_equal JsonRpcHandler::ErrorCode::METHOD_NOT_FOUND, server.handle(request("skills/get", { uri: "skill://x/SKILL.md" }))[:error][:code] + refute initialize_capabilities(server).key?(:extensions) + assert_nil server.skills + end + + test "skills: nil is the same as omitting it, skills: [] serves an empty catalog" do + assert_nil Server.new(name: "plain", skills: nil).skills + + server = Server.new(name: "empty", skills: []) + assert_equal [], server.skills + assert_predicate server.skills, :frozen? + assert_equal({ Skill::EXTENSION_ID => {} }, initialize_capabilities(server)[:extensions]) + assert_equal({ skills: [] }, server.handle(request("skills/list"))[:result]) + response = server.handle(request("skills/get", { uri: "skill://x/SKILL.md" })) + assert_equal(-32602, response[:error][:code]) + assert_equal "Skill not found: skill://x/SKILL.md", response[:error][:message] + assert_equal({ uri: "skill://x/SKILL.md" }, response[:error][:data]) + end + + test "lists and gets registered skills, paginated like the other lists" do + server = Server.new(name: "catalog", skills: [pdf_skill, git_skill], page_size: 1) + + first = server.handle(request("skills/list"))[:result] + assert_equal ["skill://pdf-processing/SKILL.md"], first[:skills].map { |skill| skill[:uri] } + assert_equal "1", first[:nextCursor] + second = server.handle(request("skills/list", { cursor: "1" }))[:result] + assert_equal ["skill://git/SKILL.md"], second[:skills].map { |skill| skill[:uri] } + refute second.key?(:nextCursor) + assert_equal "dynamic", second[:skills].first[:resources] + + got = server.handle(request("skills/get", { uri: "skill://pdf-processing/SKILL.md" }))[:result] + assert_equal pdf_skill.to_h, got[:skill] + refute got.key?(:ttlMs) + + assert_equal(-32602, server.handle(request("skills/list", { cursor: "nope" }))[:error][:code]) + end + + test "skills/list and skills/get carry the _meta of an entry and leave it out of an entry that has none" do + annotated = Skill.new(uri: "skill://git/SKILL.md", frontmatter: { name: "git", description: "Git" }, resources: "dynamic", meta: { "com.example/origin" => "catalog" }) + from_registry = Server.new(name: "catalog", skills: [pdf_skill, annotated]) + from_handlers = Server.new(name: "catalog") + from_handlers.skills_handlers(list: ->(_params) { [pdf_skill, annotated] }, get: ->(params) { [pdf_skill, annotated].find { |skill| skill.uri == params[:uri] } }) + + [from_registry, from_handlers].each do |server| + listed = server.handle(request("skills/list"))[:result][:skills] + refute listed.first.key?(:_meta) + assert_equal({ "com.example/origin" => "catalog" }, listed.last[:_meta]) + got = server.handle(request("skills/get", { uri: "skill://git/SKILL.md" }))[:result] + assert_equal annotated.to_h, got[:skill] + assert_equal({ "com.example/origin" => "catalog" }, got[:skill][:_meta]) + assert_equal '"_meta":{"com.example/origin":"catalog"}', JSON.generate(got[:skill])[/"_meta":\{[^}]*\}/] + end + end + + test "skills/list and skills/get are not run without an id, however the skills were configured" do + # Request methods of the extension need a request id like the protocol's own: framed as notifications, they + # could never be answered. + configuration = Configuration.new + configuration.instrumentation_callback = instrumentation_helper.callback + from_registry = Server.new(name: "catalog", skills: [pdf_skill], configuration: configuration) + calls = 0 + list = lambda do |_params| + calls += 1 + [pdf_skill] + end + get = lambda do |_params| + calls += 1 + pdf_skill + end + from_handlers = Server.new(name: "handlers") + from_handlers.skills_handlers(list: list, get: get) + + [from_registry, from_handlers].each do |server| + assert_nil server.handle({ jsonrpc: "2.0", method: "skills/list" }) + assert_nil server.handle({ jsonrpc: "2.0", method: "skills/get", params: { uri: pdf_skill.uri } }) + end + + assert_nil instrumentation_helper.data + assert_equal 0, calls + from_registry.handle(request("skills/list")) + assert_equal "skills/list", instrumentation_helper.data[:method] + end + + test "skills/get refuses a URI that does not name a SKILL.md before any lookup" do + server = Server.new(name: "catalog", skills: [pdf_skill]) + + ["skill://pdf-processing/references/FORMS.md", "skill://pdf-processing", "http://x/SKILL.md", nil, 42].each do |uri| + response = server.handle(request("skills/get", { uri: uri })) + assert_equal(-32602, response[:error][:code], uri.inspect) + assert_equal "Invalid params: uri is not a skill SKILL.md URI", response[:error][:message] + assert_equal({ uri: uri }, response[:error][:data]) + end + end + + test "refuses a registry with duplicate URIs and accepts an enclosing skill that lists a nested skill's files" do + error = assert_raises(ArgumentError) { Server.new(name: "x", skills: [pdf_skill, pdf_skill]) } + assert_equal "skills: skill skill://pdf-processing/SKILL.md is listed twice.", error.message + + # The specification wants the enclosing manifest complete, nested files and nested SKILL.md included. + outer = Skill.new( + uri: "skill://acme/outer/SKILL.md", + frontmatter: { name: "outer", description: "Outer" }, + resources: [ + entry("skill://acme/outer/SKILL.md", "o"), + entry("skill://acme/outer/inner/SKILL.md", "i"), + entry("skill://acme/outer/inner/notes.txt", "n"), + ], + ) + inner = Skill.new(uri: "skill://acme/outer/inner/SKILL.md", frontmatter: { name: "inner", description: "Inner" }, resources: "dynamic") + server = Server.new(name: "x", skills: [outer, inner]) + assert_equal 2, server.skills.size + listed = server.handle(request("skills/list"))[:result][:skills] + assert_equal 3, listed.find { |skill| skill[:uri] == "skill://acme/outer/SKILL.md" }[:resources].size + + assert_raises(ArgumentError) { Server.new(name: "x", skills: [pdf_skill, "nope"]) } + end + + test "handlers answer per request and receive server_context when they ask for it" do + catalog = [pdf_skill] + seen_context = nil + server = Server.new(name: "dynamic") + server.skills_handlers( + list: ->(_params, server_context:) { + seen_context = server_context + catalog }, + get: ->(params) { catalog.find { |skill| skill.uri == params[:uri] } }, + ) + + listed = server.handle(request("skills/list"))[:result] + assert_equal ["skill://pdf-processing/SKILL.md"], listed[:skills].map { |skill| skill[:uri] } + assert_kind_of ServerContext, seen_context + assert_equal pdf_skill.to_h, server.handle(request("skills/get", { uri: "skill://pdf-processing/SKILL.md" })).dig(:result, :skill) + response = server.handle(request("skills/get", { uri: "skill://git/SKILL.md" })) + assert_equal(-32602, response[:error][:code]) + assert_equal "Skill not found: skill://git/SKILL.md", response[:error][:message] + assert_equal [], server.skills + end + + test "skills_handlers refuses a server whose registry holds skills, which every caller could list and read" do + server = Server.new(name: "catalog", skills: [pdf_skill]) + + error = assert_raises(ArgumentError) { server.skills_handlers(list: ->(_p) { [] }, get: ->(_p) { nil }) } + assert_match(/cannot be combined with skills in the skills: registry/, error.message) + # The registry stays what the server serves. + assert_equal ["skill://pdf-processing/SKILL.md"], server.handle(request("skills/list"))[:result][:skills].map { |skill| skill[:uri] } + + # An empty registry holds nothing to leak, and handlers may be replaced. + empty = Server.new(name: "catalog", skills: []) + 2.times { empty.skills_handlers(list: ->(_p) { [git_skill] }, get: ->(_p) { git_skill }) } + assert_equal ["skill://git/SKILL.md"], empty.handle(request("skills/list"))[:result][:skills].map { |skill| skill[:uri] } + end + + test "a catalog that depends on the caller keeps a hidden skill out of every method when its files go through the same rule" do + Dir.mktmpdir do |dir| + root = File.realpath(dir) + { "guide" => "Public guide", "payroll" => "Payroll runs" }.each do |name, description| + FileUtils.mkdir_p(File.join(root, name)) + File.write(File.join(root, name, "SKILL.md"), "---\nname: #{name}\ndescription: #{description}\n---\n") + end + all = Skill.from_directory(root) + visible = ->(context) { context[:role] == "admin" ? all : all.reject { |skill| skill.name == "payroll" } } + server_for = lambda do |role| + server = Server.new(name: "catalog", server_context: { role: role }) + server.skills_handlers( + list: ->(_params, server_context:) { visible.call(server_context) }, + get: ->(params, server_context:) { visible.call(server_context).find { |skill| skill.uri == params[:uri] } }, + ) + server.resources_read_handler do |params, server_context:| + Skill.read_file(visible.call(server_context), params[:uri]) || [] + end + server + end + payroll = "skill://payroll/SKILL.md" + + guest = server_for.call("guest") + assert_equal ["skill://guide/SKILL.md"], guest.handle(request("skills/list"))[:result][:skills].map { |skill| skill[:uri] } + assert_equal(-32602, guest.handle(request("skills/get", { uri: payroll }))[:error][:code]) + assert_empty guest.handle(request("resources/list"))[:result][:resources] + assert_empty guest.handle(request("resources/read", { uri: payroll }))[:result][:contents] + assert_includes guest.handle(request("resources/read", { uri: "skill://guide/SKILL.md" })).dig(:result, :contents, 0, :text), "Public guide" + + admin = server_for.call("admin") + assert_equal payroll, admin.handle(request("skills/get", { uri: payroll })).dig(:result, :skill, :uri) + assert_includes admin.handle(request("resources/read", { uri: payroll })).dig(:result, :contents, 0, :text), "Payroll runs" + end + end + + test "an enclosing skill serves the files of a skill nested in it, so hiding the nested skill alone hides nothing" do + Dir.mktmpdir do |dir| + root = File.realpath(dir) + { "guide" => "Public guide", "guide/payroll" => "Payroll runs" }.each do |path, description| + FileUtils.mkdir_p(File.join(root, path)) + File.write(File.join(root, path, "SKILL.md"), "---\nname: #{File.basename(path)}\ndescription: #{description}\n---\n") + end + guide, payroll = Skill.from_directory(root) + + # The manifest of the enclosing skill is complete, as the specification requires, so the nested files are its own. + assert_includes guide.resources.map(&:uri), payroll.uri + assert_includes Skill.read_file([guide], payroll.uri).dig(0, :text), "Payroll runs" + + # A caller who must not read the nested skill is therefore not given the enclosing one either. + assert_nil Skill.read_file([], payroll.uri) + end + end + + test "a handler result outside the contract is a server fault with one fixed message" do + reported = [] + configuration = Configuration.new(exception_reporter: ->(exception, _context) { reported << exception }) + results = { list: nil, get: nil } + server = Server.new(name: "dynamic", configuration: configuration) + server.skills_handlers(list: ->(_params) { results[:list] }, get: ->(_params) { results[:get] }) + + [["not an array", pdf_skill], ["element", [pdf_skill, 42]], ["duplicate", [pdf_skill, pdf_skill]]].each do |label, result| + results[:list] = result + response = server.handle(request("skills/list")) + assert_equal(-32603, response[:error][:code], label) + assert_equal "Skills handler returned an invalid result", response[:error][:message], label + end + + results[:get] = 42 + assert_equal(-32603, server.handle(request("skills/get", { uri: "skill://pdf-processing/SKILL.md" }))[:error][:code]) + results[:get] = git_skill + assert_equal(-32603, server.handle(request("skills/get", { uri: "skill://pdf-processing/SKILL.md" }))[:error][:code]) + assert_equal 5, reported.size + assert reported.all? { |exception| exception.is_a?(StandardError) && !exception.is_a?(Server::RequestHandlerError) } + + # An entry built from the wire that lists a nested SKILL.md as a supporting file is within the contract. + results[:get] = Skill.from_entry( + "uri" => "skill://acme/outer/SKILL.md", + "frontmatter" => { "name" => "outer", "description" => "Outer" }, + "resources" => [entry("skill://acme/outer/SKILL.md", "o").to_h, entry("skill://acme/outer/inner/SKILL.md", "i").to_h], + ) + assert_equal "skill://acme/outer/SKILL.md", server.handle(request("skills/get", { uri: "skill://acme/outer/SKILL.md" }))[:result][:skill][:uri] + end + + test "skills_handlers requires both callables and leaves the previous configuration intact when a check fails" do + server = Server.new(name: "plain") + error = assert_raises(ArgumentError) { server.skills_handlers(list: ->(_p) { [] }, get: nil) } + assert_equal "skills_handlers requires callable list: and get: handlers.", error.message + assert_equal JsonRpcHandler::ErrorCode::METHOD_NOT_FOUND, server.handle(request("skills/list"))[:error][:code] + + capabilities = Server::Capabilities.new + capabilities.support_tools + server = Server.new(name: "no_resources", capabilities: capabilities) + error = assert_raises(ArgumentError) { server.skills_handlers(list: ->(_p) { [] }, get: ->(_p) { nil }) } + assert_match(/requires the resources capability/, error.message) + assert_equal JsonRpcHandler::ErrorCode::METHOD_NOT_FOUND, server.handle(request("skills/list"))[:error][:code] + assert_nil server.skills + + server = Server.new(name: "custom") + server.define_custom_method(method_name: "skills/list") { {} } + assert_raises(Server::MethodAlreadyDefinedError) { server.skills_handlers(list: ->(_p) { [] }, get: ->(_p) { nil }) } + assert_nil server.skills + + server = Server.new(name: "catalog", skills: []) + assert_raises(Server::MethodAlreadyDefinedError) { server.define_custom_method(method_name: "skills/get") { {} } } + end + + test "skills/list and skills/get carry the cache hints the way resources/list and resources/read do" do + legacy = Server.new(name: "catalog", skills: [pdf_skill]) + refute legacy.handle(request("skills/list"))[:result].key?(:ttlMs) + refute legacy.handle(request("skills/get", { uri: "skill://pdf-processing/SKILL.md" }))[:result].key?(:ttlMs) + + configured = Server.new(name: "catalog", skills: [pdf_skill], ttl_ms: 60_000, cache_scope: "public") + listed = configured.handle(request("skills/list"))[:result] + assert_equal 60_000, listed[:ttlMs] + assert_equal "public", listed[:cacheScope] + got = configured.handle(request("skills/get", { uri: "skill://pdf-processing/SKILL.md" }))[:result] + assert_equal 60_000, got[:ttlMs] + assert_equal "public", got[:cacheScope] + + modern = legacy.handle(modern_request("skills/list"))[:result] + assert_equal "complete", modern[:resultType] + assert_equal 0, modern[:ttlMs] + assert_equal "private", modern[:cacheScope] + got = legacy.handle(modern_request("skills/get", { uri: "skill://pdf-processing/SKILL.md" }))[:result] + assert_equal "complete", got[:resultType] + assert_equal 0, got[:ttlMs] + assert_equal "private", got[:cacheScope] + end + + test "a handler answers per caller, so its results are never marked for shared caches" do + server = Server.new(name: "catalog", ttl_ms: 60_000, cache_scope: "public") + server.skills_handlers(list: ->(_p) { [pdf_skill] }, get: ->(_p) { pdf_skill }) + get_params = { uri: "skill://pdf-processing/SKILL.md" } + + [request("skills/list"), request("skills/get", get_params), modern_request("skills/list"), modern_request("skills/get", get_params)].each do |call| + result = server.handle(call)[:result] + + assert_equal "private", result[:cacheScope], call[:method] + assert_equal 60_000, result[:ttlMs], call[:method] + end + + # Results that are the same for every caller keep the scope the server was configured with. + assert_equal "public", server.handle(request("tools/list"))[:result][:cacheScope] + + # The reads follow the server unless the read handler says otherwise for what it returns. + server.resources_read_handler { |_params| [] } + assert_equal "public", server.handle(request("resources/read", get_params))[:result][:cacheScope] + server.resources_read_handler { |_params| { contents: [], cacheScope: "private" } } + assert_equal "private", server.handle(request("resources/read", get_params))[:result][:cacheScope] + end + + test "resources/list carries each registered skill's SKILL.md with the frontmatter name and description" do + server = Server.new(name: "catalog", skills: [pdf_skill]) + entry = server.handle(request("resources/list"))[:result][:resources].find { |resource| resource[:uri] == "skill://pdf-processing/SKILL.md" } + assert_equal "pdf-processing", entry[:name] + assert_equal "Extract PDFs", entry[:description] + assert_equal "text/markdown", entry[:mimeType] + + # A resource the application registers at that URI takes the place of the generated entry. + server.define_resource(uri: "skill://pdf-processing/SKILL.md", name: "custom", mime_type: "text/markdown") { "---\n" } + entries = server.handle(request("resources/list"))[:result][:resources].select { |resource| resource[:uri] == "skill://pdf-processing/SKILL.md" } + assert_equal ["custom"], entries.map { |resource| resource[:name] } + + # Replacing the registry brings the generated entry back; a list handler replaces the whole listing. + server.resources = [Resource.new(uri: "other://x", name: "x")] + assert_equal ["other://x", "skill://pdf-processing/SKILL.md"], server.handle(request("resources/list"))[:result][:resources].map { |resource| resource[:uri] } + server.resources_list_handler { |_params| [Resource.new(uri: "custom://only", name: "only")] } + assert_equal ["custom://only"], server.handle(request("resources/list"))[:result][:resources].map { |resource| resource[:uri] } + + handlers_only = Server.new(name: "catalog") + handlers_only.skills_handlers(list: ->(_p) { [pdf_skill] }, get: ->(_p) { pdf_skill }) + assert_empty handlers_only.handle(request("resources/list"))[:result][:resources] + end + + test "the extension is advertised exactly when skills are configured, with resources alongside" do + error = assert_raises(ArgumentError) do + capabilities = Server::Capabilities.new + capabilities.support_resources + capabilities.support_extensions(Skill.capability) + Server.new(name: "x", capabilities: capabilities) + end + assert_match(/no skills are configured/, error.message) + + error = assert_raises(ArgumentError) do + capabilities = Server::Capabilities.new + capabilities.support_tools + Server.new(name: "x", skills: [], capabilities: capabilities) + end + assert_match(/requires the resources capability/, error.message) + + error = assert_raises(ArgumentError) do + capabilities = Server::Capabilities.new + capabilities.support_resources + capabilities.support_extensions(Skill::EXTENSION_ID => { directoryRead: true }) + Server.new(name: "x", skills: [], capabilities: capabilities) + end + assert_match(/directoryRead is not served/, error.message) + + capabilities = Server::Capabilities.new + capabilities.support_resources + capabilities.support_extensions(Skill.capability.merge("io.modelcontextprotocol/tasks" => {})) + server = Server.new(name: "x", skills: [pdf_skill], capabilities: capabilities) + assert_equal( + { resources: {}, extensions: { Skill::EXTENSION_ID => {}, "io.modelcontextprotocol/tasks" => {} } }, + initialize_capabilities(server), + ) + assert_equal initialize_capabilities(server), server.handle(request("server/discover")).dig(:result, :capabilities) + end + + test "the writer and the outgoing snapshot keep advertisement and dispatch consistent" do + server = Server.new(name: "catalog", skills: [pdf_skill]) + + error = assert_raises(ArgumentError) { server.capabilities = { tools: {} } } + assert_match(/requires the resources capability/, error.message) + assert_equal Server.new(name: "catalog", skills: [pdf_skill]).capabilities, server.capabilities + server.capabilities = { resources: {}, logging: {} } + assert_equal({ resources: {}, logging: {}, extensions: { Skill::EXTENSION_ID => {} } }, initialize_capabilities(server)) + + server.capabilities.delete(:resources) + server.capabilities[:extensions] = { Skill::EXTENSION_ID => { directoryRead: true, other: 1 } } + advertised = initialize_capabilities(server) + assert_equal({}, advertised[:resources]) + assert_equal({ other: 1 }, advertised[:extensions][Skill::EXTENSION_ID]) + assert server.handle(request("resources/read", { uri: "skill://pdf-processing/SKILL.md" })).key?(:result), "dispatch gates on the same snapshot" + + plain = Server.new(name: "plain") + plain.capabilities[:extensions] = { Skill::EXTENSION_ID => {} } + refute initialize_capabilities(plain).key?(:extensions) + end + + test "a hand-built declaration needs skills in the same constructor call, handlers may replace the catalog later" do + capabilities = Server::Capabilities.new + capabilities.support_resources + capabilities.support_extensions(Skill.capability) + server = Server.new(name: "x", skills: [], capabilities: capabilities) + server.skills_handlers(list: ->(_p) { [pdf_skill] }, get: ->(_p) { nil }) + + assert_equal ["skill://pdf-processing/SKILL.md"], server.handle(request("skills/list"))[:result][:skills].map { |skill| skill[:uri] } + end + + test "the extension identifier is recognized under either key spelling and advertised once" do + error = assert_raises(ArgumentError) do + Server.new(name: "x", capabilities: { resources: {}, extensions: { Skill::EXTENSION_ID.to_sym => {} } }) + end + assert_match(/no skills are configured/, error.message) + error = assert_raises(ArgumentError) do + Server.new(name: "x", capabilities: { "resources" => {}, "extensions" => { Skill::EXTENSION_ID => {} } }) + end + assert_match(/no skills are configured/, error.message) + error = assert_raises(ArgumentError) do + Server.new(name: "x", skills: [], capabilities: { resources: {}, extensions: { Skill::EXTENSION_ID.to_sym => { "directoryRead" => true } } }) + end + assert_match(/directoryRead is not served/, error.message) + + server = Server.new( + name: "x", + skills: [pdf_skill], + capabilities: { resources: {}, extensions: { Skill::EXTENSION_ID.to_sym => { directoryRead: false, other: 1 }, "io.modelcontextprotocol/tasks" => {} } }, + ) + advertised = JSON.parse(JSON.generate(initialize_capabilities(server))) + assert_equal({ "other" => 1 }, advertised["extensions"][Skill::EXTENSION_ID]) + assert_equal 2, advertised["extensions"].size + + plain = Server.new(name: "plain") + plain.capabilities[:extensions] = { Skill::EXTENSION_ID.to_sym => {} } + refute initialize_capabilities(plain).key?(:extensions) + + # A Hash spelling `extensions` both ways is read as one declaration, whichever side holds the Skills entry. + error = assert_raises(ArgumentError) do + Server.new(name: "x", capabilities: { extensions: { "io.modelcontextprotocol/tasks" => {} }, "extensions" => { Skill::EXTENSION_ID => {} } }) + end + assert_match(/no skills are configured/, error.message) + split = Server.new(name: "split") + split.capabilities[:extensions] = { "io.modelcontextprotocol/tasks" => {} } + split.capabilities["extensions"] = { Skill::EXTENSION_ID => {} } + assert_equal({ "io.modelcontextprotocol/tasks" => {} }, JSON.parse(JSON.generate(initialize_capabilities(split)))["extensions"]) + server = Server.new( + name: "x", + skills: [pdf_skill], + capabilities: { resources: {}, extensions: { Skill::EXTENSION_ID => { other: 1 } }, "extensions" => { "io.modelcontextprotocol/tasks" => {} } }, + ) + advertised = JSON.parse(JSON.generate(initialize_capabilities(server))) + assert_equal({ "io.modelcontextprotocol/tasks" => {}, Skill::EXTENSION_ID => { "other" => 1 } }, advertised["extensions"]) + + # `resources` is advertised under one key, so a nil second spelling cannot serialize after the Hash and undo it. + [ + { "resources" => {}, resources: nil }, + { resources: nil, "resources" => {} }, + { resources: { listChanged: true }, "resources" => nil }, + { "resources" => { subscribe: true }, resources: { listChanged: true } }, + ].each do |capabilities| + snapshot = initialize_capabilities(Server.new(name: "x", skills: [pdf_skill], capabilities: capabilities)) + assert_equal [:resources], snapshot.keys.select { |key| key.to_s == "resources" }, capabilities.inspect + assert_kind_of Hash, snapshot[:resources], capabilities.inspect + end + snapshot = initialize_capabilities(Server.new(name: "x", skills: [pdf_skill], capabilities: { "resources" => { subscribe: true }, resources: { listChanged: true } })) + assert_equal({ listChanged: true }, snapshot[:resources]) + end + + test "skills/get without a params Hash is an invalid-params error, not a server fault" do + server = Server.new(name: "catalog", skills: [pdf_skill]) + + [ + { jsonrpc: "2.0", id: 1, method: "skills/get" }, + { jsonrpc: "2.0", id: 1, method: "skills/get", params: nil }, + { jsonrpc: "2.0", id: 1, method: "skills/get", params: ["skill://pdf-processing/SKILL.md"] }, + ].each do |body| + response = server.handle(body) + assert_equal(-32602, response[:error][:code], body.inspect) + assert_equal "Invalid params: uri is not a skill SKILL.md URI", response[:error][:message] + end + end + + test "resources colliding with a directory-backed skill file are refused before anything is committed" do + Dir.mktmpdir("skills") do |root| + FileUtils.mkdir_p(File.join(root, "pdf-processing", "references")) + File.write(File.join(root, "pdf-processing", "SKILL.md"), SKILL_MD) + File.write(File.join(root, "pdf-processing", "references", "FORMS.md"), "Forms\n") + skills = Skill.from_directory(root) + duplicate = Resource.new(uri: "skill://pdf-processing/references/FORMS.md", name: "dup") + + error = assert_raises(ArgumentError) { Server.new(name: "x", skills: skills, resources: [duplicate]) } + assert_match(/is a file of a directory-backed skill/, error.message) + + server = Server.new(name: "x", skills: skills, resources: [Resource.new(uri: "other://one", name: "one")]) + assert_raises(ArgumentError) { server.define_resource(uri: "skill://pdf-processing/SKILL.md", name: "dup") { "x" } } + assert_equal ["other://one"], server.resources.map(&:uri) + assert_raises(ArgumentError) { server.resources = [duplicate] } + assert_equal ["other://one"], server.resources.map(&:uri) + + server.define_resource_template(uri_template: "skill://pdf-processing/{file}", name: "shadowed") { |_params| "template" } + assert_equal "Forms\n", server.handle(request("resources/read", { uri: "skill://pdf-processing/references/FORMS.md" })).dig(:result, :contents, 0, :text) + end + end + end +end diff --git a/test/mcp/skill/directory_test.rb b/test/mcp/skill/directory_test.rb new file mode 100644 index 00000000..fd56b5e1 --- /dev/null +++ b/test/mcp/skill/directory_test.rb @@ -0,0 +1,475 @@ +# frozen_string_literal: true + +require "test_helper" +require "tmpdir" + +module MCP + class Skill + class DirectoryTest < ActiveSupport::TestCase + SKILL_MD = "---\nname: pdf-processing\ndescription: Extract PDFs\n---\n\nBody.\n" + + def with_root + Dir.mktmpdir("skills") do |root| + yield root + end + end + + def write(root, relative, content) + path = File.join(root, relative) + FileUtils.mkdir_p(File.dirname(path)) + File.binwrite(path, content) + path + end + + def pdf_root(root) + write(root, "pdf-processing/SKILL.md", SKILL_MD) + write(root, "pdf-processing/references/FORMS.md", "Forms\n") + write(root, "pdf-processing/logo.png", "\x89PNG\r\n\x1a\n\xFF".b) + root + end + + def read_request(uri) + { jsonrpc: "2.0", id: 1, method: "resources/read", params: { uri: uri } } + end + + test "turns every directory holding a SKILL.md into a skill with digests and sizes of its files" do + with_root do |root| + pdf_root(root) + skills = Skill.from_directory(root) + + assert_equal ["skill://pdf-processing/SKILL.md"], skills.map(&:uri) + skill = skills.first + assert_equal({ "name" => "pdf-processing", "description" => "Extract PDFs" }, skill.frontmatter) + assert_equal( + ["skill://pdf-processing/SKILL.md", "skill://pdf-processing/logo.png", "skill://pdf-processing/references/FORMS.md"], + skill.resources.map(&:uri), + ) + entry = skill.resources.find { |candidate| candidate.uri == "skill://pdf-processing/SKILL.md" } + assert_equal SKILL_MD.bytesize, entry.size + assert_equal "sha256:#{Digest::SHA256.hexdigest(SKILL_MD)}", entry.digest + assert_predicate skill, :frozen? + end + end + + test "accepts a Pathname and a symbolic link to the root, and orders skills by URI" do + with_root do |root| + pdf_root(root) + write(root, "acme/outer/SKILL.md", "---\nname: outer\ndescription: Outer\n---\n") + link = File.join(root, "link") + File.symlink(root, link) + + assert_equal ["skill://acme/outer/SKILL.md", "skill://pdf-processing/SKILL.md"], Skill.from_directory(Pathname.new(link)).map(&:uri) + end + end + + test "a nested skill is published on its own and its files are supporting files of the enclosing skill" do + with_root do |root| + write(root, "acme/outer/SKILL.md", "---\nname: outer\ndescription: Outer\n---\n") + write(root, "acme/outer/guide.md", "g\n") + write(root, "acme/outer/inner/SKILL.md", "---\nname: inner\ndescription: Inner\n---\n") + write(root, "acme/outer/inner/notes.txt", "n\n") + skills = Skill.from_directory(root) + by_uri = skills.to_h { |skill| [skill.uri, skill] } + + outer = by_uri.fetch("skill://acme/outer/SKILL.md") + inner = by_uri.fetch("skill://acme/outer/inner/SKILL.md") + assert_equal( + ["skill://acme/outer/SKILL.md", "skill://acme/outer/guide.md", "skill://acme/outer/inner/SKILL.md", "skill://acme/outer/inner/notes.txt"], + outer.resources.map(&:uri), + ) + assert_equal ["skill://acme/outer/inner/SKILL.md", "skill://acme/outer/inner/notes.txt"], inner.resources.map(&:uri) + shared = ->(skill) { skill.resources.find { |entry| entry.uri == "skill://acme/outer/inner/notes.txt" } } + assert_equal shared.call(inner), shared.call(outer), "both manifests carry the same digest and size" + # The nested files count toward the enclosing skill's total as well. + assert_equal inner.total_size + "---\nname: outer\ndescription: Outer\n---\n".bytesize + "g\n".bytesize, outer.total_size + + # The shared file is served once; a URI under a skill root that names no file is -32602, and + # a URI outside every skill root keeps the base reader's answer. + server = Server.new(name: "catalog", skills: skills) + assert_equal "n\n", server.handle(read_request("skill://acme/outer/inner/notes.txt")).dig(:result, :contents, 0, :text) + response = server.handle(read_request("skill://acme/outer/missing.md")) + assert_equal(-32602, response[:error][:code]) + assert_equal "Skill file not found: skill://acme/outer/missing.md", response[:error][:message] + assert_equal [], server.handle(read_request("skill://elsewhere/x"))[:result][:contents] + + listed = server.handle({ jsonrpc: "2.0", id: 1, method: "resources/list" })[:result][:resources] + assert_equal ["inner", "outer"], listed.map { |resource| resource[:name] }.sort + end + end + + test "a hard link is refused at construction, and one added to a served file is refused at read time" do + with_root do |root| + pdf_root(root) + outside = File.join(root, "secret.txt") + File.binwrite(outside, "TOP SECRET\n") + linked = File.join(root, "pdf-processing", "linked.txt") + File.link(outside, linked) + error = assert_raises(ArgumentError) { Skill.from_directory(root) } + assert_equal "#{File.realpath(root)}/pdf-processing/linked.txt has more than one hard link", error.message + + File.delete(linked) + server = Server.new(name: "catalog", skills: Skill.from_directory(root)) + File.link(File.join(root, "pdf-processing", "references", "FORMS.md"), File.join(root, "FORMS-copy.md")) + assert_equal(-32603, server.handle(read_request("skill://pdf-processing/references/FORMS.md"))[:error][:code]) + end + end + + test "a path that no longer names the opened file is refused at construction" do + with_root do |root| + pdf_root(root) + other = File.stat(__FILE__) + begin + # What `stat` reports for the path differs from the opened handle, as after an intermediate directory swap. + File.stubs(:stat).returns(other) + error = assert_raises(ArgumentError) { Skill.from_directory(root) } + ensure + File.unstub(:stat) + end + assert_equal "#{File.realpath(root)}/pdf-processing/SKILL.md is not the file its path names", error.message + end + end + + test "three levels of nesting list every descendant in every enclosing manifest once" do + with_root do |root| + write(root, "a/SKILL.md", "---\nname: a\ndescription: A\n---\n") + write(root, "a/b/SKILL.md", "---\nname: b\ndescription: B\n---\n") + write(root, "a/b/c/SKILL.md", "---\nname: c\ndescription: C\n---\n") + write(root, "a/b/c/deep.txt", "d\n") + by_uri = Skill.from_directory(root).to_h { |skill| [skill.uri, skill.resources.map(&:uri)] } + + assert_equal ["skill://a/SKILL.md", "skill://a/b/SKILL.md", "skill://a/b/c/SKILL.md", "skill://a/b/c/deep.txt"], by_uri.fetch("skill://a/SKILL.md") + assert_equal ["skill://a/b/SKILL.md", "skill://a/b/c/SKILL.md", "skill://a/b/c/deep.txt"], by_uri.fetch("skill://a/b/SKILL.md") + assert_equal ["skill://a/b/c/SKILL.md", "skill://a/b/c/deep.txt"], by_uri.fetch("skill://a/b/c/SKILL.md") + end + end + + test "records from separate scans are accepted when equal and refused when they come from different files" do + with_root do |root| + write(root, "acme/outer/SKILL.md", "---\nname: outer\ndescription: Outer\n---\n") + write(root, "acme/outer/inner/SKILL.md", "---\nname: inner\ndescription: Inner\n---\n") + write(root, "acme/outer/inner/notes.txt", "n\n") + outer = Skill.from_directory(root).find { |skill| skill.name == "outer" } + inner_again = Skill.from_directory(root).find { |skill| skill.name == "inner" } + assert_equal 2, Server.new(name: "catalog", skills: [outer, inner_again]).skills.size + + with_root do |other| + write(other, "acme/outer/inner/SKILL.md", "---\nname: inner\ndescription: Inner\n---\n") + write(other, "acme/outer/inner/notes.txt", "n\n") + foreign_inner = Skill.from_directory(other).first + error = assert_raises(ArgumentError) { Server.new(name: "catalog", skills: [outer, foreign_inner]) } + assert_equal "The skill file skill://acme/outer/inner/SKILL.md is served by more than one skill from different files.", error.message + end + end + end + + test "the -32602 answer covers the skill subtree only and comes after registered resources" do + with_root do |root| + pdf_root(root) + skills = Skill.from_directory(root) + plain = Server.new(name: "catalog", skills: skills) + assert_equal [], plain.handle(read_request("skill://pdf-processing-extra/x"))[:result][:contents], "a sibling prefix is outside the root" + + server = Server.new(name: "catalog", skills: skills) + server.define_resource(uri: "skill://pdf-processing/extra", name: "extra", mime_type: "text/plain") { { uri: "skill://pdf-processing/extra", text: "extra" } } + server.define_resource_template(uri_template: "skill://pdf-processing/templates/{id}", name: "shadowed") { |id:| "template #{id}" } + assert_equal "extra", server.handle(read_request("skill://pdf-processing/extra")).dig(:result, :contents, 0, :text) + assert_equal(-32602, server.handle(read_request("skill://pdf-processing/templates/1"))[:error][:code], "templates are shadowed under the skill root") + end + end + + test "skips dotfiles, symbolic links, and FIFOs, publishes editor backups, and ignores files outside skill directories" do + with_root do |root| + pdf_root(root) + write(root, "pdf-processing/.env", "SECRET=1") + write(root, "pdf-processing/SKILL.md~", "backup") + write(root, "README.md", "not a skill") + File.symlink(File.join(root, "README.md"), File.join(root, "pdf-processing", "linked.md")) + File.mkfifo(File.join(root, "pdf-processing", "pipe")) if File.respond_to?(:mkfifo) + uris = Skill.from_directory(root).first.resources.map(&:uri) + + assert_includes uris, "skill://pdf-processing/SKILL.md~" + refute_includes uris, "skill://pdf-processing/.env" + refute_includes uris, "skill://pdf-processing/linked.md" + refute_includes uris, "skill://pdf-processing/pipe" + refute_includes uris, "skill://README.md" + end + end + + test "refuses a root that is missing, not a directory, holds a SKILL.md itself, or holds no skill" do + with_root do |root| + error = assert_raises(ArgumentError) { Skill.from_directory(File.join(root, "missing")) } + assert_match(/cannot be resolved/, error.message) + + file = write(root, "file.txt", "x") + error = assert_raises(ArgumentError) { Skill.from_directory(file) } + assert_match(/is not a directory/, error.message) + + error = assert_raises(ArgumentError) { Skill.from_directory(root) } + assert_match(/No directory holding a SKILL.md/, error.message) + + write(root, "SKILL.md", SKILL_MD) + error = assert_raises(ArgumentError) { Skill.from_directory(root) } + assert_match(/holds a SKILL.md itself/, error.message) + + assert_raises(ArgumentError) { Skill.from_directory(42) } + end + end + + test "refuses directory and file names outside the unreserved URI characters, naming the path" do + with_root do |root| + pdf_root(root) + write(root, "pdf-processing/My Forms.md", "x") + error = assert_raises(ArgumentError) { Skill.from_directory(root) } + assert_includes error.message, File.join(root, "pdf-processing", "My Forms.md") + assert_match(/may only use letters, digits/, error.message) + end + + with_root do |root| + write(root, "pdf tools/SKILL.md", "---\nname: pdf tools\ndescription: d\n---\n") + error = assert_raises(ArgumentError) { Skill.from_directory(root) } + assert_includes error.message, File.join(root, "pdf tools") + end + end + + test "refuses a frontmatter the shared rules cannot read, a name that differs from the directory, and over-limit skills" do + with_root do |root| + write(root, "pdf-processing/SKILL.md", "---\nname: pdf-processing\ndescription: d\nflag: yes\n---\n") + error = assert_raises(ArgumentError) { Skill.from_directory(root) } + assert_match(/frontmatter of .*SKILL.md cannot be read \(parse\)/, error.message) + end + + with_root do |root| + write(root, "pdf-processing/SKILL.md", "---\nname: other\ndescription: d\n---\n") + error = assert_raises(ArgumentError) { Skill.from_directory(root) } + assert_includes error.message, File.join(root, "pdf-processing") + assert_match(/must equal the last segment/, error.message) + end + + with_root do |root| + write(root, "pdf-processing/SKILL.md", SKILL_MD) + 512.times { |index| write(root, "pdf-processing/f#{index}", "x") } + error = assert_raises(ArgumentError) { Skill.from_directory(root) } + assert_match(/at most 512 entries/, error.message) + end + end + + test "accepts a BOM-prefixed SKILL.md and registers its digest over the raw bytes" do + with_root do |root| + content = "\xEF\xBB\xBF#{SKILL_MD}".b + write(root, "pdf-processing/SKILL.md", content) + skill = Skill.from_directory(root).first + + assert_equal "pdf-processing", skill.name + assert_equal "sha256:#{Digest::SHA256.hexdigest(content)}", skill.resources.first.digest + end + end + + test "the server serves text files as text and other files as base64 blobs, by exact URI only" do + with_root do |root| + pdf_root(root) + server = Server.new(name: "catalog", skills: Skill.from_directory(root)) + + assert_equal( + { contents: [{ uri: "skill://pdf-processing/references/FORMS.md", mimeType: "text/markdown", text: "Forms\n" }] }, + server.handle(read_request("skill://pdf-processing/references/FORMS.md"))[:result], + ) + assert_equal( + { contents: [{ uri: "skill://pdf-processing/logo.png", blob: ["\x89PNG\r\n\x1a\n\xFF".b].pack("m0") }] }, + server.handle(read_request("skill://pdf-processing/logo.png"))[:result], + ) + # URIs under the skill root that name no registered file are refused without touching the filesystem. + ["skill://pdf-processing/.env", "skill://pdf-processing/../pdf-processing/SKILL.md", "skill://pdf-processing/references/FORMS.md/"].each do |uri| + response = server.handle(read_request(uri)) + assert_equal(-32602, response[:error][:code], uri) + assert_equal "Skill file not found: #{uri}", response[:error][:message] + end + end + end + + test "a file that changed since registration is refused with one generic error and the detail goes to the reporter" do + with_root do |root| + pdf_root(root) + reported = [] + configuration = Configuration.new(exception_reporter: ->(exception, _context) { reported << exception }) + server = Server.new(name: "catalog", skills: Skill.from_directory(root), configuration: configuration) + forms = File.join(root, "pdf-processing", "references", "FORMS.md") + + File.binwrite(forms, "Formz\n") + response = server.handle(read_request("skill://pdf-processing/references/FORMS.md")) + assert_equal(-32603, response[:error][:code]) + assert_equal "Skill file changed since registration", response[:error][:message] + assert_equal 1, reported.size + assert_includes reported.first.message, forms + assert_match(/content/, reported.first.message) + + File.binwrite(forms, "Forms\n\n") + assert_equal(-32603, server.handle(read_request("skill://pdf-processing/references/FORMS.md"))[:error][:code]) + + File.delete(forms) + assert_equal(-32603, server.handle(read_request("skill://pdf-processing/references/FORMS.md"))[:error][:code]) + + File.symlink(File.join(root, "pdf-processing", "SKILL.md"), forms) + assert_equal(-32603, server.handle(read_request("skill://pdf-processing/references/FORMS.md"))[:error][:code]) + + if File.respond_to?(:mkfifo) + File.delete(forms) + File.mkfifo(forms) + assert_equal(-32603, server.handle(read_request("skill://pdf-processing/references/FORMS.md"))[:error][:code]) + end + refute_includes response[:error].to_s, root + end + end + + test "a registered file replaced by a new inode holding the same bytes is refused" do + with_root do |root| + pdf_root(root) + server = Server.new(name: "catalog", skills: Skill.from_directory(root)) + forms = File.join(root, "pdf-processing", "references", "FORMS.md") + # The original inode stays alive under another name, so the new file cannot reuse its number. + File.rename(forms, "#{forms}.old") + File.binwrite(forms, "Forms\n") + + assert_equal(-32603, server.handle(read_request("skill://pdf-processing/references/FORMS.md"))[:error][:code]) + end + end + + test "an intermediate directory swapped for one holding a same-named file with the same bytes is refused" do + with_root do |root| + pdf_root(root) + server = Server.new(name: "catalog", skills: Skill.from_directory(root)) + references = File.join(root, "pdf-processing", "references") + File.rename(references, "#{references}.old") + FileUtils.mkdir_p(references) + File.binwrite(File.join(references, "FORMS.md"), "Forms\n") + + assert_equal(-32603, server.handle(read_request("skill://pdf-processing/references/FORMS.md"))[:error][:code]) + end + end + + test "a file that would take the skill over the size limit is refused before it is read" do + with_root do |root| + pdf_root(root) + # Sorts before every other file of the skill, so the refusal happens before any file is read. + huge = File.join(root, "pdf-processing", "0huge.bin") + File.open(huge, "wb") { |io| io.truncate(Skill::MAX_TOTAL_SIZE + 1) } + File.any_instance.expects(:read).never + + error = assert_raises(ArgumentError) { Skill.from_directory(root) } + assert_includes error.message, huge + assert_match(/past the 16777216 byte limit/, error.message) + end + end + + test "a file that grows past the size budget after its size was checked is refused by the bounded read" do + with_root do |root| + pdf_root(root) + grown = File.join(root, "pdf-processing", "0grown.bin") + File.binwrite(grown, "") + # `fstat` saw an empty file; the read, bounded to one byte past the whole budget, then returns that much. + File.any_instance.expects(:read).with(Skill::MAX_TOTAL_SIZE + 1).returns(("\0" * (Skill::MAX_TOTAL_SIZE + 1)).b) + + error = assert_raises(ArgumentError) { Skill.from_directory(root) } + assert_includes error.message, grown + assert_match(/past the 16777216 byte limit/, error.message) + end + end + + test "a SKILL.md that vanishes between the directory check and the scan is refused with an ArgumentError" do + with_root do |root| + pdf_root(root) + # The scan runs on the resolved root, so the stub must match the path the scan passes. + pdf = File.join(File.realpath(root), "pdf-processing") + File.delete(File.join(pdf, "SKILL.md")) + Skill.stubs(:skill_directory?).returns(false) + Skill.stubs(:skill_directory?).with(pdf).returns(true) + + error = assert_raises(ArgumentError) { Skill.from_directory(root) } + assert_equal "The SKILL.md of #{pdf} disappeared while the directory was being scanned.", error.message + end + end + + test "resources are ordered by URI, not by the directory walk" do + with_root do |root| + pdf_root(root) + write(root, "pdf-processing/a/b.md", "b\n") + write(root, "pdf-processing/a.md", "a\n") + + uris = Skill.from_directory(root).first.resources.map(&:uri) + assert_equal uris.sort, uris + assert_operator uris.index("skill://pdf-processing/a.md"), :<, uris.index("skill://pdf-processing/a/b.md") + end + end + + test "a directory that cannot be listed or an entry that cannot be examined is an ArgumentError naming it" do + with_root do |root| + pdf_root(root) + begin + Dir.stubs(:children).raises(Errno::EACCES) + error = assert_raises(ArgumentError) { Skill.from_directory(root) } + ensure + Dir.unstub(:children) + end + assert_equal "The directory #{File.realpath(root)} cannot be listed (Errno::EACCES).", error.message + end + with_root do |root| + pdf_root(root) + begin + File.stubs(:lstat).raises(Errno::EIO) + error = assert_raises(ArgumentError) { Skill.from_directory(root) } + ensure + File.unstub(:lstat) + end + assert_equal "The entry #{File.join(File.realpath(root), "pdf-processing")} cannot be examined (Errno::EIO).", error.message + end + end + + test "without NOFOLLOW a symbolic link in place of a registered file is still refused" do + with_root do |root| + pdf_root(root) + server = Server.new(name: "catalog", skills: Skill.from_directory(root)) + forms = File.join(root, "pdf-processing", "references", "FORMS.md") + File.delete(forms) + File.symlink(File.join(root, "pdf-processing", "SKILL.md"), forms) + + Skill.stubs(:nofollow_supported?).returns(false) + assert_equal(-32603, server.handle(read_request("skill://pdf-processing/references/FORMS.md"))[:error][:code]) + end + end + + test "read_file lets a custom resources_read_handler serve skill files with the same checks" do + with_root do |root| + pdf_root(root) + skills = Skill.from_directory(root) + server = Server.new(name: "catalog", skills: skills) + server.resources_read_handler do |params| + Skill.read_file(server.skills, params[:uri]) || [{ uri: params[:uri], text: "custom" }] + end + + assert_equal "Forms\n", server.handle(read_request("skill://pdf-processing/references/FORMS.md")).dig(:result, :contents, 0, :text) + assert_equal "custom", server.handle(read_request("other://thing")).dig(:result, :contents, 0, :text) + assert_nil Skill.read_file(skills, "skill://pdf-processing/nope") + # The item's uri is the record's own String, so a handler must not be able to change it for later reads. + assert_predicate Skill.read_file(skills, "skill://pdf-processing/references/FORMS.md").first[:uri], :frozen? + + File.binwrite(File.join(root, "pdf-processing", "references", "FORMS.md"), "Formz\n") + response = server.handle(read_request("skill://pdf-processing/references/FORMS.md")) + assert_equal(-32603, response[:error][:code]) + assert_equal "Skill file changed since registration", response[:error][:message] + end + end + + test "directory-backed skills keep serving when the Array is combined or filtered" do + with_root do |root| + pdf_root(root) + write(root, "acme/outer/SKILL.md", "---\nname: outer\ndescription: Outer\n---\n") + all = Skill.from_directory(root) + only_pdf = all.select { |skill| skill.name == "pdf-processing" } + server = Server.new(name: "catalog", skills: only_pdf + []) + + assert_equal "Forms\n", server.handle(read_request("skill://pdf-processing/references/FORMS.md")).dig(:result, :contents, 0, :text) + assert_equal({ contents: [] }, server.handle(read_request("skill://acme/outer/SKILL.md"))[:result]) + end + end + end + end +end diff --git a/test/mcp/skill/frontmatter_test.rb b/test/mcp/skill/frontmatter_test.rb new file mode 100644 index 00000000..b513883b --- /dev/null +++ b/test/mcp/skill/frontmatter_test.rb @@ -0,0 +1,174 @@ +# frozen_string_literal: true + +require "test_helper" + +module MCP + class Skill + class FrontmatterTest < ActiveSupport::TestCase + FRONTMATTER = Skill.const_get(:Frontmatter) + + def load(text) + FRONTMATTER.load(text) + end + + def assert_refused(text, message = nil) + error = assert_raises(FRONTMATTER::Error, message || text.inspect) { load(text) } + assert_equal :parse, error.reason + end + + test "reads the block between the delimiters as a Hash with String keys" do + assert_equal( + { "name" => "pdf", "description" => "Extract PDFs", "tags" => ["a", "b"], "nested" => { "x" => 1, "y" => 1.5, "z" => nil } }, + load("---\nname: pdf\ndescription: Extract PDFs\ntags:\n - a\n - b\nnested:\n x: 1\n y: 1.5\n z: ~\n---\n\nBody text is never read.\n"), + ) + end + + test "skips a leading UTF-8 BOM and accepts CRLF line endings" do + assert_equal({ "name" => "pdf" }, load("\xEF\xBB\xBF---\nname: pdf\n---\n".b)) + assert_equal({ "name" => "pdf" }, load("---\r\nname: pdf\r\n---\r\nbody")) + assert_equal({ "name" => "pdf" }, load("---\nname: pdf\n---")) + end + + test "refuses text that does not open with a delimiter line, lacks the closing one, or is not UTF-8" do + assert_refused("name: pdf\n---\n") + assert_refused("--- \nname: pdf\n---\n") + assert_refused("---") + assert_refused("---\nname: pdf\n") + assert_refused("") + assert_refused("---\nname: \xFF\n---\n".b) + end + + test "refuses line breaks Psych honors but the delimiter scan or a YAML 1.2 reader does not" do + assert_refused("---\r\r\nname: pdf\r\n---\r\n", "doubled CR on the opening line") + assert_refused("---\nname: pdf\r\r\n---\n", "doubled CR on a block line") + assert_refused("---\nname: pdf\rdescription: d\n---\n", "lone CR inside a block line") + assert_refused("---\nname: pdf\n---\r", "lone CR after the closing delimiter at EOF") + { "NEL" => 0x85, "LINE SEPARATOR" => 0x2028, "PARAGRAPH SEPARATOR" => 0x2029 }.each do |label, codepoint| + separator = codepoint.chr(Encoding::UTF_8) + assert_refused("---\nname: pdf#{separator}evil: injected\n---\n", label) + assert_refused("---#{separator}name: pdf\n---\n", "#{label} on the opening line") + assert_refused("---\nname: \"pdf#{separator}more\"\n---\n", "#{label} in a quoted scalar") + assert_refused("---\nname: |\n pdf#{separator}more\n---\n", "#{label} in a block scalar") + end + assert_equal({ "name" => "pdf" }, load("---\r\nname: pdf\r\n---")) + end + + test "refuses integers beyond the range every JSON reader keeps exact" do + assert_refused("---\nname: pdf\nbig: 9007199254740992\n---\n") + assert_refused("---\nname: pdf\nbig: -9007199254740992\n---\n") + assert_equal({ "name" => "pdf", "big" => 9_007_199_254_740_991 }, load("---\nname: pdf\nbig: 9007199254740991\n---\n")) + end + + test "refuses a block over 64 KiB" do + long = "description: #{"x" * (64 * 1024)}\n" + assert_refused("---\nname: pdf\n#{long}---\n") + end + + test "refuses a second document, an explicit document end, and directives" do + assert_refused("---\nname: a\n...\n---\nname: b\n---\n", "second document") + assert_refused("---\nname: a\n...\n---\n", "trailing document end") + assert_refused("---\n%YAML 1.2\n---\nname: a\n---\n", "YAML directive") + assert_refused("---\n%TAG ! tag:example.com,2000:\n---\nname: a\n---\n", "TAG directive") + end + + test "refuses an empty block and a block whose top level is not a mapping" do + assert_refused("---\n---\n") + assert_refused("---\n# only a comment\n---\n") + assert_refused("---\n- a\n- b\n---\n") + assert_refused("---\njust a scalar\n---\n") + end + + test "refuses tags on keys, values, and collections, aliases, and merge keys" do + assert_refused("---\ncount: !!int \"010\"\n---\n", "tagged quoted value") + assert_refused("---\ncount: !!str 1\n---\n", "tagged plain value") + assert_refused("---\n!!str name: a\n---\n", "tagged key") + assert_refused("---\nlist: !!seq\n - a\n---\n", "tagged collection") + assert_refused("---\nbase: &b\n x: 1\nother: *b\n---\n", "alias") + assert_refused("---\n<<: {x: 1}\nname: a\n---\n", "merge key") + end + + test "refuses duplicate keys, which safe_load would silently collapse" do + assert_refused("---\nname: a\nname: b\n---\n") + assert_refused("---\nnested:\n x: 1\n x: 2\n---\n") + end + + test "requires every key to be an untagged scalar whose resolved type is String" do + assert_refused("---\n1: a\n---\n", "integer key") + assert_refused("---\ntrue: a\n---\n", "boolean key") + assert_refused("---\n~: a\n---\n", "null key") + assert_refused("---\n? [a, b]\n: c\n---\n", "collection key") + assert_equal({ "1" => "a", "true" => "b", "name" => "c" }, load("---\n\"1\": a\n'true': b\nname: c\n---\n")) + end + + test "refuses nesting deeper than 64 levels before Psych builds objects" do + deep = (1..66).map { |level| "#{" " * (level - 1)}a:\n" }.join + assert_refused("---\n#{deep}#{" " * 66}x: 1\n---\n") + end + + test "stops parsing at the depth limit, before the scan costs the square of the nesting" do + nested = ->(levels, leaf) { "---\nname: a\ndescription: d\nx: #{"[" * levels}#{leaf}#{"]" * levels}\n---\n" } + + # The root mapping is level 1, so a scalar under 62 sequences and an empty 63rd sequence sit at level 64. + assert load(nested.call(62, "0")) + assert_refused(nested.call(63, "0")) + assert load(nested.call(63, "")) + assert_refused(nested.call(64, "")) + + # About 60 KiB, within the block limit; parsed whole before the depth was looked at, it took seconds. + started = Process.clock_gettime(Process::CLOCK_MONOTONIC) + assert_refused(nested.call(30_000, "0"), "30,000 levels") + assert_operator Process.clock_gettime(Process::CLOCK_MONOTONIC) - started, :<, 1 + end + + test "refuses plain scalars whose YAML 1.1 and 1.2 core-schema readings differ, and accepts their quoted forms" do + divergent = ["yes", "no", "on", "off", "Yes", "NO", "010", "0777", "08", "1:20", "12:30:45", "1_000", "0o10", "1e3", "1E3", "1e+3", ".5e3", "2001-01-01", ":sym"] + divergent.each do |scalar| + assert_refused("---\nvalue: #{scalar}\n---\n", "value #{scalar}") + assert_refused("---\n#{scalar}: 1\n---\n", "key #{scalar}") + assert_equal({ "value" => scalar }, load("---\nvalue: \"#{scalar}\"\n---\n"), "quoted #{scalar}") + end + end + + test "accepts plain scalars both schemas read the same way, with their resolved values" do + { + "y" => "y", + "n" => "n", + "True" => true, + "TRUE" => true, + "false" => false, + "null" => nil, + "Null" => nil, + "~" => nil, + "+1" => 1, + "-1" => -1, + "00" => 0, + "0x1F" => 31, + ".5" => 0.5, + "1." => 1.0, + "1.0" => 1.0, + "1.5e+3" => 1500.0, + "0.5e+3" => 500.0, + "-1.5e-3" => -0.0015, + "plain words" => "plain words", + }.each do |scalar, expected| + assert_equal({ "value" => expected }, load("---\nvalue: #{scalar}\n---\n"), scalar) + end + end + + test "refuses non-finite numbers, which JSON cannot carry" do + assert_refused("---\nvalue: .inf\n---\n") + assert_refused("---\nvalue: -.inf\n---\n") + assert_refused("---\nvalue: .nan\n---\n") + end + + test "compares normalized values with Integer and Float kept distinct and Hash keys as sets" do + assert FRONTMATTER.equal?({ "a" => 1, "b" => [1, { "c" => "d" }] }, { "b" => [1, { "c" => "d" }], "a" => 1 }) + refute FRONTMATTER.equal?({ "a" => 1 }, { "a" => 1.0 }) + refute FRONTMATTER.equal?({ "a" => 1 }, { "a" => 1, "b" => nil }) + refute FRONTMATTER.equal?({ "a" => [1, 2] }, { "a" => [2, 1] }) + refute FRONTMATTER.equal?({ "a" => "1" }, { "a" => 1 }) + refute FRONTMATTER.equal?({ "a" => nil }, { "a" => false }) + end + end + end +end diff --git a/test/mcp/skill/resource_entry_test.rb b/test/mcp/skill/resource_entry_test.rb new file mode 100644 index 00000000..ab97bd48 --- /dev/null +++ b/test/mcp/skill/resource_entry_test.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +require "test_helper" + +module MCP + class Skill + class ResourceEntryTest < ActiveSupport::TestCase + DIGEST = "sha256:#{"a" * 64}" + + test "holds the uri, digest, and size of one skill file and serializes them" do + entry = ResourceEntry.new(uri: "skill://pdf/SKILL.md", digest: DIGEST, size: 12) + + assert_equal "skill://pdf/SKILL.md", entry.uri + assert_equal DIGEST, entry.digest + assert_equal 12, entry.size + assert_equal({ uri: "skill://pdf/SKILL.md", digest: DIGEST, size: 12 }, entry.to_h) + assert_predicate entry, :frozen? + end + + test "from accepts an entry, a Hash with Symbol keys, and a Hash with String keys" do + entry = ResourceEntry.new(uri: "skill://pdf/SKILL.md", digest: DIGEST, size: 1) + + assert_same entry, ResourceEntry.from(entry) + assert_equal entry, ResourceEntry.from(uri: "skill://pdf/SKILL.md", digest: DIGEST, size: 1) + assert_equal entry, ResourceEntry.from("uri" => "skill://pdf/SKILL.md", "digest" => DIGEST, "size" => 1) + end + + test "from refuses other values, unknown keys, and a member given twice" do + error = assert_raises(ArgumentError) { ResourceEntry.from("skill://pdf/SKILL.md") } + assert_equal "A skill resource entry must be an MCP::Skill::ResourceEntry or a Hash (got String).", error.message + + error = assert_raises(ArgumentError) { ResourceEntry.from(uri: "skill://pdf/SKILL.md", digest: DIGEST, size: 1, mime: "x") } + assert_equal "A skill resource entry Hash may only hold uri, digest, and size (got :mime).", error.message + + error = assert_raises(ArgumentError) { ResourceEntry.from(uri: "skill://pdf/SKILL.md", "uri" => "x", digest: DIGEST, size: 1) } + assert_equal "A skill resource entry Hash gives uri twice.", error.message + end + + test "from_content computes the size and digest from the exact bytes served" do + content = "---\nname: pdf\n---\n" + entry = ResourceEntry.from_content(uri: "skill://pdf/SKILL.md", content: content) + + assert_equal content.bytesize, entry.size + assert_equal "sha256:#{Digest::SHA256.hexdigest(content)}", entry.digest + + binary = "\x89PNG\xFF".b + assert_equal "sha256:#{Digest::SHA256.hexdigest(binary)}", ResourceEntry.from_content(uri: "skill://pdf/logo.png", content: binary).digest + + error = assert_raises(ArgumentError) { ResourceEntry.from_content(uri: "skill://pdf/SKILL.md", content: 42) } + assert_equal "The value of content must be a String (got Integer).", error.message + end + + test "refuses a uri that is not a structurally valid skill URI" do + ["http://x/SKILL.md", "skill://", "skill://a//b", "skill://a/./b", "skill://a/../b", "skill://a/b".encode("UTF-16LE"), 42].each do |uri| + error = assert_raises(ArgumentError, uri.inspect) { ResourceEntry.new(uri: uri, digest: DIGEST, size: 1) } + assert_equal "The value of uri must be a skill:// URI with non-empty path segments.", error.message + end + end + + test "refuses a digest that is not sha256 over 64 lowercase hex characters" do + ["sha256:#{"A" * 64}", "sha256:#{"a" * 63}", "sha1:#{"a" * 64}", "a" * 64, DIGEST.encode("UTF-16LE"), nil].each do |digest| + error = assert_raises(ArgumentError, digest.inspect) { ResourceEntry.new(uri: "skill://pdf/SKILL.md", digest: digest, size: 1) } + assert_equal "The value of digest must be sha256: followed by 64 lowercase hex characters.", error.message + end + end + + test "refuses a size that is not a non-negative Integer" do + [-1, 1.5, "12", nil].each do |size| + error = assert_raises(ArgumentError, size.inspect) { ResourceEntry.new(uri: "skill://pdf/SKILL.md", digest: DIGEST, size: size) } + assert_equal "The value of size must be a non-negative Integer (got #{size.class}).", error.message + end + assert_equal 0, ResourceEntry.new(uri: "skill://pdf/SKILL.md", digest: DIGEST, size: 0).size + end + end + end +end diff --git a/test/mcp/skill_test.rb b/test/mcp/skill_test.rb new file mode 100644 index 00000000..676ea365 --- /dev/null +++ b/test/mcp/skill_test.rb @@ -0,0 +1,378 @@ +# frozen_string_literal: true + +require "test_helper" +require "digest" + +module MCP + class SkillTest < ActiveSupport::TestCase + SKILL_MD = "---\nname: pdf-processing\ndescription: Extract PDFs\n---\n\nBody.\n" + FORMS = "Forms\n" + + def digest(content) + "sha256:#{Digest::SHA256.hexdigest(content)}" + end + + def entry(uri, content) + { uri: uri, digest: digest(content), size: content.bytesize } + end + + def build(uri: "skill://pdf-processing/SKILL.md", frontmatter: { name: "pdf-processing", description: "Extract PDFs" }, resources: nil, meta: nil) + resources ||= [entry(uri, SKILL_MD), entry("skill://pdf-processing/references/FORMS.md", FORMS)] + Skill.new(uri: uri, frontmatter: frontmatter, resources: resources, meta: meta) + end + + test "exposes the SEP-2640 vocabulary" do + assert_equal "io.modelcontextprotocol/skills", Skill::EXTENSION_ID + assert_equal 512, Skill::MAX_RESOURCES + assert_equal 16 * 1024 * 1024, Skill::MAX_TOTAL_SIZE + assert_equal({ "io.modelcontextprotocol/skills" => {} }, Skill.capability) + end + + test "holds a frozen entry whose frontmatter has String keys and whose resources are entries" do + skill = build + + assert_equal "skill://pdf-processing/SKILL.md", skill.uri + assert_equal ["pdf-processing"], skill.skill_path + assert_equal "pdf-processing", skill.name + assert_equal({ "name" => "pdf-processing", "description" => "Extract PDFs" }, skill.frontmatter) + assert_equal ["skill://pdf-processing/SKILL.md", "skill://pdf-processing/references/FORMS.md"], skill.resources.map(&:uri) + assert_kind_of Skill::ResourceEntry, skill.resources.first + assert_equal SKILL_MD.bytesize + FORMS.bytesize, skill.total_size + refute_predicate skill, :dynamic? + assert_predicate skill, :frozen? + assert_predicate skill.frontmatter, :frozen? + assert_predicate skill.resources, :frozen? + assert_equal( + { + uri: "skill://pdf-processing/SKILL.md", + frontmatter: { "name" => "pdf-processing", "description" => "Extract PDFs" }, + resources: [ + { uri: "skill://pdf-processing/SKILL.md", digest: digest(SKILL_MD), size: SKILL_MD.bytesize }, + { uri: "skill://pdf-processing/references/FORMS.md", digest: digest(FORMS), size: FORMS.bytesize }, + ], + }, + skill.to_h, + ) + end + + test "copies the frontmatter so a caller mutating its Hash afterwards changes nothing" do + frontmatter = { name: "pdf-processing", description: "Extract PDFs", tags: ["a"] } + skill = build(frontmatter: frontmatter) + frontmatter[:tags] << "b" + frontmatter[:description] = "changed" + + assert_equal({ "name" => "pdf-processing", "description" => "Extract PDFs", "tags" => ["a"] }, skill.frontmatter) + end + + test "carries the _meta of the entry as a frozen copy under the frontmatter's JSON rules, or none at all" do + assert_nil build.meta + refute build.to_h.key?(:_meta) + + tags = ["a"] + skill = build(meta: { "com.example/origin": "catalog", "io.modelcontextprotocol.skills/tags": tags }) + tags << "b" + assert_equal({ "com.example/origin" => "catalog", "io.modelcontextprotocol.skills/tags" => ["a"] }, skill.meta) + assert_predicate skill.meta, :frozen? + assert_predicate skill.meta["io.modelcontextprotocol.skills/tags"], :frozen? + assert_equal skill.meta, skill.to_h[:_meta] + assert_equal({}, build(meta: {}).to_h[:_meta]) + + error = assert_raises(ArgumentError) { build(meta: "origin=catalog") } + assert_equal "The value of meta must be a Hash (got String).", error.message + error = assert_raises(ArgumentError) { build(meta: { when: Time.at(0) }) } + assert_equal "The value of meta must be JSON-representable (got Time).", error.message + error = assert_raises(ArgumentError) { build(meta: { 1 => "one" }) } + assert_equal "The value of meta keys must be Strings or Symbols (got Integer).", error.message + + remote = Skill.from_entry("uri" => "skill://pdf/SKILL.md", "frontmatter" => { "name" => "pdf", "description" => "d" }, "resources" => "dynamic", "_meta" => { "a" => 1 }) + assert_equal({ "a" => 1 }, remote.meta) + assert_predicate remote.meta, :frozen? + assert_equal({ "a" => 1 }, remote.to_h[:_meta]) + remote = Skill.from_entry(uri: "skill://pdf/SKILL.md", frontmatter: { name: "pdf", description: "d" }, resources: "dynamic", _meta: { a: 1 }) + assert_equal({ "a" => 1 }, remote.meta) + assert_nil Skill.from_entry("uri" => "skill://pdf/SKILL.md", "frontmatter" => { "name" => "pdf", "description" => "d" }, "resources" => "dynamic").meta + error = assert_raises(ArgumentError) { Skill.from_entry("uri" => "skill://pdf/SKILL.md", "frontmatter" => { "name" => "pdf", "description" => "d" }, "resources" => "dynamic", "_meta" => []) } + assert_equal "The value of meta must be a Hash (got Array).", error.message + end + + test "a dynamic skill lists no entries and has no total size" do + skill = build(resources: "dynamic") + + assert_predicate skill, :dynamic? + assert_equal "dynamic", skill.resources + assert_nil skill.total_size + assert_equal "dynamic", skill.to_h[:resources] + end + + test "allows nested skill paths and recovers the name from the last segment" do + skill = Skill.new( + uri: "skill://acme/billing/refunds/SKILL.md", + frontmatter: { name: "refunds", description: "Refund flow" }, + resources: [entry("skill://acme/billing/refunds/SKILL.md", "x")], + ) + + assert_equal ["acme", "billing", "refunds"], skill.skill_path + assert_equal "refunds", skill.name + end + + test "refuses a uri that does not name a SKILL.md under at least one segment" do + wide = "skill://pdf/SKILL.md".encode("UTF-16LE") + ["skill://SKILL.md", "skill://pdf/skill.md", "skill://pdf/SKILL.md/", "http://pdf/SKILL.md", "skill://pdf//SKILL.md", wide, nil].each do |uri| + error = assert_raises(ArgumentError, uri.inspect) { build(uri: uri, frontmatter: { name: "pdf", description: "d" }, resources: "dynamic") } + assert_equal "The value of uri must be a skill:///SKILL.md URI with non-empty path segments.", error.message + end + end + + test "refuses local segments outside the unreserved URI characters" do + error = assert_raises(ArgumentError) { build(uri: "skill://pdf%20tools/SKILL.md", frontmatter: { name: "pdf%20tools", description: "d" }, resources: "dynamic") } + assert_equal "The value of uri may only use letters, digits, -, ., _, and ~ in its segments.", error.message + end + + test "refuses frontmatter that is not a Hash with non-empty name and description" do + error = assert_raises(ArgumentError) { build(frontmatter: "name: pdf", resources: "dynamic") } + assert_equal "The value of frontmatter must be a Hash (got String).", error.message + + error = assert_raises(ArgumentError) { build(frontmatter: { description: "d" }, resources: "dynamic") } + assert_equal "The value of frontmatter must hold a non-empty String under name.", error.message + + error = assert_raises(ArgumentError) { build(frontmatter: { name: "pdf-processing", description: "" }, resources: "dynamic") } + assert_equal "The value of frontmatter must hold a non-empty String under description.", error.message + + error = assert_raises(ArgumentError) { build(frontmatter: { name: "other", description: "d" }, resources: "dynamic") } + assert_equal "The frontmatter name must equal the last segment of the skill path.", error.message + end + + test "holds a locally defined skill to the Agent Skills rules for name and description" do + named = ->(name) { build(uri: "skill://#{name}/SKILL.md", frontmatter: { name: name, description: "d" }, resources: "dynamic") } + + ["PDF", "pdf_tools", "pdf.tools", "-pdf", "pdf-", "pdf--tools", "a" * 65].each do |name| + error = assert_raises(ArgumentError, name) { named.call(name) } + assert_equal "The frontmatter name must be 1 to 64 lowercase letters, digits, and single hyphens between them.", error.message + end + ["pdf", "pdf-tools", "a1-b2-c3", "a" * 64].each { |name| assert_equal name, named.call(name).name } + + error = assert_raises(ArgumentError) { build(frontmatter: { name: "pdf-processing", description: "d" * 1025 }, resources: "dynamic") } + assert_equal "The frontmatter description must hold at most 1024 characters.", error.message + assert build(frontmatter: { name: "pdf-processing", description: "d" * 1024 }, resources: "dynamic") + + # A leading segment is a namespace, not a name, and an entry a server sent is not held to these rules. + assert build(uri: "skill://Acme_Corp/pdf/SKILL.md", frontmatter: { name: "pdf", description: "d" }, resources: "dynamic") + remote = Skill.from_entry( + "uri" => "skill://PDF_Tools/SKILL.md", + "frontmatter" => { "name" => "PDF_Tools", "description" => "d" * 2000 }, + "resources" => "dynamic", + ) + assert_equal "PDF_Tools", remote.name + end + + test "refuses frontmatter values JSON cannot carry, cycles, and keys that collide once stringified" do + error = assert_raises(ArgumentError) { build(frontmatter: { name: "pdf-processing", description: "d", when: Time.at(0) }, resources: "dynamic") } + assert_equal "The value of frontmatter must be JSON-representable (got Time).", error.message + + error = assert_raises(ArgumentError) { build(frontmatter: { name: "pdf-processing", description: "d", ratio: Float::NAN }, resources: "dynamic") } + assert_equal "The value of frontmatter must hold finite numbers.", error.message + + error = assert_raises(ArgumentError) { build(frontmatter: { name: "pdf-processing", description: "d", big: 2**53 }) } + assert_equal "The value of frontmatter must hold integers within 2**53.", error.message + assert_equal (2**53) - 1, build(frontmatter: { name: "pdf-processing", description: "d", big: (2**53) - 1 }).frontmatter["big"] + + cyclic = { name: "pdf-processing", description: "d" } + cyclic[:self] = cyclic + error = assert_raises(ArgumentError) { build(frontmatter: cyclic, resources: "dynamic") } + assert_equal "The value of frontmatter must not be cyclic.", error.message + + error = assert_raises(ArgumentError) { build(frontmatter: { name: "pdf-processing", description: "d", "x" => 1, x: 2 }, resources: "dynamic") } + assert_equal 'The value of frontmatter keys collide after stringification ("x").', error.message + + error = assert_raises(ArgumentError) { build(frontmatter: { name: "pdf-processing", description: "d", 1 => "one" }, resources: "dynamic") } + assert_equal "The value of frontmatter keys must be Strings or Symbols (got Integer).", error.message + end + + test "refuses frontmatter and meta strings JSON cannot encode, whatever encoding they are labeled with" do + binary = "\xFF".b + mislabeled = "\xFF".dup.force_encoding(Encoding::UTF_8) + refute_predicate mislabeled, :valid_encoding? + assert_predicate binary, :valid_encoding? + + # The second binary String holds the UTF-8 bytes of one character; only its label keeps `utf8_encodable?` from accepting it. + [binary, "\u00E9".b, mislabeled, "\x82".dup.force_encoding(Encoding::Shift_JIS)].each do |value| + error = assert_raises(ArgumentError, value.inspect) { build(frontmatter: { name: "pdf-processing", description: "d", note: value }, resources: "dynamic") } + assert_equal "The value of frontmatter must hold strings JSON can encode.", error.message + error = assert_raises(ArgumentError, value.inspect) { build(meta: { note: value }) } + assert_equal "The value of meta must hold strings JSON can encode.", error.message + end + error = assert_raises(ArgumentError) { build(frontmatter: { name: "pdf-processing", description: "d", binary => "x" }, resources: "dynamic") } + assert_equal "The value of frontmatter must hold strings JSON can encode.", error.message + error = assert_raises(ArgumentError) { build(meta: { binary.to_sym => "x" }) } + assert_equal "The value of meta must hold strings JSON can encode.", error.message + error = assert_raises(ArgumentError) { Skill.from_entry("uri" => "skill://pdf/SKILL.md", "frontmatter" => { "name" => "pdf", "description" => "d", "note" => binary }, "resources" => "dynamic") } + assert_equal "The value of frontmatter must hold strings JSON can encode.", error.message + + # Labeled encodings JSON can convert stay as given; the generated document carries the characters. + skill = build(frontmatter: { name: "pdf-processing", description: "d", note: "\u3042".encode(Encoding::Shift_JIS), ascii: "plain".b }, resources: "dynamic", meta: { note: "a".encode(Encoding::UTF_16LE) }) + assert_equal Encoding::Shift_JIS, skill.frontmatter["note"].encoding + generated = JSON.parse(JSON.generate(skill.to_h)) + assert_equal "\u3042", generated["frontmatter"]["note"] + assert_equal "plain", generated["frontmatter"]["ascii"] + assert_equal "a", generated["_meta"]["note"] + end + + test "refuses frontmatter nested deeper than JSON.parse reads by default" do + nested = ->(levels) { levels.times.inject("leaf") { |value, _| [value] } } + frontmatter = ->(levels) { { name: "pdf-processing", description: "d", metadata: nested.call(levels) } } + + # The frontmatter Hash is the first level. + assert build(frontmatter: frontmatter.call(Skill::Json::MAX_DEPTH - 1), resources: "dynamic") + error = assert_raises(ArgumentError) { build(frontmatter: frontmatter.call(Skill::Json::MAX_DEPTH), resources: "dynamic") } + assert_equal "The value of frontmatter must not nest deeper than 100 levels.", error.message + end + + test "refuses resources that are neither dynamic nor an Array, and entries that break the manifest rules" do + error = assert_raises(ArgumentError) { build(resources: { uri: "x" }) } + assert_equal 'The value of resources must be "dynamic" or an Array of entries (got Hash).', error.message + + error = assert_raises(ArgumentError) { build(resources: [entry("skill://pdf-processing/SKILL.md", "x"), "nope"]) } + assert_equal "resources[1]: A skill resource entry must be an MCP::Skill::ResourceEntry or a Hash (got String).", error.message + + error = assert_raises(ArgumentError) { build(resources: [entry("skill://pdf-processing/SKILL.md", "x"), entry("skill://pdf-processing-extra/x", "y")]) } + assert_equal 'resources[1]: The entry uri must lie under "skill://pdf-processing".', error.message + + error = assert_raises(ArgumentError) { build(resources: [entry("skill://pdf-processing/SKILL.md", "x"), entry("skill://pdf-processing/SKILL.md", "y")]) } + assert_equal "resources[1]: The entry uri is listed twice.", error.message + + error = assert_raises(ArgumentError) { build(resources: [entry("skill://pdf-processing/references/FORMS.md", "x")]) } + assert_equal "The value of resources must list the skill's own SKILL.md.", error.message + + # A nested skill's SKILL.md is a supporting file of the enclosing skill, so listing it is fine. + enclosing = build(resources: [entry("skill://pdf-processing/SKILL.md", "x"), entry("skill://pdf-processing/inner/SKILL.md", "y")]) + assert_equal ["skill://pdf-processing/SKILL.md", "skill://pdf-processing/inner/SKILL.md"], enclosing.resources.map(&:uri) + end + + test "refuses a local skill over the SEP limits" do + many = Array.new(512) { |index| entry("skill://pdf-processing/f#{index}", "x") } + [entry("skill://pdf-processing/SKILL.md", "x")] + error = assert_raises(ArgumentError) { build(resources: many) } + assert_equal "The value of resources must hold at most 512 entries.", error.message + + assert_equal 512, build(resources: many[1..-1]).resources.size + + big = [entry("skill://pdf-processing/SKILL.md", "x"), { uri: "skill://pdf-processing/big", digest: digest("x"), size: Skill::MAX_TOTAL_SIZE }] + error = assert_raises(ArgumentError) { build(resources: big) } + assert_equal "The value of resources must list at most 16777216 bytes in total.", error.message + end + + test "from_entry accepts a remote entry structurally, beyond the limits and the local character set" do + resources = Array.new(600) { |index| { "uri" => "skill://acme/r%20d/f#{index}", "digest" => digest("x"), "size" => 100_000 } } + resources << { "uri" => "skill://acme/r%20d/SKILL.md", "digest" => digest("x"), "size" => 1 } + resources << { "uri" => "skill://acme/r%20d/inner/SKILL.md", "digest" => digest("x"), "size" => 1 } + skill = Skill.from_entry("uri" => "skill://acme/r%20d/SKILL.md", "frontmatter" => { "name" => "r%20d", "description" => "d" }, "resources" => resources) + + assert_equal 602, skill.resources.size + assert_equal 60_000_002, skill.total_size + assert_predicate skill, :frozen? + + error = assert_raises(ArgumentError) { Skill.from_entry("uri" => "skill://acme/SKILL.md", "frontmatter" => { "name" => "acme", "description" => "d" }, "resources" => [{ "uri" => "skill://acme/x", "digest" => digest("x"), "size" => 1 }]) } + assert_equal "The value of resources must list the skill's own SKILL.md.", error.message + + error = assert_raises(ArgumentError) { Skill.from_entry("not a hash") } + assert_equal "A skill entry must be a Hash (got String).", error.message + end + + test "verify_resource checks the size then the digest of the bytes against the listed entry" do + skill = build + + assert Skill.verify_resource(skill, "skill://pdf-processing/references/FORMS.md", FORMS.b) + + error = assert_raises(Client::SkillVerificationError) { Skill.verify_resource(skill, "skill://pdf-processing/references/FORMS.md", "Forms!\n".b) } + assert_equal :size, error.reason + assert_equal "skill://pdf-processing/references/FORMS.md", error.uri + assert_equal "skill://pdf-processing/SKILL.md", error.skill_uri + + error = assert_raises(Client::SkillVerificationError) { Skill.verify_resource(skill, "skill://pdf-processing/references/FORMS.md", "Formz\n".b) } + assert_equal :digest, error.reason + + error = assert_raises(Client::SkillReadRefusedError) { Skill.verify_resource(skill, "skill://pdf-processing/other.md", "x") } + assert_equal :not_in_manifest, error.reason + + # RIGHT-TO-LEFT OVERRIDE, which `inspect` would leave as it is. + reordering = "skill://pdf-processing/#{[0x202E].pack("U")}other.md" + error = assert_raises(Client::SkillReadRefusedError) { Skill.verify_resource(skill, reordering, "x") } + assert_predicate error.message, :ascii_only? + assert_equal reordering, error.uri + + error = assert_raises(Client::SkillReadRefusedError) { Skill.verify_resource(build(resources: "dynamic"), "skill://pdf-processing/SKILL.md", "x") } + assert_equal :dynamic, error.reason + + assert_raises(ArgumentError) { Skill.verify_resource("skill", "skill://pdf-processing/SKILL.md", "x") } + assert_raises(ArgumentError) { Skill.verify_resource(skill, "skill://pdf-processing/SKILL.md", 1) } + end + + test "verify_frontmatter compares the fetched SKILL.md with the entry field by field" do + skill = build + + assert Skill.verify_frontmatter(skill, SKILL_MD) + assert_raises(ArgumentError) { Skill.verify_frontmatter(nil, SKILL_MD) } + assert_raises(ArgumentError) { Skill.verify_frontmatter(skill, nil) } + assert Skill.verify_frontmatter(skill, "---\ndescription: Extract PDFs\nname: pdf-processing\n---\nother body\n") + + error = assert_raises(Client::SkillVerificationError) { Skill.verify_frontmatter(skill, SKILL_MD.sub("Extract PDFs", "Exfiltrate credentials")) } + assert_equal :frontmatter, error.reason + assert_equal "skill://pdf-processing/SKILL.md", error.uri + + error = assert_raises(Client::SkillVerificationError) { Skill.verify_frontmatter(skill, "---\nname: pdf-processing\ndescription: Extract PDFs\nextra: 1\n---\n") } + assert_equal :frontmatter, error.reason + + error = assert_raises(Client::SkillVerificationError) { Skill.verify_frontmatter(skill, "no frontmatter here") } + assert_equal :parse, error.reason + + error = assert_raises(Client::SkillVerificationError) { Skill.verify_frontmatter(skill, "---\nname: pdf-processing\ndescription: Extract PDFs\nflag: yes\n---\n") } + assert_equal :parse, error.reason + refute_includes error.message, "yes" + end + + test "local resource URIs are held to the unreserved character set, remote ones only to the structure" do + error = assert_raises(ArgumentError) do + build(resources: [entry("skill://pdf-processing/SKILL.md", "x"), entry("skill://pdf-processing/My%20Forms.md", "y")]) + end + assert_equal "resources[1]: The entry uri may only use letters, digits, -, ., _, and ~ in its segments.", error.message + + remote = Skill.from_entry( + "uri" => "skill://pdf-processing/SKILL.md", + "frontmatter" => { "name" => "pdf-processing", "description" => "d" }, + "resources" => [ + entry("skill://pdf-processing/SKILL.md", "x").to_h, + { "uri" => "skill://pdf-processing/My%20Forms.md", "digest" => digest("y"), "size" => 1, "mimeType" => "text/markdown" }, + ], + ) + assert_equal ["skill://pdf-processing/SKILL.md", "skill://pdf-processing/My%20Forms.md"], remote.resources.map(&:uri) + + error = assert_raises(ArgumentError) { build(resources: [entry("skill://pdf-processing/SKILL.md", "x").to_h.merge(mimeType: "text/markdown")]) } + assert_match(/may only hold uri, digest, and size/, error.message) + end + + test "Uri derives candidate skill URIs from the segments and applies the segment boundary" do + uri = Skill::Uri + + assert uri.structural?("skill://a/b") + refute uri.structural?("skill://a//b") + refute uri.structural?("skill://") + assert uri.manifest_uri?("skill://a/b/SKILL.md") + refute uri.manifest_uri?("skill://SKILL.md") + refute uri.manifest_uri?("skill://a/b/skill.md") + assert_equal ["a", "b"], uri.skill_path("skill://a/b/SKILL.md") + assert_equal "skill://a/b", uri.root(["a", "b"]) + assert uri.under?("skill://a/b/x", "skill://a/b") + refute uri.under?("skill://ab/x", "skill://a") + refute uri.under?("skill://a/b", "skill://a/b") + assert_equal ["skill://a/SKILL.md", "skill://a/b/SKILL.md", "skill://a/b/c/SKILL.md"], uri.candidate_skill_uris("skill://a/b/c/file.md") + assert_equal ["skill://a/SKILL.md", "skill://a/b/SKILL.md"], uri.candidate_skill_uris("skill://a/b/SKILL.md") + assert_empty uri.candidate_skill_uris("skill://a") + assert_empty uri.candidate_skill_uris("http://a/b") + + assert uri.resolvable?("skill://#{Array.new(uri::MAX_RESOLVED_SEGMENTS, "a").join("/")}") + refute uri.resolvable?("skill://#{Array.new(uri::MAX_RESOLVED_SEGMENTS + 1, "a").join("/")}") + prefix = "skill://a/" + assert uri.resolvable?(prefix + "x" * (uri::MAX_RESOLVED_BYTES - prefix.bytesize)) + refute uri.resolvable?(prefix + "x" * (uri::MAX_RESOLVED_BYTES - prefix.bytesize + 1)) + end + end +end