diff --git a/docs/advanced/skills.md b/docs/advanced/skills.md index 18d3ae7825..a24db2fa76 100644 --- a/docs/advanced/skills.md +++ b/docs/advanced/skills.md @@ -17,10 +17,13 @@ skim that page first. `Skills` gives you the **protocol** primitives: request/response handling, capability advertisement, and SEP-2640 conformance validation. - It does **not** discover, read, or hash skills from a filesystem. You supply handlers that - answer from wherever your catalog actually lives — a database, a generated index, an in-memory - list, or a directory you walk yourself — and you serve each skill's files as ordinary resources - through `MCPServer.add_resource` or an `@mcp.resource(...)` template handler. + On its own it doesn't discover, read, or hash anything. You supply handlers that answer from + wherever your catalog lives — a database, a generated index, an in-memory list — and you serve + each skill's files as ordinary resources through `MCPServer.add_resource` or an + `@mcp.resource(...)` template handler. + + If your skills live in a directory, skip the handlers and use `SkillCatalog`. See + [Serving skills from a directory](#serving-skills-from-a-directory). ## Serving a skill @@ -116,6 +119,207 @@ capabilities. Omitting it advertises neither the setting nor the method — a cl `read_directory` raises before it sends anything if the connected server hasn't advertised the setting. +## Serving skills from a directory + +Most of the time your skills are just a folder on disk. For that, you don't need to write handlers +or register each file: `SkillCatalog` reads the folder and answers everything for you. + +Say you have this layout: + +```text +skills/ <-- the path you give FilesystemSkillProvider +└── git-workflow/ <-- one skill: a folder named after the skill + ├── SKILL.md <-- required: YAML frontmatter, then instructions + └── references/ + └── conventions.md <-- any other files the skill needs +``` + +The `SKILL.md` starts with the skill's `name` and `description`, and `name` must match the +folder name: + +```markdown +--- +name: git-workflow +description: Follow this team's Git conventions for branching and commits +--- + +Branch from `main` using `type/short-description`. ... +``` + +Load it and pass it to your server: + +```python title="server.py" hl_lines="11-15" +--8<-- "docs_src/skills/tutorial002.py" +``` + +That's the whole server. `skills/list`, `skills/get`, and `resources/directory/read` are answered +from the folder, and every file in it is served as a `skill://` resource. + +* `FilesystemSkillProvider(root)` tells the catalog where the files are. +* `parse_frontmatter` turns the YAML between the `---` lines of a `SKILL.md` into a dict. The SDK + doesn't ship a YAML parser, so bring your own. This example uses + [PyYAML](https://pypi.org/project/PyYAML/) (`uv add pyyaml`). +* `directory_read=True` also serves `resources/directory/read` (more on that + [below](#directory-reads-from-a-catalog)). + +`SkillCatalog.load()` is a coroutine, so build the server inside a running event loop, as +`main()` does above. + +### How skills are found + +The path you give `FilesystemSkillProvider` is the **root**: a folder that holds one folder per +skill. A fuller example: + +```text +skills/ <-- FilesystemSkillProvider(Path("skills")) +├── git-workflow/ <-- a skill, served as skill://git-workflow/SKILL.md +│ ├── SKILL.md +│ ├── references/ +│ │ └── conventions.md +│ └── scripts/ +│ └── check-branch.sh +├── pdf-processing/ <-- another skill +│ ├── SKILL.md +│ └── assets/ +│ └── template.pdf +├── .git/ <-- ignored: names starting with `.` are skipped +├── README.md <-- not served: it is outside any skill folder +└── draft/ <-- not a skill: it has no SKILL.md + └── notes.txt +``` + +A skill is a directory that contains a `SKILL.md`. Its files are everything beneath that +directory, including the files of any skill nested inside it, which is also listed as a skill in +its own right. + +Each file's URI is `skill://` followed by its path, with every segment percent-encoded. Files and +directories whose names start with `.` are ignored entirely, and files outside any skill directory +are not served. + +!!! warning + Point the provider at the folder that **contains** your skill folders, not at a skill folder + itself. A skill's URI is built from its path under the root, and its last segment must match + the skill's `name`. With `FilesystemSkillProvider(Path("skills/git-workflow"))`, the + `SKILL.md` sits directly in the root, so it has no directory name and the skill is skipped + (an error with `strict=True`). Use `FilesystemSkillProvider(Path("skills"))` instead. + +!!! note + The catalog reads and hashes every file **once**, when `load()` runs, and serves that snapshot. + That's how the `digest` and `size` in each skill's manifest are guaranteed to match what + `resources/read` returns. + + It also means the catalog is fixed for the lifetime of the server. If a file changes + afterwards, reading it fails with an internal error (`-32603`) until you restart the server or + create a new catalog. And because `load()` reads every file, a large asset directory makes + startup slower. + +### Directory reads from a catalog + +Pass `directory_read=True` and every directory that contains a served file can be read, including +directories above a skill such as `skill://acme`. A directory lists its direct children; +subdirectories come back with the MIME type `inode/directory`. Empty directories are not served. + +### Caching hints + +`ttl_ms` and `cache_scope` set the `ttlMs` and `cacheScope` values on every result: + +```python +async def build_catalog(root: Path) -> SkillCatalog: + return await SkillCatalog.load( + FilesystemSkillProvider(root), + parse_frontmatter=yaml.safe_load, + ttl_ms=60_000, + cache_scope="public", + ) +``` + +They follow the rules in [Protocol version and caching](#protocol-version-and-caching). A negative +`ttl_ms` raises `ValueError`. + +### Pagination + +By default every entry comes back in one response. If your catalog is big, set `page_size`: + +```python +async def build_catalog(root: Path) -> SkillCatalog: + return await SkillCatalog.load( + FilesystemSkillProvider(root), + parse_frontmatter=yaml.safe_load, + directory_read=True, + page_size=50, + ) +``` + +Each response then holds at most 50 entries and carries a `nextCursor` while more remain. +`SkillsClient.list_skills()` and `read_directory()` follow it for you. A skill entry is never split +across pages. + +The cursor is opaque, and the catalog keeps no record of the cursors it has handed out. It accepts +a cursor that marks a page boundary of its listing. A cursor that is malformed, out of range, or +not on a page boundary is rejected with Invalid params (`-32602`), and so is any cursor when +`page_size` isn't set. A `page_size` below 1 raises `ValueError`. + +### Invalid skills + +A skill is invalid when its `SKILL.md` has no frontmatter block, has an unterminated block, can't +be parsed, or fails the SEP-2640 requirements for `name` and `description`. A `SKILL.md` placed +directly in the root is also invalid, because the skill has no directory name. + +By default (`strict=False`) an invalid skill is left out and a warning is logged, so one bad skill +doesn't take the server down. Pass `strict=True` to make `load()` raise a single `ValueError` that +lists every invalid skill, which is handy in CI. + +The frontmatter is returned in `skills/list` and `skills/get` as written, including keys the SDK +doesn't interpret. Values are converted to JSON, so YAML dates become ISO 8601 strings. A value with +no JSON form, such as NaN, makes the skill invalid. + +A skill whose manifest exceeds the 512 files or 16 MiB recommended by SEP-2640 is still served, +and a warning is logged. + +### File content + +`SKILL.md` and other Markdown files are served as `text/markdown`, and YAML files as +`application/yaml`. The MIME type of any other file is determined with `mimetypes`, with +`application/octet-stream` as the fallback. + +File content is returned unchanged. A file whose bytes are valid UTF-8 is returned as text, +including any byte order mark and line endings; any other file is returned as a base64 blob. In +both cases the content matches the `digest` and `size` in the skill's manifest. Each read is +verified against the values recorded at load time, and a file that has changed is reported as an +internal error (`-32603`) until you restart the server or create a new catalog. + +`load()` reads and hashes every file of every valid skill once, so a large asset directory makes +startup slower and temporarily uses memory for the largest file. + +### Providers + +A `SkillProvider` supplies the files to the catalog. It is a `typing.Protocol`, so an +implementation does not need to subclass it. + +```python +class SkillProvider(Protocol): + async def list_files(self) -> Sequence[str]: ... + async def read_file(self, path: str) -> bytes: ... +``` + +`list_files()` returns the path of every file it serves, recursively and without directories. It +may leave files out; `FilesystemSkillProvider` omits any name that starts with `.`. `read_file()` +returns the raw bytes of one file. Paths use `/` as the separator and are relative to the root of +the provider. A provider can therefore be backed by any storage, such as an archive, a database, +or object storage. + +`load()` raises `ValueError` if the provider lists a path that is empty, absolute, contains a `.` +or `..` segment, is listed more than once, or lies beneath another listed file. This applies +regardless of `strict`, because it indicates an error in the provider rather than in a skill. + +`FilesystemSkillProvider(root)` is the provider for a local directory. It serves regular files +only. Symbolic links and Windows junctions found while listing are omitted, with a warning, and +ones found while reading are refused, so a skill directory doesn't expose files outside itself. + +The root is trusted. This is a best-effort check, not a defence against someone who can modify the +directory while the server runs. As a backstop, the catalog refuses to serve any file whose bytes +no longer match the digest taken at `load()`. Blocking file system calls run in a worker thread. + ## Protocol version and caching In protocol version `2026-07-28` and later, `skills/list`, `skills/get`, and @@ -128,9 +332,15 @@ You don't have to branch on protocol version yourself. ## What this SDK doesn't do -`Skills` is a protocol adapter, not a skills provider. It has no opinion on where a skill's bytes -live, how they're indexed, or when a catalog is refreshed — that's for a higher-level library, or -your own handler, to decide. +`Skills` is a protocol adapter, not a skills store. It has no opinion on where a skill's bytes +live or when a catalog is refreshed. + +`SkillCatalog` covers the common case of a directory (or any other file source behind a +`SkillProvider`), with a few limits: + +* It doesn't include a YAML parser. `parse_frontmatter` supplies one. +* The catalog isn't reloaded. To publish changes, create a new catalog. +* Empty directories aren't served by directory reads. -If you're looking for "scan this directory and serve whatever's in it," you're looking for a -provider built on top of `Skills`, not `Skills` itself. +For anything else, such as a database or a catalog that changes while the server runs, write your +own handlers for `Skills`. diff --git a/docs_src/skills/skills/git-workflow/SKILL.md b/docs_src/skills/skills/git-workflow/SKILL.md new file mode 100644 index 0000000000..c666988821 --- /dev/null +++ b/docs_src/skills/skills/git-workflow/SKILL.md @@ -0,0 +1,9 @@ +--- +name: git-workflow +description: Follow this team's Git conventions for branching and commits +--- + +Branch from `main` using `type/short-description`. Write commit subjects in the +imperative mood, under 72 characters. + +See [references/conventions.md](references/conventions.md) for the full list. diff --git a/docs_src/skills/skills/git-workflow/references/conventions.md b/docs_src/skills/skills/git-workflow/references/conventions.md new file mode 100644 index 0000000000..b996285661 --- /dev/null +++ b/docs_src/skills/skills/git-workflow/references/conventions.md @@ -0,0 +1,5 @@ +# Conventions + +- Branch names: `feat/`, `fix/` or `docs/` followed by a short description. +- One logical change per commit. +- Reference the issue number in the commit body, never in the subject. diff --git a/docs_src/skills/tutorial002.py b/docs_src/skills/tutorial002.py new file mode 100644 index 0000000000..f9d7c10b34 --- /dev/null +++ b/docs_src/skills/tutorial002.py @@ -0,0 +1,25 @@ +from pathlib import Path + +import anyio +import yaml + +from mcp.server.mcpserver import MCPServer +from mcp.server.skills import FilesystemSkillProvider, SkillCatalog + + +async def build_server(root: Path) -> MCPServer: + skills_catalog = await SkillCatalog.load( + FilesystemSkillProvider(root), + parse_frontmatter=yaml.safe_load, + directory_read=True, + ) + return MCPServer("MCP-over-skills", extensions=[skills_catalog]) + + +async def main() -> None: + server = await build_server(Path(__file__).parent / "skills") + await server.run_stdio_async() + + +if __name__ == "__main__": + anyio.run(main) diff --git a/src/mcp/server/skills/__init__.py b/src/mcp/server/skills/__init__.py new file mode 100644 index 0000000000..7d4b504b4b --- /dev/null +++ b/src/mcp/server/skills/__init__.py @@ -0,0 +1,39 @@ +"""The Skills extension (`io.modelcontextprotocol/skills`, SEP-2640). + +SEP-2640 defines a convention for serving Agent Skills over MCP using the +Resources primitive: a skill is a directory of files, conventionally exposed +under the `skill://` scheme, and enumerated and fetched through two required +methods (`skills/list`, `skills/get`) plus one optional one +(`resources/directory/read`). See +https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2640. + +`Skills` is the protocol-level plumbing: request/response handling, SEP-2640 +conformance validation, and capability advertisement. A server author supplies +handlers that answer `skills/list`/`skills/get`/`resources/directory/read` +however their catalog is stored, and serves the underlying `skill://` file +content through the server's ordinary resource-registration APIs. + +To serve skills from a directory (or any other source of files) without writing +handlers, use `SkillCatalog` with a `SkillProvider` such as `FilesystemSkillProvider`. + + async def list_skills(ctx, params): + return ListSkillsResult(skills=[...]) + + async def get_skill(ctx, params): + if params.uri != "skill://git-workflow/SKILL.md": + raise MCPError(code=INVALID_PARAMS, message="unknown skill") + return GetSkillResult(skill=...) + + mcp = MCPServer("catalog", extensions=[Skills(list_skills=list_skills, get_skill=get_skill)]) +""" + +# The handler aliases were importable from `mcp.server.skills` when it was a single module. +from mcp.server.skills._extension import GetSkillHandler as GetSkillHandler +from mcp.server.skills._extension import ListSkillsHandler as ListSkillsHandler +from mcp.server.skills._extension import ReadDirectoryHandler as ReadDirectoryHandler +from mcp.server.skills._extension import Skills +from mcp.server.skills.catalog import SkillCatalog +from mcp.server.skills.filesystem import FilesystemSkillProvider +from mcp.server.skills.provider import SkillProvider + +__all__ = ["Skills", "SkillCatalog", "SkillProvider", "FilesystemSkillProvider"] diff --git a/src/mcp/server/skills.py b/src/mcp/server/skills/_extension.py similarity index 97% rename from src/mcp/server/skills.py rename to src/mcp/server/skills/_extension.py index 36eee9fa5c..0d1ad12b62 100644 --- a/src/mcp/server/skills.py +++ b/src/mcp/server/skills/_extension.py @@ -9,11 +9,12 @@ This module provides the protocol-level plumbing only: request/response handling, SEP-2640 conformance validation, and capability advertisement. It -does not discover, read, or hash skills from a filesystem — a server author -supplies handlers that answer `skills/list`/`skills/get`/`resources/directory/read` +does not discover, read, or hash skills itself — a server author supplies +handlers that answer `skills/list`/`skills/get`/`resources/directory/read` however their catalog is stored, and serves the underlying `skill://` file content through the server's ordinary resource-registration APIs -(`MCPServer.add_resource`, or an `@mcp.resource(...)` template). +(`MCPServer.add_resource`, or an `@mcp.resource(...)` template). `SkillCatalog` +builds on `Skills` to do the discovery for a `SkillProvider`. async def list_skills(ctx, params): return ListSkillsResult(skills=[...]) diff --git a/src/mcp/server/skills/catalog.py b/src/mcp/server/skills/catalog.py new file mode 100644 index 0000000000..e11ebbd79a --- /dev/null +++ b/src/mcp/server/skills/catalog.py @@ -0,0 +1,459 @@ +"""`SkillCatalog`: a `Skills` extension backed by a `SkillProvider`.""" + +from __future__ import annotations + +import base64 +import hashlib +import json +import logging +import mimetypes +import re +from bisect import bisect_left +from collections.abc import Callable, Mapping, Sequence +from pathlib import PurePosixPath +from typing import Any, Literal, Self, TypeVar +from urllib.parse import quote + +import anyio.to_thread +from mcp_types import INVALID_PARAMS, CacheableResult +from mcp_types import Resource as ResourceEntry +from pydantic import BaseModel, PrivateAttr, TypeAdapter, ValidationError +from pydantic_core import to_jsonable_python + +from mcp.server.context import ServerRequestContext +from mcp.server.extension import ResourceBinding +from mcp.server.mcpserver.exceptions import ResourceError +from mcp.server.mcpserver.resources import Resource +from mcp.server.skills._extension import Skills +from mcp.server.skills.provider import SkillProvider +from mcp.shared.exceptions import MCPError +from mcp.shared.skills import ( + MAX_RESOURCES_PER_SKILL, + MAX_TOTAL_SIZE, + Frontmatter, + GetSkillParams, + GetSkillResult, + ListSkillsParams, + ListSkillsResult, + ReadDirectoryParams, + ReadDirectoryResult, + Skill, + SkillResource, +) + +__all__ = ["SkillCatalog"] + +logger = logging.getLogger(__name__) + +_SKILL_FILE = "SKILL.md" +_DIRECTORY_MIME_TYPE = "inode/directory" +_FRONTMATTER_DELIMITER = "---" +_MIME_TYPE_OVERRIDES = { + ".md": "text/markdown", + ".markdown": "text/markdown", + ".yaml": "application/yaml", + ".yml": "application/yaml", +} + + +class _Fingerprint(BaseModel, frozen=True): + digest: str + size: int + + +_FRONTMATTER_ADAPTER: TypeAdapter[Frontmatter] = TypeAdapter(Frontmatter) + + +class _SkillFileResource(Resource): + """One file of a skill, read from the provider on every `resources/read`. + + The bytes are checked against the digest taken at load time, so a manifest + entry never describes content other than what is served. + """ + + _provider: SkillProvider = PrivateAttr() + _path: str = PrivateAttr() + _fingerprint: _Fingerprint = PrivateAttr() + + def __init__(self, *, provider: SkillProvider, path: str, fingerprint: _Fingerprint, **data: Any) -> None: + super().__init__(**data) + self._provider = provider + self._path = path + self._fingerprint = fingerprint + + @property + def size(self) -> int: + return self._fingerprint.size + + async def read(self) -> str | bytes: + data = await self._provider.read_file(self._path) + if _fingerprint(data) != self._fingerprint: + raise ResourceError(f"{self.uri} changed since the catalog was loaded") + try: + return data.decode("utf-8") + except UnicodeDecodeError: + return data + + +class SkillCatalog(Skills): + """A `Skills` extension that serves the skills found in a `SkillProvider`. + + Build one with `SkillCatalog.load()`, which scans the provider once; the + catalog is then fixed for the server's lifetime. Every file of every valid + skill becomes a `skill://` resource, so `skills/list` and `skills/get` + manifests always match what `resources/read` returns. + + Pass it to `MCPServer(extensions=[catalog])`. + """ + + def __init__( + self, + *, + skills: Sequence[Skill], + files: Mapping[str, Resource], + directories: Mapping[str, Sequence[ResourceEntry]] | None, + ttl_ms: int, + cache_scope: Literal["public", "private"], + page_size: int | None, + ) -> None: + """Assemble a catalog from already-validated parts. Use `SkillCatalog.load()`, which builds them. + + Raises: + ValidationError: If `ttl_ms` is negative. + """ + super().__init__( + list_skills=self._list_handler, + get_skill=self._get_handler, + read_directory=self._read_directory_handler if directories is not None else None, + ) + self._catalog_skills = tuple(skills) + self._by_uri = {skill.uri: skill for skill in skills} + self._files = dict(files) + self._directories = directories + self._cache = CacheableResult(ttl_ms=ttl_ms, cache_scope=cache_scope) + self._page_size = page_size + + @classmethod + async def load( + cls, + provider: SkillProvider, + *, + parse_frontmatter: Callable[[str], Mapping[str, Any]], + strict: bool = False, + directory_read: bool = False, + ttl_ms: int = 0, + cache_scope: Literal["public", "private"] = "private", + page_size: int | None = None, + ) -> Self: + """Scan `provider` and build a catalog of the skills it holds. + + A skill is a directory containing a `SKILL.md`; the skill's files are everything + beneath that directory, including files of skills nested inside it. Paths with a + segment starting with `.` are ignored entirely. + + Args: + provider: Where the skill files live. + parse_frontmatter: Parses the text between the `---` lines of a `SKILL.md` (for + example `yaml.safe_load`). It must return a mapping with `str` keys; the + SDK deliberately has no YAML dependency of its own. + strict: When `False`, a skill that is not valid (bad frontmatter, a name that + doesn't match its directory, ...) is skipped with a warning. When `True`, + `load()` raises instead. + directory_read: Also serve `resources/directory/read`, listing every directory + that holds a served file. + ttl_ms: `ttlMs` cache hint (SEP-2549) set on `skills/list`, `skills/get` and + directory reads. + cache_scope: `cacheScope` cache hint set on the same results. + page_size: The maximum number of entries in one `skills/list` or directory read + page. When `None`, every entry is returned in a single response. A skill entry + is never split across pages. + + Raises: + ValueError: If `strict` is set and any skill is invalid, if the provider + lists a path that is empty, absolute, contains a `.` or `..` segment, appears + twice, or lies beneath another listed file, or if `ttl_ms` is negative or + `page_size` is less than 1. + OSError: If the provider fails to list or read a file. + """ + if page_size is not None and page_size < 1: + raise ValueError("page_size must be at least 1") + if ttl_ms < 0: + raise ValueError("ttl_ms must not be negative") + paths = _check_paths(list(await provider.list_files())) + visible = sorted(path for path in paths if not any(segment.startswith(".") for segment in path.split("/"))) + skill_paths = [path for path in visible if path.split("/")[-1] == _SKILL_FILE] + + skills: dict[str, Skill] = {} + served: set[str] = set() + problems: list[str] = [] + skill_mds: dict[str, bytes] = {} + fingerprints: dict[str, _Fingerprint] = {} + for skill_path in skill_paths: + if skill_path == _SKILL_FILE: + problems.append(f"{skill_path}: a skill needs a directory name") + continue + skill_mds[skill_path] = await provider.read_file(skill_path) + fingerprints[skill_path] = _fingerprint(skill_mds[skill_path]) + + for skill_path, skill_md in skill_mds.items(): + try: + skill, skill_files = await _load_skill( + provider, parse_frontmatter, skill_path, skill_md, visible, fingerprints + ) + except ValueError as exc: + problems.append(f"{skill_path}: {exc}") + continue + skills[skill_path] = skill + served.update(skill_files) + + if problems: + if strict: + raise ValueError("invalid skills:\n" + "\n".join(problems)) + for problem in problems: + logger.warning("Skipping invalid skill: %s", problem) + + # `mimetypes` reads system files the first time it is used, which must not block the event loop. + mime_types = await anyio.to_thread.run_sync(_guess_mime_types, sorted(served)) + files = { + path: _file_resource( + provider, + path, + fingerprints[path], + mime_types[path], + skills[path].frontmatter if path in skills else None, + ) + for path in sorted(served) + } + return cls( + skills=sorted(skills.values(), key=lambda skill: skill.uri), + files=files, + directories=_directory_index(files) if directory_read else None, + ttl_ms=ttl_ms, + cache_scope=cache_scope, + page_size=page_size, + ) + + def resources(self) -> Sequence[ResourceBinding]: + """One binding per served file, sorted by URI.""" + return [ResourceBinding(resource=file) for file in sorted(self._files.values(), key=lambda f: f.uri)] + + async def _list_handler(self, ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> ListSkillsResult: + page, next_cursor = _paginate(self._catalog_skills, params.cursor, self._page_size) + return ListSkillsResult( + skills=page, + next_cursor=next_cursor, + ttl_ms=self._cache.ttl_ms, + cache_scope=self._cache.cache_scope, + ) + + async def _get_handler(self, ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> GetSkillResult: + skill = self._by_uri.get(params.uri) + if skill is None: + raise MCPError(code=INVALID_PARAMS, message=f"unknown skill: {params.uri}") + return GetSkillResult(skill=skill, ttl_ms=self._cache.ttl_ms, cache_scope=self._cache.cache_scope) + + async def _read_directory_handler( + self, ctx: ServerRequestContext[Any, Any], params: ReadDirectoryParams + ) -> ReadDirectoryResult: + assert self._directories is not None + children = self._directories.get(params.uri) + if children is None: + raise MCPError(code=INVALID_PARAMS, message=f"unknown directory: {params.uri}") + page, next_cursor = _paginate(children, params.cursor, self._page_size) + return ReadDirectoryResult( + resources=page, + next_cursor=next_cursor, + ttl_ms=self._cache.ttl_ms, + cache_scope=self._cache.cache_scope, + ) + + +_T = TypeVar("_T") + + +def _encode_cursor(offset: int) -> str: + return base64.urlsafe_b64encode(str(offset).encode("ascii")).decode("ascii") + + +def _paginate(items: Sequence[_T], cursor: str | None, page_size: int | None) -> tuple[list[_T], str | None]: + """Return one page of `items` and the cursor for the next page, if any. + + The catalog is immutable, so a cursor is just an offset into `items`, and no state is kept + about which cursors were issued. Only an offset on a page boundary, in canonical form, is + accepted; anything else is Invalid params. + """ + if page_size is None: + if cursor is not None: + raise MCPError(code=INVALID_PARAMS, message="this server does not paginate; unexpected cursor") + return list(items), None + start = 0 + if cursor is not None: + try: + start = int(base64.urlsafe_b64decode(cursor.encode("ascii"))) + except ValueError: + start = -1 + if not (0 < start < len(items) and start % page_size == 0 and _encode_cursor(start) == cursor): + raise MCPError(code=INVALID_PARAMS, message="invalid cursor") + end = start + page_size + return list(items[start:end]), _encode_cursor(end) if end < len(items) else None + + +def _check_paths(paths: list[str]) -> list[str]: + seen: set[str] = set() + for path in paths: + if any(segment in ("", ".", "..") for segment in path.split("/")): + raise ValueError(f"provider listed an invalid path {path!r}") + if path in seen: + raise ValueError(f"provider listed {path!r} more than once") + seen.add(path) + for path in paths: + segments = path.split("/") + for depth in range(1, len(segments)): + if "/".join(segments[:depth]) in seen: + raise ValueError(f"provider listed {path!r} beneath {'/'.join(segments[:depth])!r}, which is a file") + return paths + + +def _paths_under(sorted_paths: Sequence[str], prefix: str) -> list[str]: + """Return the paths that start with `prefix`; they are contiguous in sorted order, so no full scan is needed.""" + start = bisect_left(sorted_paths, prefix) + end = start + while end < len(sorted_paths) and sorted_paths[end].startswith(prefix): + end += 1 + return list(sorted_paths[start:end]) + + +def _fingerprint(data: bytes) -> _Fingerprint: + return _Fingerprint(digest=hashlib.sha256(data).hexdigest(), size=len(data)) + + +async def _load_skill( + provider: SkillProvider, + parse_frontmatter: Callable[[str], Mapping[str, Any]], + skill_path: str, + skill_md: bytes, + sorted_visible: Sequence[str], + fingerprints: dict[str, _Fingerprint], +) -> tuple[Skill, list[str]]: + """Build the `Skill` whose `SKILL.md` is `skill_path`, along with the paths of its files. + + Raises: + ValueError: If the skill is not valid. + """ + frontmatter = _parse_frontmatter(skill_md, parse_frontmatter) + root = skill_path.removesuffix(_SKILL_FILE) + skill_files = _paths_under(sorted_visible, root) + for path in skill_files: + if path not in fingerprints: + fingerprints[path] = _fingerprint(await provider.read_file(path)) + resources = sorted( + ( + SkillResource(uri=_uri(path), digest=f"sha256:{fingerprints[path].digest}", size=fingerprints[path].size) + for path in skill_files + ), + key=lambda resource: resource.uri, + ) + try: + skill = Skill(uri=_uri(skill_path), frontmatter=frontmatter, resources=resources) + except ValidationError as exc: + raise ValueError("; ".join(error["msg"].removeprefix("Value error, ") for error in exc.errors())) from None + _warn_over_limits(skill.uri, resources) + return skill, skill_files + + +def _parse_frontmatter(data: bytes, parse: Callable[[str], Mapping[str, Any]]) -> dict[str, Any]: + try: + text = data.decode("utf-8").removeprefix("\ufeff") + except UnicodeDecodeError: + raise ValueError("SKILL.md is not valid UTF-8") from None + lines = re.split(r"\r?\n", text) + if lines[0] != _FRONTMATTER_DELIMITER: + raise ValueError("SKILL.md must start with a '---' frontmatter line") + try: + end = lines.index(_FRONTMATTER_DELIMITER, 1) + except ValueError: + raise ValueError("SKILL.md frontmatter has no closing '---' line") from None + try: + parsed = parse("\n".join(lines[1:end])) + except Exception as exc: # the parser is caller-supplied, so any failure marks the skill invalid + raise ValueError(f"frontmatter could not be parsed: {exc}") from exc + try: + frontmatter = to_jsonable_python(_FRONTMATTER_ADAPTER.validate_python(parsed)) + json.dumps( + frontmatter, allow_nan=False + ) # NaN and infinity have no JSON form, so they can't pass through verbatim + return frontmatter + except (ValidationError, ValueError) as exc: + raise ValueError("frontmatter must be a mapping with string keys and JSON-compatible values") from exc + + +def _warn_over_limits(skill_uri: str, resources: Sequence[SkillResource]) -> None: + total = sum(resource.size for resource in resources) + if len(resources) > MAX_RESOURCES_PER_SKILL: + logger.warning( + "Skill %s lists %d files, over the recommended %d", skill_uri, len(resources), MAX_RESOURCES_PER_SKILL + ) + if total > MAX_TOTAL_SIZE: + logger.warning("Skill %s totals %d bytes, over the recommended %d", skill_uri, total, MAX_TOTAL_SIZE) + + +def _uri(path: str) -> str: + return "skill://" + "/".join(quote(segment, safe="") for segment in path.split("/")) + + +def _guess_mime_type(path: str) -> str: + # `mimetypes` consults the host's tables, which differ by platform and don't always know Markdown. + return ( + _MIME_TYPE_OVERRIDES.get(PurePosixPath(path).suffix.lower()) + or mimetypes.guess_type(path)[0] + or "application/octet-stream" + ) + + +def _guess_mime_types(paths: Sequence[str]) -> dict[str, str]: + return {path: _guess_mime_type(path) for path in paths} + + +def _file_resource( + provider: SkillProvider, + path: str, + fingerprint: _Fingerprint, + mime_type: str, + skill_frontmatter: Mapping[str, Any] | None, +) -> _SkillFileResource: + name = path.split("/")[-1] + description = None + if skill_frontmatter is not None: + name = skill_frontmatter["name"] + description = skill_frontmatter["description"] + return _SkillFileResource( + provider=provider, + path=path, + fingerprint=fingerprint, + uri=_uri(path), + name=name, + description=description, + mime_type=mime_type, + ) + + +def _directory_index(files: Mapping[str, _SkillFileResource]) -> dict[str, list[ResourceEntry]]: + """Map each directory URI to its direct children, built from the served files only.""" + index: dict[str, dict[str, ResourceEntry]] = {} + for path, file in files.items(): + segments = path.split("/") + for depth in range(1, len(segments)): + parent = _uri("/".join(segments[:depth])) + child_path = "/".join(segments[: depth + 1]) + if depth + 1 == len(segments): + entry = ResourceEntry( + uri=file.uri, + name=file.name or segments[-1], + description=file.description, + mime_type=file.mime_type, + size=file.size, + ) + else: + entry = ResourceEntry(uri=_uri(child_path), name=segments[depth], mime_type=_DIRECTORY_MIME_TYPE) + index.setdefault(parent, {})[entry.uri] = entry + return {uri: [children[child] for child in sorted(children)] for uri, children in index.items()} diff --git a/src/mcp/server/skills/filesystem.py b/src/mcp/server/skills/filesystem.py new file mode 100644 index 0000000000..d13a06b524 --- /dev/null +++ b/src/mcp/server/skills/filesystem.py @@ -0,0 +1,113 @@ +"""`FilesystemSkillProvider`: a `SkillProvider` over a local directory.""" + +from __future__ import annotations + +import logging +import os +import stat +from collections.abc import Sequence +from pathlib import Path + +import anyio.to_thread + +__all__ = ["FilesystemSkillProvider"] + +logger = logging.getLogger(__name__) + +# NONBLOCK keeps `open()` from hanging on a FIFO swapped in after listing; the `fstat` check then refuses it. +_READ_FLAGS = os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) | getattr(os, "O_BINARY", 0) | getattr(os, "O_NONBLOCK", 0) + + +# Windows reads `\` as a separator and `:` as drive syntax, so a "/"-only check lets `..\x` or `C:x` leave the root. +# On POSIX both are ordinary file name characters. +_FORBIDDEN_SEGMENT_CHARS = "\\:" if os.name == "nt" else "" + +# Windows junctions and symlinks are reparse points tagged IO_REPARSE_TAG_MOUNT_POINT / IO_REPARSE_TAG_SYMLINK, +# constants that `stat` only types on Windows. Other reparse points (cloud placeholders, dedup) are ordinary files. +_WINDOWS_LINK_REPARSE_TAGS = frozenset({0xA0000003, 0xA000000C}) + + +class FilesystemSkillProvider: + """A `SkillProvider` that reads skill files from a local directory. + + Only regular files are served. Symlinks (and Windows junctions) found while + listing or reading are skipped with a warning or refused, so a skill directory + doesn't expose files that live outside it. The root is trusted: this is a + best-effort check, not a defence against someone who can modify the directory + while the server runs. `SkillCatalog` additionally refuses any file whose bytes + no longer match the digest taken when it was loaded. The blocking file system + calls run in a worker thread. + """ + + def __init__(self, root: str | os.PathLike[str]) -> None: + """Create a provider rooted at `root`. Nothing is read until `list_files()`.""" + self._root = Path(root).absolute() + + async def list_files(self) -> Sequence[str]: + """Return the `/`-separated path of every regular file under the root, recursively. + + Entries whose name starts with `.` are not descended into or listed. + + Raises: + OSError: If the root is missing, isn't a directory, or can't be read. + """ + return await anyio.to_thread.run_sync(self._list_files) + + async def read_file(self, path: str) -> bytes: + """Return the bytes of the regular file at `path`, relative to the root. + + Raises: + ValueError: If `path` has an empty, `.` or `..` segment, or, on Windows, a segment + containing `\\` or `:`. + OSError: If the file is missing, can't be read, is a symlink, or isn't a + regular file, or if a directory on the way to it is a symlink. + """ + segments = path.split("/") + if any( + segment in ("", ".", "..") or any(char in segment for char in _FORBIDDEN_SEGMENT_CHARS) + for segment in segments + ): + raise ValueError(f"invalid skill file path {path!r}") + return await anyio.to_thread.run_sync(self._read_file, segments) + + def _list_files(self) -> list[str]: + files: list[str] = [] + self._walk(self._root, [], files) + return files + + def _walk(self, directory: Path, prefix: list[str], files: list[str]) -> None: + with os.scandir(directory) as entries: + ordered = sorted(entries, key=lambda entry: entry.name) + for entry in ordered: + if entry.name.startswith("."): + continue + segments = [*prefix, entry.name] + result = entry.stat(follow_symlinks=False) + if _is_link(result): + logger.warning("Skipping symlink %s: skill files are never read through links", entry.path) + elif stat.S_ISDIR(result.st_mode): + self._walk(Path(entry.path), segments, files) + elif stat.S_ISREG(result.st_mode): + files.append("/".join(segments)) + else: # pragma: lax no cover # FIFO/device: no test on Windows + logger.warning("Skipping %s: not a regular file", entry.path) + + def _read_file(self, segments: list[str]) -> bytes: + current = self._root + for segment in segments[:-1]: + current = current / segment + result = os.lstat(current) + if _is_link(result) or not stat.S_ISDIR(result.st_mode): + raise OSError(f"{current} is not a plain directory") + current = current / segments[-1] + if _is_link(os.lstat(current)): + raise OSError(f"{current} is a symlink") + fd = os.open(current, _READ_FLAGS) + with os.fdopen(fd, "rb") as file: + if not stat.S_ISREG(os.fstat(file.fileno()).st_mode): + raise OSError(f"{current} is not a regular file") # pragma: lax no cover # only if swapped after lstat + return file.read() + + +def _is_link(result: os.stat_result) -> bool: + return stat.S_ISLNK(result.st_mode) or getattr(result, "st_reparse_tag", 0) in _WINDOWS_LINK_REPARSE_TAGS diff --git a/src/mcp/server/skills/provider.py b/src/mcp/server/skills/provider.py new file mode 100644 index 0000000000..da7839b49a --- /dev/null +++ b/src/mcp/server/skills/provider.py @@ -0,0 +1,32 @@ +"""The storage interface behind `SkillCatalog`.""" + +from collections.abc import Sequence +from typing import Protocol + +__all__ = ["SkillProvider"] + + +class SkillProvider(Protocol): + """A source of skill files. + + Implementations are structural: they need not subclass this class. A path is a + `/`-separated string relative to the provider's root, with no leading `/` and + no empty, `.` or `..` segment. A provider reports paths exactly as stored; + it neither parses skills nor decides which files belong to which skill. + """ + + async def list_files(self) -> Sequence[str]: + """Return the path of every file the provider serves, recursively. + + Directories are not listed, and each path appears once. A provider may leave files out; + `FilesystemSkillProvider` omits any name that starts with `.`. + """ + ... + + async def read_file(self, path: str) -> bytes: + """Return the raw, undecoded bytes of the file at `path`. + + Raises: + OSError: If the file can't be read. + """ + ... diff --git a/tests/docs_src/test_skills.py b/tests/docs_src/test_skills.py index de64e523b2..6044c410d6 100644 --- a/tests/docs_src/test_skills.py +++ b/tests/docs_src/test_skills.py @@ -1,9 +1,11 @@ """`docs/advanced/skills.md`: every claim the page makes, proved against the real SDK.""" +from pathlib import Path + import pytest from mcp_types import INVALID_PARAMS, TextResourceContents -from docs_src.skills import tutorial001 +from docs_src.skills import tutorial001, tutorial002 from mcp import Client from mcp.client.extensions.skills import Skills from mcp.shared.exceptions import MCPError @@ -45,3 +47,24 @@ async def test_the_skill_file_is_served_as_an_ordinary_resource() -> None: assert isinstance(contents, TextResourceContents) assert contents.mime_type == "text/markdown" assert contents.text == tutorial001.SKILL_MD + + +async def test_a_directory_of_skills_is_served_with_no_handlers() -> None: + """tutorial002: `SkillCatalog` over `FilesystemSkillProvider` lists the sample skill and its files.""" + server = await tutorial002.build_server(Path(tutorial002.__file__).parent / "skills") + skills = Skills() + async with Client(server, extensions=[skills]) as client: + skill = (await client.extension(Skills).list_skills())[0] + children = await client.extension(Skills).read_directory("skill://git-workflow/references") + contents = (await client.read_resource("skill://git-workflow/references/conventions.md")).contents[0] + + assert skill.uri == "skill://git-workflow/SKILL.md" + assert skill.frontmatter["name"] == "git-workflow" + assert isinstance(skill.resources, list) + assert [r.uri for r in skill.resources] == [ + "skill://git-workflow/SKILL.md", + "skill://git-workflow/references/conventions.md", + ] + assert [r.uri for r in children] == ["skill://git-workflow/references/conventions.md"] + assert isinstance(contents, TextResourceContents) + assert contents.text.startswith("# Conventions") diff --git a/tests/server/skills/__init__.py b/tests/server/skills/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/server/skills/test_catalog.py b/tests/server/skills/test_catalog.py new file mode 100644 index 0000000000..42b6cf63ed --- /dev/null +++ b/tests/server/skills/test_catalog.py @@ -0,0 +1,762 @@ +"""Tests for `SkillCatalog` (`mcp.server.skills.catalog`): the provider-backed Skills extension. + +Every test drives a real `MCPServer` through an in-memory `Client`. The provider is a dict of +bytes and the frontmatter parser a `key: value` stand-in, so the SDK's no-YAML-dependency +stance holds in the tests too. +""" + +import base64 +import datetime +import hashlib +import logging +from collections.abc import Mapping, Sequence +from pathlib import Path +from typing import Any, cast + +import pytest +from mcp_types import INTERNAL_ERROR, INVALID_PARAMS, BlobResourceContents, TextResourceContents +from pydantic import BaseModel, ConfigDict + +import mcp.server.skills as skills_package +from mcp.client.client import Client +from mcp.client.extensions.skills import Skills as ClientSkills +from mcp.server.mcpserver import MCPServer +from mcp.server.skills import ( + FilesystemSkillProvider, + GetSkillHandler, + ListSkillsHandler, + ReadDirectoryHandler, + SkillCatalog, + SkillProvider, +) +from mcp.shared.exceptions import MCPError +from mcp.shared.skills import ( + MAX_RESOURCES_PER_SKILL, + MAX_TOTAL_SIZE, + GetSkillParams, + GetSkillRequest, + GetSkillResult, + ListSkillsParams, + ListSkillsRequest, + ListSkillsResult, + ReadDirectoryParams, + ReadDirectoryRequest, + ReadDirectoryResult, + Skill, +) + +pytestmark = pytest.mark.anyio + +CATALOG_LOGGER = "mcp.server.skills.catalog" + + +class _RawResult(BaseModel): + """Captures every wire field so a test can see which cache fields were sent.""" + + model_config = ConfigDict(extra="allow") + + +class _MemoryProvider: + """A `SkillProvider` over a dict. `listing` overrides what `list_files` reports.""" + + def __init__(self, files: Mapping[str, bytes], listing: Sequence[str] | None = None) -> None: + self.files = dict(files) + self._listing = listing + + async def list_files(self) -> Sequence[str]: + return list(self.files) if self._listing is None else self._listing + + async def read_file(self, path: str) -> bytes: + return self.files[path] + + +def _parse(text: str) -> Mapping[str, Any]: + """A stand-in for `yaml.safe_load` that understands flat `key: value` lines.""" + result: dict[str, Any] = {} + for line in text.splitlines(): + key, separator, value = line.partition(":") + if not separator: + raise ValueError(f"not a `key: value` line: {line!r}") + result[key.strip()] = value.strip() + return result + + +def _skill_md(name: str, description: str = "Does the thing", extra: str = "") -> bytes: + return f"---\nname: {name}\ndescription: {description}\n{extra}---\nBody for {name}.\n".encode() + + +def _digest(data: bytes) -> str: + return f"sha256:{hashlib.sha256(data).hexdigest()}" + + +async def _listed(catalog: SkillCatalog) -> list[Skill]: + server = MCPServer("skills", extensions=[catalog]) + async with Client(server, extensions=[ClientSkills()]) as client: + return list(await client.extension(ClientSkills).list_skills()) + + +async def _server(files: Mapping[str, bytes], **options: Any) -> MCPServer: + catalog = await SkillCatalog.load(_MemoryProvider(files), parse_frontmatter=_parse, **options) + return MCPServer("skills", extensions=[catalog]) + + +async def test_list_skills_returns_every_valid_skill_sorted_with_complete_manifests() -> None: + """SEP-2640: `skills/list` enumerates skills, each with a manifest of `{uri, digest, size}`.""" + skill_md = _skill_md("git-workflow") + guide = b"# Conventions\n" + server = await _server( + { + "zebra/SKILL.md": _skill_md("zebra"), + "git-workflow/SKILL.md": skill_md, + "git-workflow/references/conventions.md": guide, + "outside-any-skill.txt": b"ignored", + } + ) + async with Client(server, extensions=[ClientSkills()]) as client: + skills = await client.extension(ClientSkills).list_skills() + + assert [skill.uri for skill in skills] == ["skill://git-workflow/SKILL.md", "skill://zebra/SKILL.md"] + assert skills[0].frontmatter == {"name": "git-workflow", "description": "Does the thing"} + assert isinstance(skills[0].resources, list) + assert [(r.uri, r.digest, r.size) for r in skills[0].resources] == [ + ("skill://git-workflow/SKILL.md", _digest(skill_md), len(skill_md)), + ("skill://git-workflow/references/conventions.md", _digest(guide), len(guide)), + ] + + +async def test_get_skill_returns_the_same_entry_as_the_listing() -> None: + """SEP-2640: `skills/get` answers for a single skill by URI.""" + server = await _server({"git-workflow/SKILL.md": _skill_md("git-workflow")}) + async with Client(server, extensions=[ClientSkills()]) as client: + listed = (await client.extension(ClientSkills).list_skills())[0] + fetched = await client.extension(ClientSkills).get_skill(listed.uri) + assert fetched == listed + + +async def test_get_skill_rejects_an_unknown_uri_with_invalid_params() -> None: + """SEP-2640: an unknown skill URI is Invalid params (-32602).""" + server = await _server({"git-workflow/SKILL.md": _skill_md("git-workflow")}) + async with Client(server) as client: + with pytest.raises(MCPError) as exc_info: + await client.session.send_request( + GetSkillRequest(params=GetSkillParams(uri="skill://other/SKILL.md")), GetSkillResult + ) + assert exc_info.value.code == INVALID_PARAMS + + +async def test_list_skills_rejects_a_cursor_because_the_catalog_never_issues_one() -> None: + """SDK-defined: the catalog answers on a single page, so any cursor is Invalid params.""" + server = await _server({"git-workflow/SKILL.md": _skill_md("git-workflow")}) + async with Client(server) as client: + with pytest.raises(MCPError) as exc_info: + await client.session.send_request( + ListSkillsRequest(params=ListSkillsParams(cursor="page-2")), ListSkillsResult + ) + assert exc_info.value.code == INVALID_PARAMS + + +async def test_a_nested_skill_is_listed_and_its_files_also_belong_to_the_parent_manifest() -> None: + """SEP-2640: a skill's files are everything under its directory, nested skills included.""" + server = await _server( + { + "acme/SKILL.md": _skill_md("acme"), + "acme/billing/SKILL.md": _skill_md("billing"), + "acme/billing/rules.md": b"rules", + } + ) + async with Client(server, extensions=[ClientSkills()]) as client: + skills = {skill.uri: skill for skill in await client.extension(ClientSkills).list_skills()} + + assert sorted(skills) == ["skill://acme/SKILL.md", "skill://acme/billing/SKILL.md"] + assert isinstance(skills["skill://acme/SKILL.md"].resources, list) + assert [r.uri for r in skills["skill://acme/SKILL.md"].resources] == [ + "skill://acme/SKILL.md", + "skill://acme/billing/SKILL.md", + "skill://acme/billing/rules.md", + ] + assert isinstance(skills["skill://acme/billing/SKILL.md"].resources, list) + assert [r.uri for r in skills["skill://acme/billing/SKILL.md"].resources] == [ + "skill://acme/billing/SKILL.md", + "skill://acme/billing/rules.md", + ] + + +async def test_a_skills_files_exclude_siblings_that_only_share_a_name_prefix() -> None: + """SDK-defined: a manifest holds only what is under `acme/`, not `acme-tools/`, `acme0/` or `acme.md`. + + The provider lists in reverse order, so the result can't depend on the listing order. + """ + files = { + "acme/SKILL.md": _skill_md("acme"), + "acme/guide.md": b"guide", + "acme-tools/SKILL.md": _skill_md("acme-tools"), + "acme0/SKILL.md": _skill_md("acme0"), + "acme.md": b"loose", + } + provider = _MemoryProvider(files, listing=sorted(files, reverse=True)) + catalog = await SkillCatalog.load(provider, parse_frontmatter=_parse) + async with Client(MCPServer("skills", extensions=[catalog]), extensions=[ClientSkills()]) as client: + skills = {skill.uri: skill for skill in await client.extension(ClientSkills).list_skills()} + + assert isinstance(skills["skill://acme/SKILL.md"].resources, list) + assert [r.uri for r in skills["skill://acme/SKILL.md"].resources] == [ + "skill://acme/SKILL.md", + "skill://acme/guide.md", + ] + + +async def test_hidden_files_and_directories_are_never_listed_or_served() -> None: + """SDK-defined (matches typescript-sdk #3047): a path segment starting with `.` is skipped entirely.""" + server = await _server( + { + "git-workflow/SKILL.md": _skill_md("git-workflow"), + "git-workflow/.env": b"SECRET=1", + "git-workflow/references/.hidden/notes.md": b"hidden", + "git-workflow/references/visible.md": b"visible", + ".dotted/SKILL.md": _skill_md("dotted"), + } + ) + async with Client(server, extensions=[ClientSkills()]) as client: + skills = await client.extension(ClientSkills).list_skills() + served = [resource.uri for resource in (await client.list_resources()).resources] + + assert [skill.uri for skill in skills] == ["skill://git-workflow/SKILL.md"] + assert isinstance(skills[0].resources, list) + assert [r.uri for r in skills[0].resources] == [ + "skill://git-workflow/SKILL.md", + "skill://git-workflow/references/visible.md", + ] + assert served == ["skill://git-workflow/SKILL.md", "skill://git-workflow/references/visible.md"] + + +@pytest.mark.parametrize( + ("skill_md", "path"), + [ + pytest.param(b"name: bad\ndescription: d\n", "bad/SKILL.md", id="no-opening-delimiter"), + pytest.param(b"---\nname: bad\ndescription: d\n", "bad/SKILL.md", id="unterminated-frontmatter"), + pytest.param(b"---\nnot a mapping line\n---\n", "bad/SKILL.md", id="parser-error"), + pytest.param(b"---\nname: other\ndescription: d\n---\n", "bad/SKILL.md", id="name-differs-from-directory"), + pytest.param(b"---\nname: Bad_Name\ndescription: d\n---\n", "Bad_Name/SKILL.md", id="name-grammar"), + pytest.param(b"---\nname: bad\ndescription:\n---\n", "bad/SKILL.md", id="empty-description"), + pytest.param(b"---\nname: bad\n---\n", "bad/SKILL.md", id="missing-description"), + pytest.param(b"\xff\xfe", "bad/SKILL.md", id="not-utf-8"), + pytest.param(_skill_md("root"), "SKILL.md", id="skill-md-at-the-provider-root"), + ], +) +async def test_an_invalid_skill_is_skipped_with_a_warning_when_not_strict( + skill_md: bytes, path: str, caplog: pytest.LogCaptureFixture +) -> None: + """SDK-defined (matches typescript-sdk #3047): invalid skills are skipped, the valid ones still served.""" + with caplog.at_level(logging.WARNING, logger=CATALOG_LOGGER): + server = await _server({path: skill_md, "good/SKILL.md": _skill_md("good")}) + async with Client(server, extensions=[ClientSkills()]) as client: + skills = await client.extension(ClientSkills).list_skills() + + assert [skill.uri for skill in skills] == ["skill://good/SKILL.md"] + assert [record.getMessage() for record in caplog.records if path in record.getMessage()] != [] + + +async def test_a_non_mapping_frontmatter_result_makes_the_skill_invalid() -> None: + """SDK-defined: the injected parser must return a mapping with string keys.""" + + def parse_to_list(text: str) -> Any: + return ["not", "a", "mapping"] + + provider = _MemoryProvider({"bad/SKILL.md": _skill_md("bad")}) + with pytest.raises(ValueError) as exc_info: + await SkillCatalog.load(provider, parse_frontmatter=parse_to_list, strict=True) + assert "bad/SKILL.md" in str(exc_info.value) + + +async def test_a_frontmatter_mapping_with_non_string_keys_makes_the_skill_invalid() -> None: + """SDK-defined: frontmatter is rendered as JSON, so its keys must be strings.""" + + def parse_with_int_key(text: str) -> Mapping[Any, Any]: + return {1: "one"} + + provider = _MemoryProvider({"bad/SKILL.md": _skill_md("bad")}) + with pytest.raises(ValueError) as exc_info: + await SkillCatalog.load(provider, parse_frontmatter=parse_with_int_key, strict=True) + assert "bad/SKILL.md" in str(exc_info.value) + + +async def test_a_frontmatter_value_that_is_not_json_serializable_makes_the_skill_invalid() -> None: + """SDK-defined: frontmatter must be representable as JSON on the wire.""" + + def parse_with_unserializable(text: str) -> Mapping[str, Any]: + return {"name": "bad", "description": "d", "broken": float("nan"), "key": object()} + + provider = _MemoryProvider({"bad/SKILL.md": _skill_md("bad")}) + with pytest.raises(ValueError) as exc_info: + await SkillCatalog.load(provider, parse_frontmatter=parse_with_unserializable, strict=True) + assert "bad/SKILL.md" in str(exc_info.value) + + +async def test_a_frontmatter_value_with_no_json_form_makes_the_skill_invalid() -> None: + """SDK-defined: NaN can't be carried verbatim as JSON, so it is rejected instead of being silently rewritten.""" + + def parse_with_nan(text: str) -> Mapping[str, Any]: + return {"name": "bad", "description": "d", "ratio": float("nan")} + + provider = _MemoryProvider({"bad/SKILL.md": _skill_md("bad")}) + with pytest.raises(ValueError) as exc_info: + await SkillCatalog.load(provider, parse_frontmatter=parse_with_nan, strict=True) + assert "bad/SKILL.md" in str(exc_info.value) + + +async def test_a_provider_may_return_any_iterable_of_paths() -> None: + """SDK-defined: the listing is consumed once, so a generator doesn't silently yield an empty catalog.""" + + class GeneratorProvider(_MemoryProvider): + async def list_files(self) -> Sequence[str]: + return cast(Sequence[str], (path for path in self.files)) + + catalog = await SkillCatalog.load( + GeneratorProvider({"git-workflow/SKILL.md": _skill_md("git-workflow")}), parse_frontmatter=_parse + ) + assert [skill.uri for skill in await _listed(catalog)] == ["skill://git-workflow/SKILL.md"] + + +async def test_strict_load_raises_one_error_naming_every_invalid_skill() -> None: + """SDK-defined (matches typescript-sdk #3047 `strict`): all problems are reported together.""" + provider = _MemoryProvider( + { + "first/SKILL.md": b"no frontmatter", + "second/SKILL.md": _skill_md("mismatch"), + "good/SKILL.md": _skill_md("good"), + } + ) + with pytest.raises(ValueError) as exc_info: + await SkillCatalog.load(provider, parse_frontmatter=_parse, strict=True) + message = str(exc_info.value) + assert "first/SKILL.md" in message + assert "second/SKILL.md" in message + assert "good/SKILL.md" not in message + + +@pytest.mark.parametrize( + "listing", + [ + pytest.param([""], id="empty"), + pytest.param(["/abs/SKILL.md"], id="absolute"), + pytest.param(["a/./SKILL.md"], id="dot-segment"), + pytest.param(["a/../SKILL.md"], id="dot-dot-segment"), + pytest.param(["a//SKILL.md"], id="empty-segment"), + pytest.param(["a/SKILL.md", "a/SKILL.md"], id="duplicate"), + pytest.param(["a/SKILL.md", "a/notes", "a/notes/more.txt"], id="file-beneath-a-file"), + ], +) +async def test_a_provider_listing_a_malformed_path_is_a_value_error_even_when_not_strict( + listing: list[str], +) -> None: + """SDK-defined: a malformed listing is a provider bug, not an invalid skill, so it never degrades to a warning.""" + provider = _MemoryProvider({"a/SKILL.md": _skill_md("a")}, listing=listing) + with pytest.raises(ValueError): + await SkillCatalog.load(provider, parse_frontmatter=_parse) + + +async def test_a_provider_error_while_loading_propagates() -> None: + """SDK-defined: only skill invalidity is tolerated; a provider that fails to read aborts the load.""" + provider = _MemoryProvider({}, listing=["a/SKILL.md"]) + with pytest.raises(KeyError): + await SkillCatalog.load(provider, parse_frontmatter=_parse) + + +async def test_frontmatter_passes_through_verbatim_and_dates_become_iso_strings() -> None: + """SEP-2640: the listing carries the frontmatter verbatim, as JSON; unknown keys survive (ext-skills#126).""" + + def parse_with_date(text: str) -> Mapping[str, Any]: + return { + "name": "git-workflow", + "description": "d", + "released": datetime.date(2026, 7, 28), + "x-custom": {"nested": [1, 2]}, + } + + catalog = await SkillCatalog.load( + _MemoryProvider({"git-workflow/SKILL.md": _skill_md("git-workflow")}), parse_frontmatter=parse_with_date + ) + assert (await _listed(catalog))[0].frontmatter == { + "name": "git-workflow", + "description": "d", + "released": "2026-07-28", + "x-custom": {"nested": [1, 2]}, + } + + +async def test_files_are_served_byte_exact_including_bom_and_crlf() -> None: + """SEP-2640: `resources/read` returns the same bytes the manifest digest describes.""" + skill_md = b"\xef\xbb\xbf---\r\nname: git-workflow\r\ndescription: d\r\n---\r\nBody\r\n" + server = await _server({"git-workflow/SKILL.md": skill_md}) + async with Client(server, extensions=[ClientSkills()]) as client: + skill = (await client.extension(ClientSkills).list_skills())[0] + contents = (await client.read_resource("skill://git-workflow/SKILL.md")).contents[0] + + assert isinstance(contents, TextResourceContents) + assert contents.text.encode("utf-8") == skill_md + assert isinstance(skill.resources, list) + assert skill.resources[0].digest == _digest(skill_md) + + +async def test_a_file_that_is_not_utf_8_is_served_as_a_blob() -> None: + """SDK-defined: content that can't be text is returned as a base64 blob, byte-exact.""" + binary = bytes(range(256)) + server = await _server({"git-workflow/SKILL.md": _skill_md("git-workflow"), "git-workflow/assets/data.bin": binary}) + async with Client(server) as client: + contents = (await client.read_resource("skill://git-workflow/assets/data.bin")).contents[0] + + assert isinstance(contents, BlobResourceContents) + assert base64.b64decode(contents.blob) == binary + assert contents.mime_type == "application/octet-stream" + + +async def test_skill_md_is_served_as_markdown_with_its_own_name_and_description() -> None: + """SEP-2640: `SKILL.md` is `text/markdown`; other files carry their own type, or a generic one.""" + server = await _server( + { + "git-workflow/SKILL.md": _skill_md("git-workflow", description="Follow our Git rules"), + "git-workflow/notes.json": b"{}", + "git-workflow/references/GUIDE.MD": b"# Guide", + "git-workflow/blob.unknownext": b"x", + "git-workflow/no-extension": b"x", + } + ) + async with Client(server) as client: + listed = {resource.uri: resource for resource in (await client.list_resources()).resources} + + skill_md = listed["skill://git-workflow/SKILL.md"] + assert (skill_md.name, skill_md.description, skill_md.mime_type) == ( + "git-workflow", + "Follow our Git rules", + "text/markdown", + ) + assert listed["skill://git-workflow/notes.json"].name == "notes.json" + assert listed["skill://git-workflow/notes.json"].mime_type == "application/json" + assert listed["skill://git-workflow/references/GUIDE.MD"].mime_type == "text/markdown" + assert listed["skill://git-workflow/blob.unknownext"].mime_type == "application/octet-stream" + assert listed["skill://git-workflow/no-extension"].mime_type == "application/octet-stream" + + +async def test_reading_a_file_that_changed_after_load_is_an_internal_error() -> None: + """SDK-defined: the served bytes must match the manifest digest, so drift fails the read.""" + provider = _MemoryProvider({"git-workflow/SKILL.md": _skill_md("git-workflow")}) + catalog = await SkillCatalog.load(provider, parse_frontmatter=_parse) + provider.files["git-workflow/SKILL.md"] = _skill_md("git-workflow", description="tampered") + async with Client(MCPServer("skills", extensions=[catalog])) as client: + with pytest.raises(MCPError) as exc_info: + await client.read_resource("skill://git-workflow/SKILL.md") + assert exc_info.value.code == INTERNAL_ERROR + + +async def test_reading_a_uri_outside_the_catalog_is_invalid_params() -> None: + """SEP-2640: only listed files are served; the lookup is an exact URI match.""" + server = await _server({"git-workflow/SKILL.md": _skill_md("git-workflow")}) + async with Client(server) as client: + with pytest.raises(MCPError) as exc_info: + await client.read_resource("skill://git-workflow/../git-workflow/SKILL.md") + assert exc_info.value.code == INVALID_PARAMS + + +async def test_special_characters_in_a_path_are_percent_encoded_and_still_readable() -> None: + """SEP-2640 Resource Mapping: URIs must be valid, so a space or `#` in a file name is percent-encoded.""" + server = await _server( + {"git-workflow/SKILL.md": _skill_md("git-workflow"), "git-workflow/my notes #1.md": b"notes"} + ) + async with Client(server, extensions=[ClientSkills()]) as client: + skill = (await client.extension(ClientSkills).list_skills())[0] + contents = (await client.read_resource("skill://git-workflow/my%20notes%20%231.md")).contents[0] + + assert isinstance(skill.resources, list) + assert "skill://git-workflow/my%20notes%20%231.md" in [r.uri for r in skill.resources] + assert isinstance(contents, TextResourceContents) + assert contents.text == "notes" + + +async def test_a_skill_over_the_recommended_limits_is_still_served_with_a_warning( + caplog: pytest.LogCaptureFixture, +) -> None: + """SEP-2640: 512 files and 16 MiB are SHOULD NOT limits, so the skill is served and the overage is logged.""" + files = {"big/SKILL.md": _skill_md("big"), "big/huge.bin": bytes(MAX_TOTAL_SIZE)} + files.update({f"big/f{i}.txt": b"x" for i in range(MAX_RESOURCES_PER_SKILL)}) + with caplog.at_level(logging.WARNING, logger=CATALOG_LOGGER): + catalog = await SkillCatalog.load(_MemoryProvider(files), parse_frontmatter=_parse) + + assert [skill.uri for skill in await _listed(catalog)] == ["skill://big/SKILL.md"] + assert len([record for record in caplog.records if "skill://big/SKILL.md" in record.getMessage()]) == 2 + + +async def test_directory_read_is_neither_advertised_nor_registered_by_default() -> None: + """SEP-2640: `directoryRead` is opt-in; without it the capability is empty and the method absent.""" + server = await _server({"git-workflow/SKILL.md": _skill_md("git-workflow")}) + async with Client(server) as client: + assert client.server_capabilities.extensions == {"io.modelcontextprotocol/skills": {}} + with pytest.raises(MCPError): + await client.session.send_request( + ReadDirectoryRequest(params=ReadDirectoryParams(uri="skill://git-workflow")), ReadDirectoryResult + ) + + +async def test_directory_read_lists_direct_children_at_every_level() -> None: + """SEP-2640 Directory resources: each directory lists its direct children, including organisation levels.""" + server = await _server( + { + "acme/billing/SKILL.md": _skill_md("billing"), + "acme/billing/references/guide.txt": b"guide", + "acme/billing/references/deep/more.txt": b"more", + "acme/billing/.git/config": b"hidden", + }, + directory_read=True, + ) + + async def children(client: Client, uri: str) -> list[tuple[str, str | None]]: + return [(r.uri, r.mime_type) for r in await client.extension(ClientSkills).read_directory(uri)] + + async with Client(server, extensions=[ClientSkills()]) as client: + assert client.server_capabilities.extensions == {"io.modelcontextprotocol/skills": {"directoryRead": True}} + assert await children(client, "skill://acme") == [("skill://acme/billing", "inode/directory")] + assert await children(client, "skill://acme/billing") == [ + ("skill://acme/billing/SKILL.md", "text/markdown"), + ("skill://acme/billing/references", "inode/directory"), + ] + assert await children(client, "skill://acme/billing/references") == [ + ("skill://acme/billing/references/deep", "inode/directory"), + ("skill://acme/billing/references/guide.txt", "text/plain"), + ] + names = {r.uri: r.name for r in await client.extension(ClientSkills).read_directory("skill://acme/billing")} + + assert names["skill://acme/billing/references"] == "references" + + +@pytest.mark.parametrize( + "uri", + [ + pytest.param("skill://acme/unknown", id="unknown-directory"), + pytest.param("skill://acme/billing/SKILL.md", id="file"), + pytest.param("skill://acme/billing/.git", id="hidden-directory"), + ], +) +async def test_directory_read_of_anything_but_a_served_directory_is_invalid_params(uri: str) -> None: + """SDK-defined: only directories holding served files can be read.""" + server = await _server( + {"acme/billing/SKILL.md": _skill_md("billing"), "acme/billing/.git/config": b"hidden"}, directory_read=True + ) + async with Client(server) as client: + with pytest.raises(MCPError) as exc_info: + await client.session.send_request( + ReadDirectoryRequest(params=ReadDirectoryParams(uri=uri)), ReadDirectoryResult + ) + assert exc_info.value.code == INVALID_PARAMS + + +async def test_directory_read_rejects_a_cursor() -> None: + """SDK-defined: the catalog answers on a single page, so any cursor is Invalid params.""" + server = await _server({"acme/billing/SKILL.md": _skill_md("billing")}, directory_read=True) + async with Client(server) as client: + with pytest.raises(MCPError) as exc_info: + await client.session.send_request( + ReadDirectoryRequest(params=ReadDirectoryParams(uri="skill://acme", cursor="page-2")), + ReadDirectoryResult, + ) + assert exc_info.value.code == INVALID_PARAMS + + +async def test_cache_hints_reach_list_get_and_directory_reads_on_the_2026_07_28_wire() -> None: + """SEP-2549: the configured `ttl_ms`/`cache_scope` are sent on every catalog result.""" + server = await _server( + {"git-workflow/SKILL.md": _skill_md("git-workflow")}, directory_read=True, ttl_ms=60_000, cache_scope="public" + ) + async with Client(server, mode="2026-07-28") as client: + raws = [ + await client.session.send_request(ListSkillsRequest(), _RawResult), + await client.session.send_request( + GetSkillRequest(params=GetSkillParams(uri="skill://git-workflow/SKILL.md")), _RawResult + ), + await client.session.send_request( + ReadDirectoryRequest(params=ReadDirectoryParams(uri="skill://git-workflow")), _RawResult + ), + ] + assert [(raw.model_extra or {})["ttlMs"] for raw in raws] == [60_000] * 3 + assert [(raw.model_extra or {})["cacheScope"] for raw in raws] == ["public"] * 3 + + +def _five_skills() -> dict[str, bytes]: + return {f"skill-{n}/SKILL.md": _skill_md(f"skill-{n}") for n in range(5)} + + +async def test_list_skills_pages_through_the_catalog_when_a_page_size_is_set() -> None: + """SEP-2640: `skills/list` mirrors base-protocol pagination; each `nextCursor` is passed back as `cursor`.""" + server = await _server(_five_skills(), page_size=2) + async with Client(server) as client: + first = await client.session.send_request(ListSkillsRequest(), ListSkillsResult) + second = await client.session.send_request( + ListSkillsRequest(params=ListSkillsParams(cursor=first.next_cursor)), ListSkillsResult + ) + assert second.next_cursor is not None + third = await client.session.send_request( + ListSkillsRequest(params=ListSkillsParams(cursor=second.next_cursor)), ListSkillsResult + ) + + pages = [[skill.uri for skill in page.skills] for page in (first, second, third)] + assert pages == [ + ["skill://skill-0/SKILL.md", "skill://skill-1/SKILL.md"], + ["skill://skill-2/SKILL.md", "skill://skill-3/SKILL.md"], + ["skill://skill-4/SKILL.md"], + ] + assert first.next_cursor is not None + assert third.next_cursor is None + + +async def test_the_client_follows_next_cursor_to_collect_every_skill_with_whole_entries() -> None: + """SEP-2640: an entry is atomic, so a skill's manifest is complete on whichever page carries it.""" + files = {**_five_skills(), "skill-0/references/guide.md": b"guide", "skill-0/references/more.md": b"more"} + server = await _server(files, page_size=2) + async with Client(server, extensions=[ClientSkills()]) as client: + skills = await client.extension(ClientSkills).list_skills() + + assert [skill.uri for skill in skills] == [f"skill://skill-{n}/SKILL.md" for n in range(5)] + assert isinstance(skills[0].resources, list) + assert len(skills[0].resources) == 3 + + +async def test_a_page_size_larger_than_the_catalog_returns_one_page_without_a_cursor() -> None: + """SDK-defined: `nextCursor` is only present when another page follows.""" + server = await _server(_five_skills(), page_size=5) + async with Client(server) as client: + result = await client.session.send_request(ListSkillsRequest(), ListSkillsResult) + assert len(result.skills) == 5 + assert result.next_cursor is None + + +@pytest.mark.parametrize( + "cursor", + [ + pytest.param("not base64 !", id="not-base64"), + pytest.param("\u00e9", id="not-ascii"), + pytest.param("", id="empty"), + pytest.param(base64.urlsafe_b64encode(b"abc").decode(), id="not-a-number"), + pytest.param(base64.urlsafe_b64encode(b"0").decode(), id="zero"), + pytest.param(base64.urlsafe_b64encode(b"3").decode(), id="not-a-page-boundary"), + pytest.param(base64.urlsafe_b64encode(b"10").decode(), id="past-the-end"), + pytest.param(base64.urlsafe_b64encode(b" 2").decode(), id="non-canonical"), + ], +) +async def test_a_cursor_the_catalog_did_not_issue_is_invalid_params(cursor: str) -> None: + """Base protocol: an invalid cursor is Invalid params (-32602).""" + server = await _server(_five_skills(), page_size=2) + async with Client(server) as client: + with pytest.raises(MCPError) as exc_info: + await client.session.send_request( + ListSkillsRequest(params=ListSkillsParams(cursor=cursor)), ListSkillsResult + ) + assert exc_info.value.code == INVALID_PARAMS + + +async def test_directory_read_pages_through_the_children_when_a_page_size_is_set() -> None: + """SEP-2640: `resources/directory/read` mirrors `resources/list` pagination.""" + files = {"git-workflow/SKILL.md": _skill_md("git-workflow")} + files.update({f"git-workflow/references/f{n}.txt": b"x" for n in range(5)}) + server = await _server(files, directory_read=True, page_size=2) + async with Client(server, extensions=[ClientSkills()]) as client: + first = await client.session.send_request( + ReadDirectoryRequest(params=ReadDirectoryParams(uri="skill://git-workflow/references")), + ReadDirectoryResult, + ) + everything = await client.extension(ClientSkills).read_directory("skill://git-workflow/references") + with pytest.raises(MCPError) as exc_info: + await client.session.send_request( + ReadDirectoryRequest(params=ReadDirectoryParams(uri="skill://git-workflow/references", cursor="bogus")), + ReadDirectoryResult, + ) + + assert len(first.resources) == 2 + assert first.next_cursor is not None + assert [r.uri for r in everything] == [f"skill://git-workflow/references/f{n}.txt" for n in range(5)] + assert exc_info.value.code == INVALID_PARAMS + + +@pytest.mark.parametrize("page_size", [0, -1]) +async def test_a_page_size_below_one_is_rejected_when_the_catalog_loads(page_size: int) -> None: + """SDK-defined: a page must hold at least one entry.""" + provider = _MemoryProvider({"git-workflow/SKILL.md": _skill_md("git-workflow")}) + with pytest.raises(ValueError): + await SkillCatalog.load(provider, parse_frontmatter=_parse, page_size=page_size) + + +async def test_a_negative_ttl_is_rejected_before_the_provider_is_used() -> None: + """SEP-2549: `ttlMs` is non-negative, and a bad option fails at load before the provider is scanned. + + The provider has no methods, so reaching it would raise `AttributeError` instead of `ValueError`. + """ + provider = cast(SkillProvider, object()) + with pytest.raises(ValueError): + await SkillCatalog.load(provider, parse_frontmatter=_parse, ttl_ms=-1) + + +async def test_load_returns_an_instance_of_the_subclass_it_is_called_on() -> None: + """SDK-defined: `load()` is a classmethod constructor, so a subclass gets its own type back.""" + + class _CustomCatalog(SkillCatalog): + pass + + provider = _MemoryProvider({"git-workflow/SKILL.md": _skill_md("git-workflow")}) + catalog = await _CustomCatalog.load(provider, parse_frontmatter=_parse) + + assert isinstance(catalog, _CustomCatalog) + + +async def test_the_handler_aliases_stay_importable_from_the_package() -> None: + """Compatibility: `mcp.server.skills` was a module before it became a package; its public names survive.""" + assert (GetSkillHandler, ListSkillsHandler, ReadDirectoryHandler) == ( + skills_package.GetSkillHandler, + skills_package.ListSkillsHandler, + skills_package.ReadDirectoryHandler, + ) + assert skills_package.__all__ == ["Skills", "SkillCatalog", "SkillProvider", "FilesystemSkillProvider"] + + +async def test_cache_hints_default_to_a_private_zero_ttl() -> None: + """SEP-2549: with no options set, results carry the `CacheableResult` defaults.""" + server = await _server({"git-workflow/SKILL.md": _skill_md("git-workflow")}) + async with Client(server, mode="2026-07-28") as client: + raw = await client.session.send_request(ListSkillsRequest(), _RawResult) + extra = raw.model_extra or {} + assert (extra["ttlMs"], extra["cacheScope"]) == (0, "private") + + +async def test_cache_hints_are_omitted_on_a_legacy_wire() -> None: + """SEP-2549 fields are 2026-07-28+ only; `Skills` strips them for an older connection.""" + server = await _server({"git-workflow/SKILL.md": _skill_md("git-workflow")}, ttl_ms=60_000, cache_scope="public") + async with Client(server, mode="legacy") as client: + raw = await client.session.send_request(ListSkillsRequest(), _RawResult) + extra = raw.model_extra or {} + assert "ttlMs" not in extra + assert "cacheScope" not in extra + + +async def test_a_catalog_over_a_directory_serves_files_whose_bytes_match_the_manifest(tmp_path: Path) -> None: + """End to end over `FilesystemSkillProvider`: list, get, directory read and read agree with the disk.""" + skill_dir = tmp_path / "git-workflow" + (skill_dir / "references").mkdir(parents=True) + (skill_dir / "SKILL.md").write_bytes(_skill_md("git-workflow")) + (skill_dir / "references" / "conventions.md").write_bytes(b"# Conventions\n") + (skill_dir / "references" / "logo.png").write_bytes(b"\x89PNG\xff\x00") + + catalog = await SkillCatalog.load(FilesystemSkillProvider(tmp_path), parse_frontmatter=_parse, directory_read=True) + async with Client(MCPServer("skills", extensions=[catalog]), extensions=[ClientSkills()]) as client: + skills_client = client.extension(ClientSkills) + listed = (await skills_client.list_skills())[0] + assert await skills_client.get_skill(listed.uri) == listed + children = await skills_client.read_directory("skill://git-workflow/references") + assert [r.uri for r in children] == [ + "skill://git-workflow/references/conventions.md", + "skill://git-workflow/references/logo.png", + ] + assert isinstance(listed.resources, list) + for entry in listed.resources: + contents = (await client.read_resource(entry.uri)).contents[0] + if isinstance(contents, BlobResourceContents): + data = base64.b64decode(contents.blob) + else: + assert isinstance(contents, TextResourceContents) + data = contents.text.encode("utf-8") + assert (_digest(data), len(data)) == (entry.digest, entry.size) diff --git a/tests/server/skills/test_filesystem.py b/tests/server/skills/test_filesystem.py new file mode 100644 index 0000000000..f66db2b461 --- /dev/null +++ b/tests/server/skills/test_filesystem.py @@ -0,0 +1,345 @@ +"""Tests for `FilesystemSkillProvider` (`mcp.server.skills.filesystem`).""" + +import logging +import os +import stat +import sys +from collections.abc import Iterator +from contextlib import contextmanager +from pathlib import Path +from types import SimpleNamespace +from typing import Any + +import pytest + +from mcp.server.skills import FilesystemSkillProvider + +pytestmark = pytest.mark.anyio + +FILESYSTEM_LOGGER = "mcp.server.skills.filesystem" + +# Creating a file symlink needs a privilege on Windows, and named pipes don't exist there. Skipped test +# bodies are excluded from coverage by a pragma on each decorator. +requires_file_symlinks = pytest.mark.skipif(sys.platform == "win32", reason="needs symlink privilege") +requires_fifos = pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="POSIX-only") +requires_posix_names = pytest.mark.skipif(sys.platform == "win32", reason="Windows forbids `\\` and `:` in names") + +IO_REPARSE_TAG_MOUNT_POINT = 0xA0000003 +IO_REPARSE_TAG_SYMLINK = 0xA000000C +IO_REPARSE_TAG_CLOUD_FILE = 0x9000601A # OneDrive placeholder: a reparse point that is an ordinary file + + +def link_directory(link: Path, target: Path) -> None: + """Make `link` a directory link to `target`: a symlink, or a junction where symlinks need a privilege.""" + try: + link.symlink_to(target, target_is_directory=True) + except OSError as exc: # pragma: lax no cover + if sys.platform != "win32" or exc.winerror != 1314: # ERROR_PRIVILEGE_NOT_HELD; a junction needs no privilege + raise + import _winapi + + _winapi.CreateJunction(str(target), str(link)) + + +def reparse_stat(mode: int, tag: int) -> Any: + """A `stat` result for a Windows reparse point, which POSIX can't produce.""" + return SimpleNamespace(st_mode=mode, st_reparse_tag=tag) + + +def pretend_reparse_points(monkeypatch: pytest.MonkeyPatch, points: dict[str, tuple[int, int]]) -> None: + """Make `os.scandir`/`os.lstat` report the named entries as Windows reparse points: `{name: (mode, tag)}`.""" + real_lstat = os.lstat + real_scandir = os.scandir + + class Entry: + def __init__(self, real: os.DirEntry[str]) -> None: + self.real = real + self.name = real.name + self.path = real.path + + def stat(self, *, follow_symlinks: bool) -> Any: + if self.name in points: + return reparse_stat(*points[self.name]) + return self.real.stat(follow_symlinks=follow_symlinks) + + @contextmanager + def scandir(path: Any) -> Iterator[list[Entry]]: + with real_scandir(path) as entries: + yield [Entry(entry) for entry in entries] + + def lstat(path: Any, *args: Any, **kwargs: Any) -> Any: + name = Path(path).name + if name in points: + return reparse_stat(*points[name]) + return real_lstat(path, *args, **kwargs) + + monkeypatch.setattr(os, "scandir", scandir) + monkeypatch.setattr(os, "lstat", lstat) + + +async def test_list_files_returns_slash_separated_relative_paths_of_nested_files(tmp_path: Path) -> None: + (tmp_path / "git-workflow" / "references").mkdir(parents=True) + (tmp_path / "git-workflow" / "SKILL.md").write_bytes(b"skill") + (tmp_path / "git-workflow" / "references" / "conventions.md").write_bytes(b"conventions") + (tmp_path / "top.txt").write_bytes(b"top") + + files = await FilesystemSkillProvider(tmp_path).list_files() + + assert sorted(files) == ["git-workflow/SKILL.md", "git-workflow/references/conventions.md", "top.txt"] + + +async def test_list_files_prunes_hidden_entries(tmp_path: Path) -> None: + """SDK-defined: hidden directories are never walked (so `.git` costs nothing).""" + (tmp_path / ".git").mkdir() + (tmp_path / ".git" / "config").write_bytes(b"x") + (tmp_path / "skill").mkdir() + (tmp_path / "skill" / ".env").write_bytes(b"x") + (tmp_path / "skill" / "SKILL.md").write_bytes(b"x") + + assert await FilesystemSkillProvider(tmp_path).list_files() == ["skill/SKILL.md"] + + +async def test_list_files_skips_a_linked_directory_with_a_warning( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """SDK-defined: links (symlinks, and junctions on Windows) are never followed, so a skill directory can't + expose files outside the root.""" + outside = tmp_path / "outside" + outside.mkdir() + (outside / "secret.txt").write_bytes(b"secret") + root = tmp_path / "root" + (root / "skill").mkdir(parents=True) + (root / "skill" / "SKILL.md").write_bytes(b"x") + link_directory(root / "skill" / "linked-dir", outside) + + with caplog.at_level(logging.WARNING, logger=FILESYSTEM_LOGGER): + files = await FilesystemSkillProvider(root).list_files() + + assert files == ["skill/SKILL.md"] + assert any("linked-dir" in record.getMessage() for record in caplog.records) + + +@requires_file_symlinks # pragma: lax no cover +async def test_list_files_skips_a_symlinked_file_with_a_warning( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """SDK-defined: a symlink to a file outside the root is not listed.""" + (tmp_path / "secret.txt").write_bytes(b"secret") + root = tmp_path / "root" + root.mkdir() + (root / "SKILL.md").write_bytes(b"x") + (root / "linked-file.txt").symlink_to(tmp_path / "secret.txt") + + with caplog.at_level(logging.WARNING, logger=FILESYSTEM_LOGGER): + files = await FilesystemSkillProvider(root).list_files() + + assert files == ["SKILL.md"] + assert any("linked-file.txt" in record.getMessage() for record in caplog.records) + + +@pytest.mark.parametrize("tag", [IO_REPARSE_TAG_MOUNT_POINT, IO_REPARSE_TAG_SYMLINK], ids=["junction", "symlink"]) +async def test_list_files_skips_a_windows_reparse_point_that_links( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture, tag: int +) -> None: + """SDK-defined: Windows reports junctions and symlinks as reparse points with these tags, which + POSIX can't create, so the platform's `stat` result is simulated.""" + (tmp_path / "skill").mkdir() + (tmp_path / "skill" / "SKILL.md").write_bytes(b"x") + (tmp_path / "junction").mkdir() + (tmp_path / "junction" / "secret.txt").write_bytes(b"secret") + pretend_reparse_points(monkeypatch, {"junction": (stat.S_IFDIR | 0o755, tag)}) + + with caplog.at_level(logging.WARNING, logger=FILESYSTEM_LOGGER): + files = await FilesystemSkillProvider(tmp_path).list_files() + + assert files == ["skill/SKILL.md"] + assert any("junction" in record.getMessage() for record in caplog.records) + + +async def test_list_files_keeps_a_windows_reparse_point_that_is_not_a_link( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """SDK-defined: cloud placeholders and similar reparse points are ordinary files, not links to refuse.""" + (tmp_path / "SKILL.md").write_bytes(b"x") + (tmp_path / "placeholder.txt").write_bytes(b"x") + pretend_reparse_points(monkeypatch, {"placeholder.txt": (stat.S_IFREG | 0o644, IO_REPARSE_TAG_CLOUD_FILE)}) + + assert await FilesystemSkillProvider(tmp_path).list_files() == ["SKILL.md", "placeholder.txt"] + + +@requires_fifos # pragma: lax no cover +async def test_list_files_skips_a_named_pipe_with_a_warning(tmp_path: Path, caplog: pytest.LogCaptureFixture) -> None: + """SDK-defined: only regular files are listed.""" + (tmp_path / "SKILL.md").write_bytes(b"x") + os.mkfifo(tmp_path / "pipe") + + with caplog.at_level(logging.WARNING, logger=FILESYSTEM_LOGGER): + files = await FilesystemSkillProvider(tmp_path).list_files() + + assert files == ["SKILL.md"] + assert any("pipe" in record.getMessage() for record in caplog.records) + + +async def test_list_files_raises_os_error_for_a_missing_root(tmp_path: Path) -> None: + with pytest.raises(OSError): + await FilesystemSkillProvider(tmp_path / "missing").list_files() + + +async def test_read_file_returns_the_raw_bytes(tmp_path: Path) -> None: + (tmp_path / "skill").mkdir() + content = b"\xef\xbb\xbfline\r\n\xff" + (tmp_path / "skill" / "SKILL.md").write_bytes(content) + + assert await FilesystemSkillProvider(tmp_path).read_file("skill/SKILL.md") == content + + +@pytest.mark.parametrize("path", ["", "/skill/SKILL.md", "skill//SKILL.md", "./skill/SKILL.md", "skill/../SKILL.md"]) +async def test_read_file_rejects_empty_dot_and_dot_dot_segments(tmp_path: Path, path: str) -> None: + """SDK-defined: a path that could leave the root is refused before the file system is touched.""" + with pytest.raises(ValueError): + await FilesystemSkillProvider(tmp_path).read_file(path) + + +@pytest.mark.parametrize("path", ["..\\secret.txt", "skill\\SKILL.md", "C:secret.txt", "skill/C:SKILL.md"]) +async def test_read_file_refuses_windows_path_syntax_that_could_leave_the_root( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, path: str +) -> None: + """SDK-defined: on Windows `\\` separates and `:` names a drive, so a "/"-only check would let these escape. + + The Windows rule is switched on here because the escape can't be shown from a POSIX path. + """ + root = tmp_path / "root" + root.mkdir() + (tmp_path / "secret.txt").write_bytes(b"outside the root") + monkeypatch.setattr("mcp.server.skills.filesystem._FORBIDDEN_SEGMENT_CHARS", "\\:") + + with pytest.raises(ValueError): + await FilesystemSkillProvider(root).read_file(path) + + +@requires_posix_names # pragma: lax no cover +async def test_names_containing_a_colon_or_backslash_are_served_off_windows(tmp_path: Path) -> None: + """SDK-defined: `:` and `\\` are ordinary name characters on POSIX, so the Windows rule must not reject them.""" + (tmp_path / "skill").mkdir() + (tmp_path / "skill" / "a:b\\c.md").write_bytes(b"content") + provider = FilesystemSkillProvider(tmp_path) + + assert await provider.list_files() == ["skill/a:b\\c.md"] + assert await provider.read_file("skill/a:b\\c.md") == b"content" + + +async def test_read_file_raises_os_error_for_a_missing_file(tmp_path: Path) -> None: + with pytest.raises(OSError): + await FilesystemSkillProvider(tmp_path).read_file("missing.md") + + +@requires_file_symlinks # pragma: lax no cover +async def test_read_file_refuses_a_file_swapped_for_a_symlink_after_listing(tmp_path: Path) -> None: + """SDK-defined: a symlink put in place of a listed file is not followed.""" + secret = tmp_path / "secret.txt" + secret.write_bytes(b"secret") + root = tmp_path / "root" + (root / "skill").mkdir(parents=True) + (root / "skill" / "SKILL.md").write_bytes(b"x") + provider = FilesystemSkillProvider(root) + assert "skill/SKILL.md" in await provider.list_files() + + (root / "skill" / "SKILL.md").unlink() + (root / "skill" / "SKILL.md").symlink_to(secret) + + with pytest.raises(OSError): + await provider.read_file("skill/SKILL.md") + + +async def test_read_file_refuses_a_final_component_that_is_a_linked_directory(tmp_path: Path) -> None: + """SDK-defined: a link as the last path component is refused rather than followed.""" + outside = tmp_path / "outside" + outside.mkdir() + root = tmp_path / "root" + root.mkdir() + link_directory(root / "linked", outside) + + with pytest.raises(OSError): + await FilesystemSkillProvider(root).read_file("linked") + + +async def test_read_file_refuses_a_directory_swapped_for_a_link_after_listing(tmp_path: Path) -> None: + """SDK-defined: an intermediate directory replaced by a link (a symlink, or a junction on Windows) is + not traversed.""" + outside = tmp_path / "outside" + outside.mkdir() + (outside / "SKILL.md").write_bytes(b"secret") + root = tmp_path / "root" + (root / "skill").mkdir(parents=True) + (root / "skill" / "SKILL.md").write_bytes(b"x") + provider = FilesystemSkillProvider(root) + assert "skill/SKILL.md" in await provider.list_files() + + (root / "skill" / "SKILL.md").unlink() + (root / "skill").rmdir() + link_directory(root / "skill", outside) + + with pytest.raises(OSError): + await provider.read_file("skill/SKILL.md") + + +@pytest.mark.parametrize("name", ["skill/junction/SKILL.md", "skill/junction"]) +async def test_read_file_refuses_a_windows_reparse_point_that_links( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, name: str +) -> None: + """SDK-defined: a junction, as an intermediate or final component, is refused (simulated, as above).""" + (tmp_path / "skill" / "junction").mkdir(parents=True) + (tmp_path / "skill" / "junction" / "SKILL.md").write_bytes(b"secret") + pretend_reparse_points(monkeypatch, {"junction": (stat.S_IFDIR | 0o755, IO_REPARSE_TAG_MOUNT_POINT)}) + + with pytest.raises(OSError): + await FilesystemSkillProvider(tmp_path).read_file(name) + + +async def test_read_file_reads_a_windows_reparse_point_that_is_not_a_link( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """SDK-defined: a cloud placeholder is an ordinary file and is read normally.""" + (tmp_path / "placeholder.txt").write_bytes(b"content") + pretend_reparse_points(monkeypatch, {"placeholder.txt": (stat.S_IFREG | 0o644, IO_REPARSE_TAG_CLOUD_FILE)}) + + assert await FilesystemSkillProvider(tmp_path).read_file("placeholder.txt") == b"content" + + +async def test_read_file_refuses_an_intermediate_component_that_is_a_file(tmp_path: Path) -> None: + """SDK-defined: only directories are traversed on the way to a file.""" + (tmp_path / "skill").write_bytes(b"not a directory") + + with pytest.raises(OSError): + await FilesystemSkillProvider(tmp_path).read_file("skill/SKILL.md") + + +async def test_read_file_refuses_a_directory(tmp_path: Path) -> None: + """SDK-defined: only regular files are readable.""" + (tmp_path / "skill").mkdir() + + with pytest.raises(OSError): + await FilesystemSkillProvider(tmp_path).read_file("skill") + + +@requires_fifos # pragma: lax no cover +async def test_read_file_refuses_a_named_pipe_without_blocking(tmp_path: Path) -> None: + """SDK-defined: a pipe swapped in after listing is refused rather than waited on.""" + os.mkfifo(tmp_path / "pipe") + + with pytest.raises(OSError): + await FilesystemSkillProvider(tmp_path).read_file("pipe") + + +async def test_a_root_that_is_itself_a_link_is_served(tmp_path: Path) -> None: + """SDK-defined: the root is trusted configuration, so only entries below it are never followed.""" + real = tmp_path / "real" + (real / "skill").mkdir(parents=True) + (real / "skill" / "SKILL.md").write_bytes(b"x") + link = tmp_path / "link" + link_directory(link, real) + + provider = FilesystemSkillProvider(link) + + assert await provider.list_files() == ["skill/SKILL.md"] + assert await provider.read_file("skill/SKILL.md") == b"x"