From 8b8049f5cd92472020f285b524508e8b3d0a0367 Mon Sep 17 00:00:00 2001 From: vijaydeepsinha Date: Sun, 25 Oct 2026 20:24:20 +0530 Subject: [PATCH 01/12] Add SkillCatalog and FilesystemSkillProvider for serving skills from a provider --- src/mcp/server/skills/__init__.py | 32 + .../{skills.py => skills/_extension.py} | 0 src/mcp/server/skills/catalog.py | 386 ++++++++++++ src/mcp/server/skills/filesystem.py | 101 ++++ src/mcp/server/skills/provider.py | 31 + tests/server/skills/__init__.py | 0 tests/server/skills/test_catalog.py | 558 ++++++++++++++++++ tests/server/skills/test_filesystem.py | 161 +++++ 8 files changed, 1269 insertions(+) create mode 100644 src/mcp/server/skills/__init__.py rename src/mcp/server/{skills.py => skills/_extension.py} (100%) create mode 100644 src/mcp/server/skills/catalog.py create mode 100644 src/mcp/server/skills/filesystem.py create mode 100644 src/mcp/server/skills/provider.py create mode 100644 tests/server/skills/__init__.py create mode 100644 tests/server/skills/test_catalog.py create mode 100644 tests/server/skills/test_filesystem.py diff --git a/src/mcp/server/skills/__init__.py b/src/mcp/server/skills/__init__.py new file mode 100644 index 0000000000..b8e64e619b --- /dev/null +++ b/src/mcp/server/skills/__init__.py @@ -0,0 +1,32 @@ +"""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. + + 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)]) +""" + +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 100% rename from src/mcp/server/skills.py rename to src/mcp/server/skills/_extension.py diff --git a/src/mcp/server/skills/catalog.py b/src/mcp/server/skills/catalog.py new file mode 100644 index 0000000000..aaa87b0006 --- /dev/null +++ b/src/mcp/server/skills/catalog.py @@ -0,0 +1,386 @@ +"""`SkillCatalog`: a `Skills` extension backed by a `SkillProvider`.""" + +from __future__ import annotations + +import hashlib +import logging +import mimetypes +import re +from collections.abc import Callable, Mapping, Sequence +from dataclasses import dataclass +from typing import Any, Literal, cast +from urllib.parse import quote + +import anyio.to_thread +from mcp_types import INVALID_PARAMS +from mcp_types import Resource as ResourceEntry +from pydantic import PrivateAttr, 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, + 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 = "---" + + +@dataclass(frozen=True) +class _Fingerprint: + digest: str + size: int + + +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, _SkillFileResource], + directories: Mapping[str, Sequence[ResourceEntry]] | None, + ttl_ms: int, + cache_scope: Literal["public", "private"], + ) -> None: + 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._ttl_ms = ttl_ms + self._cache_scope: Literal["public", "private"] = cache_scope + + @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", + ) -> SkillCatalog: + """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. + + Raises: + ValueError: If `strict` is set and any skill is invalid, or if the provider + lists a path that is empty, absolute, contains a `.` or `..` segment, or + appears twice. + OSError: If the provider fails to list or read a file. + """ + paths = _check_paths(await provider.list_files()) + visible = [path for path in paths if not any(segment.startswith(".") for segment in path.split("/"))] + skill_paths = sorted(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, + ) + + @property + def skills(self) -> Sequence[Skill]: + """The valid skills, sorted by URI.""" + return self._catalog_skills + + 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: + _reject_cursor(params.cursor) + return ListSkillsResult(skills=list(self._catalog_skills), ttl_ms=self._ttl_ms, cache_scope=self._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._ttl_ms, cache_scope=self._cache_scope) + + async def _read_directory_handler( + self, ctx: ServerRequestContext[Any, Any], params: ReadDirectoryParams + ) -> ReadDirectoryResult: + assert self._directories is not None + _reject_cursor(params.cursor) + children = self._directories.get(params.uri) + if children is None: + raise MCPError(code=INVALID_PARAMS, message=f"unknown directory: {params.uri}") + return ReadDirectoryResult(resources=list(children), ttl_ms=self._ttl_ms, cache_scope=self._cache_scope) + + +def _reject_cursor(cursor: str | None) -> None: + # The catalog returns everything on one page, so it never issues a cursor. + if cursor is not None: + raise MCPError(code=INVALID_PARAMS, message="this server does not paginate; unexpected cursor") + + +def _check_paths(paths: Sequence[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) + return list(paths) + + +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, + 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 = [path for path in visible if path.startswith(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 + if not _is_str_mapping(parsed): + raise ValueError("frontmatter must be a mapping with string keys") + try: + return to_jsonable_python(parsed) + except ValueError as exc: + raise ValueError(f"frontmatter is not JSON-serializable: {exc}") from exc + + +def _is_str_mapping(value: object) -> bool: + return isinstance(value, Mapping) and all(isinstance(key, str) for key in cast(Mapping[object, object], value)) + + +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_types(paths: Sequence[str]) -> dict[str, str]: + return { + path: "text/markdown" + if path.split("/")[-1] == _SKILL_FILE + else mimetypes.guess_type(path)[0] or "application/octet-stream" + 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..f68674a8db --- /dev/null +++ b/src/mcp/server/skills/filesystem.py @@ -0,0 +1,101 @@ +"""`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 reports junctions as reparse points rather than as symlinks; `stat` only types this constant on Windows. +_WINDOWS_REPARSE_POINT = 0x400 + + +class FilesystemSkillProvider: + """A `SkillProvider` that reads skill files from a local directory. + + Only regular files are served. Symlinks (and Windows junctions) are never + followed, so a skill directory can't expose files that live outside it; they + are skipped with a warning when listing and refused when read. 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. + 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 ("", ".", "..") 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: + 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") + return file.read() + + +def _is_link(result: os.stat_result) -> bool: + return stat.S_ISLNK(result.st_mode) or bool(getattr(result, "st_file_attributes", 0) & _WINDOWS_REPARSE_POINT) diff --git a/src/mcp/server/skills/provider.py b/src/mcp/server/skills/provider.py new file mode 100644 index 0000000000..8e0f40bf0d --- /dev/null +++ b/src/mcp/server/skills/provider.py @@ -0,0 +1,31 @@ +"""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, recursively. + + Directories are not listed, and each path appears once. + """ + ... + + 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/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..f4fef82b20 --- /dev/null +++ b/tests/server/skills/test_catalog.py @@ -0,0 +1,558 @@ +"""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 + +import pytest +from mcp_types import INTERNAL_ERROR, INVALID_PARAMS, BlobResourceContents, TextResourceContents +from pydantic import BaseModel, ConfigDict + +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, SkillCatalog +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, +) + +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 _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_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_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"), + ], +) +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 catalog.skills[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/blob.unknownext": 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/blob.unknownext"].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 catalog.skills] == ["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 + + +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..5fd2f97d25 --- /dev/null +++ b/tests/server/skills/test_filesystem.py @@ -0,0 +1,161 @@ +"""Tests for `FilesystemSkillProvider` (`mcp.server.skills.filesystem`).""" + +import logging +import os +import sys +from pathlib import Path + +import pytest + +from mcp.server.skills import FilesystemSkillProvider + +pytestmark = pytest.mark.anyio + +FILESYSTEM_LOGGER = "mcp.server.skills.filesystem" + +requires_symlinks = pytest.mark.skipif(sys.platform == "win32", reason="creating symlinks needs privileges on Windows") +requires_fifos = pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="named pipes are POSIX-only") + + +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"] + + +@requires_symlinks +async def test_list_files_skips_symlinks_with_a_warning(tmp_path: Path, caplog: pytest.LogCaptureFixture) -> None: + """SDK-defined: links 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") + (root / "skill" / "linked-file.txt").symlink_to(outside / "secret.txt") + (root / "skill" / "linked-dir").symlink_to(outside, target_is_directory=True) + + with caplog.at_level(logging.WARNING, logger=FILESYSTEM_LOGGER): + files = await FilesystemSkillProvider(root).list_files() + + assert files == ["skill/SKILL.md"] + messages = [record.getMessage() for record in caplog.records] + assert any("linked-file.txt" in message for message in messages) + assert any("linked-dir" in message for message in messages) + + +@requires_fifos +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) + + +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_symlinks +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") + + +@requires_symlinks +async def test_read_file_refuses_a_directory_swapped_for_a_symlink_after_listing(tmp_path: Path) -> None: + """SDK-defined: an intermediate directory replaced by a symlink 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() + (root / "skill").symlink_to(outside, target_is_directory=True) + + with pytest.raises(OSError): + await provider.read_file("skill/SKILL.md") + + +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 +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") From baabbd1e728fdd96d18a84fd140ce9fefe5f7294 Mon Sep 17 00:00:00 2001 From: vijaydeepsinha Date: Sun, 25 Oct 2026 21:47:39 +0530 Subject: [PATCH 02/12] Add optional pagination and harden SkillCatalog and FilesystemSkillProvider --- src/mcp/server/skills/__init__.py | 7 + src/mcp/server/skills/_extension.py | 7 +- src/mcp/server/skills/catalog.py | 139 +++++++++++++----- src/mcp/server/skills/filesystem.py | 11 +- tests/server/skills/test_catalog.py | 161 ++++++++++++++++++++- tests/server/skills/test_filesystem.py | 189 ++++++++++++++++++++++--- 6 files changed, 450 insertions(+), 64 deletions(-) diff --git a/src/mcp/server/skills/__init__.py b/src/mcp/server/skills/__init__.py index b8e64e619b..7d4b504b4b 100644 --- a/src/mcp/server/skills/__init__.py +++ b/src/mcp/server/skills/__init__.py @@ -13,6 +13,9 @@ 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=[...]) @@ -24,6 +27,10 @@ async def get_skill(ctx, params): 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 diff --git a/src/mcp/server/skills/_extension.py b/src/mcp/server/skills/_extension.py index 36eee9fa5c..0d1ad12b62 100644 --- a/src/mcp/server/skills/_extension.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 index aaa87b0006..070dd85f9b 100644 --- a/src/mcp/server/skills/catalog.py +++ b/src/mcp/server/skills/catalog.py @@ -2,19 +2,21 @@ from __future__ import annotations +import base64 import hashlib +import json import logging import mimetypes import re from collections.abc import Callable, Mapping, Sequence -from dataclasses import dataclass -from typing import Any, Literal, cast +from pathlib import PurePosixPath +from typing import Any, Literal, TypeVar from urllib.parse import quote import anyio.to_thread -from mcp_types import INVALID_PARAMS +from mcp_types import INVALID_PARAMS, CacheableResult from mcp_types import Resource as ResourceEntry -from pydantic import PrivateAttr, ValidationError +from pydantic import BaseModel, PrivateAttr, TypeAdapter, ValidationError from pydantic_core import to_jsonable_python from mcp.server.context import ServerRequestContext @@ -27,6 +29,7 @@ from mcp.shared.skills import ( MAX_RESOURCES_PER_SKILL, MAX_TOTAL_SIZE, + Frontmatter, GetSkillParams, GetSkillResult, ListSkillsParams, @@ -44,14 +47,22 @@ _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", +} -@dataclass(frozen=True) -class _Fingerprint: +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`. @@ -102,7 +113,13 @@ def __init__( 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()` instead. + + Raises: + ValidationError: If `ttl_ms` is negative. + """ super().__init__( list_skills=self._list_handler, get_skill=self._get_handler, @@ -112,8 +129,8 @@ def __init__( self._by_uri = {skill.uri: skill for skill in skills} self._files = dict(files) self._directories = directories - self._ttl_ms = ttl_ms - self._cache_scope: Literal["public", "private"] = cache_scope + self._cache = CacheableResult(ttl_ms=ttl_ms, cache_scope=cache_scope) + self._page_size = page_size @classmethod async def load( @@ -125,6 +142,7 @@ async def load( directory_read: bool = False, ttl_ms: int = 0, cache_scope: Literal["public", "private"] = "private", + page_size: int | None = None, ) -> SkillCatalog: """Scan `provider` and build a catalog of the skills it holds. @@ -145,14 +163,20 @@ async def load( 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, or if the provider - lists a path that is empty, absolute, contains a `.` or `..` segment, or - appears twice. + 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. """ - paths = _check_paths(await provider.list_files()) + if page_size is not None and page_size < 1: + raise ValueError("page_size must be at least 1") + paths = _check_paths(list(await provider.list_files())) visible = [path for path in paths if not any(segment.startswith(".") for segment in path.split("/"))] skill_paths = sorted(path for path in visible if path.split("/")[-1] == _SKILL_FILE) @@ -203,6 +227,7 @@ async def load( directories=_directory_index(files) if directory_read else None, ttl_ms=ttl_ms, cache_scope=cache_scope, + page_size=page_size, ) @property @@ -215,33 +240,66 @@ def resources(self) -> Sequence[ResourceBinding]: 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: - _reject_cursor(params.cursor) - return ListSkillsResult(skills=list(self._catalog_skills), ttl_ms=self._ttl_ms, cache_scope=self._cache_scope) + 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._ttl_ms, cache_scope=self._cache_scope) + 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 - _reject_cursor(params.cursor) children = self._directories.get(params.uri) if children is None: raise MCPError(code=INVALID_PARAMS, message=f"unknown directory: {params.uri}") - return ReadDirectoryResult(resources=list(children), ttl_ms=self._ttl_ms, cache_scope=self._cache_scope) + 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 _reject_cursor(cursor: str | None) -> None: - # The catalog returns everything on one page, so it never issues a cursor. +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`. Only the offsets this + function issues are valid; 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: - raise MCPError(code=INVALID_PARAMS, message="this server does not paginate; unexpected cursor") + 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: Sequence[str]) -> list[str]: +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("/")): @@ -249,7 +307,12 @@ def _check_paths(paths: Sequence[str]) -> list[str]: if path in seen: raise ValueError(f"provider listed {path!r} more than once") seen.add(path) - return list(paths) + 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 _fingerprint(data: bytes) -> _Fingerprint: @@ -306,16 +369,14 @@ def _parse_frontmatter(data: bytes, parse: Callable[[str], Mapping[str, Any]]) - 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 - if not _is_str_mapping(parsed): - raise ValueError("frontmatter must be a mapping with string keys") try: - return to_jsonable_python(parsed) - except ValueError as exc: - raise ValueError(f"frontmatter is not JSON-serializable: {exc}") from exc - - -def _is_str_mapping(value: object) -> bool: - return isinstance(value, Mapping) and all(isinstance(key, str) for key in cast(Mapping[object, object], value)) + 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: @@ -332,13 +393,17 @@ 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: "text/markdown" - if path.split("/")[-1] == _SKILL_FILE - else mimetypes.guess_type(path)[0] or "application/octet-stream" - for path in paths - } + return {path: _guess_mime_type(path) for path in paths} def _file_resource( diff --git a/src/mcp/server/skills/filesystem.py b/src/mcp/server/skills/filesystem.py index f68674a8db..66e9a4d5d6 100644 --- a/src/mcp/server/skills/filesystem.py +++ b/src/mcp/server/skills/filesystem.py @@ -18,8 +18,9 @@ _READ_FLAGS = os.O_RDONLY | getattr(os, "O_NOFOLLOW", 0) | getattr(os, "O_BINARY", 0) | getattr(os, "O_NONBLOCK", 0) -# Windows reports junctions as reparse points rather than as symlinks; `stat` only types this constant on Windows. -_WINDOWS_REPARSE_POINT = 0x400 +# 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: @@ -77,7 +78,7 @@ def _walk(self, directory: Path, prefix: list[str], files: list[str]) -> None: self._walk(Path(entry.path), segments, files) elif stat.S_ISREG(result.st_mode): files.append("/".join(segments)) - else: + else: # pragma: lax no cover logger.warning("Skipping %s: not a regular file", entry.path) def _read_file(self, segments: list[str]) -> bytes: @@ -93,9 +94,9 @@ def _read_file(self, segments: list[str]) -> bytes: 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") + raise OSError(f"{current} is not a regular file") # pragma: lax no cover return file.read() def _is_link(result: os.stat_result) -> bool: - return stat.S_ISLNK(result.st_mode) or bool(getattr(result, "st_file_attributes", 0) & _WINDOWS_REPARSE_POINT) + return stat.S_ISLNK(result.st_mode) or getattr(result, "st_reparse_tag", 0) in _WINDOWS_LINK_REPARSE_TAGS diff --git a/tests/server/skills/test_catalog.py b/tests/server/skills/test_catalog.py index f4fef82b20..8eac66af80 100644 --- a/tests/server/skills/test_catalog.py +++ b/tests/server/skills/test_catalog.py @@ -11,16 +11,23 @@ import logging from collections.abc import Mapping, Sequence from pathlib import Path -from typing import Any +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, SkillCatalog +from mcp.server.skills import ( + FilesystemSkillProvider, + GetSkillHandler, + ListSkillsHandler, + ReadDirectoryHandler, + SkillCatalog, +) from mcp.shared.exceptions import MCPError from mcp.shared.skills import ( MAX_RESOURCES_PER_SKILL, @@ -253,6 +260,31 @@ def parse_with_unserializable(text: str) -> Mapping[str, Any]: 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 catalog.skills] == ["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( @@ -279,6 +311,7 @@ async def test_strict_load_raises_one_error_naming_every_invalid_skill() -> None 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( @@ -351,7 +384,9 @@ async def test_skill_md_is_served_as_markdown_with_its_own_name_and_description( { "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: @@ -365,7 +400,9 @@ async def test_skill_md_is_served_as_markdown_with_its_own_name_and_description( ) 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: @@ -510,6 +547,126 @@ async def test_cache_hints_reach_list_get_and_directory_reads_on_the_2026_07_28_ 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_when_the_catalog_loads() -> None: + """SEP-2549: `ttlMs` is a non-negative integer, so a bad option fails at load rather than on every request.""" + provider = _MemoryProvider({"git-workflow/SKILL.md": _skill_md("git-workflow")}) + with pytest.raises(ValueError): + await SkillCatalog.load(provider, parse_frontmatter=_parse, ttl_ms=-1) + + +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")}) diff --git a/tests/server/skills/test_filesystem.py b/tests/server/skills/test_filesystem.py index 5fd2f97d25..f1dc715da0 100644 --- a/tests/server/skills/test_filesystem.py +++ b/tests/server/skills/test_filesystem.py @@ -2,8 +2,13 @@ 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 @@ -13,8 +18,62 @@ FILESYSTEM_LOGGER = "mcp.server.skills.filesystem" -requires_symlinks = pytest.mark.skipif(sys.platform == "win32", reason="creating symlinks needs privileges on Windows") -requires_fifos = pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="named pipes are POSIX-only") +# 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") + +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: @@ -39,28 +98,75 @@ async def test_list_files_prunes_hidden_entries(tmp_path: Path) -> None: assert await FilesystemSkillProvider(tmp_path).list_files() == ["skill/SKILL.md"] -@requires_symlinks -async def test_list_files_skips_symlinks_with_a_warning(tmp_path: Path, caplog: pytest.LogCaptureFixture) -> None: - """SDK-defined: links are never followed, so a skill directory can't expose files outside the root.""" +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") - (root / "skill" / "linked-file.txt").symlink_to(outside / "secret.txt") - (root / "skill" / "linked-dir").symlink_to(outside, target_is_directory=True) + 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"] - messages = [record.getMessage() for record in caplog.records] - assert any("linked-file.txt" in message for message in messages) - assert any("linked-dir" in message for message in messages) + 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 + +@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") @@ -98,7 +204,7 @@ async def test_read_file_raises_os_error_for_a_missing_file(tmp_path: Path) -> N await FilesystemSkillProvider(tmp_path).read_file("missing.md") -@requires_symlinks +@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" @@ -116,9 +222,21 @@ async def test_read_file_refuses_a_file_swapped_for_a_symlink_after_listing(tmp_ await provider.read_file("skill/SKILL.md") -@requires_symlinks -async def test_read_file_refuses_a_directory_swapped_for_a_symlink_after_listing(tmp_path: Path) -> None: - """SDK-defined: an intermediate directory replaced by a symlink is not traversed.""" +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") @@ -130,12 +248,35 @@ async def test_read_file_refuses_a_directory_swapped_for_a_symlink_after_listing (root / "skill" / "SKILL.md").unlink() (root / "skill").rmdir() - (root / "skill").symlink_to(outside, target_is_directory=True) + 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") @@ -152,10 +293,24 @@ async def test_read_file_refuses_a_directory(tmp_path: Path) -> None: await FilesystemSkillProvider(tmp_path).read_file("skill") -@requires_fifos +@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" From 036abf724feb3dc4346249b45680b68c05876584 Mon Sep 17 00:00:00 2001 From: vijaydeepsinha Date: Sun, 25 Oct 2026 21:48:06 +0530 Subject: [PATCH 03/12] Document serving skills from a directory --- docs/advanced/skills.md | 131 ++++++++++++++++++ docs_src/skills/skills/git-workflow/SKILL.md | 9 ++ .../git-workflow/references/conventions.md | 5 + docs_src/skills/tutorial002.py | 25 ++++ tests/docs_src/test_skills.py | 25 +++- 5 files changed, 194 insertions(+), 1 deletion(-) create mode 100644 docs_src/skills/skills/git-workflow/SKILL.md create mode 100644 docs_src/skills/skills/git-workflow/references/conventions.md create mode 100644 docs_src/skills/tutorial002.py diff --git a/docs/advanced/skills.md b/docs/advanced/skills.md index 18d3ae7825..0dc046da45 100644 --- a/docs/advanced/skills.md +++ b/docs/advanced/skills.md @@ -22,6 +22,10 @@ skim that page first. 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. + `SkillCatalog` is a provider built on top of `Skills` that implements these handlers for a + source of skill files, such as a local directory. See + [Serving skills from a directory](#serving-skills-from-a-directory). + ## Serving a skill Here's a server that serves one skill: @@ -116,6 +120,124 @@ 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 + +`SkillCatalog` is a `Skills` extension that discovers skills in a `SkillProvider` and answers +`skills/list`, `skills/get`, and optionally `resources/directory/read` from what it finds. It +removes the need to write handlers or to register each file as a resource. + +```python title="server.py" hl_lines="11-15" +--8<-- "docs_src/skills/tutorial002.py" +``` + +`SkillCatalog.load()` is a coroutine, so the server is constructed inside a running event loop. +`FilesystemSkillProvider` reads skills from a local directory. `parse_frontmatter` supplies the +YAML parser; the SDK does not depend on one. + +### How skills are discovered + +A skill is a directory that contains a `SKILL.md` file. The skill's files are all files beneath +that directory, including the files of any skill nested inside it. A nested skill is also listed +as a skill in its own right. + +Each file is served as a `skill://` resource. Its URI is `skill://` followed by its path, with +each path segment percent-encoded. A file has the same URI regardless of which skill lists it. + +Files and directories whose names begin with `.` are excluded. They are never read, listed, or +served. Files outside any skill directory are not served. + +The catalog reads and hashes every file once, when `load()` runs, and serves that snapshot. The +`resources` manifest of each skill therefore describes the files exactly as they were read. +The catalog is fixed for the lifetime of the server; to publish changes, create a new catalog. + +### Options + +`SkillCatalog.load(provider, *, parse_frontmatter, ...)` accepts the following arguments. + +| Argument | Default | Description | +|---|---|---| +| `provider` | required | The `SkillProvider` that lists and reads the files. | +| `parse_frontmatter` | required | Receives the text between the `---` lines of a `SKILL.md` and returns a mapping with `str` keys, for example `yaml.safe_load`. | +| `strict` | `False` | Controls the handling of invalid skills. See [Invalid skills](#invalid-skills). | +| `directory_read` | `False` | Serves `resources/directory/read` and advertises the `directoryRead` setting. | +| `ttl_ms` | `0` | The `ttlMs` value set on `skills/list`, `skills/get` and directory read results. | +| `cache_scope` | `"private"` | The `cacheScope` value set on the same results. | +| `page_size` | `None` | The maximum number of entries in one `skills/list` or directory read response. See [Pagination](#pagination). | + +`ttl_ms` and `cache_scope` follow the rules described in +[Protocol version and caching](#protocol-version-and-caching). A negative `ttl_ms` raises +`ValueError`. + +The frontmatter mapping is returned in `skills/list` and `skills/get` without modification, +including keys the SDK does not interpret. Values are converted to JSON; for example, YAML dates +are returned as ISO 8601 strings. A value with no JSON representation, such as NaN, makes the +skill invalid. + +With `directory_read=True`, every directory that contains a served file can be read, including +directories above the skill root such as `skill://acme`. A directory lists its direct children; +subdirectories are returned with the MIME type `inode/directory`. Empty directories are not served. + +### Pagination + +By default, `skills/list` and `resources/directory/read` return every entry in a single response. +When `page_size` is set, each response contains at most that many entries, and a response that is +followed by another page carries a `nextCursor`. The client passes it back as `cursor`, as for any +other paginated list in the protocol. `SkillsClient.list_skills()` and `read_directory()` do this +automatically. + +A skill entry is never split across pages. The cursor is opaque to clients. A cursor that the +catalog did not issue is rejected with Invalid params (`-32602`), and so is any cursor when +`page_size` is not 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, cannot +be parsed, or fails the SEP-2640 requirements for `name` and `description`. A `SKILL.md` placed +directly in the provider root is also invalid, because the skill has no directory name. + +* With `strict=False`, an invalid skill is omitted and a warning is logged. +* With `strict=True`, `load()` raises a single `ValueError` that lists every invalid skill. + +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`). + +### 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, recursively and without directories. `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 are never followed: they are omitted, with a warning, +when listing, and refused when read. A skill directory therefore cannot expose files outside +itself. 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 @@ -134,3 +256,12 @@ your own handler, to decide. 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. + +[`SkillCatalog`](#serving-skills-from-a-directory) is such a provider. It has the following +limitations: + +* It does not include a YAML parser. The `parse_frontmatter` argument supplies one. +* The catalog is not reloaded. To publish changes, create a new catalog. +* Empty directories are not served by directory reads. +* Pagination is opt-in through `page_size`. Without it, `skills/list` and directory reads return + all entries in a single response. 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..17b69e6988 --- /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: + catalog = await SkillCatalog.load( + FilesystemSkillProvider(root), + parse_frontmatter=yaml.safe_load, + directory_read=True, + ) + return MCPServer("skills", extensions=[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/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") From 7cc4f9942fd802165678f27e4332e454ee6d45bb Mon Sep 17 00:00:00 2001 From: vijaydeepsinha Date: Sun, 1 Nov 2026 00:15:54 +0530 Subject: [PATCH 04/12] updated the sample code --- docs_src/skills/tutorial002.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs_src/skills/tutorial002.py b/docs_src/skills/tutorial002.py index 17b69e6988..82a4b9ddb0 100644 --- a/docs_src/skills/tutorial002.py +++ b/docs_src/skills/tutorial002.py @@ -8,12 +8,12 @@ async def build_server(root: Path) -> MCPServer: - catalog = await SkillCatalog.load( + ext_skills = await SkillCatalog.load( FilesystemSkillProvider(root), parse_frontmatter=yaml.safe_load, directory_read=True, ) - return MCPServer("skills", extensions=[catalog]) + return MCPServer("MCP-over-skills", extensions=[ext_skills]) async def main() -> None: From 3c63a53627ed9a88a190d0c99501daebc1b7c3df Mon Sep 17 00:00:00 2001 From: vijaydeepsinha Date: Sun, 1 Nov 2026 00:34:52 +0530 Subject: [PATCH 05/12] made some cleanup, added uts and few checks --- docs/advanced/skills.md | 8 ++++++-- src/mcp/server/skills/catalog.py | 6 ++++-- src/mcp/server/skills/filesystem.py | 15 +++++++++------ tests/server/skills/test_catalog.py | 26 ++++++++++++++++++++++++++ 4 files changed, 45 insertions(+), 10 deletions(-) diff --git a/docs/advanced/skills.md b/docs/advanced/skills.md index 0dc046da45..ac7941fc9b 100644 --- a/docs/advanced/skills.md +++ b/docs/advanced/skills.md @@ -132,7 +132,8 @@ removes the need to write handlers or to register each file as a resource. `SkillCatalog.load()` is a coroutine, so the server is constructed inside a running event loop. `FilesystemSkillProvider` reads skills from a local directory. `parse_frontmatter` supplies the -YAML parser; the SDK does not depend on one. +YAML parser. The SDK does not install one, so this example uses [PyYAML](https://pypi.org/project/PyYAML/), +which you add to your own project (`uv add pyyaml`). ### How skills are discovered @@ -211,7 +212,10 @@ File content is returned unchanged. A file whose bytes are valid UTF-8 is return 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`). +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 diff --git a/src/mcp/server/skills/catalog.py b/src/mcp/server/skills/catalog.py index 070dd85f9b..9e5daa2a96 100644 --- a/src/mcp/server/skills/catalog.py +++ b/src/mcp/server/skills/catalog.py @@ -10,7 +10,7 @@ import re from collections.abc import Callable, Mapping, Sequence from pathlib import PurePosixPath -from typing import Any, Literal, TypeVar +from typing import Any, Literal, Self, TypeVar from urllib.parse import quote import anyio.to_thread @@ -143,7 +143,7 @@ async def load( ttl_ms: int = 0, cache_scope: Literal["public", "private"] = "private", page_size: int | None = None, - ) -> SkillCatalog: + ) -> 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 @@ -176,6 +176,8 @@ async def load( """ 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 = [path for path in paths if not any(segment.startswith(".") for segment in path.split("/"))] skill_paths = sorted(path for path in visible if path.split("/")[-1] == _SKILL_FILE) diff --git a/src/mcp/server/skills/filesystem.py b/src/mcp/server/skills/filesystem.py index 66e9a4d5d6..5e1bc705f2 100644 --- a/src/mcp/server/skills/filesystem.py +++ b/src/mcp/server/skills/filesystem.py @@ -26,10 +26,13 @@ class FilesystemSkillProvider: """A `SkillProvider` that reads skill files from a local directory. - Only regular files are served. Symlinks (and Windows junctions) are never - followed, so a skill directory can't expose files that live outside it; they - are skipped with a warning when listing and refused when read. The blocking - file system calls run in a worker thread. + 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: @@ -78,7 +81,7 @@ def _walk(self, directory: Path, prefix: list[str], files: list[str]) -> None: self._walk(Path(entry.path), segments, files) elif stat.S_ISREG(result.st_mode): files.append("/".join(segments)) - else: # pragma: lax no cover + 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: @@ -94,7 +97,7 @@ def _read_file(self, segments: list[str]) -> bytes: 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 + raise OSError(f"{current} is not a regular file") # pragma: lax no cover # only if swapped after lstat return file.read() diff --git a/tests/server/skills/test_catalog.py b/tests/server/skills/test_catalog.py index 8eac66af80..3d3c7d95f0 100644 --- a/tests/server/skills/test_catalog.py +++ b/tests/server/skills/test_catalog.py @@ -657,6 +657,32 @@ async def test_a_negative_ttl_is_rejected_when_the_catalog_loads() -> None: await SkillCatalog.load(provider, parse_frontmatter=_parse, ttl_ms=-1) +async def test_invalid_options_are_rejected_before_the_provider_is_touched() -> None: + """SDK-defined: option errors surface before any provider call; a call would raise `NotImplementedError`.""" + + class _UntouchableProvider: + async def list_files(self) -> Sequence[str]: # pragma: no cover + raise NotImplementedError + + async def read_file(self, path: str) -> bytes: # pragma: no cover + raise NotImplementedError + + with pytest.raises(ValueError): + await SkillCatalog.load(_UntouchableProvider(), 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) == ( From 18eece67177dd2e9d437b5134496b7f656091557 Mon Sep 17 00:00:00 2001 From: vijaydeepsinha Date: Sun, 1 Nov 2026 00:46:02 +0530 Subject: [PATCH 06/12] modified the tests --- tests/server/skills/test_catalog.py | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/tests/server/skills/test_catalog.py b/tests/server/skills/test_catalog.py index 3d3c7d95f0..1f5b064682 100644 --- a/tests/server/skills/test_catalog.py +++ b/tests/server/skills/test_catalog.py @@ -27,6 +27,7 @@ ListSkillsHandler, ReadDirectoryHandler, SkillCatalog, + SkillProvider, ) from mcp.shared.exceptions import MCPError from mcp.shared.skills import ( @@ -650,25 +651,14 @@ async def test_a_page_size_below_one_is_rejected_when_the_catalog_loads(page_siz await SkillCatalog.load(provider, parse_frontmatter=_parse, page_size=page_size) -async def test_a_negative_ttl_is_rejected_when_the_catalog_loads() -> None: - """SEP-2549: `ttlMs` is a non-negative integer, so a bad option fails at load rather than on every request.""" - provider = _MemoryProvider({"git-workflow/SKILL.md": _skill_md("git-workflow")}) - with pytest.raises(ValueError): - await SkillCatalog.load(provider, parse_frontmatter=_parse, ttl_ms=-1) - - -async def test_invalid_options_are_rejected_before_the_provider_is_touched() -> None: - """SDK-defined: option errors surface before any provider call; a call would raise `NotImplementedError`.""" - - class _UntouchableProvider: - async def list_files(self) -> Sequence[str]: # pragma: no cover - raise NotImplementedError - - async def read_file(self, path: str) -> bytes: # pragma: no cover - raise NotImplementedError +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(_UntouchableProvider(), parse_frontmatter=_parse, ttl_ms=-1) + 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: From c71635cf4cec17fcdd5cfd43728c2a00aff8cab5 Mon Sep 17 00:00:00 2001 From: vijaydeepsinha Date: Sun, 1 Nov 2026 01:00:27 +0530 Subject: [PATCH 07/12] Made SkillCatalog's public surface minimal --- src/mcp/server/skills/catalog.py | 9 ++------- tests/server/skills/test_catalog.py | 13 ++++++++++--- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/mcp/server/skills/catalog.py b/src/mcp/server/skills/catalog.py index 9e5daa2a96..703eba84d8 100644 --- a/src/mcp/server/skills/catalog.py +++ b/src/mcp/server/skills/catalog.py @@ -109,13 +109,13 @@ def __init__( self, *, skills: Sequence[Skill], - files: Mapping[str, _SkillFileResource], + 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()` instead. + """Assemble a catalog from already-validated parts. Use `SkillCatalog.load()`, which builds them. Raises: ValidationError: If `ttl_ms` is negative. @@ -232,11 +232,6 @@ async def load( page_size=page_size, ) - @property - def skills(self) -> Sequence[Skill]: - """The valid skills, sorted by URI.""" - return self._catalog_skills - 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)] diff --git a/tests/server/skills/test_catalog.py b/tests/server/skills/test_catalog.py index 1f5b064682..259025d108 100644 --- a/tests/server/skills/test_catalog.py +++ b/tests/server/skills/test_catalog.py @@ -42,6 +42,7 @@ ReadDirectoryParams, ReadDirectoryRequest, ReadDirectoryResult, + Skill, ) pytestmark = pytest.mark.anyio @@ -88,6 +89,12 @@ 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]) @@ -283,7 +290,7 @@ async def list_files(self) -> Sequence[str]: catalog = await SkillCatalog.load( GeneratorProvider({"git-workflow/SKILL.md": _skill_md("git-workflow")}), parse_frontmatter=_parse ) - assert [skill.uri for skill in catalog.skills] == ["skill://git-workflow/SKILL.md"] + 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: @@ -345,7 +352,7 @@ def parse_with_date(text: str) -> Mapping[str, Any]: catalog = await SkillCatalog.load( _MemoryProvider({"git-workflow/SKILL.md": _skill_md("git-workflow")}), parse_frontmatter=parse_with_date ) - assert catalog.skills[0].frontmatter == { + assert (await _listed(catalog))[0].frontmatter == { "name": "git-workflow", "description": "d", "released": "2026-07-28", @@ -450,7 +457,7 @@ async def test_a_skill_over_the_recommended_limits_is_still_served_with_a_warnin with caplog.at_level(logging.WARNING, logger=CATALOG_LOGGER): catalog = await SkillCatalog.load(_MemoryProvider(files), parse_frontmatter=_parse) - assert [skill.uri for skill in catalog.skills] == ["skill://big/SKILL.md"] + 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 From 2fc19322bb22ac37c003ad6bc5976975cde9c9d9 Mon Sep 17 00:00:00 2001 From: vijaydeepsinha Date: Sun, 1 Nov 2026 01:23:19 +0530 Subject: [PATCH 08/12] Rewrite the SkillCatalog docs tutorial-first --- docs/advanced/skills.md | 167 +++++++++++++++++++-------------- docs_src/skills/tutorial002.py | 4 +- 2 files changed, 100 insertions(+), 71 deletions(-) diff --git a/docs/advanced/skills.md b/docs/advanced/skills.md index ac7941fc9b..f26e092f32 100644 --- a/docs/advanced/skills.md +++ b/docs/advanced/skills.md @@ -17,13 +17,12 @@ 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. - `SkillCatalog` is a provider built on top of `Skills` that implements these handlers for a - source of skill files, such as a local directory. See + 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 @@ -122,82 +121,115 @@ setting. ## Serving skills from a directory -`SkillCatalog` is a `Skills` extension that discovers skills in a `SkillProvider` and answers -`skills/list`, `skills/get`, and optionally `resources/directory/read` from what it finds. It -removes the need to write handlers or to register each file as a resource. +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/ +└── git-workflow/ + ├── SKILL.md + └── references/ + └── conventions.md +``` + +Load it and pass it to your server: ```python title="server.py" hl_lines="11-15" --8<-- "docs_src/skills/tutorial002.py" ``` -`SkillCatalog.load()` is a coroutine, so the server is constructed inside a running event loop. -`FilesystemSkillProvider` reads skills from a local directory. `parse_frontmatter` supplies the -YAML parser. The SDK does not install one, so this example uses [PyYAML](https://pypi.org/project/PyYAML/), -which you add to your own project (`uv add pyyaml`). +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. -### How skills are discovered +* `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)). -A skill is a directory that contains a `SKILL.md` file. The skill's files are all files beneath -that directory, including the files of any skill nested inside it. A nested skill is also listed -as a skill in its own right. +`SkillCatalog.load()` is a coroutine, so build the server inside a running event loop, as +`main()` does above. -Each file is served as a `skill://` resource. Its URI is `skill://` followed by its path, with -each path segment percent-encoded. A file has the same URI regardless of which skill lists it. +### How skills are found -Files and directories whose names begin with `.` are excluded. They are never read, listed, or -served. Files outside any skill directory are not served. +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. -The catalog reads and hashes every file once, when `load()` runs, and serves that snapshot. The -`resources` manifest of each skill therefore describes the files exactly as they were read. -The catalog is fixed for the lifetime of the server; to publish changes, create a new catalog. +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. -### Options +!!! 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. -`SkillCatalog.load(provider, *, parse_frontmatter, ...)` accepts the following arguments. + 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. -| Argument | Default | Description | -|---|---|---| -| `provider` | required | The `SkillProvider` that lists and reads the files. | -| `parse_frontmatter` | required | Receives the text between the `---` lines of a `SKILL.md` and returns a mapping with `str` keys, for example `yaml.safe_load`. | -| `strict` | `False` | Controls the handling of invalid skills. See [Invalid skills](#invalid-skills). | -| `directory_read` | `False` | Serves `resources/directory/read` and advertises the `directoryRead` setting. | -| `ttl_ms` | `0` | The `ttlMs` value set on `skills/list`, `skills/get` and directory read results. | -| `cache_scope` | `"private"` | The `cacheScope` value set on the same results. | -| `page_size` | `None` | The maximum number of entries in one `skills/list` or directory read response. See [Pagination](#pagination). | +### Directory reads from a catalog -`ttl_ms` and `cache_scope` follow the rules described in -[Protocol version and caching](#protocol-version-and-caching). A negative `ttl_ms` raises -`ValueError`. +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. -The frontmatter mapping is returned in `skills/list` and `skills/get` without modification, -including keys the SDK does not interpret. Values are converted to JSON; for example, YAML dates -are returned as ISO 8601 strings. A value with no JSON representation, such as NaN, makes the -skill invalid. +### Caching hints -With `directory_read=True`, every directory that contains a served file can be read, including -directories above the skill root such as `skill://acme`. A directory lists its direct children; -subdirectories are returned with the MIME type `inode/directory`. Empty directories are not served. +`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, `skills/list` and `resources/directory/read` return every entry in a single response. -When `page_size` is set, each response contains at most that many entries, and a response that is -followed by another page carries a `nextCursor`. The client passes it back as `cursor`, as for any -other paginated list in the protocol. `SkillsClient.list_skills()` and `read_directory()` do this -automatically. +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. -A skill entry is never split across pages. The cursor is opaque to clients. A cursor that the -catalog did not issue is rejected with Invalid params (`-32602`), and so is any cursor when -`page_size` is not set. A `page_size` below 1 raises `ValueError`. +The cursor is opaque. One the catalog didn't issue 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, cannot +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 provider root is also invalid, because the skill has no directory name. +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. -* With `strict=False`, an invalid skill is omitted and a warning is logged. -* With `strict=True`, `load()` raises a single `ValueError` that lists every invalid skill. +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. @@ -254,18 +286,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. -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. +`SkillCatalog` covers the common case of a directory (or any other file source behind a +`SkillProvider`), with a few limits: -[`SkillCatalog`](#serving-skills-from-a-directory) is such a provider. It has the following -limitations: +* 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. -* It does not include a YAML parser. The `parse_frontmatter` argument supplies one. -* The catalog is not reloaded. To publish changes, create a new catalog. -* Empty directories are not served by directory reads. -* Pagination is opt-in through `page_size`. Without it, `skills/list` and directory reads return - all entries in a single response. +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/tutorial002.py b/docs_src/skills/tutorial002.py index 82a4b9ddb0..f9d7c10b34 100644 --- a/docs_src/skills/tutorial002.py +++ b/docs_src/skills/tutorial002.py @@ -8,12 +8,12 @@ async def build_server(root: Path) -> MCPServer: - ext_skills = await SkillCatalog.load( + skills_catalog = await SkillCatalog.load( FilesystemSkillProvider(root), parse_frontmatter=yaml.safe_load, directory_read=True, ) - return MCPServer("MCP-over-skills", extensions=[ext_skills]) + return MCPServer("MCP-over-skills", extensions=[skills_catalog]) async def main() -> None: From 4402d2abd6307f53d2257357e985a7fd8e8623f1 Mon Sep 17 00:00:00 2001 From: vijaydeepsinha Date: Sun, 1 Nov 2026 01:54:52 +0530 Subject: [PATCH 09/12] modified the documentation to include skills folder structure --- docs/advanced/skills.md | 48 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 44 insertions(+), 4 deletions(-) diff --git a/docs/advanced/skills.md b/docs/advanced/skills.md index f26e092f32..161d81d5af 100644 --- a/docs/advanced/skills.md +++ b/docs/advanced/skills.md @@ -127,11 +127,23 @@ or register each file: `SkillCatalog` reads the folder and answers everything fo Say you have this layout: ```text -skills/ -└── git-workflow/ - ├── SKILL.md +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 + └── 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: @@ -155,6 +167,27 @@ from the folder, and every file in it is served as a `skill://` resource. ### 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. @@ -163,6 +196,13 @@ Each file's URI is `skill://` followed by its path, with every segment percent-e 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 From be15d594f31356d8c43aae5f7ca704d6ece3f23d Mon Sep 17 00:00:00 2001 From: vijaydeepsinha Date: Sun, 1 Nov 2026 02:36:19 +0530 Subject: [PATCH 10/12] Reject Windows path syntax in FilesystemSkillProvider.read_file and tighten wording --- docs/advanced/skills.md | 13 +++++++----- src/mcp/server/skills/filesystem.py | 12 +++++++++-- src/mcp/server/skills/provider.py | 5 +++-- tests/server/skills/test_filesystem.py | 29 ++++++++++++++++++++++++++ 4 files changed, 50 insertions(+), 9 deletions(-) diff --git a/docs/advanced/skills.md b/docs/advanced/skills.md index 161d81d5af..6fbec98a22 100644 --- a/docs/advanced/skills.md +++ b/docs/advanced/skills.md @@ -254,8 +254,8 @@ Each response then holds at most 50 entries and carries a `nextCursor` while mor `SkillsClient.list_skills()` and `read_directory()` follow it for you. A skill entry is never split across pages. -The cursor is opaque. One the catalog didn't issue is rejected with Invalid params (`-32602`), and -so is any cursor when `page_size` isn't set. A `page_size` below 1 raises `ValueError`. +The cursor is opaque. A cursor the catalog would not issue 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 @@ -310,9 +310,12 @@ or `..` segment, is listed more than once, or lies beneath another listed file. 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 are never followed: they are omitted, with a warning, -when listing, and refused when read. A skill directory therefore cannot expose files outside -itself. Blocking file system calls run in a worker thread. +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 diff --git a/src/mcp/server/skills/filesystem.py b/src/mcp/server/skills/filesystem.py index 5e1bc705f2..d13a06b524 100644 --- a/src/mcp/server/skills/filesystem.py +++ b/src/mcp/server/skills/filesystem.py @@ -18,6 +18,10 @@ _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}) @@ -53,12 +57,16 @@ 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. + 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 ("", ".", "..") for segment in segments): + 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) diff --git a/src/mcp/server/skills/provider.py b/src/mcp/server/skills/provider.py index 8e0f40bf0d..da7839b49a 100644 --- a/src/mcp/server/skills/provider.py +++ b/src/mcp/server/skills/provider.py @@ -16,9 +16,10 @@ class SkillProvider(Protocol): """ async def list_files(self) -> Sequence[str]: - """Return the path of every file, recursively. + """Return the path of every file the provider serves, recursively. - Directories are not listed, and each path appears once. + Directories are not listed, and each path appears once. A provider may leave files out; + `FilesystemSkillProvider` omits any name that starts with `.`. """ ... diff --git a/tests/server/skills/test_filesystem.py b/tests/server/skills/test_filesystem.py index f1dc715da0..f66db2b461 100644 --- a/tests/server/skills/test_filesystem.py +++ b/tests/server/skills/test_filesystem.py @@ -22,6 +22,7 @@ # 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 @@ -199,6 +200,34 @@ async def test_read_file_rejects_empty_dot_and_dot_dot_segments(tmp_path: Path, 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") From 10e13e223946d21d63105e62263d55752456933d Mon Sep 17 00:00:00 2001 From: vijaydeepsinha Date: Sun, 1 Nov 2026 02:48:40 +0530 Subject: [PATCH 11/12] Build skill manifests with a bisect range instead of scanning every path --- src/mcp/server/skills/catalog.py | 18 ++++++++++++++---- tests/server/skills/test_catalog.py | 24 ++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/src/mcp/server/skills/catalog.py b/src/mcp/server/skills/catalog.py index 703eba84d8..6917341ea4 100644 --- a/src/mcp/server/skills/catalog.py +++ b/src/mcp/server/skills/catalog.py @@ -8,6 +8,7 @@ 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 @@ -179,8 +180,8 @@ async def load( if ttl_ms < 0: raise ValueError("ttl_ms must not be negative") paths = _check_paths(list(await provider.list_files())) - visible = [path for path in paths if not any(segment.startswith(".") for segment in path.split("/"))] - skill_paths = sorted(path for path in visible if path.split("/")[-1] == _SKILL_FILE) + 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() @@ -312,6 +313,15 @@ def _check_paths(paths: list[str]) -> list[str]: 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)) @@ -321,7 +331,7 @@ async def _load_skill( parse_frontmatter: Callable[[str], Mapping[str, Any]], skill_path: str, skill_md: bytes, - visible: Sequence[str], + 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. @@ -331,7 +341,7 @@ async def _load_skill( """ frontmatter = _parse_frontmatter(skill_md, parse_frontmatter) root = skill_path.removesuffix(_SKILL_FILE) - skill_files = [path for path in visible if path.startswith(root)] + 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)) diff --git a/tests/server/skills/test_catalog.py b/tests/server/skills/test_catalog.py index 259025d108..42b6cf63ed 100644 --- a/tests/server/skills/test_catalog.py +++ b/tests/server/skills/test_catalog.py @@ -181,6 +181,30 @@ async def test_a_nested_skill_is_listed_and_its_files_also_belong_to_the_parent_ ] +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( From f986b9a3d1018e624f376ef1da416249e7ba0bb1 Mon Sep 17 00:00:00 2001 From: vijaydeepsinha Date: Sun, 1 Nov 2026 02:56:55 +0530 Subject: [PATCH 12/12] modified the description of the cursor and list_files contracts as implemented --- docs/advanced/skills.md | 9 ++++++--- src/mcp/server/skills/catalog.py | 5 +++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/docs/advanced/skills.md b/docs/advanced/skills.md index 6fbec98a22..a24db2fa76 100644 --- a/docs/advanced/skills.md +++ b/docs/advanced/skills.md @@ -254,8 +254,10 @@ Each response then holds at most 50 entries and carries a `nextCursor` while mor `SkillsClient.list_skills()` and `read_directory()` follow it for you. A skill entry is never split across pages. -The cursor is opaque. A cursor the catalog would not issue is rejected with Invalid params -(`-32602`), and so is any cursor when `page_size` isn't set. A `page_size` below 1 raises `ValueError`. +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 @@ -300,7 +302,8 @@ class SkillProvider(Protocol): async def read_file(self, path: str) -> bytes: ... ``` -`list_files()` returns the path of every file, recursively and without directories. `read_file()` +`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. diff --git a/src/mcp/server/skills/catalog.py b/src/mcp/server/skills/catalog.py index 6917341ea4..e11ebbd79a 100644 --- a/src/mcp/server/skills/catalog.py +++ b/src/mcp/server/skills/catalog.py @@ -278,8 +278,9 @@ def _encode_cursor(offset: int) -> str: 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`. Only the offsets this - function issues are valid; anything else is Invalid params. + 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: