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