Skip to content

Commit 3c63a53

Browse files
made some cleanup, added uts and few checks
1 parent b5d40d6 commit 3c63a53

4 files changed

Lines changed: 45 additions & 10 deletions

File tree

‎docs/advanced/skills.md‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,8 @@ removes the need to write handlers or to register each file as a resource.
132132

133133
`SkillCatalog.load()` is a coroutine, so the server is constructed inside a running event loop.
134134
`FilesystemSkillProvider` reads skills from a local directory. `parse_frontmatter` supplies the
135-
YAML parser; the SDK does not depend on one.
135+
YAML parser. The SDK does not install one, so this example uses [PyYAML](https://pypi.org/project/PyYAML/),
136+
which you add to your own project (`uv add pyyaml`).
136137

137138
### How skills are discovered
138139

@@ -211,7 +212,10 @@ File content is returned unchanged. A file whose bytes are valid UTF-8 is return
211212
including any byte order mark and line endings; any other file is returned as a base64 blob. In
212213
both cases the content matches the `digest` and `size` in the skill's manifest. Each read is
213214
verified against the values recorded at load time, and a file that has changed is reported as an
214-
internal error (`-32603`).
215+
internal error (`-32603`) until you restart the server or create a new catalog.
216+
217+
`load()` reads and hashes every file of every valid skill once, so a large asset directory makes
218+
startup slower and temporarily uses memory for the largest file.
215219

216220
### Providers
217221

‎src/mcp/server/skills/catalog.py‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
import re
1111
from collections.abc import Callable, Mapping, Sequence
1212
from pathlib import PurePosixPath
13-
from typing import Any, Literal, TypeVar
13+
from typing import Any, Literal, Self, TypeVar
1414
from urllib.parse import quote
1515

1616
import anyio.to_thread
@@ -143,7 +143,7 @@ async def load(
143143
ttl_ms: int = 0,
144144
cache_scope: Literal["public", "private"] = "private",
145145
page_size: int | None = None,
146-
) -> SkillCatalog:
146+
) -> Self:
147147
"""Scan `provider` and build a catalog of the skills it holds.
148148
149149
A skill is a directory containing a `SKILL.md`; the skill's files are everything
@@ -176,6 +176,8 @@ async def load(
176176
"""
177177
if page_size is not None and page_size < 1:
178178
raise ValueError("page_size must be at least 1")
179+
if ttl_ms < 0:
180+
raise ValueError("ttl_ms must not be negative")
179181
paths = _check_paths(list(await provider.list_files()))
180182
visible = [path for path in paths if not any(segment.startswith(".") for segment in path.split("/"))]
181183
skill_paths = sorted(path for path in visible if path.split("/")[-1] == _SKILL_FILE)

‎src/mcp/server/skills/filesystem.py‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,13 @@
2626
class FilesystemSkillProvider:
2727
"""A `SkillProvider` that reads skill files from a local directory.
2828
29-
Only regular files are served. Symlinks (and Windows junctions) are never
30-
followed, so a skill directory can't expose files that live outside it; they
31-
are skipped with a warning when listing and refused when read. The blocking
32-
file system calls run in a worker thread.
29+
Only regular files are served. Symlinks (and Windows junctions) found while
30+
listing or reading are skipped with a warning or refused, so a skill directory
31+
doesn't expose files that live outside it. The root is trusted: this is a
32+
best-effort check, not a defence against someone who can modify the directory
33+
while the server runs. `SkillCatalog` additionally refuses any file whose bytes
34+
no longer match the digest taken when it was loaded. The blocking file system
35+
calls run in a worker thread.
3336
"""
3437

3538
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:
7881
self._walk(Path(entry.path), segments, files)
7982
elif stat.S_ISREG(result.st_mode):
8083
files.append("/".join(segments))
81-
else: # pragma: lax no cover
84+
else: # pragma: lax no cover # FIFO/device: no test on Windows
8285
logger.warning("Skipping %s: not a regular file", entry.path)
8386

8487
def _read_file(self, segments: list[str]) -> bytes:
@@ -94,7 +97,7 @@ def _read_file(self, segments: list[str]) -> bytes:
9497
fd = os.open(current, _READ_FLAGS)
9598
with os.fdopen(fd, "rb") as file:
9699
if not stat.S_ISREG(os.fstat(file.fileno()).st_mode):
97-
raise OSError(f"{current} is not a regular file") # pragma: lax no cover
100+
raise OSError(f"{current} is not a regular file") # pragma: lax no cover # only if swapped after lstat
98101
return file.read()
99102

100103

‎tests/server/skills/test_catalog.py‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -657,6 +657,32 @@ async def test_a_negative_ttl_is_rejected_when_the_catalog_loads() -> None:
657657
await SkillCatalog.load(provider, parse_frontmatter=_parse, ttl_ms=-1)
658658

659659

660+
async def test_invalid_options_are_rejected_before_the_provider_is_touched() -> None:
661+
"""SDK-defined: option errors surface before any provider call; a call would raise `NotImplementedError`."""
662+
663+
class _UntouchableProvider:
664+
async def list_files(self) -> Sequence[str]: # pragma: no cover
665+
raise NotImplementedError
666+
667+
async def read_file(self, path: str) -> bytes: # pragma: no cover
668+
raise NotImplementedError
669+
670+
with pytest.raises(ValueError):
671+
await SkillCatalog.load(_UntouchableProvider(), parse_frontmatter=_parse, ttl_ms=-1)
672+
673+
674+
async def test_load_returns_an_instance_of_the_subclass_it_is_called_on() -> None:
675+
"""SDK-defined: `load()` is a classmethod constructor, so a subclass gets its own type back."""
676+
677+
class _CustomCatalog(SkillCatalog):
678+
pass
679+
680+
provider = _MemoryProvider({"git-workflow/SKILL.md": _skill_md("git-workflow")})
681+
catalog = await _CustomCatalog.load(provider, parse_frontmatter=_parse)
682+
683+
assert isinstance(catalog, _CustomCatalog)
684+
685+
660686
async def test_the_handler_aliases_stay_importable_from_the_package() -> None:
661687
"""Compatibility: `mcp.server.skills` was a module before it became a package; its public names survive."""
662688
assert (GetSkillHandler, ListSkillsHandler, ReadDirectoryHandler) == (

0 commit comments

Comments
 (0)