From a4c6ae3cc3faacada86fa2b1238ff7218aa67b10 Mon Sep 17 00:00:00 2001 From: vijay Date: Wed, 9 Sep 2026 20:07:28 +0530 Subject: [PATCH 01/25] Add SEP-2640 protocol types and validation Wire types, request/result models, and SEP-2640 conformance validation (name/URI/frontmatter rules, resource-manifest completeness, digest and size verification) for the Skills extension, shared by the server and client surfaces. --- src/mcp/shared/skills.py | 289 +++++++++++++++++++++++++++++++++ tests/shared/test_skills.py | 314 ++++++++++++++++++++++++++++++++++++ 2 files changed, 603 insertions(+) create mode 100644 src/mcp/shared/skills.py create mode 100644 tests/shared/test_skills.py diff --git a/src/mcp/shared/skills.py b/src/mcp/shared/skills.py new file mode 100644 index 0000000000..06f643576c --- /dev/null +++ b/src/mcp/shared/skills.py @@ -0,0 +1,289 @@ +"""Wire types and SEP-2640 conformance checks for the Skills extension. + +Shared by the server (`mcp.server.skills`) and client (`mcp.client.skills`) +surfaces, mirroring how `mcp.shared.extension` hosts the identifier grammar +both tiers need. See https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2640. +""" + +from __future__ import annotations + +import hashlib +import re +from typing import Any, Literal +from urllib.parse import urlsplit + +from mcp_types import CacheableResult, PaginatedRequestParams, PaginatedResult, Request, RequestParams, Resource, Result +from pydantic import BaseModel, ConfigDict +from pydantic.alias_generators import to_camel + +EXTENSION_ID = "io.modelcontextprotocol/skills" +"""The Skills extension identifier, advertised under `ServerCapabilities.extensions`.""" + +METHOD_LIST = "skills/list" +METHOD_GET = "skills/get" +METHOD_READ_DIRECTORY = "resources/directory/read" + +MAX_RESOURCES_PER_SKILL = 512 +"""SEP-2640 per-skill resource-count limit, `SKILL.md` included.""" + +MAX_TOTAL_SIZE = 16 * 1024 * 1024 +"""SEP-2640 per-skill total-byte-size limit (16 MiB), summed over `resources[].size`.""" + +_NAME_RE = re.compile(r"^[a-z0-9]+(?:-[a-z0-9]+)*$") +_DIGEST_RE = re.compile(r"^sha256:[0-9a-f]{64}$") + + +class _SkillModel(BaseModel): + """Base for Skills value types: matches `mcp_types`' internal `MCPModel` config. + + `MCPModel` itself isn't public; every field defined below is already a + single word, so this only matters if a future field needs camelCase. + """ + + model_config = ConfigDict(alias_generator=to_camel, populate_by_name=True) + + +class SkillResource(_SkillModel): + """One file in a skill's manifest: `{uri, digest, size}`.""" + + uri: str + digest: str + """SHA-256 digest of the file's raw bytes, formatted `sha256:{64 hex chars}`.""" + size: int + """Length in bytes of the file's raw content.""" + + +Frontmatter = dict[str, Any] +"""A skill's `SKILL.md` YAML frontmatter, rendered verbatim as JSON.""" + +SkillResources = list[SkillResource] | Literal["dynamic"] +"""A skill's complete resource manifest, or the `"dynamic"` marker (SEP-2640 Resources).""" + + +class Skill(_SkillModel): + """An entry returned by `skills/list` or `skills/get`.""" + + uri: str + """Resource URI of the skill's `SKILL.md`.""" + frontmatter: Frontmatter + resources: SkillResources + + +class ListSkillsParams(PaginatedRequestParams): + """Parameters for `skills/list`.""" + + +class ListSkillsResult(PaginatedResult, CacheableResult): + """Result of `skills/list`. + + `ttl_ms`/`cache_scope` are SEP-2549 fields inherited from `CacheableResult`; + unlike a core spec method, nothing sieves them off the wire for a + pre-2026-07-28 connection automatically (see `mcp.server.skills`), so + callers constructing this directly for such a connection must omit them. + """ + + skills: list[Skill] + + +class GetSkillParams(RequestParams): + """Parameters for `skills/get`.""" + + uri: str + """URI of the skill's `SKILL.md`.""" + + +class GetSkillResult(Result): + """Result of `skills/get`.""" + + skill: Skill + + +class ReadDirectoryParams(PaginatedRequestParams): + """Parameters for `resources/directory/read`.""" + + uri: str + """URI of the directory resource whose direct children are listed.""" + + +class ReadDirectoryResult(PaginatedResult): + """Result of `resources/directory/read`.""" + + resources: list[Resource] + + +class ListSkillsRequest(Request[ListSkillsParams | None, Literal["skills/list"]]): + method: Literal["skills/list"] = "skills/list" + params: ListSkillsParams | None = None + + +class GetSkillRequest(Request[GetSkillParams, Literal["skills/get"]]): + method: Literal["skills/get"] = "skills/get" + params: GetSkillParams + + +class ReadDirectoryRequest(Request[ReadDirectoryParams, Literal["resources/directory/read"]]): + method: Literal["resources/directory/read"] = "resources/directory/read" + params: ReadDirectoryParams + + +def skill_name_from_uri(uri: str) -> str: + """Return the skill `name` encoded in a `SKILL.md` resource URI. + + Per SEP-2640 Resource Mapping, the final `` segment equals the + skill's `name`; for a bare `skill:///SKILL.md` (no organizational + prefix) that segment is the authority. + + Raises: + ValueError: If `uri` is not an absolute URI ending in `/SKILL.md`. + """ + parts = urlsplit(uri) + if not parts.scheme or parts.query or parts.fragment: + raise ValueError(f"skill URI {uri!r} is not a valid absolute resource URI") + if not parts.path.endswith("/SKILL.md"): + raise ValueError(f"skill URI {uri!r} must end in /SKILL.md") + directory = parts.path[: -len("/SKILL.md")].strip("/") + if directory: + name = directory.rsplit("/", 1)[-1] + else: + name = parts.hostname or "" + if not name: + raise ValueError(f"skill URI {uri!r} has no skill name") + return name + + +def _resource_uri_in_skill(skill_uri: str, resource_uri: str) -> None: + """Raise `ValueError` unless `resource_uri` names a file within `skill_uri`'s directory.""" + skill_parts = urlsplit(skill_uri) + resource_parts = urlsplit(resource_uri) + if not resource_parts.scheme or resource_parts.query or resource_parts.fragment: + raise ValueError(f"resource URI {resource_uri!r} is invalid") + if skill_parts.scheme != resource_parts.scheme or skill_parts.netloc != resource_parts.netloc: + raise ValueError(f"resource URI {resource_uri!r} is outside the skill root {skill_uri!r}") + root = skill_parts.path[: -len("/SKILL.md")] + if resource_parts.path != skill_parts.path and not resource_parts.path.startswith(root + "/"): + raise ValueError(f"resource URI {resource_uri!r} is outside the skill root {skill_uri!r}") + if any(segment in (".", "..") for segment in resource_parts.path.split("/")): + raise ValueError(f"resource URI {resource_uri!r} contains a traversal segment") + + +def validate_skill(skill: Skill) -> None: + """Validate `skill` against the SEP-2640 and Agent Skills conformance rules. + + Checks the frontmatter's `name`/`description` fields, that `resources` (when + not `"dynamic"`) is complete and within the `MAX_RESOURCES_PER_SKILL`/ + `MAX_TOTAL_SIZE` limits, and that every resource entry names a file within + the skill's own directory with a well-formed digest. + + Raises: + ValueError: If `skill` violates any of the above. + """ + name = skill_name_from_uri(skill.uri) + frontmatter_name = skill.frontmatter.get("name") + if not isinstance(frontmatter_name, str) or not _NAME_RE.fullmatch(frontmatter_name) or len(frontmatter_name) > 64: + raise ValueError(f"skill {skill.uri!r} frontmatter name must be 1-64 lowercase, digits, or hyphens") + if frontmatter_name != name: + raise ValueError(f"skill {skill.uri!r} frontmatter name {frontmatter_name!r} does not match URI name {name!r}") + description = skill.frontmatter.get("description") + if not isinstance(description, str) or not (1 <= len(description) <= 1024): + raise ValueError(f"skill {skill.uri!r} frontmatter description must contain 1 to 1024 characters") + + if skill.resources == "dynamic": + return + resources = skill.resources + if len(resources) > MAX_RESOURCES_PER_SKILL: + raise ValueError(f"skill {skill.uri!r} has {len(resources)} resources, exceeding {MAX_RESOURCES_PER_SKILL}") + seen: set[str] = set() + total_size = 0 + for resource in resources: + _resource_uri_in_skill(skill.uri, resource.uri) + if resource.uri in seen: + raise ValueError(f"skill {skill.uri!r} lists resource {resource.uri!r} more than once") + seen.add(resource.uri) + if not _DIGEST_RE.fullmatch(resource.digest): + raise ValueError(f"skill {skill.uri!r} resource {resource.uri!r} has an invalid SHA-256 digest") + if resource.size < 0: + raise ValueError(f"skill {skill.uri!r} resource {resource.uri!r} has a negative size") + total_size += resource.size + if skill.uri not in seen: + raise ValueError(f"skill {skill.uri!r} resources does not include its own SKILL.md") + if total_size > MAX_TOTAL_SIZE: + raise ValueError(f"skill {skill.uri!r} has {total_size} bytes, exceeding {MAX_TOTAL_SIZE}") + + +def validate_list_result(result: ListSkillsResult) -> None: + """Validate every skill in `result.skills` and reject duplicate URIs. + + Raises: + ValueError: If any skill is invalid, or two entries share a `uri`. + """ + seen: set[str] = set() + for skill in result.skills: + validate_skill(skill) + if skill.uri in seen: + raise ValueError(f"skills/list result lists skill {skill.uri!r} more than once") + seen.add(skill.uri) + + +def parse_directory_uri(uri: str) -> tuple[str, str, str]: + """Split a directory resource URI into `(scheme, netloc, path)`. + + Raises: + ValueError: If `uri` has a trailing slash, or is otherwise not a valid + absolute resource URI. + """ + if uri.endswith("/"): + raise ValueError(f"directory URI {uri!r} must not have a trailing slash") + parts = urlsplit(uri) + if not parts.scheme or parts.query or parts.fragment: + raise ValueError(f"directory URI {uri!r} is not a valid absolute resource URI") + return parts.scheme, parts.netloc, parts.path + + +def validate_directory_result(uri: str, result: ReadDirectoryResult) -> None: + """Validate that `result.resources` are exactly the direct children of `uri`. + + Raises: + ValueError: If `uri` is malformed, or any entry is not a direct child, + or two entries share a `uri` or `name`. + """ + scheme, netloc, parent_path = parse_directory_uri(uri) + seen_uris: set[str] = set() + seen_names: set[str] = set() + prefix = parent_path.rstrip("/") + "/" if parent_path.rstrip("/") else "/" + for resource in result.resources: + child = urlsplit(resource.uri) + if not child.scheme or child.query or child.fragment: + raise ValueError(f"directory {uri!r} child has invalid URI {resource.uri!r}") + if child.scheme != scheme or child.netloc != netloc: + raise ValueError(f"resource {resource.uri!r} is not a child of directory {uri!r}") + relative = child.path.removeprefix(prefix) + if relative == child.path or not relative or "/" in relative: + raise ValueError(f"resource {resource.uri!r} is not a direct child of directory {uri!r}") + if resource.uri in seen_uris or resource.name in seen_names: + raise ValueError(f"directory {uri!r} contains a duplicate child {resource.uri!r}") + seen_uris.add(resource.uri) + seen_names.add(resource.name) + + +def verify_skill_resource(skill: Skill, uri: str, content: bytes) -> None: + """Verify `content` (the bytes read from `uri`) against `skill`'s held manifest. + + Implements the SEP-2640 Integrity and verification requirement: a host + MUST verify a retrieved file's bytes against its manifest entry before + using them. Not applicable to a skill whose `resources` is `"dynamic"`, + which offers no digest to verify against. + + Raises: + ValueError: If `uri` is not one of `skill`'s resources, `skill.resources` + is `"dynamic"`, or `content` does not match the entry's `size`/`digest`. + """ + if skill.resources == "dynamic": + raise ValueError(f"skill {skill.uri!r} has dynamic resources and cannot be integrity-verified") + entry = next((r for r in skill.resources if r.uri == uri), None) + if entry is None: + raise ValueError(f"{uri!r} is not in skill {skill.uri!r}'s held manifest") + if len(content) != entry.size: + raise ValueError(f"resource {uri!r} has size {len(content)}, expected {entry.size}") + digest = f"sha256:{hashlib.sha256(content).hexdigest()}" + if digest != entry.digest: + raise ValueError(f"resource {uri!r} has digest {digest!r}, expected {entry.digest!r}") diff --git a/tests/shared/test_skills.py b/tests/shared/test_skills.py new file mode 100644 index 0000000000..525b25303a --- /dev/null +++ b/tests/shared/test_skills.py @@ -0,0 +1,314 @@ +"""SEP-2640 conformance checks in `mcp.shared.skills`: types, validation, and verification. + +Server- and client-side end-to-end wiring live in `tests/server/test_skills.py` and +`tests/client/test_skills.py`; this file pins the pure functions both depend on. +""" + +import hashlib + +import pytest +from mcp_types import Resource + +from mcp.shared.skills import ( + ListSkillsResult, + ReadDirectoryResult, + Skill, + SkillResource, + parse_directory_uri, + skill_name_from_uri, + validate_directory_result, + validate_list_result, + validate_skill, + verify_skill_resource, +) + +_DIGEST = "sha256:" + "a" * 64 + + +def _resource(uri: str, *, size: int = 4) -> SkillResource: + return SkillResource(uri=uri, digest=_DIGEST, size=size) + + +def _skill(name: str = "git-workflow", *, extra_resources: list[SkillResource] | None = None) -> Skill: + uri = f"skill://{name}/SKILL.md" + return Skill( + uri=uri, + frontmatter={"name": name, "description": "d"}, + resources=[_resource(uri), *(extra_resources or [])], + ) + + +@pytest.mark.parametrize( + ("uri", "name"), + [ + ("skill://git-workflow/SKILL.md", "git-workflow"), + ("skill://acme/billing/refunds/SKILL.md", "refunds"), + ("https://example.com/skills/pdf/SKILL.md", "pdf"), + ], +) +def test_skill_name_from_uri_reads_the_final_path_segment(uri: str, name: str) -> None: + """SEP-2640 Resource Mapping: the final `` segment is the skill name.""" + assert skill_name_from_uri(uri) == name + + +@pytest.mark.parametrize( + "uri", + [ + "skill://git-workflow/README.md", # doesn't end in /SKILL.md + "not-a-uri", # no scheme + "skill://git-workflow/SKILL.md?x=1", # query component + ], +) +def test_skill_name_from_uri_rejects_malformed_uris(uri: str) -> None: + with pytest.raises(ValueError, match="SKILL.md|absolute resource URI"): + skill_name_from_uri(uri) + + +def test_skill_name_from_uri_rejects_a_uri_with_no_recoverable_name() -> None: + with pytest.raises(ValueError, match="has no skill name"): + skill_name_from_uri("skill:///SKILL.md") + + +def test_validate_skill_accepts_a_conformant_skill() -> None: + validate_skill(_skill()) + + +def test_validate_skill_rejects_a_non_string_frontmatter_name() -> None: + skill = Skill( + uri="skill://git-workflow/SKILL.md", + frontmatter={"name": 1, "description": "d"}, + resources=[_resource("skill://git-workflow/SKILL.md")], + ) + with pytest.raises(ValueError, match="frontmatter name"): + validate_skill(skill) + + +def test_validate_skill_rejects_an_invalid_resource_uri() -> None: + """A resource URI with a query component fails the same shape check as a skill URI.""" + skill = _skill(extra_resources=[_resource("skill://git-workflow/x.md?y=1")]) + with pytest.raises(ValueError, match="is invalid"): + validate_skill(skill) + + +def test_validate_skill_rejects_a_resource_with_the_same_authority_but_a_sibling_path() -> None: + """Same scheme+authority as the skill (so the URI passes the authority check) but a path + outside the skill's own directory, per SEP-2640 Resources ('a file within the skill's + directory').""" + root = "skill://acme/billing/refunds/SKILL.md" + skill = Skill( + uri=root, + frontmatter={"name": "refunds", "description": "d"}, + resources=[_resource(root), _resource("skill://acme/billing/other/x.md")], + ) + with pytest.raises(ValueError, match="outside the skill root"): + validate_skill(skill) + + +def test_validate_skill_rejects_a_traversal_segment_in_a_resource_uri() -> None: + skill = _skill(extra_resources=[_resource("skill://git-workflow/../evil.md")]) + with pytest.raises(ValueError, match="traversal segment"): + validate_skill(skill) + + +def test_validate_skill_rejects_a_negative_resource_size() -> None: + root = "skill://git-workflow/SKILL.md" + skill = Skill( + uri=root, + frontmatter={"name": "git-workflow", "description": "d"}, + resources=[SkillResource(uri=root, digest=_DIGEST, size=-1)], + ) + with pytest.raises(ValueError, match="negative size"): + validate_skill(skill) + + +def test_validate_skill_rejects_frontmatter_name_uri_mismatch() -> None: + """SEP-2640 Resource Mapping: `frontmatter.name` MUST equal the URI-derived name.""" + skill = Skill( + uri="skill://git-workflow/SKILL.md", + frontmatter={"name": "other-name", "description": "d"}, + resources=[_resource("skill://git-workflow/SKILL.md")], + ) + with pytest.raises(ValueError, match="does not match URI name"): + validate_skill(skill) + + +@pytest.mark.parametrize("description", ["", "x" * 1025]) +def test_validate_skill_rejects_out_of_range_description_length(description: str) -> None: + skill = Skill( + uri="skill://git-workflow/SKILL.md", + frontmatter={"name": "git-workflow", "description": description}, + resources=[_resource("skill://git-workflow/SKILL.md")], + ) + with pytest.raises(ValueError, match="description"): + validate_skill(skill) + + +def test_validate_skill_rejects_a_resource_outside_the_skill_root() -> None: + """SEP-2640 Resources: every entry MUST name a file within the skill's own directory.""" + skill = _skill(extra_resources=[_resource("skill://other-skill/file.md")]) + with pytest.raises(ValueError, match="outside the skill root"): + validate_skill(skill) + + +def test_validate_skill_rejects_missing_skill_md_entry() -> None: + """SEP-2640 Resources: `resources` MUST include an entry for the skill's own `SKILL.md`.""" + skill = Skill( + uri="skill://git-workflow/SKILL.md", + frontmatter={"name": "git-workflow", "description": "d"}, + resources=[_resource("skill://git-workflow/references/GUIDE.md")], + ) + with pytest.raises(ValueError, match="does not include its own SKILL.md"): + validate_skill(skill) + + +def test_validate_skill_rejects_duplicate_resource_uris() -> None: + root = "skill://git-workflow/SKILL.md" + skill = Skill( + uri=root, frontmatter={"name": "git-workflow", "description": "d"}, resources=[_resource(root), _resource(root)] + ) + with pytest.raises(ValueError, match="more than once"): + validate_skill(skill) + + +def test_validate_skill_rejects_an_invalid_digest_format() -> None: + root = "skill://git-workflow/SKILL.md" + bad = SkillResource(uri=root, digest="not-a-digest", size=1) + skill = Skill(uri=root, frontmatter={"name": "git-workflow", "description": "d"}, resources=[bad]) + with pytest.raises(ValueError, match="digest"): + validate_skill(skill) + + +def test_validate_skill_rejects_more_than_512_resources() -> None: + """SEP-2640 Limits: 512 entries per skill, `SKILL.md` included.""" + root = "skill://git-workflow/SKILL.md" + resources = [_resource(root)] + [_resource(f"skill://git-workflow/f{i}.md") for i in range(512)] + skill = Skill(uri=root, frontmatter={"name": "git-workflow", "description": "d"}, resources=resources) + with pytest.raises(ValueError, match="exceeding 512"): + validate_skill(skill) + + +def test_validate_skill_rejects_total_size_over_16mib() -> None: + """SEP-2640 Limits: 16 MiB total per skill, summed over `resources[].size`.""" + root = "skill://git-workflow/SKILL.md" + skill = Skill( + uri=root, + frontmatter={"name": "git-workflow", "description": "d"}, + resources=[_resource(root, size=16 * 1024 * 1024 + 1)], + ) + with pytest.raises(ValueError, match="exceeding"): + validate_skill(skill) + + +def test_validate_skill_accepts_dynamic_resources_without_further_checks() -> None: + """SEP-2640 Resources: `"dynamic"` offers no manifest to check against limits.""" + skill = Skill( + uri="skill://generated/SKILL.md", frontmatter={"name": "generated", "description": "d"}, resources="dynamic" + ) + validate_skill(skill) + + +def test_validate_list_result_rejects_duplicate_skill_uris_across_entries() -> None: + result = ListSkillsResult(skills=[_skill(), _skill()]) + with pytest.raises(ValueError, match="more than once"): + validate_list_result(result) + + +def test_validate_list_result_accepts_an_empty_listing() -> None: + """SEP-2640 Enumeration: `skills/list` MAY return an empty result.""" + validate_list_result(ListSkillsResult(skills=[])) + + +@pytest.mark.parametrize("uri", ["skill://pdf/templates/", "not-a-uri"]) +def test_parse_directory_uri_rejects_malformed_uris(uri: str) -> None: + with pytest.raises(ValueError, match="directory URI"): + parse_directory_uri(uri) + + +def test_validate_directory_result_rejects_a_child_with_a_query_component() -> None: + result = ReadDirectoryResult(resources=[Resource(uri="skill://pdf/templates/x.md?y=1", name="x.md")]) + with pytest.raises(ValueError, match="invalid URI"): + validate_directory_result("skill://pdf/templates", result) + + +def test_validate_directory_result_rejects_a_duplicate_child_name() -> None: + result = ReadDirectoryResult( + resources=[ + Resource(uri="skill://pdf/templates/a.md", name="dup"), + Resource(uri="skill://pdf/templates/b.md", name="dup"), + ] + ) + with pytest.raises(ValueError, match="duplicate child"): + validate_directory_result("skill://pdf/templates", result) + + +def test_validate_directory_result_accepts_direct_children() -> None: + result = ReadDirectoryResult( + resources=[ + Resource(uri="skill://pdf/templates/invoice.md", name="invoice.md", mime_type="text/markdown"), + Resource(uri="skill://pdf/templates/regional", name="regional", mime_type="inode/directory"), + ] + ) + validate_directory_result("skill://pdf/templates", result) + + +def test_validate_directory_result_rejects_a_grandchild() -> None: + """SEP-2640 Directory Listing: the listing is not recursive.""" + result = ReadDirectoryResult( + resources=[Resource(uri="skill://pdf/templates/regional/eu.md", name="eu.md", mime_type="text/markdown")] + ) + with pytest.raises(ValueError, match="direct child"): + validate_directory_result("skill://pdf/templates", result) + + +def test_validate_directory_result_rejects_a_uri_outside_the_directory() -> None: + result = ReadDirectoryResult(resources=[Resource(uri="skill://other/file.md", name="file.md")]) + with pytest.raises(ValueError, match="not a child"): + validate_directory_result("skill://pdf/templates", result) + + +def test_verify_skill_resource_accepts_matching_content() -> None: + content = b"hello skill" + digest = f"sha256:{hashlib.sha256(content).hexdigest()}" + uri = "skill://git-workflow/SKILL.md" + skill = Skill( + uri=uri, + frontmatter={"name": "git-workflow", "description": "d"}, + resources=[SkillResource(uri=uri, digest=digest, size=len(content))], + ) + verify_skill_resource(skill, uri, content) + + +def test_verify_skill_resource_rejects_a_digest_mismatch() -> None: + """SEP-2640 Integrity and verification: a mismatch MUST be treated as a verification failure.""" + uri = "skill://git-workflow/SKILL.md" + skill = Skill( + uri=uri, + frontmatter={"name": "git-workflow", "description": "d"}, + resources=[SkillResource(uri=uri, digest=_DIGEST, size=5)], + ) + with pytest.raises(ValueError, match="digest"): + verify_skill_resource(skill, uri, b"wrong") + + +def test_verify_skill_resource_rejects_a_size_mismatch_before_hashing() -> None: + uri = "skill://git-workflow/SKILL.md" + skill = Skill( + uri=uri, + frontmatter={"name": "git-workflow", "description": "d"}, + resources=[SkillResource(uri=uri, digest=_DIGEST, size=999)], + ) + with pytest.raises(ValueError, match="size"): + verify_skill_resource(skill, uri, b"short") + + +def test_verify_skill_resource_rejects_dynamic_resources() -> None: + uri = "skill://generated/SKILL.md" + skill = Skill(uri=uri, frontmatter={"name": "generated", "description": "d"}, resources="dynamic") + with pytest.raises(ValueError, match="dynamic"): + verify_skill_resource(skill, uri, b"anything") + + +def test_verify_skill_resource_rejects_a_uri_not_in_the_manifest() -> None: + skill = _skill() + with pytest.raises(ValueError, match="not in skill"): + verify_skill_resource(skill, "skill://git-workflow/unlisted.md", b"x") From 573fc062da755fffd8ef160689a779420a9d798e Mon Sep 17 00:00:00 2001 From: vijay Date: Wed, 9 Sep 2026 20:07:37 +0530 Subject: [PATCH 02/25] Add SEP-2640 server support Skills extension (io.modelcontextprotocol/skills): serves skills/list, skills/get, and the optional resources/directory/read behind the directoryRead capability setting. Handlers are supplied by the server author; the extension validates results against SEP-2640 before they reach the wire and gates the SEP-2549 ttlMs/cacheScope fields to protocol version 2026-07-28+. --- src/mcp/server/skills.py | 165 ++++++++++++++++++++ tests/server/test_skills.py | 291 ++++++++++++++++++++++++++++++++++++ 2 files changed, 456 insertions(+) create mode 100644 src/mcp/server/skills.py create mode 100644 tests/server/test_skills.py diff --git a/src/mcp/server/skills.py b/src/mcp/server/skills.py new file mode 100644 index 0000000000..82107aac30 --- /dev/null +++ b/src/mcp/server/skills.py @@ -0,0 +1,165 @@ +"""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. + +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` +however their catalog is stored, and serves the underlying `skill://` file +content through the server's ordinary resource-registration APIs +(`MCPServer.add_resource`, `add_resource_template`, ...). + + 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 __future__ import annotations + +from collections.abc import Awaitable, Callable, Sequence +from typing import Any + +from mcp_types.jsonrpc import INVALID_PARAMS +from mcp_types.version import MODERN_PROTOCOL_VERSIONS + +from mcp.server.context import HandlerResult, ServerRequestContext +from mcp.server.extension import Extension, MethodBinding +from mcp.shared.exceptions import MCPError +from mcp.shared.skills import ( + EXTENSION_ID, + METHOD_GET, + METHOD_LIST, + METHOD_READ_DIRECTORY, + GetSkillParams, + GetSkillResult, + ListSkillsParams, + ListSkillsResult, + ReadDirectoryParams, + ReadDirectoryResult, + parse_directory_uri, + skill_name_from_uri, + validate_directory_result, + validate_list_result, + validate_skill, +) + +__all__ = ["Skills"] + +ListSkillsHandler = Callable[[ServerRequestContext[Any, Any], ListSkillsParams], Awaitable[ListSkillsResult]] +GetSkillHandler = Callable[[ServerRequestContext[Any, Any], GetSkillParams], Awaitable[GetSkillResult]] +ReadDirectoryHandler = Callable[[ServerRequestContext[Any, Any], ReadDirectoryParams], Awaitable[ReadDirectoryResult]] + + +class Skills(Extension): + """The Skills extension: serve `skills/list`, `skills/get`, and directory reads. + + `list_skills` and `get_skill` are required; a server MUST answer both per + SEP-2640, whether or not a skill appears in the listing. `read_directory` + is optional — supplying it advertises the `directoryRead` capability + setting and serves `resources/directory/read`; omitting it advertises + neither. Handlers run per request, so a catalog that changes over time + (or is too large to enumerate) can return a partial or empty listing. + """ + + identifier = EXTENSION_ID + + def __init__( + self, + *, + list_skills: ListSkillsHandler, + get_skill: GetSkillHandler, + read_directory: ReadDirectoryHandler | None = None, + ) -> None: + self._list_skills = list_skills + self._get_skill = get_skill + self._read_directory = read_directory + + def settings(self) -> dict[str, Any]: + return {"directoryRead": True} if self._read_directory is not None else {} + + def methods(self) -> Sequence[MethodBinding]: + bindings = [ + MethodBinding(METHOD_LIST, ListSkillsParams, self._handle_list), + MethodBinding(METHOD_GET, GetSkillParams, self._handle_get), + ] + if self._read_directory is not None: + bindings.append(MethodBinding(METHOD_READ_DIRECTORY, ReadDirectoryParams, self._handle_read_directory)) + return bindings + + async def _handle_list(self, ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> HandlerResult: + result = await self._list_skills(ctx, params) + try: + validate_list_result(result) + except ValueError as exc: + raise MCPError( + code=INVALID_PARAMS, message=f"list_skills handler returned an invalid result: {exc}" + ) from exc + return _finalize_cacheable(result, ctx.protocol_version) + + async def _handle_get(self, ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> HandlerResult: + _require_skill_md_uri(params.uri) + result = await self._get_skill(ctx, params) + if result.skill.uri != params.uri: + raise MCPError( + code=INVALID_PARAMS, + message=f"get_skill handler returned {result.skill.uri!r} for requested {params.uri!r}", + ) + try: + validate_skill(result.skill) + except ValueError as exc: + raise MCPError(code=INVALID_PARAMS, message=f"get_skill handler returned an invalid result: {exc}") from exc + return result + + async def _handle_read_directory( + self, ctx: ServerRequestContext[Any, Any], params: ReadDirectoryParams + ) -> HandlerResult: + assert self._read_directory is not None + try: + parse_directory_uri(params.uri) + except ValueError as exc: + raise MCPError(code=INVALID_PARAMS, message=str(exc)) from exc + result = await self._read_directory(ctx, params) + try: + validate_directory_result(params.uri, result) + except ValueError as exc: + raise MCPError( + code=INVALID_PARAMS, message=f"read_directory handler returned an invalid result: {exc}" + ) from exc + return result + + +def _require_skill_md_uri(uri: str) -> None: + try: + skill_name_from_uri(uri) + except ValueError as exc: + raise MCPError(code=INVALID_PARAMS, message=str(exc)) from exc + + +def _finalize_cacheable(result: ListSkillsResult, protocol_version: str) -> HandlerResult: + """Gate SEP-2549's `ttlMs`/`cacheScope` to protocol version 2026-07-28+. + + `skills/list` is an extension method, so — unlike a core spec method — the + runner's per-version surface sieve never runs on its result; nothing else + strips these fields for a legacy connection. `CacheableResult` defaults to + `cache_scope="private"`; SEP-2640 calls for `"public"` when unset. + """ + if protocol_version in MODERN_PROTOCOL_VERSIONS: + if "cache_scope" not in result.model_fields_set: + result = result.model_copy(update={"cache_scope": "public"}) + return result + dumped = result.model_dump(by_alias=True, mode="json", exclude_none=True) + dumped.pop("ttlMs", None) + dumped.pop("cacheScope", None) + return dumped diff --git a/tests/server/test_skills.py b/tests/server/test_skills.py new file mode 100644 index 0000000000..88bc43b9a0 --- /dev/null +++ b/tests/server/test_skills.py @@ -0,0 +1,291 @@ +"""Tests for the Skills extension (`io.modelcontextprotocol/skills`, SEP-2640). + +`mcp.shared.skills`'s validators are unit-tested in `tests/shared/test_skills.py`; this +file covers this module's own wiring: request dispatch, capability advertisement, the +`skills/get`/`resources/directory/read` URI checks the extension performs itself before +calling the handler, and — the one piece of SEP-2549 behavior a hand-rolled extension +method must implement itself — gating `ttlMs`/`cacheScope` to protocol version 2026-07-28+. +""" + +from typing import Any + +import pytest +from inline_snapshot import snapshot +from mcp_types import INVALID_PARAMS, METHOD_NOT_FOUND, Resource +from pydantic import BaseModel, ConfigDict + +from mcp.client.client import Client +from mcp.server.context import ServerRequestContext +from mcp.server.mcpserver import MCPServer +from mcp.server.skills import Skills +from mcp.shared.exceptions import MCPError +from mcp.shared.skills import ( + GetSkillParams, + GetSkillRequest, + GetSkillResult, + ListSkillsParams, + ListSkillsRequest, + ListSkillsResult, + ReadDirectoryParams, + ReadDirectoryRequest, + ReadDirectoryResult, + Skill, + SkillResource, +) + +pytestmark = pytest.mark.anyio + + +class _RawResult(BaseModel): + """Captures every wire field, typed, so assertions can inspect fields our own + `ListSkillsResult` model doesn't declare being absent (there are none) or present.""" + + model_config = ConfigDict(extra="allow") + + +_DIGEST = "sha256:" + "a" * 64 +_SKILL_URI = "skill://git-workflow/SKILL.md" + + +def _git_workflow_skill() -> Skill: + return Skill( + uri=_SKILL_URI, + frontmatter={"name": "git-workflow", "description": "Follow this team's Git conventions"}, + resources=[SkillResource(uri=_SKILL_URI, digest=_DIGEST, size=10)], + ) + + +async def _list_skills(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> ListSkillsResult: + return ListSkillsResult(skills=[_git_workflow_skill()]) + + +async def _get_skill(ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> GetSkillResult: + if params.uri != _SKILL_URI: + raise MCPError(code=INVALID_PARAMS, message="unknown skill") + return GetSkillResult(skill=_git_workflow_skill()) + + +async def _read_directory(ctx: ServerRequestContext[Any, Any], params: ReadDirectoryParams) -> ReadDirectoryResult: + return ReadDirectoryResult( + resources=[Resource(uri="skill://git-workflow/references", name="references", mime_type="inode/directory")] + ) + + +def _server(*, with_directory_read: bool = False) -> MCPServer: + return MCPServer( + "catalog", + extensions=[ + Skills( + list_skills=_list_skills, + get_skill=_get_skill, + read_directory=_read_directory if with_directory_read else None, + ) + ], + ) + + +async def test_skills_list_returns_the_handlers_skills() -> None: + async with Client(_server(), mode="2026-07-28") as client: + result = await client.session.send_request(ListSkillsRequest(), ListSkillsResult) + assert [s.uri for s in result.skills] == [_SKILL_URI] + + +async def test_skills_list_may_return_an_empty_result() -> None: + """SEP-2640 Enumeration: the result MAY be empty.""" + + async def empty(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> ListSkillsResult: + return ListSkillsResult(skills=[]) + + server = MCPServer("catalog", extensions=[Skills(list_skills=empty, get_skill=_get_skill)]) + async with Client(server, mode="2026-07-28") as client: + result = await client.session.send_request(ListSkillsRequest(), ListSkillsResult) + assert result.skills == [] + + +async def test_skills_list_carries_cache_fields_on_the_2026_07_28_wire() -> None: + """SEP-2640 Dependencies: on 2026-07-28+, the result carries the SEP-2549 cache fields, + defaulting `cacheScope` to `"public"` when the handler left it unset.""" + 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["cacheScope"] == "public" + assert extra["ttlMs"] == 0 + + +async def test_skills_list_omits_cache_fields_on_a_legacy_wire() -> None: + """The runner's per-version sieve only applies to core spec methods, so this extension + method must strip SEP-2549 fields itself for a pre-2026-07-28 connection.""" + async with Client(_server(), mode="legacy") as client: + raw = await client.session.send_request(ListSkillsRequest(), _RawResult) + extra = raw.model_extra or {} + assert "cacheScope" not in extra + assert "ttlMs" not in extra + + +async def test_skills_list_handler_setting_cache_scope_explicitly_is_not_overridden() -> None: + async def private_list(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> ListSkillsResult: + return ListSkillsResult(skills=[_git_workflow_skill()], cache_scope="private") + + server = MCPServer("catalog", extensions=[Skills(list_skills=private_list, get_skill=_get_skill)]) + 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["cacheScope"] == "private" + + +async def test_skills_list_rejects_a_handler_result_with_an_invalid_skill() -> None: + """SDK-defined: a `list_skills` handler bug (a non-conformant skill entry) is caught + before it reaches the client, as an Invalid params error rather than a silently + non-conformant listing.""" + + async def bad_list(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> ListSkillsResult: + return ListSkillsResult( + skills=[Skill(uri=_SKILL_URI, frontmatter={"name": "git-workflow", "description": "d"}, resources=[])] + ) + + server = MCPServer("catalog", extensions=[Skills(list_skills=bad_list, get_skill=_get_skill)]) + async with Client(server) as client: + with pytest.raises(MCPError) as exc_info: + await client.session.send_request(ListSkillsRequest(), ListSkillsResult) + assert exc_info.value.code == INVALID_PARAMS + + +async def test_skills_get_returns_the_matching_skill() -> None: + async with Client(_server()) as client: + result = await client.session.send_request( + GetSkillRequest(params=GetSkillParams(uri=_SKILL_URI)), GetSkillResult + ) + assert result.skill.uri == _SKILL_URI + + +async def test_skills_get_answers_for_a_skill_absent_from_the_listing() -> None: + """SEP-2640 Retrieval: a server MUST answer for every skill it serves, whether or not + that skill appears in `skills/list`.""" + + async def list_without_it(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> ListSkillsResult: + return ListSkillsResult(skills=[]) + + server = MCPServer("catalog", extensions=[Skills(list_skills=list_without_it, get_skill=_get_skill)]) + async with Client(server) as client: + listing = await client.session.send_request(ListSkillsRequest(), ListSkillsResult) + result = await client.session.send_request( + GetSkillRequest(params=GetSkillParams(uri=_SKILL_URI)), GetSkillResult + ) + assert listing.skills == [] + assert result.skill.uri == _SKILL_URI + + +async def test_skills_get_rejects_an_unknown_uri_with_invalid_params() -> None: + """SEP-2640 Retrieval: an unknown skill URI MUST be rejected with -32602 (Invalid params).""" + 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_skills_get_rejects_a_uri_that_does_not_end_in_skill_md() -> None: + async with Client(_server()) as client: + with pytest.raises(MCPError) as exc_info: + await client.session.send_request( + GetSkillRequest(params=GetSkillParams(uri="skill://git-workflow/README.md")), GetSkillResult + ) + assert exc_info.value.code == INVALID_PARAMS + + +async def test_skills_get_rejects_a_handler_that_returns_a_mismatched_uri() -> None: + """SDK-defined: a `get_skill` handler bug (returning the wrong skill) is caught before + it reaches the client, as an Invalid params error rather than a silently wrong answer.""" + + async def wrong_skill(ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> GetSkillResult: + return GetSkillResult(skill=_git_workflow_skill()) + + server = MCPServer("catalog", extensions=[Skills(list_skills=_list_skills, get_skill=wrong_skill)]) + 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_skills_get_rejects_a_matching_but_non_conformant_skill() -> None: + """SDK-defined: a `get_skill` handler bug (a non-conformant skill body, distinct from a + URI mismatch) is caught the same way, as an Invalid params error.""" + + async def bad_skill(ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> GetSkillResult: + return GetSkillResult( + skill=Skill(uri=params.uri, frontmatter={"name": "git-workflow", "description": "d"}, resources=[]) + ) + + server = MCPServer("catalog", extensions=[Skills(list_skills=_list_skills, get_skill=bad_skill)]) + async with Client(server) as client: + with pytest.raises(MCPError) as exc_info: + await client.session.send_request(GetSkillRequest(params=GetSkillParams(uri=_SKILL_URI)), GetSkillResult) + assert exc_info.value.code == INVALID_PARAMS + + +async def test_missing_uri_param_is_rejected_before_the_handler_runs() -> None: + """SDK-defined: `uri` is a required field on `GetSkillParams`, so an omitted param is + rejected by params validation - the handler never sees it.""" + async with Client(_server()) as client: + with pytest.raises(MCPError) as exc_info: + await client.session.send_request(GetSkillRequest.model_construct(params=None), GetSkillResult) + assert exc_info.value.code == INVALID_PARAMS + + +async def test_directory_read_is_not_advertised_without_a_handler() -> None: + async with Client(_server(with_directory_read=False)) as client: + assert client.server_capabilities.extensions == {"io.modelcontextprotocol/skills": {}} + + +async def test_directory_read_is_advertised_when_a_handler_is_supplied() -> None: + async with Client(_server(with_directory_read=True)) as client: + assert client.server_capabilities.extensions == snapshot( + {"io.modelcontextprotocol/skills": {"directoryRead": True}} + ) + + +async def test_directory_read_returns_the_handlers_children() -> None: + async with Client(_server(with_directory_read=True)) as client: + result = await client.session.send_request( + ReadDirectoryRequest(params=ReadDirectoryParams(uri="skill://git-workflow")), ReadDirectoryResult + ) + assert [r.uri for r in result.resources] == ["skill://git-workflow/references"] + + +async def test_directory_read_is_not_registered_without_a_handler() -> None: + """SDK-defined: omitting `read_directory` doesn't just skip the capability ad - the + method itself isn't registered, so calling it anyway is Method not found.""" + async with Client(_server(with_directory_read=False)) as client: + with pytest.raises(MCPError) as exc_info: + await client.session.send_request( + ReadDirectoryRequest(params=ReadDirectoryParams(uri="skill://git-workflow")), ReadDirectoryResult + ) + assert exc_info.value.code == METHOD_NOT_FOUND + + +async def test_directory_read_rejects_a_trailing_slash_uri() -> None: + """SEP-2640 Directory resources: directory URIs are written without a trailing slash.""" + async with Client(_server(with_directory_read=True)) as client: + with pytest.raises(MCPError) as exc_info: + await client.session.send_request( + ReadDirectoryRequest(params=ReadDirectoryParams(uri="skill://git-workflow/")), ReadDirectoryResult + ) + assert exc_info.value.code == INVALID_PARAMS + + +async def test_directory_read_rejects_a_handler_result_with_a_grandchild() -> None: + async def bad_directory(ctx: ServerRequestContext[Any, Any], params: ReadDirectoryParams) -> ReadDirectoryResult: + return ReadDirectoryResult(resources=[Resource(uri="skill://git-workflow/a/b/c.md", name="c.md")]) + + server = MCPServer( + "catalog", extensions=[Skills(list_skills=_list_skills, get_skill=_get_skill, read_directory=bad_directory)] + ) + async with Client(server) as client: + with pytest.raises(MCPError) as exc_info: + await client.session.send_request( + ReadDirectoryRequest(params=ReadDirectoryParams(uri="skill://git-workflow")), ReadDirectoryResult + ) + assert exc_info.value.code == INVALID_PARAMS From 0cc8dbca14f982cde3213ca2323604451210c7c9 Mon Sep 17 00:00:00 2001 From: vijay Date: Wed, 9 Sep 2026 20:07:45 +0530 Subject: [PATCH 03/25] Add SEP-2640 client support Thin client wrappers for skills/list, skills/get, resources/directory/read, and resources/read: list_skills and read_directory follow nextCursor to completion, all four validate the server's response before returning it, and verify_skill_resource checks a read's bytes against a held skill's manifest entry. --- src/mcp/client/skills.py | 126 ++++++++++++++++++++ tests/client/test_skills.py | 226 ++++++++++++++++++++++++++++++++++++ 2 files changed, 352 insertions(+) create mode 100644 src/mcp/client/skills.py create mode 100644 tests/client/test_skills.py diff --git a/src/mcp/client/skills.py b/src/mcp/client/skills.py new file mode 100644 index 0000000000..e0dc0a6e97 --- /dev/null +++ b/src/mcp/client/skills.py @@ -0,0 +1,126 @@ +"""Client-side convenience wrappers for the Skills extension (SEP-2640). + +SEP-2640 needs no client-side method registration: `skills/list`, `skills/get`, +and `resources/directory/read` are ordinary vendor requests sent through +`ClientSession.send_request`, exactly like [Extension verbs](../advanced/extensions.md#extension-verbs). +The functions below are the thin, named wrappers SEP-2640's "SDKs: Convenience +Wrappers" section recommends — each validates the server's advertised support +before sending, and `list_skills`/`read_directory` follow `nextCursor` to +completion so a caller sees one page's worth of ergonomics regardless of how +many requests it took. + + async with Client("http://localhost:8000/mcp") as client: + for skill in await list_skills(client.session): + print(skill.uri, skill.frontmatter["description"]) +""" + +from __future__ import annotations + +from mcp_types import ReadResourceResult, Resource + +from mcp.client.session import ClientSession +from mcp.shared.skills import ( + EXTENSION_ID, + GetSkillParams, + GetSkillRequest, + GetSkillResult, + ListSkillsParams, + ListSkillsRequest, + ListSkillsResult, + ReadDirectoryParams, + ReadDirectoryRequest, + ReadDirectoryResult, + Skill, + validate_directory_result, + validate_list_result, + validate_skill, +) +from mcp.shared.skills import verify_skill_resource as verify_skill_resource + +__all__ = ["get_skill", "list_skills", "read_directory", "read_skill_uri", "verify_skill_resource"] + + +def _require_extension(session: ClientSession, *, directory_read: bool = False) -> None: + capabilities = session.server_capabilities + settings = (capabilities.extensions or {}).get(EXTENSION_ID) if capabilities else None + if settings is None: + raise ValueError(f"server does not advertise the {EXTENSION_ID!r} extension") + if directory_read and not settings.get("directoryRead"): + raise ValueError(f"server does not advertise {EXTENSION_ID!r}'s directoryRead setting") + + +async def list_skills(session: ClientSession, params: ListSkillsParams | None = None) -> list[Skill]: + """Call `skills/list`, following `nextCursor` to completion, and validate the result. + + Raises: + ValueError: If the server doesn't advertise the Skills extension, or + its response is not SEP-2640 conformant. + """ + _require_extension(session) + cursor = params.cursor if params is not None else None + skills: list[Skill] = [] + seen_cursors: set[str] = set() + while True: + page = await session.send_request(ListSkillsRequest(params=ListSkillsParams(cursor=cursor)), ListSkillsResult) + validate_list_result(page) + skills.extend(page.skills) + if page.next_cursor is None: + return skills + if page.next_cursor in seen_cursors: + raise ValueError(f"server repeated skills/list pagination cursor {page.next_cursor!r}") + seen_cursors.add(page.next_cursor) + cursor = page.next_cursor + + +async def get_skill(session: ClientSession, uri: str) -> Skill: + """Call `skills/get` for `uri` and validate the result. + + Unlike `list_skills`, this succeeds for a skill absent from any listing — + per SEP-2640, a server MUST answer `skills/get` for every skill it serves. + + Raises: + ValueError: If the server doesn't advertise the Skills extension, its + response names a different skill, or the skill is not conformant. + """ + _require_extension(session) + result = await session.send_request(GetSkillRequest(params=GetSkillParams(uri=uri)), GetSkillResult) + if result.skill.uri != uri: + raise ValueError(f"server returned skill {result.skill.uri!r} for requested {uri!r}") + validate_skill(result.skill) + return result.skill + + +async def read_skill_uri(session: ClientSession, uri: str) -> ReadResourceResult: + """Read a skill file's content via `resources/read`. + + A thin, discoverable alias: works for any `skill://` (or other-scheme) + file regardless of whether the skill was ever enumerated. Verify the + result against a held `Skill` entry with `verify_skill_resource` before + treating it as trusted content — this call does not verify anything itself. + """ + return await session.read_resource(uri) + + +async def read_directory(session: ClientSession, uri: str, params: ReadDirectoryParams | None = None) -> list[Resource]: + """Call `resources/directory/read` for `uri`, following `nextCursor` to completion. + + Raises: + ValueError: If the server doesn't advertise the `directoryRead` + setting, or its response is not a valid child listing of `uri`. + """ + _require_extension(session, directory_read=True) + cursor = params.cursor if params is not None else None + resources: list[Resource] = [] + seen_cursors: set[str] = set() + while True: + page = await session.send_request( + ReadDirectoryRequest(params=ReadDirectoryParams(uri=uri, cursor=cursor)), ReadDirectoryResult + ) + validate_directory_result(uri, page) + resources.extend(page.resources) + if page.next_cursor is None: + return resources + if page.next_cursor in seen_cursors: + raise ValueError(f"server repeated resources/directory/read pagination cursor {page.next_cursor!r}") + seen_cursors.add(page.next_cursor) + cursor = page.next_cursor diff --git a/tests/client/test_skills.py b/tests/client/test_skills.py new file mode 100644 index 0000000000..878c9249bd --- /dev/null +++ b/tests/client/test_skills.py @@ -0,0 +1,226 @@ +"""Tests for the client-side Skills convenience wrappers (SEP-2640, `mcp.client.skills`).""" + +import hashlib +from typing import Any + +import pytest +from mcp_types import INVALID_PARAMS, Resource, TextResourceContents + +from mcp.client.client import Client +from mcp.client.skills import get_skill, list_skills, read_directory, read_skill_uri, verify_skill_resource +from mcp.server.context import ServerRequestContext +from mcp.server.extension import Extension, MethodBinding +from mcp.server.mcpserver import MCPServer +from mcp.server.mcpserver.resources import TextResource +from mcp.server.skills import Skills +from mcp.shared.exceptions import MCPError +from mcp.shared.skills import ( + EXTENSION_ID, + METHOD_GET, + GetSkillParams, + GetSkillResult, + ListSkillsParams, + ListSkillsResult, + ReadDirectoryParams, + ReadDirectoryResult, + Skill, + SkillResource, +) + +pytestmark = pytest.mark.anyio + +_SKILL_URI = "skill://git-workflow/SKILL.md" +_SKILL_CONTENT = "# git-workflow\n" +_SKILL_DIGEST = f"sha256:{hashlib.sha256(_SKILL_CONTENT.encode()).hexdigest()}" + + +def _skill() -> Skill: + return Skill( + uri=_SKILL_URI, + frontmatter={"name": "git-workflow", "description": "d"}, + resources=[SkillResource(uri=_SKILL_URI, digest=_SKILL_DIGEST, size=len(_SKILL_CONTENT))], + ) + + +async def _get_skill(ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> GetSkillResult: + if params.uri != _SKILL_URI: + raise MCPError(code=INVALID_PARAMS, message="unknown skill") + return GetSkillResult(skill=_skill()) + + +def _paginated_list_handler() -> Any: + """A `list_skills` handler serving two skills across two pages, by URI order.""" + pages = { + None: ([_skill()], "page-2"), + "page-2": ( + [ + Skill( + uri="skill://other/SKILL.md", frontmatter={"name": "other", "description": "d"}, resources="dynamic" + ) + ], + None, + ), + } + + async def handler(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> ListSkillsResult: + skills, next_cursor = pages[params.cursor] + return ListSkillsResult(skills=skills, next_cursor=next_cursor) + + return handler + + +def _repeating_cursor_list_handler() -> Any: + async def handler(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> ListSkillsResult: + return ListSkillsResult(skills=[_skill()], next_cursor="same-cursor-forever") + + return handler + + +class _NonConformantGetSkill(Extension): + """A server that advertises the extension but answers `skills/get` with the wrong skill - + the case the SDK's own `Skills` extension already rules out server-side, so this + exercises `mcp.client.skills.get_skill`'s own defense-in-depth check.""" + + identifier = EXTENSION_ID + + def methods(self) -> Any: + async def handler(ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> GetSkillResult: + return GetSkillResult(skill=_skill()) + + return [MethodBinding(METHOD_GET, GetSkillParams, handler)] + + +def _repeating_cursor_directory_handler() -> Any: + async def handler(ctx: ServerRequestContext[Any, Any], params: ReadDirectoryParams) -> ReadDirectoryResult: + return ReadDirectoryResult( + resources=[Resource(uri="skill://git-workflow/references/A.md", name="A.md")], + next_cursor="same-cursor-forever", + ) + + return handler + + +def _paginated_directory_handler() -> Any: + pages = { + None: ( + [Resource(uri="skill://git-workflow/references/A.md", name="A.md")], + "page-2", + ), + "page-2": ( + [Resource(uri="skill://git-workflow/references/B.md", name="B.md")], + None, + ), + } + + async def handler(ctx: ServerRequestContext[Any, Any], params: ReadDirectoryParams) -> ReadDirectoryResult: + resources, next_cursor = pages[params.cursor] + return ReadDirectoryResult(resources=resources, next_cursor=next_cursor) + + return handler + + +def _server(*, with_directory_read: bool = False) -> MCPServer: + server = MCPServer( + "catalog", + extensions=[ + Skills( + list_skills=_paginated_list_handler(), + get_skill=_get_skill, + read_directory=_paginated_directory_handler() if with_directory_read else None, + ) + ], + ) + server.add_resource(TextResource(uri=_SKILL_URI, name="SKILL.md", text=_SKILL_CONTENT)) + return server + + +async def test_list_skills_follows_next_cursor_to_completion() -> None: + async with Client(_server()) as client: + skills = await list_skills(client.session) + assert [s.uri for s in skills] == [_SKILL_URI, "skill://other/SKILL.md"] + + +async def test_list_skills_raises_on_a_server_that_repeats_its_cursor() -> None: + server = MCPServer( + "catalog", extensions=[Skills(list_skills=_repeating_cursor_list_handler(), get_skill=_get_skill)] + ) + async with Client(server) as client: + with pytest.raises(ValueError, match="repeated"): + await list_skills(client.session) + + +async def test_list_skills_requires_the_extension_to_be_advertised() -> None: + """No `Skills` extension at all: the server never advertises `io.modelcontextprotocol/skills`.""" + async with Client(MCPServer("plain")) as client: + with pytest.raises(ValueError, match="does not advertise"): + await list_skills(client.session) + + +async def test_get_skill_returns_the_matching_entry() -> None: + async with Client(_server()) as client: + skill = await get_skill(client.session, _SKILL_URI) + assert skill.uri == _SKILL_URI + + +async def test_get_skill_propagates_the_servers_unknown_skill_error() -> None: + async with Client(_server()) as client: + with pytest.raises(MCPError) as exc_info: + await get_skill(client.session, "skill://missing/SKILL.md") + assert exc_info.value.code == INVALID_PARAMS + + +async def test_read_skill_uri_reads_the_registered_resource() -> None: + async with Client(_server()) as client: + result = await read_skill_uri(client.session, _SKILL_URI) + contents = result.contents[0] + assert isinstance(contents, TextResourceContents) + assert contents.text == _SKILL_CONTENT + + +async def test_read_skill_uri_content_verifies_against_the_held_skill() -> None: + """End-to-end: `skills/get`'s digest and `resources/read`'s bytes agree.""" + async with Client(_server()) as client: + skill = await get_skill(client.session, _SKILL_URI) + result = await read_skill_uri(client.session, _SKILL_URI) + contents = result.contents[0] + assert isinstance(contents, TextResourceContents) + verify_skill_resource(skill, _SKILL_URI, contents.text.encode()) + + +async def test_read_directory_follows_next_cursor_to_completion() -> None: + async with Client(_server(with_directory_read=True)) as client: + resources = await read_directory(client.session, "skill://git-workflow/references") + assert [r.uri for r in resources] == [ + "skill://git-workflow/references/A.md", + "skill://git-workflow/references/B.md", + ] + + +async def test_read_directory_requires_the_directory_read_setting() -> None: + """The extension is advertised, but without `directoryRead: true`.""" + async with Client(_server(with_directory_read=False)) as client: + with pytest.raises(ValueError, match="directoryRead"): + await read_directory(client.session, "skill://git-workflow/references") + + +async def test_read_directory_raises_on_a_server_that_repeats_its_cursor() -> None: + server = MCPServer( + "catalog", + extensions=[ + Skills( + list_skills=_paginated_list_handler(), + get_skill=_get_skill, + read_directory=_repeating_cursor_directory_handler(), + ) + ], + ) + async with Client(server) as client: + with pytest.raises(ValueError, match="repeated"): + await read_directory(client.session, "skill://git-workflow/references") + + +async def test_get_skill_rejects_a_mismatched_uri_from_a_non_conformant_server() -> None: + server = MCPServer("catalog", extensions=[_NonConformantGetSkill()]) + async with Client(server) as client: + with pytest.raises(ValueError, match="returned skill"): + await get_skill(client.session, "skill://other/SKILL.md") From 0729d95b97d804f41a5bf9e32380884b892f7bcf Mon Sep 17 00:00:00 2001 From: vijay Date: Wed, 9 Sep 2026 20:07:53 +0530 Subject: [PATCH 04/25] Document SEP-2640 Python SDK support Adds the Skills page under Advanced, with a runnable server/client example, and tests proving every claim the page makes against the real SDK. --- docs/advanced/skills.md | 106 ++++++++++++++++++++++++++ docs_src/skills/__init__.py | 0 docs_src/skills/tutorial001.py | 55 +++++++++++++ docs_src/skills/tutorial001_client.py | 22 ++++++ mkdocs.yml | 1 + tests/docs_src/test_skills.py | 55 +++++++++++++ 6 files changed, 239 insertions(+) create mode 100644 docs/advanced/skills.md create mode 100644 docs_src/skills/__init__.py create mode 100644 docs_src/skills/tutorial001.py create mode 100644 docs_src/skills/tutorial001_client.py create mode 100644 tests/docs_src/test_skills.py diff --git a/docs/advanced/skills.md b/docs/advanced/skills.md new file mode 100644 index 0000000000..1ce41edbf9 --- /dev/null +++ b/docs/advanced/skills.md @@ -0,0 +1,106 @@ +# Skills + +[SEP-2640](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2640) defines a +convention for serving [Agent Skills](https://agentskills.io/) over MCP: a skill is a directory +of files — minimally a `SKILL.md` with YAML frontmatter — exposed as ordinary MCP resources, +conventionally under a `skill://` URI. A server enumerates its skills with `skills/list`, +answers for any one of them by URI with `skills/get`, and — optionally — lists a directory's +direct children with `resources/directory/read`. + +The SDK ships this as the built-in `Skills` extension (`io.modelcontextprotocol/skills`). If +[Extensions](extensions.md) are new to you, skim that page first. + +`Skills` provides 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 serve +each skill's files as ordinary resources through `MCPServer.add_resource` or +`add_resource_template`. + +## Serving a skill + +```python title="server.py" hl_lines="19-20 39-40 42" +--8<-- "docs_src/skills/tutorial001.py" +``` + +Three moves: + +* `Skill(uri=..., frontmatter=..., resources=[...])`: one entry, identical in shape whether it + comes back from `skills/list` or `skills/get`. `resources` is the skill's complete file + manifest — every file, `SKILL.md` included, each with a `sha256:...` digest and byte size — or + the string `"dynamic"` for content generated on demand. +* `list_skills`/`get_skill`: plain async callables, invoked per request. `get_skill` **must** + answer for a skill even if a real `list_skills` implementation omitted it — SEP-2640 requires + a server to answer by URI for every skill it serves, listed or not. +* `mcp.add_resource(TextResource(uri=SKILL_URI, ...))`: the skill's actual file content, served + through the SDK's ordinary resource machinery. `Skills` never reads or writes resource content + itself. + +`Skills(list_skills=..., get_skill=...)` is all a server needs; `resources/directory/read` is +optional (below). + +## Fetching a skill + +```python title="client.py" hl_lines="4" +--8<-- "docs_src/skills/tutorial001_client.py" +``` + +`list_skills` and `read_directory` follow `nextCursor` to completion, so you get every page's +skills or resources in one call. `get_skill` and `read_skill_uri` (a thin, discoverable alias for +`resources/read`) each cost exactly one request. All four validate the server's response against +the SEP-2640 conformance rules before returning it — a name that doesn't match its URI, a digest +in the wrong shape, or an incomplete manifest raises `ValueError` rather than reaching your code. + +`verify_skill_resource(skill, uri, content)` checks a file's bytes — size, then SHA-256 digest — +against the entry you hold for it. Call it after `read_skill_uri` and before treating the content +as trustworthy: `resources/read` returns whatever bytes the server sends *right now*, verification +is what ties those bytes back to the manifest you already validated. + +!!! warning + Skill content is untrusted model input, exactly like any other server-provided text. SEP-2640 + requires a host to tag it with its originating server before it reaches the model, and to + never grant the frontmatter's `allowed-tools` field (or any other permission-widening field) + without explicit per-skill user approval. Both are host responsibilities the SDK cannot + discharge for you — see the SEP's [Security Implications](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2640) + section before building a host on top of this extension. + +## Directory reads + +A skill's instructions often point at a directory rather than a file ("pick the matching +template from `templates/`"). `resources/list` cannot answer that — it enumerates a server's +entire resource space, not one subtree — so SEP-2640 adds `resources/directory/read`, gated +behind the `directoryRead` capability setting: + +```python +mcp = MCPServer( + "catalog", + extensions=[ + Skills( + list_skills=list_skills, + get_skill=get_skill, + read_directory=read_directory, # lists uri's direct children + ) + ], +) +``` + +Supplying `read_directory` advertises `{"directoryRead": true}` under the extension's +capabilities; omitting it advertises neither the setting nor the method — a client calling +`resources/directory/read` against such a server gets `METHOD_NOT_FOUND`. +`mcp.client.skills.read_directory` raises before sending if the connected server hasn't +advertised the setting. + +## Protocol version and caching + +In protocol version `2026-07-28` and later, `skills/list` results carry the base protocol's +list-caching fields, [`ttlMs` and `cacheScope`](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2549) — the +same freshness hint `tools/list` and `resources/list` carry. `Skills` fills `cacheScope` with +`"public"` when your handler leaves it unset, and omits both fields entirely on an +older connection, so 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. 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. diff --git a/docs_src/skills/__init__.py b/docs_src/skills/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/docs_src/skills/tutorial001.py b/docs_src/skills/tutorial001.py new file mode 100644 index 0000000000..07044d2282 --- /dev/null +++ b/docs_src/skills/tutorial001.py @@ -0,0 +1,55 @@ +import hashlib +from typing import Any + +from mcp_types import INVALID_PARAMS + +from mcp.server.context import ServerRequestContext +from mcp.server.mcpserver import MCPServer +from mcp.server.mcpserver.resources import TextResource +from mcp.server.skills import Skills +from mcp.shared.exceptions import MCPError +from mcp.shared.skills import ( + GetSkillParams, + GetSkillResult, + ListSkillsParams, + ListSkillsResult, + Skill, + SkillResource, +) + +SKILL_URI = "skill://git-workflow/SKILL.md" +SKILL_MD = """\ +--- +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. +""" + +GIT_WORKFLOW = Skill( + uri=SKILL_URI, + frontmatter={"name": "git-workflow", "description": "Follow this team's Git conventions for branching and commits"}, + resources=[ + SkillResource( + uri=SKILL_URI, + digest=f"sha256:{hashlib.sha256(SKILL_MD.encode()).hexdigest()}", + size=len(SKILL_MD.encode()), + ) + ], +) + + +async def list_skills(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> ListSkillsResult: + return ListSkillsResult(skills=[GIT_WORKFLOW]) + + +async def get_skill(ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> GetSkillResult: + if params.uri != SKILL_URI: + raise MCPError(code=INVALID_PARAMS, message=f"unknown skill: {params.uri}") + return GetSkillResult(skill=GIT_WORKFLOW) + + +mcp = MCPServer("catalog", extensions=[Skills(list_skills=list_skills, get_skill=get_skill)]) +mcp.add_resource(TextResource(uri=SKILL_URI, name="SKILL.md", mime_type="text/markdown", text=SKILL_MD)) diff --git a/docs_src/skills/tutorial001_client.py b/docs_src/skills/tutorial001_client.py new file mode 100644 index 0000000000..7e23428f77 --- /dev/null +++ b/docs_src/skills/tutorial001_client.py @@ -0,0 +1,22 @@ +import anyio +from mcp_types import TextResourceContents + +from mcp import Client +from mcp.client.skills import get_skill, list_skills, read_skill_uri, verify_skill_resource + + +async def main() -> None: + async with Client("http://localhost:8000/mcp") as client: + for skill in await list_skills(client.session): + print(skill.uri, skill.frontmatter["description"]) + + skill = await get_skill(client.session, "skill://git-workflow/SKILL.md") + result = await read_skill_uri(client.session, skill.uri) + content = result.contents[0] + if isinstance(content, TextResourceContents): + verify_skill_resource(skill, skill.uri, content.text.encode()) + print(content.text) + + +if __name__ == "__main__": + anyio.run(main) diff --git a/mkdocs.yml b/mkdocs.yml index a75053326f..6d2ee1d526 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -67,6 +67,7 @@ nav: - Middleware: advanced/middleware.md - Extensions: advanced/extensions.md - MCP Apps: advanced/apps.md + - Skills: advanced/skills.md - Troubleshooting: troubleshooting.md - Translations: translations.md - Migration Guide: migration.md diff --git a/tests/docs_src/test_skills.py b/tests/docs_src/test_skills.py new file mode 100644 index 0000000000..cc68175db1 --- /dev/null +++ b/tests/docs_src/test_skills.py @@ -0,0 +1,55 @@ +"""`docs/advanced/skills.md`: every claim the page makes, proved against the real SDK.""" + +import pytest +from mcp_types import INVALID_PARAMS, TextResourceContents + +from docs_src.skills import tutorial001 +from mcp import Client +from mcp.client.skills import get_skill, list_skills, read_skill_uri, verify_skill_resource +from mcp.shared.exceptions import MCPError + +pytestmark = pytest.mark.anyio + + +async def test_list_skills_returns_the_registered_skill() -> None: + """tutorial001: `list_skills` returns the one skill the server declared.""" + async with Client(tutorial001.mcp) as client: + skills = await list_skills(client.session) + assert [s.uri for s in skills] == [tutorial001.SKILL_URI] + + +async def test_get_skill_answers_by_uri() -> None: + """tutorial001: `get_skill` returns the same entry `list_skills` does.""" + async with Client(tutorial001.mcp) as client: + skill = await get_skill(client.session, tutorial001.SKILL_URI) + assert skill.frontmatter["name"] == "git-workflow" + + +async def test_get_skill_rejects_an_unknown_uri() -> None: + """tutorial001: `get_skill` raises `-32602` (Invalid params) for an unknown skill.""" + async with Client(tutorial001.mcp) as client: + with pytest.raises(MCPError) as exc_info: + await get_skill(client.session, "skill://unknown/SKILL.md") + assert exc_info.value.code == INVALID_PARAMS + + +async def test_the_skill_file_is_served_as_an_ordinary_resource() -> None: + """tutorial001: `Skills` never reads or serves content itself — the file is registered + through `mcp.add_resource`, the SDK's ordinary resource machinery.""" + async with Client(tutorial001.mcp) as client: + result = await client.read_resource(tutorial001.SKILL_URI) + contents = result.contents[0] + assert isinstance(contents, TextResourceContents) + assert contents.mime_type == "text/markdown" + assert contents.text == tutorial001.SKILL_MD + + +async def test_read_skill_uri_content_verifies_against_the_held_skill() -> None: + """tutorial001_client: fetch the entry, read the file, verify the bytes against it - + the digest/size check `verify_skill_resource` performs.""" + async with Client(tutorial001.mcp) as client: + skill = await get_skill(client.session, tutorial001.SKILL_URI) + result = await read_skill_uri(client.session, skill.uri) + contents = result.contents[0] + assert isinstance(contents, TextResourceContents) + verify_skill_resource(skill, skill.uri, contents.text.encode()) From 3a3725b48cc34cad2ad83524fcd7cc73ec74fd61 Mon Sep 17 00:00:00 2001 From: vijay Date: Wed, 9 Sep 2026 21:08:30 +0530 Subject: [PATCH 05/25] Strengthen SEP-2640 validation test coverage Parametrize the digest-format rejection test over near-miss cases (uppercase, wrong length, missing/wrong prefix), and add explicit JSON round-trip tests for both shapes of the resources union type (a static array and the "dynamic" marker) to prove neither collapses or mistags on the wire. --- tests/shared/test_skills.py | 40 +++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/tests/shared/test_skills.py b/tests/shared/test_skills.py index 525b25303a..943b4b1e22 100644 --- a/tests/shared/test_skills.py +++ b/tests/shared/test_skills.py @@ -69,6 +69,29 @@ def test_skill_name_from_uri_rejects_a_uri_with_no_recoverable_name() -> None: skill_name_from_uri("skill:///SKILL.md") +def test_skill_with_a_static_resources_array_round_trips_through_json() -> None: + """SEP-2640 Resources: `resources` MUST serialize as a JSON array of `{uri, digest, size}` + triples - proves the union type doesn't collapse or mistag on the wire.""" + original = _skill() + dumped = original.model_dump(mode="json", by_alias=True) + assert isinstance(dumped["resources"], list) + restored = Skill.model_validate(dumped) + assert restored == original + + +def test_skill_with_dynamic_resources_round_trips_through_json_as_the_literal_string() -> None: + """SEP-2640 Resources: a dynamically generated skill MUST carry the literal string + `"dynamic"` in place of an array - not `null`, not `{}`, not omitted.""" + original = Skill( + uri="skill://generated/SKILL.md", frontmatter={"name": "generated", "description": "d"}, resources="dynamic" + ) + dumped = original.model_dump(mode="json", by_alias=True) + assert dumped["resources"] == "dynamic" + restored = Skill.model_validate(dumped) + assert restored == original + assert restored.resources == "dynamic" + + def test_validate_skill_accepts_a_conformant_skill() -> None: validate_skill(_skill()) @@ -170,9 +193,22 @@ def test_validate_skill_rejects_duplicate_resource_uris() -> None: validate_skill(skill) -def test_validate_skill_rejects_an_invalid_digest_format() -> None: +@pytest.mark.parametrize( + "digest", + [ + "not-a-digest", # no sha256: prefix at all + "sha256:" + "A" * 64, # uppercase hex - spec requires lowercase + "sha256:" + "a" * 63, # one hex char short + "sha256:" + "a" * 65, # one hex char long + "sha1:" + "a" * 40, # wrong algorithm prefix + "sha256:" + "g" * 64, # non-hex characters + ], +) +def test_validate_skill_rejects_malformed_digest_formats(digest: str) -> None: + """SEP-2640 Integrity and verification: `sha256:{hex}` where `{hex}` is exactly 64 + lowercase hexadecimal characters - each of these near-misses must still be rejected.""" root = "skill://git-workflow/SKILL.md" - bad = SkillResource(uri=root, digest="not-a-digest", size=1) + bad = SkillResource(uri=root, digest=digest, size=1) skill = Skill(uri=root, frontmatter={"name": "git-workflow", "description": "d"}, resources=[bad]) with pytest.raises(ValueError, match="digest"): validate_skill(skill) From d31511cc02ee20ca797b03f987db613172ee6916 Mon Sep 17 00:00:00 2001 From: vijay Date: Wed, 9 Sep 2026 22:12:05 +0530 Subject: [PATCH 06/25] Rename skill resource-URI validator for clarity _resource_uri_in_skill reads like a boolean predicate but returns None and raises; rename to _validate_resource_uri_in_skill to match its sibling validators (validate_skill, validate_list_result, validate_directory_result) and signal that it asserts. --- src/mcp/shared/skills.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mcp/shared/skills.py b/src/mcp/shared/skills.py index 06f643576c..22bccfce77 100644 --- a/src/mcp/shared/skills.py +++ b/src/mcp/shared/skills.py @@ -151,7 +151,7 @@ def skill_name_from_uri(uri: str) -> str: return name -def _resource_uri_in_skill(skill_uri: str, resource_uri: str) -> None: +def _validate_resource_uri_in_skill(skill_uri: str, resource_uri: str) -> None: """Raise `ValueError` unless `resource_uri` names a file within `skill_uri`'s directory.""" skill_parts = urlsplit(skill_uri) resource_parts = urlsplit(resource_uri) @@ -195,7 +195,7 @@ def validate_skill(skill: Skill) -> None: seen: set[str] = set() total_size = 0 for resource in resources: - _resource_uri_in_skill(skill.uri, resource.uri) + _validate_resource_uri_in_skill(skill.uri, resource.uri) if resource.uri in seen: raise ValueError(f"skill {skill.uri!r} lists resource {resource.uri!r} more than once") seen.add(resource.uri) From c00a1e7c7e3a21518188c964b635e019d616833a Mon Sep 17 00:00:00 2001 From: vijay Date: Wed, 9 Sep 2026 22:56:46 +0530 Subject: [PATCH 07/25] Extract parallel directory-URI validator in Skills handlers _handle_read_directory inlined the same "validate incoming URI, convert ValueError to MCPError" pattern that _handle_get had already extracted into a helper. Add a parallel _require_directory_uri so both handlers open with a symmetric one-line precondition check, matching the _require_ui_scheme helper idiom from the Apps extension. --- src/mcp/server/skills.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/mcp/server/skills.py b/src/mcp/server/skills.py index 82107aac30..10f3f7145b 100644 --- a/src/mcp/server/skills.py +++ b/src/mcp/server/skills.py @@ -126,10 +126,7 @@ async def _handle_read_directory( self, ctx: ServerRequestContext[Any, Any], params: ReadDirectoryParams ) -> HandlerResult: assert self._read_directory is not None - try: - parse_directory_uri(params.uri) - except ValueError as exc: - raise MCPError(code=INVALID_PARAMS, message=str(exc)) from exc + _require_directory_uri(params.uri) result = await self._read_directory(ctx, params) try: validate_directory_result(params.uri, result) @@ -147,6 +144,13 @@ def _require_skill_md_uri(uri: str) -> None: raise MCPError(code=INVALID_PARAMS, message=str(exc)) from exc +def _require_directory_uri(uri: str) -> None: + try: + parse_directory_uri(uri) + except ValueError as exc: + raise MCPError(code=INVALID_PARAMS, message=str(exc)) from exc + + def _finalize_cacheable(result: ListSkillsResult, protocol_version: str) -> HandlerResult: """Gate SEP-2549's `ttlMs`/`cacheScope` to protocol version 2026-07-28+. From b4edd5303424000599fadbc255f78322e63d90cb Mon Sep 17 00:00:00 2001 From: vijay Date: Wed, 9 Sep 2026 23:10:25 +0530 Subject: [PATCH 08/25] Pin the skill-name grammar rejection cases validate_skill already rejects names that violate the Agent Skills grammar (SEP-2640 defers to it), but nothing pinned the edge cases. Add a parametrized test covering consecutive, leading, and trailing hyphens, uppercase, underscores, and the 64-character ceiling. --- tests/shared/test_skills.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/shared/test_skills.py b/tests/shared/test_skills.py index 943b4b1e22..f0faf72190 100644 --- a/tests/shared/test_skills.py +++ b/tests/shared/test_skills.py @@ -106,6 +106,31 @@ def test_validate_skill_rejects_a_non_string_frontmatter_name() -> None: validate_skill(skill) +@pytest.mark.parametrize( + "name", + [ + "foo--bar", # consecutive hyphens + "-foo", # leading hyphen + "foo-", # trailing hyphen + "UPPER", # uppercase not allowed + "with_underscore", # underscore not allowed + "a" * 65, # exceeds the 64-char limit + ], +) +def test_validate_skill_rejects_names_violating_the_agent_skills_grammar(name: str) -> None: + """SEP-2640 defers naming to the Agent Skills spec: 1-64 chars, lowercase alphanumeric and + hyphens, no leading/trailing/consecutive hyphens. A URI whose final path segment carries the + bad name (so `frontmatter.name` can match it) still fails the name-grammar check first.""" + uri = f"skill://acme/{name}/SKILL.md" + skill = Skill( + uri=uri, + frontmatter={"name": name, "description": "d"}, + resources=[SkillResource(uri=uri, digest=_DIGEST, size=4)], + ) + with pytest.raises(ValueError, match="frontmatter name"): + validate_skill(skill) + + def test_validate_skill_rejects_an_invalid_resource_uri() -> None: """A resource URI with a query component fails the same shape check as a skill URI.""" skill = _skill(extra_resources=[_resource("skill://git-workflow/x.md?y=1")]) From b845490cc2fc08295f24d65fc6dea02af2bdcf5e Mon Sep 17 00:00:00 2001 From: vijay Date: Wed, 9 Sep 2026 23:19:59 +0530 Subject: [PATCH 09/25] Fix documentation accuracy in the Skills guide - Correct the server/client snippet hl_lines, which highlighted blank and unrelated lines after the example imports were expanded. - Fix "all four validate": read_skill_uri is a thin resources/read pass-through that validates nothing, contradicting the same section's own next paragraph. Only list_skills/get_skill/read_directory validate. - Replace the phantom add_resource_template API (no such method) with the @mcp.resource(...) template decorator, in both the guide and the mcp.server.skills module docstring. --- docs/advanced/skills.md | 17 +++++++++-------- src/mcp/server/skills.py | 2 +- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/docs/advanced/skills.md b/docs/advanced/skills.md index 1ce41edbf9..a2527cd2e6 100644 --- a/docs/advanced/skills.md +++ b/docs/advanced/skills.md @@ -14,12 +14,12 @@ The SDK ships this as the built-in `Skills` extension (`io.modelcontextprotocol/ 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 serve -each skill's files as ordinary resources through `MCPServer.add_resource` or -`add_resource_template`. +each skill's files as ordinary resources through `MCPServer.add_resource` or an +`@mcp.resource(...)` template handler. ## Serving a skill -```python title="server.py" hl_lines="19-20 39-40 42" +```python title="server.py" hl_lines="31-41 44-51 55" --8<-- "docs_src/skills/tutorial001.py" ``` @@ -41,15 +41,16 @@ optional (below). ## Fetching a skill -```python title="client.py" hl_lines="4" +```python title="client.py" hl_lines="5" --8<-- "docs_src/skills/tutorial001_client.py" ``` `list_skills` and `read_directory` follow `nextCursor` to completion, so you get every page's -skills or resources in one call. `get_skill` and `read_skill_uri` (a thin, discoverable alias for -`resources/read`) each cost exactly one request. All four validate the server's response against -the SEP-2640 conformance rules before returning it — a name that doesn't match its URI, a digest -in the wrong shape, or an incomplete manifest raises `ValueError` rather than reaching your code. +skills or resources in one call; `get_skill` costs exactly one request. These three validate the +server's response against the SEP-2640 conformance rules before returning it — a name that doesn't +match its URI, a digest in the wrong shape, or an incomplete manifest raises `ValueError` rather +than reaching your code. `read_skill_uri` is the exception: a thin, discoverable alias for +`resources/read` that returns bytes and validates nothing itself (see the next paragraph). `verify_skill_resource(skill, uri, content)` checks a file's bytes — size, then SHA-256 digest — against the entry you hold for it. Call it after `read_skill_uri` and before treating the content diff --git a/src/mcp/server/skills.py b/src/mcp/server/skills.py index 10f3f7145b..0c90f66243 100644 --- a/src/mcp/server/skills.py +++ b/src/mcp/server/skills.py @@ -13,7 +13,7 @@ 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`, `add_resource_template`, ...). +(`MCPServer.add_resource`, or an `@mcp.resource(...)` template). async def list_skills(ctx, params): return ListSkillsResult(skills=[...]) From 2f363d58790d74b0ab22dbac62decea8a12327d1 Mon Sep 17 00:00:00 2001 From: vijay Date: Thu, 10 Sep 2026 05:52:22 +0530 Subject: [PATCH 10/25] Add end-to-end dynamic-skill and cursor-resume client tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two behavioral cases the existing suite left unpinned: - A "dynamic" skill now round-trips through the real server extension, the wire, and the client wrapper (validated on both ends), proving the resources union survives intact rather than only in an isolated model round-trip. - list_skills honours a caller-supplied starting cursor, skipping the pages before it — the resume-from-a-saved-cursor contract. --- tests/client/test_skills.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/client/test_skills.py b/tests/client/test_skills.py index 878c9249bd..82df3858ba 100644 --- a/tests/client/test_skills.py +++ b/tests/client/test_skills.py @@ -224,3 +224,32 @@ async def test_get_skill_rejects_a_mismatched_uri_from_a_non_conformant_server() async with Client(server) as client: with pytest.raises(ValueError, match="returned skill"): await get_skill(client.session, "skill://other/SKILL.md") + + +async def test_get_skill_round_trips_a_dynamic_skill() -> None: + """SEP-2640 Resources: the `"dynamic"` marker survives the full server -> wire -> client + path — validated on both ends — with the union type intact, not coerced to a list or null.""" + dynamic = Skill( + uri="skill://generated/SKILL.md", + frontmatter={"name": "generated", "description": "instructions generated on demand"}, + resources="dynamic", + ) + + async def get_dynamic(ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> GetSkillResult: + # The server's own _handle_get already enforces the requested-uri match; this test only + # ever asks for `dynamic.uri`, so the handler just returns it. + return GetSkillResult(skill=dynamic) + + server = MCPServer("catalog", extensions=[Skills(list_skills=_paginated_list_handler(), get_skill=get_dynamic)]) + async with Client(server) as client: + skill = await get_skill(client.session, dynamic.uri) + assert skill.resources == "dynamic" + assert skill.frontmatter["name"] == "generated" + + +async def test_list_skills_starts_from_a_caller_supplied_cursor() -> None: + """A host resuming from a saved cursor: `list_skills` begins at that cursor rather than the + top, so only the pages after it come back (here, page 1's skill is skipped).""" + async with Client(_server()) as client: + skills = await list_skills(client.session, ListSkillsParams(cursor="page-2")) + assert [s.uri for s in skills] == ["skill://other/SKILL.md"] From 1adba1dccbe6e7a888eb4594f03c7a4ec6c3e1ee Mon Sep 17 00:00:00 2001 From: vijay Date: Thu, 10 Sep 2026 06:20:12 +0530 Subject: [PATCH 11/25] Report Skills handler-output faults as INTERNAL_ERROR, not INVALID_PARAMS A non-conformant result from a `list_skills`/`get_skill`/`read_directory` handler (or a `get_skill` URI mismatch) is a server-side bug, not a bad caller request, so -32603 is the correct code rather than -32602. Log the real cause server-side and return a generic message, mirroring the runner's existing handling of invalid handler results. Input validation (`_require_skill_md_uri`, `_require_directory_uri`, params) stays -32602. --- src/mcp/server/skills.py | 30 +++++++++++++++--------------- tests/server/test_skills.py | 21 ++++++++++++--------- 2 files changed, 27 insertions(+), 24 deletions(-) diff --git a/src/mcp/server/skills.py b/src/mcp/server/skills.py index 0c90f66243..33b95184da 100644 --- a/src/mcp/server/skills.py +++ b/src/mcp/server/skills.py @@ -28,10 +28,11 @@ async def get_skill(ctx, params): from __future__ import annotations +import logging from collections.abc import Awaitable, Callable, Sequence from typing import Any -from mcp_types.jsonrpc import INVALID_PARAMS +from mcp_types.jsonrpc import INTERNAL_ERROR, INVALID_PARAMS from mcp_types.version import MODERN_PROTOCOL_VERSIONS from mcp.server.context import HandlerResult, ServerRequestContext @@ -57,6 +58,8 @@ async def get_skill(ctx, params): __all__ = ["Skills"] +logger = logging.getLogger(__name__) + ListSkillsHandler = Callable[[ServerRequestContext[Any, Any], ListSkillsParams], Awaitable[ListSkillsResult]] GetSkillHandler = Callable[[ServerRequestContext[Any, Any], GetSkillParams], Awaitable[GetSkillResult]] ReadDirectoryHandler = Callable[[ServerRequestContext[Any, Any], ReadDirectoryParams], Awaitable[ReadDirectoryResult]] @@ -102,24 +105,22 @@ async def _handle_list(self, ctx: ServerRequestContext[Any, Any], params: ListSk result = await self._list_skills(ctx, params) try: validate_list_result(result) - except ValueError as exc: - raise MCPError( - code=INVALID_PARAMS, message=f"list_skills handler returned an invalid result: {exc}" - ) from exc + except ValueError: + logger.exception("list_skills handler returned an invalid result") + raise MCPError(code=INTERNAL_ERROR, message="Handler returned an invalid result") from None return _finalize_cacheable(result, ctx.protocol_version) async def _handle_get(self, ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> HandlerResult: _require_skill_md_uri(params.uri) result = await self._get_skill(ctx, params) if result.skill.uri != params.uri: - raise MCPError( - code=INVALID_PARAMS, - message=f"get_skill handler returned {result.skill.uri!r} for requested {params.uri!r}", - ) + logger.error("get_skill handler returned %r for requested %r", result.skill.uri, params.uri) + raise MCPError(code=INTERNAL_ERROR, message="Handler returned an invalid result") try: validate_skill(result.skill) - except ValueError as exc: - raise MCPError(code=INVALID_PARAMS, message=f"get_skill handler returned an invalid result: {exc}") from exc + except ValueError: + logger.exception("get_skill handler returned an invalid result") + raise MCPError(code=INTERNAL_ERROR, message="Handler returned an invalid result") from None return result async def _handle_read_directory( @@ -130,10 +131,9 @@ async def _handle_read_directory( result = await self._read_directory(ctx, params) try: validate_directory_result(params.uri, result) - except ValueError as exc: - raise MCPError( - code=INVALID_PARAMS, message=f"read_directory handler returned an invalid result: {exc}" - ) from exc + except ValueError: + logger.exception("read_directory handler returned an invalid result") + raise MCPError(code=INTERNAL_ERROR, message="Handler returned an invalid result") from None return result diff --git a/tests/server/test_skills.py b/tests/server/test_skills.py index 88bc43b9a0..1327e4c8c4 100644 --- a/tests/server/test_skills.py +++ b/tests/server/test_skills.py @@ -11,7 +11,7 @@ import pytest from inline_snapshot import snapshot -from mcp_types import INVALID_PARAMS, METHOD_NOT_FOUND, Resource +from mcp_types import INTERNAL_ERROR, INVALID_PARAMS, METHOD_NOT_FOUND, Resource from pydantic import BaseModel, ConfigDict from mcp.client.client import Client @@ -135,8 +135,8 @@ async def private_list(ctx: ServerRequestContext[Any, Any], params: ListSkillsPa async def test_skills_list_rejects_a_handler_result_with_an_invalid_skill() -> None: """SDK-defined: a `list_skills` handler bug (a non-conformant skill entry) is caught - before it reaches the client, as an Invalid params error rather than a silently - non-conformant listing.""" + before it reaches the client, as an Internal error — the fault is the server's, not the + caller's params — rather than a silently non-conformant listing.""" async def bad_list(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> ListSkillsResult: return ListSkillsResult( @@ -147,7 +147,7 @@ async def bad_list(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams async with Client(server) as client: with pytest.raises(MCPError) as exc_info: await client.session.send_request(ListSkillsRequest(), ListSkillsResult) - assert exc_info.value.code == INVALID_PARAMS + assert exc_info.value.code == INTERNAL_ERROR async def test_skills_get_returns_the_matching_skill() -> None: @@ -196,7 +196,7 @@ async def test_skills_get_rejects_a_uri_that_does_not_end_in_skill_md() -> None: async def test_skills_get_rejects_a_handler_that_returns_a_mismatched_uri() -> None: """SDK-defined: a `get_skill` handler bug (returning the wrong skill) is caught before - it reaches the client, as an Invalid params error rather than a silently wrong answer.""" + it reaches the client, as an Internal error rather than a silently wrong answer.""" async def wrong_skill(ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> GetSkillResult: return GetSkillResult(skill=_git_workflow_skill()) @@ -207,12 +207,12 @@ async def wrong_skill(ctx: ServerRequestContext[Any, Any], params: GetSkillParam await client.session.send_request( GetSkillRequest(params=GetSkillParams(uri="skill://other/SKILL.md")), GetSkillResult ) - assert exc_info.value.code == INVALID_PARAMS + assert exc_info.value.code == INTERNAL_ERROR async def test_skills_get_rejects_a_matching_but_non_conformant_skill() -> None: """SDK-defined: a `get_skill` handler bug (a non-conformant skill body, distinct from a - URI mismatch) is caught the same way, as an Invalid params error.""" + URI mismatch) is caught the same way, as an Internal error.""" async def bad_skill(ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> GetSkillResult: return GetSkillResult( @@ -223,7 +223,7 @@ async def bad_skill(ctx: ServerRequestContext[Any, Any], params: GetSkillParams) async with Client(server) as client: with pytest.raises(MCPError) as exc_info: await client.session.send_request(GetSkillRequest(params=GetSkillParams(uri=_SKILL_URI)), GetSkillResult) - assert exc_info.value.code == INVALID_PARAMS + assert exc_info.value.code == INTERNAL_ERROR async def test_missing_uri_param_is_rejected_before_the_handler_runs() -> None: @@ -277,6 +277,9 @@ async def test_directory_read_rejects_a_trailing_slash_uri() -> None: async def test_directory_read_rejects_a_handler_result_with_a_grandchild() -> None: + """SDK-defined: a `read_directory` handler bug (a non-direct child) is a server fault, + surfaced as an Internal error, not the caller's Invalid params.""" + async def bad_directory(ctx: ServerRequestContext[Any, Any], params: ReadDirectoryParams) -> ReadDirectoryResult: return ReadDirectoryResult(resources=[Resource(uri="skill://git-workflow/a/b/c.md", name="c.md")]) @@ -288,4 +291,4 @@ async def bad_directory(ctx: ServerRequestContext[Any, Any], params: ReadDirecto await client.session.send_request( ReadDirectoryRequest(params=ReadDirectoryParams(uri="skill://git-workflow")), ReadDirectoryResult ) - assert exc_info.value.code == INVALID_PARAMS + assert exc_info.value.code == INTERNAL_ERROR From 70493de2f03ea8133edfd20b7e54477300c64e37 Mon Sep 17 00:00:00 2001 From: vijay Date: Thu, 10 Sep 2026 06:20:24 +0530 Subject: [PATCH 12/25] Reject directory-shaped skill resource URIs and dot-segment directory children `_validate_resource_uri_in_skill` now rejects a resource URI ending in `/`, which names a directory rather than a file, and `validate_directory_result` now rejects a `.`/`..` child, which is a traversal segment rather than a real direct child. Both slipped past the prior checks. --- src/mcp/shared/skills.py | 4 +++- tests/shared/test_skills.py | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/mcp/shared/skills.py b/src/mcp/shared/skills.py index 22bccfce77..c37d1e9044 100644 --- a/src/mcp/shared/skills.py +++ b/src/mcp/shared/skills.py @@ -157,6 +157,8 @@ def _validate_resource_uri_in_skill(skill_uri: str, resource_uri: str) -> None: resource_parts = urlsplit(resource_uri) if not resource_parts.scheme or resource_parts.query or resource_parts.fragment: raise ValueError(f"resource URI {resource_uri!r} is invalid") + if resource_uri.endswith("/"): + raise ValueError(f"resource URI {resource_uri!r} names a directory, not a file") if skill_parts.scheme != resource_parts.scheme or skill_parts.netloc != resource_parts.netloc: raise ValueError(f"resource URI {resource_uri!r} is outside the skill root {skill_uri!r}") root = skill_parts.path[: -len("/SKILL.md")] @@ -257,7 +259,7 @@ def validate_directory_result(uri: str, result: ReadDirectoryResult) -> None: if child.scheme != scheme or child.netloc != netloc: raise ValueError(f"resource {resource.uri!r} is not a child of directory {uri!r}") relative = child.path.removeprefix(prefix) - if relative == child.path or not relative or "/" in relative: + if relative == child.path or not relative or "/" in relative or relative in (".", ".."): raise ValueError(f"resource {resource.uri!r} is not a direct child of directory {uri!r}") if resource.uri in seen_uris or resource.name in seen_names: raise ValueError(f"directory {uri!r} contains a duplicate child {resource.uri!r}") diff --git a/tests/shared/test_skills.py b/tests/shared/test_skills.py index f0faf72190..daef3781dc 100644 --- a/tests/shared/test_skills.py +++ b/tests/shared/test_skills.py @@ -158,6 +158,13 @@ def test_validate_skill_rejects_a_traversal_segment_in_a_resource_uri() -> None: validate_skill(skill) +def test_validate_skill_rejects_a_resource_uri_with_a_trailing_slash() -> None: + """A manifest entry names a file, not a directory: a trailing slash is rejected.""" + skill = _skill(extra_resources=[_resource("skill://git-workflow/references/")]) + with pytest.raises(ValueError, match="names a directory"): + validate_skill(skill) + + def test_validate_skill_rejects_a_negative_resource_size() -> None: root = "skill://git-workflow/SKILL.md" skill = Skill( @@ -321,6 +328,14 @@ def test_validate_directory_result_rejects_a_grandchild() -> None: validate_directory_result("skill://pdf/templates", result) +@pytest.mark.parametrize("child_uri", ["skill://pdf/templates/.", "skill://pdf/templates/.."]) +def test_validate_directory_result_rejects_a_dot_segment_child(child_uri: str) -> None: + """A `.`/`..` child is a traversal segment, not a real direct child of the directory.""" + result = ReadDirectoryResult(resources=[Resource(uri=child_uri, name="x")]) + with pytest.raises(ValueError, match="direct child"): + validate_directory_result("skill://pdf/templates", result) + + def test_validate_directory_result_rejects_a_uri_outside_the_directory() -> None: result = ReadDirectoryResult(resources=[Resource(uri="skill://other/file.md", name="file.md")]) with pytest.raises(ValueError, match="not a child"): From 38706c1927d3611034a4c9d64e53f918dd2734e8 Mon Sep 17 00:00:00 2001 From: vijay Date: Thu, 10 Sep 2026 06:20:24 +0530 Subject: [PATCH 13/25] Seed the pagination cursor and preserve request _meta across skills pages `list_skills`/`read_directory` now seed the seen-cursor set with a caller-supplied starting cursor, so a server echoing that cursor is caught on the first page instead of being chased a second time. Rebuilding each page request from the caller's own params (via model_copy) also carries `_meta` forward to every page rather than dropping it after the first. --- src/mcp/client/skills.py | 16 ++++++++------ tests/client/test_skills.py | 43 +++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/src/mcp/client/skills.py b/src/mcp/client/skills.py index e0dc0a6e97..646af59221 100644 --- a/src/mcp/client/skills.py +++ b/src/mcp/client/skills.py @@ -57,11 +57,14 @@ async def list_skills(session: ClientSession, params: ListSkillsParams | None = its response is not SEP-2640 conformant. """ _require_extension(session) - cursor = params.cursor if params is not None else None + base = params if params is not None else ListSkillsParams() + cursor = base.cursor skills: list[Skill] = [] - seen_cursors: set[str] = set() + seen_cursors: set[str] = {cursor} if cursor is not None else set() while True: - page = await session.send_request(ListSkillsRequest(params=ListSkillsParams(cursor=cursor)), ListSkillsResult) + page = await session.send_request( + ListSkillsRequest(params=base.model_copy(update={"cursor": cursor})), ListSkillsResult + ) validate_list_result(page) skills.extend(page.skills) if page.next_cursor is None: @@ -109,12 +112,13 @@ async def read_directory(session: ClientSession, uri: str, params: ReadDirectory setting, or its response is not a valid child listing of `uri`. """ _require_extension(session, directory_read=True) - cursor = params.cursor if params is not None else None + base = params if params is not None else ReadDirectoryParams(uri=uri) + cursor = base.cursor resources: list[Resource] = [] - seen_cursors: set[str] = set() + seen_cursors: set[str] = {cursor} if cursor is not None else set() while True: page = await session.send_request( - ReadDirectoryRequest(params=ReadDirectoryParams(uri=uri, cursor=cursor)), ReadDirectoryResult + ReadDirectoryRequest(params=base.model_copy(update={"uri": uri, "cursor": cursor})), ReadDirectoryResult ) validate_directory_result(uri, page) resources.extend(page.resources) diff --git a/tests/client/test_skills.py b/tests/client/test_skills.py index 82df3858ba..59ce631d16 100644 --- a/tests/client/test_skills.py +++ b/tests/client/test_skills.py @@ -253,3 +253,46 @@ async def test_list_skills_starts_from_a_caller_supplied_cursor() -> None: async with Client(_server()) as client: skills = await list_skills(client.session, ListSkillsParams(cursor="page-2")) assert [s.uri for s in skills] == ["skill://other/SKILL.md"] + + +async def test_list_skills_detects_a_server_repeating_the_caller_supplied_cursor() -> None: + """The starting cursor seeds the seen-cursor set: a server that hands back the very cursor + the caller resumed from is caught as a repeat on the first page, not chased a second time.""" + calls = 0 + + async def handler(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> ListSkillsResult: + nonlocal calls + calls += 1 + return ListSkillsResult(skills=[_skill()], next_cursor="resume-here") + + server = MCPServer("catalog", extensions=[Skills(list_skills=handler, get_skill=_get_skill)]) + async with Client(server) as client: + with pytest.raises(ValueError, match="repeated"): + await list_skills(client.session, ListSkillsParams(cursor="resume-here")) + assert calls == 1 + + +async def test_list_skills_threads_request_meta_onto_every_page() -> None: + """A caller-supplied `_meta` rides along with each page request, not just the first.""" + seen_meta: list[Any] = [] + + async def handler(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> ListSkillsResult: + seen_meta.append(params.meta) + if params.cursor is None: + return ListSkillsResult(skills=[_skill()], next_cursor="page-2") + return ListSkillsResult( + skills=[ + Skill( + uri="skill://other/SKILL.md", frontmatter={"name": "other", "description": "d"}, resources="dynamic" + ) + ], + next_cursor=None, + ) + + server = MCPServer("catalog", extensions=[Skills(list_skills=handler, get_skill=_get_skill)]) + async with Client(server) as client: + await list_skills(client.session, ListSkillsParams(meta={"progressToken": "t"})) + # The transport enriches `_meta` with its own keys; what matters is the caller's token + # reaching the server on both the first page and the cursor-following second one. + assert len(seen_meta) == 2 + assert all(m is not None and m.get("progress_token") == "t" for m in seen_meta) From f1d29209e6650cd1f1ed52767a7e5365433a3bac Mon Sep 17 00:00:00 2001 From: vijay Date: Thu, 10 Sep 2026 06:20:24 +0530 Subject: [PATCH 14/25] Correct Skills docs on read_skill_uri return type and verify on dynamic skills `read_skill_uri` returns a `ReadResourceResult`, not bytes, and `verify_skill_resource` raises for a `"dynamic"` skill (no digests to check). Import the tutorials' symbols from `mcp.types` rather than the internal `mcp_types` package. --- docs/advanced/skills.md | 6 ++++-- docs_src/skills/tutorial001.py | 3 +-- docs_src/skills/tutorial001_client.py | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/docs/advanced/skills.md b/docs/advanced/skills.md index a2527cd2e6..be412d37b2 100644 --- a/docs/advanced/skills.md +++ b/docs/advanced/skills.md @@ -50,12 +50,14 @@ skills or resources in one call; `get_skill` costs exactly one request. These th server's response against the SEP-2640 conformance rules before returning it — a name that doesn't match its URI, a digest in the wrong shape, or an incomplete manifest raises `ValueError` rather than reaching your code. `read_skill_uri` is the exception: a thin, discoverable alias for -`resources/read` that returns bytes and validates nothing itself (see the next paragraph). +`resources/read` that returns a `ReadResourceResult` (text or blob contents) and validates nothing +itself (see the next paragraph). `verify_skill_resource(skill, uri, content)` checks a file's bytes — size, then SHA-256 digest — against the entry you hold for it. Call it after `read_skill_uri` and before treating the content as trustworthy: `resources/read` returns whatever bytes the server sends *right now*, verification -is what ties those bytes back to the manifest you already validated. +is what ties those bytes back to the manifest you already validated. It applies to a static +manifest only — a `"dynamic"` skill carries no digests, so calling it on one raises `ValueError`. !!! warning Skill content is untrusted model input, exactly like any other server-provided text. SEP-2640 diff --git a/docs_src/skills/tutorial001.py b/docs_src/skills/tutorial001.py index 07044d2282..61cc43c2e8 100644 --- a/docs_src/skills/tutorial001.py +++ b/docs_src/skills/tutorial001.py @@ -1,8 +1,6 @@ import hashlib from typing import Any -from mcp_types import INVALID_PARAMS - from mcp.server.context import ServerRequestContext from mcp.server.mcpserver import MCPServer from mcp.server.mcpserver.resources import TextResource @@ -16,6 +14,7 @@ Skill, SkillResource, ) +from mcp.types import INVALID_PARAMS SKILL_URI = "skill://git-workflow/SKILL.md" SKILL_MD = """\ diff --git a/docs_src/skills/tutorial001_client.py b/docs_src/skills/tutorial001_client.py index 7e23428f77..4613733aaa 100644 --- a/docs_src/skills/tutorial001_client.py +++ b/docs_src/skills/tutorial001_client.py @@ -1,8 +1,8 @@ import anyio -from mcp_types import TextResourceContents from mcp import Client from mcp.client.skills import get_skill, list_skills, read_skill_uri, verify_skill_resource +from mcp.types import TextResourceContents async def main() -> None: From 63254f8d54e60e7c175e781cd930a0dd077b1c89 Mon Sep 17 00:00:00 2001 From: vijay Date: Thu, 10 Sep 2026 06:28:43 +0530 Subject: [PATCH 15/25] Construct ListSkillsParams _meta via model_validate in the skills client test The keyword form `ListSkillsParams(meta=...)` fails pyright: the field's alias is `_meta`, so the synthesized constructor only accepts the alias. Build the params through `model_validate` instead, matching how the field is populated off the wire. --- tests/client/test_skills.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/client/test_skills.py b/tests/client/test_skills.py index 59ce631d16..959fb368db 100644 --- a/tests/client/test_skills.py +++ b/tests/client/test_skills.py @@ -291,7 +291,8 @@ async def handler(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) server = MCPServer("catalog", extensions=[Skills(list_skills=handler, get_skill=_get_skill)]) async with Client(server) as client: - await list_skills(client.session, ListSkillsParams(meta={"progressToken": "t"})) + params = ListSkillsParams.model_validate({"_meta": {"progressToken": "t"}}) + await list_skills(client.session, params) # The transport enriches `_meta` with its own keys; what matters is the caller's token # reaching the server on both the first page and the cursor-following second one. assert len(seen_meta) == 2 From 7e32fc97115036a818c8119ee1eee60dd6bb468f Mon Sep 17 00:00:00 2001 From: vijay Date: Thu, 10 Sep 2026 06:41:15 +0530 Subject: [PATCH 16/25] Test that request _meta is camelCase on the wire and snake_case to a handler A caller's `_meta.progressToken` is carried over the wire under its camelCase JSON alias but deserialized back to the snake_case field `progress_token`, so a server handler reading `params.meta` finds `progress_token`. Pin both forms of the same params object for `skills/list` and `resources/directory/read`, and expand the client round-trip test's comment to spell out the distinction. --- tests/client/test_skills.py | 4 +++- tests/shared/test_skills.py | 23 +++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/tests/client/test_skills.py b/tests/client/test_skills.py index 959fb368db..9bda9e5834 100644 --- a/tests/client/test_skills.py +++ b/tests/client/test_skills.py @@ -293,7 +293,9 @@ async def handler(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) async with Client(server) as client: params = ListSkillsParams.model_validate({"_meta": {"progressToken": "t"}}) await list_skills(client.session, params) - # The transport enriches `_meta` with its own keys; what matters is the caller's token + # `_meta` crosses the wire camelCase (`progressToken`), but the server deserializes it back + # through the meta model, which exposes the known field snake_case as `progress_token`; the + # transport also enriches `_meta` with its own keys. What matters is the caller's token # reaching the server on both the first page and the cursor-following second one. assert len(seen_meta) == 2 assert all(m is not None and m.get("progress_token") == "t" for m in seen_meta) diff --git a/tests/shared/test_skills.py b/tests/shared/test_skills.py index daef3781dc..a15a4f12b9 100644 --- a/tests/shared/test_skills.py +++ b/tests/shared/test_skills.py @@ -5,12 +5,15 @@ """ import hashlib +from typing import Any import pytest from mcp_types import Resource from mcp.shared.skills import ( + ListSkillsParams, ListSkillsResult, + ReadDirectoryParams, ReadDirectoryResult, Skill, SkillResource, @@ -286,6 +289,26 @@ def test_validate_list_result_accepts_an_empty_listing() -> None: validate_list_result(ListSkillsResult(skills=[])) +@pytest.mark.parametrize( + ("params_type", "payload"), + [ + (ListSkillsParams, {"_meta": {"progressToken": "t"}}), + (ReadDirectoryParams, {"uri": "skill://pdf/templates", "_meta": {"progressToken": "t"}}), + ], +) +def test_request_meta_is_camelcase_on_the_wire_but_snakecase_to_a_handler( + params_type: type[ListSkillsParams] | type[ReadDirectoryParams], payload: dict[str, Any] +) -> None: + """A caller's `_meta.progressToken` rides the wire camelCase (its JSON alias) yet is read back + snake_case as `progress_token` once deserialized - two spellings, one value. This is what lets + `list_skills`/`read_directory` forward a caller's `_meta` to every page while a server handler + still finds `progress_token` in `params.meta`; both spellings working is the point.""" + params = params_type.model_validate(payload) + wire = params.model_dump(by_alias=True, mode="json", exclude_none=True) + assert wire["_meta"] == {"progressToken": "t"} # camelCase over the wire + assert params.meta is not None and params.meta.get("progress_token") == "t" # snake_case to a handler + + @pytest.mark.parametrize("uri", ["skill://pdf/templates/", "not-a-uri"]) def test_parse_directory_uri_rejects_malformed_uris(uri: str) -> None: with pytest.raises(ValueError, match="directory URI"): From 473804e77fae927d6a6ff97473de3061706001ef Mon Sep 17 00:00:00 2001 From: vijay Date: Thu, 10 Sep 2026 06:54:24 +0530 Subject: [PATCH 17/25] Empty commit to re-trigger CI after a transient PyPI network flake From a2e49d572e70e4cd001b838509975be52fda2ef5 Mon Sep 17 00:00:00 2001 From: vijay Date: Thu, 10 Sep 2026 06:55:08 +0530 Subject: [PATCH 18/25] empty coomit to re-trigger ci check From 071ad0e8b2bc3a661e94c9fc00990be5d8889e6b Mon Sep 17 00:00:00 2001 From: vijay Date: Fri, 11 Sep 2026 10:52:15 +0530 Subject: [PATCH 19/25] added changes for cache-attributes and its tests --- docs/advanced/skills.md | 8 ++++---- src/mcp/server/skills.py | 13 +++++++------ src/mcp/shared/skills.py | 14 +++++++++++--- tests/server/test_skills.py | 21 +++++++++++++++++++++ 4 files changed, 43 insertions(+), 13 deletions(-) diff --git a/docs/advanced/skills.md b/docs/advanced/skills.md index be412d37b2..15208aa96c 100644 --- a/docs/advanced/skills.md +++ b/docs/advanced/skills.md @@ -95,10 +95,10 @@ advertised the setting. ## Protocol version and caching -In protocol version `2026-07-28` and later, `skills/list` results carry the base protocol's -list-caching fields, [`ttlMs` and `cacheScope`](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2549) — the -same freshness hint `tools/list` and `resources/list` carry. `Skills` fills `cacheScope` with -`"public"` when your handler leaves it unset, and omits both fields entirely on an +In protocol version `2026-07-28` and later, `skills/list` and `skills/get` results carry the base +protocol's caching fields, [`ttlMs` and `cacheScope`](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2549) — the +same freshness hint `tools/list`, `resources/list`, and `resources/read` carry. `Skills` fills +`cacheScope` with `"public"` when your handler leaves it unset, and omits both fields entirely on an older connection, so you don't have to branch on protocol version yourself. ## What this SDK doesn't do diff --git a/src/mcp/server/skills.py b/src/mcp/server/skills.py index 33b95184da..0b94edbe63 100644 --- a/src/mcp/server/skills.py +++ b/src/mcp/server/skills.py @@ -32,6 +32,7 @@ async def get_skill(ctx, params): from collections.abc import Awaitable, Callable, Sequence from typing import Any +from mcp_types import CacheableResult from mcp_types.jsonrpc import INTERNAL_ERROR, INVALID_PARAMS from mcp_types.version import MODERN_PROTOCOL_VERSIONS @@ -121,7 +122,7 @@ async def _handle_get(self, ctx: ServerRequestContext[Any, Any], params: GetSkil except ValueError: logger.exception("get_skill handler returned an invalid result") raise MCPError(code=INTERNAL_ERROR, message="Handler returned an invalid result") from None - return result + return _finalize_cacheable(result, ctx.protocol_version) async def _handle_read_directory( self, ctx: ServerRequestContext[Any, Any], params: ReadDirectoryParams @@ -151,13 +152,13 @@ def _require_directory_uri(uri: str) -> None: raise MCPError(code=INVALID_PARAMS, message=str(exc)) from exc -def _finalize_cacheable(result: ListSkillsResult, protocol_version: str) -> HandlerResult: +def _finalize_cacheable(result: CacheableResult, protocol_version: str) -> HandlerResult: """Gate SEP-2549's `ttlMs`/`cacheScope` to protocol version 2026-07-28+. - `skills/list` is an extension method, so — unlike a core spec method — the - runner's per-version surface sieve never runs on its result; nothing else - strips these fields for a legacy connection. `CacheableResult` defaults to - `cache_scope="private"`; SEP-2640 calls for `"public"` when unset. + `skills/list` and `skills/get` are extension methods, so — unlike a core spec + method — the runner's per-version surface sieve never runs on their results; + nothing else strips these fields for a legacy connection. `CacheableResult` + defaults to `cache_scope="private"`; SEP-2640 calls for `"public"` when unset. """ if protocol_version in MODERN_PROTOCOL_VERSIONS: if "cache_scope" not in result.model_fields_set: diff --git a/src/mcp/shared/skills.py b/src/mcp/shared/skills.py index c37d1e9044..9242f55bc0 100644 --- a/src/mcp/shared/skills.py +++ b/src/mcp/shared/skills.py @@ -12,7 +12,7 @@ from typing import Any, Literal from urllib.parse import urlsplit -from mcp_types import CacheableResult, PaginatedRequestParams, PaginatedResult, Request, RequestParams, Resource, Result +from mcp_types import CacheableResult, PaginatedRequestParams, PaginatedResult, Request, RequestParams, Resource from pydantic import BaseModel, ConfigDict from pydantic.alias_generators import to_camel @@ -92,8 +92,16 @@ class GetSkillParams(RequestParams): """URI of the skill's `SKILL.md`.""" -class GetSkillResult(Result): - """Result of `skills/get`.""" +class GetSkillResult(CacheableResult): + """Result of `skills/get`. + + Like `ListSkillsResult`, this extends `CacheableResult`: the stable spec page + makes `GetSkillResult` carry SEP-2549's `ttl_ms`/`cache_scope`, the same + freshness hint `resources/read` gives. As on `skills/list`, nothing sieves + these fields off the wire for a pre-2026-07-28 connection automatically (see + `mcp.server.skills`), so a caller constructing this directly for such a + connection must omit them. + """ skill: Skill diff --git a/tests/server/test_skills.py b/tests/server/test_skills.py index 1327e4c8c4..3f04e22217 100644 --- a/tests/server/test_skills.py +++ b/tests/server/test_skills.py @@ -158,6 +158,27 @@ async def test_skills_get_returns_the_matching_skill() -> None: assert result.skill.uri == _SKILL_URI +async def test_skills_get_carries_cache_fields_on_the_2026_07_28_wire() -> None: + """SEP-2640 Retrieval: on 2026-07-28+, `GetSkillResult` extends `CacheableResult`, so the + result carries the SEP-2549 cache fields, defaulting `cacheScope` to `"public"` when unset — + the same treatment `skills/list` gets.""" + async with Client(_server(), mode="2026-07-28") as client: + raw = await client.session.send_request(GetSkillRequest(params=GetSkillParams(uri=_SKILL_URI)), _RawResult) + extra = raw.model_extra or {} + assert extra["cacheScope"] == "public" + assert extra["ttlMs"] == 0 + + +async def test_skills_get_omits_cache_fields_on_a_legacy_wire() -> None: + """As with `skills/list`, this extension method strips the SEP-2549 fields itself for a + pre-2026-07-28 connection, since the runner's per-version sieve only applies to core methods.""" + async with Client(_server(), mode="legacy") as client: + raw = await client.session.send_request(GetSkillRequest(params=GetSkillParams(uri=_SKILL_URI)), _RawResult) + extra = raw.model_extra or {} + assert "cacheScope" not in extra + assert "ttlMs" not in extra + + async def test_skills_get_answers_for_a_skill_absent_from_the_listing() -> None: """SEP-2640 Retrieval: a server MUST answer for every skill it serves, whether or not that skill appears in `skills/list`.""" From cde5cffa2563f94b79912033f6192c4cdef50198 Mon Sep 17 00:00:00 2001 From: vijay Date: Wed, 23 Sep 2026 22:24:58 +0530 Subject: [PATCH 20/25] incorporated review comments and defined skills as extensions --- docs/advanced/skills.md | 127 +++++++++------ docs_src/skills/tutorial001_client.py | 12 +- src/mcp/client/skills.py | 217 +++++++++++++++----------- tests/client/test_skills.py | 81 +++++----- tests/docs_src/test_skills.py | 27 ++-- 5 files changed, 269 insertions(+), 195 deletions(-) diff --git a/docs/advanced/skills.md b/docs/advanced/skills.md index 15208aa96c..62fbc2b986 100644 --- a/docs/advanced/skills.md +++ b/docs/advanced/skills.md @@ -1,76 +1,97 @@ # Skills [SEP-2640](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2640) defines a -convention for serving [Agent Skills](https://agentskills.io/) over MCP: a skill is a directory -of files — minimally a `SKILL.md` with YAML frontmatter — exposed as ordinary MCP resources, -conventionally under a `skill://` URI. A server enumerates its skills with `skills/list`, -answers for any one of them by URI with `skills/get`, and — optionally — lists a directory's -direct children with `resources/directory/read`. - -The SDK ships this as the built-in `Skills` extension (`io.modelcontextprotocol/skills`). If -[Extensions](extensions.md) are new to you, skim that page first. - -`Skills` provides 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 serve -each skill's files as ordinary resources through `MCPServer.add_resource` or an -`@mcp.resource(...)` template handler. +convention for serving [Agent Skills](https://agentskills.io/) over MCP. A skill is just a +directory of files — at minimum a `SKILL.md` with YAML frontmatter — that you expose as ordinary +MCP resources, conventionally under a `skill://` URI. + +A server enumerates its skills with `skills/list`, answers for any single one by URI with +`skills/get`, and — optionally — lists a directory's direct children with +`resources/directory/read`. + +The SDK ships this as the built-in `Skills` extension (`io.modelcontextprotocol/skills`). There's +one on the server side and one on the client side. If [Extensions](extensions.md) are new to you, +skim that page first. + +!!! info + `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. ## Serving a skill +Here's a server that serves one skill: + ```python title="server.py" hl_lines="31-41 44-51 55" --8<-- "docs_src/skills/tutorial001.py" ``` -Three moves: +There are three moves here: -* `Skill(uri=..., frontmatter=..., resources=[...])`: one entry, identical in shape whether it +* `Skill(uri=..., frontmatter=..., resources=[...])` is one entry. It has the same shape whether it comes back from `skills/list` or `skills/get`. `resources` is the skill's complete file manifest — every file, `SKILL.md` included, each with a `sha256:...` digest and byte size — or the string `"dynamic"` for content generated on demand. -* `list_skills`/`get_skill`: plain async callables, invoked per request. `get_skill` **must** - answer for a skill even if a real `list_skills` implementation omitted it — SEP-2640 requires - a server to answer by URI for every skill it serves, listed or not. -* `mcp.add_resource(TextResource(uri=SKILL_URI, ...))`: the skill's actual file content, served - through the SDK's ordinary resource machinery. `Skills` never reads or writes resource content - itself. +* `list_skills` and `get_skill` are plain async callables, invoked once per request. `get_skill` + **must** answer for a skill even if your `list_skills` left it out — SEP-2640 requires a server + to answer by URI for every skill it serves, listed or not. +* `mcp.add_resource(TextResource(uri=SKILL_URI, ...))` registers the skill's actual file content, + served through the SDK's ordinary resource machinery. `Skills` never reads or writes resource + content itself. -`Skills(list_skills=..., get_skill=...)` is all a server needs; `resources/directory/read` is -optional (below). +And that's it. `Skills(list_skills=..., get_skill=...)` is all a server needs; +`resources/directory/read` is optional (more on that below). ## Fetching a skill -```python title="client.py" hl_lines="5" +On the client side, `Skills` is a [`ClientExtension`](extensions.md). You register it the same way +you register any other one — by passing it to `Client(extensions=[...])` — and then call `bind` to +get the verbs tied to that connection: + +```python title="client.py" hl_lines="9-11" --8<-- "docs_src/skills/tutorial001_client.py" ``` -`list_skills` and `read_directory` follow `nextCursor` to completion, so you get every page's -skills or resources in one call; `get_skill` costs exactly one request. These three validate the -server's response against the SEP-2640 conformance rules before returning it — a name that doesn't -match its URI, a digest in the wrong shape, or an incomplete manifest raises `ValueError` rather -than reaching your code. `read_skill_uri` is the exception: a thin, discoverable alias for -`resources/read` that returns a `ReadResourceResult` (text or blob contents) and validates nothing -itself (see the next paragraph). +`skills.bind(client)` hands you a `BoundSkills`, and its methods are the SEP-2640 verbs: -`verify_skill_resource(skill, uri, content)` checks a file's bytes — size, then SHA-256 digest — -against the entry you hold for it. Call it after `read_skill_uri` and before treating the content -as trustworthy: `resources/read` returns whatever bytes the server sends *right now*, verification -is what ties those bytes back to the manifest you already validated. It applies to a static -manifest only — a `"dynamic"` skill carries no digests, so calling it on one raises `ValueError`. +* `list_skills` and `read_directory` follow `nextCursor` to completion, so a single call gives you + every page's skills or resources. +* `get_skill` costs exactly one request. + +These three validate the server's response against the SEP-2640 conformance rules before returning +it. A name that doesn't match its URI, a digest in the wrong shape, or an incomplete manifest raises +`ValueError` rather than reaching your code. + +`read_skill_uri` is the exception. It's a thin, discoverable alias for `resources/read` that returns +a `ReadResourceResult` (text or blob contents) and validates nothing itself — that's the next step. + +!!! tip + `verify_skill_resource(skill, uri, content)` checks a file's bytes — size, then SHA-256 + digest — against the entry you hold for it. Call it after `read_skill_uri` and *before* you + treat the content as trustworthy. + + `resources/read` returns whatever bytes the server sends *right now*; verification is what ties + those bytes back to the manifest you already validated. It applies to a static manifest only — + a `"dynamic"` skill carries no digests, so calling it on one raises `ValueError`. !!! warning Skill content is untrusted model input, exactly like any other server-provided text. SEP-2640 requires a host to tag it with its originating server before it reaches the model, and to never grant the frontmatter's `allowed-tools` field (or any other permission-widening field) - without explicit per-skill user approval. Both are host responsibilities the SDK cannot - discharge for you — see the SEP's [Security Implications](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2640) + without explicit per-skill user approval. + + Both are host responsibilities the SDK cannot discharge for you — read the SEP's + [Security Implications](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2640) section before building a host on top of this extension. ## Directory reads A skill's instructions often point at a directory rather than a file ("pick the matching -template from `templates/`"). `resources/list` cannot answer that — it enumerates a server's +template from `templates/`"). `resources/list` can't answer that — it enumerates a server's entire resource space, not one subtree — so SEP-2640 adds `resources/directory/read`, gated behind the `directoryRead` capability setting: @@ -88,22 +109,24 @@ mcp = MCPServer( ``` Supplying `read_directory` advertises `{"directoryRead": true}` under the extension's -capabilities; omitting it advertises neither the setting nor the method — a client calling -`resources/directory/read` against such a server gets `METHOD_NOT_FOUND`. -`mcp.client.skills.read_directory` raises before sending if the connected server hasn't -advertised the setting. +capabilities. Omitting it advertises neither the setting nor the method — a client calling +`resources/directory/read` against such a server gets `METHOD_NOT_FOUND`. On the client side, +`read_directory` raises before it sends anything if the connected server hasn't advertised the +setting. ## Protocol version and caching In protocol version `2026-07-28` and later, `skills/list` and `skills/get` results carry the base -protocol's caching fields, [`ttlMs` and `cacheScope`](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2549) — the -same freshness hint `tools/list`, `resources/list`, and `resources/read` carry. `Skills` fills +protocol's caching fields, [`ttlMs` and `cacheScope`](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2549) — +the same freshness hint `tools/list`, `resources/list`, and `resources/read` carry. `Skills` fills `cacheScope` with `"public"` when your handler leaves it unset, and omits both fields entirely on an -older connection, so you don't have to branch on protocol version yourself. +older connection. So 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. 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. +`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. + +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. diff --git a/docs_src/skills/tutorial001_client.py b/docs_src/skills/tutorial001_client.py index 4613733aaa..4064bc9e95 100644 --- a/docs_src/skills/tutorial001_client.py +++ b/docs_src/skills/tutorial001_client.py @@ -1,17 +1,19 @@ import anyio from mcp import Client -from mcp.client.skills import get_skill, list_skills, read_skill_uri, verify_skill_resource +from mcp.client.skills import Skills, verify_skill_resource from mcp.types import TextResourceContents async def main() -> None: - async with Client("http://localhost:8000/mcp") as client: - for skill in await list_skills(client.session): + skills = Skills() + async with Client("http://localhost:8000/mcp", extensions=[skills]) as client: + catalog = skills.bind(client) + for skill in await catalog.list_skills(): print(skill.uri, skill.frontmatter["description"]) - skill = await get_skill(client.session, "skill://git-workflow/SKILL.md") - result = await read_skill_uri(client.session, skill.uri) + skill = await catalog.get_skill("skill://git-workflow/SKILL.md") + result = await catalog.read_skill_uri(skill.uri) content = result.contents[0] if isinstance(content, TextResourceContents): verify_skill_resource(skill, skill.uri, content.text.encode()) diff --git a/src/mcp/client/skills.py b/src/mcp/client/skills.py index 646af59221..451ebd3cfa 100644 --- a/src/mcp/client/skills.py +++ b/src/mcp/client/skills.py @@ -1,23 +1,26 @@ -"""Client-side convenience wrappers for the Skills extension (SEP-2640). - -SEP-2640 needs no client-side method registration: `skills/list`, `skills/get`, -and `resources/directory/read` are ordinary vendor requests sent through -`ClientSession.send_request`, exactly like [Extension verbs](../advanced/extensions.md#extension-verbs). -The functions below are the thin, named wrappers SEP-2640's "SDKs: Convenience -Wrappers" section recommends — each validates the server's advertised support -before sending, and `list_skills`/`read_directory` follow `nextCursor` to -completion so a caller sees one page's worth of ergonomics regardless of how -many requests it took. - - async with Client("http://localhost:8000/mcp") as client: - for skill in await list_skills(client.session): +"""Client-side Skills extension (SEP-2640). + +`Skills` is an opt-in [`ClientExtension`](../advanced/extensions.md) for talking +to a skills catalog server. Register it with `Client(extensions=[Skills()])`, +then call `bind(client)` for the SEP-2640 verbs — `list_skills`, `get_skill`, +`read_skill_uri`, and `read_directory` — tied to that connection: + + async with Client("http://localhost:8000/mcp", extensions=[skills := Skills()]) as client: + for skill in await skills.bind(client).list_skills(): print(skill.uri, skill.frontmatter["description"]) + +`bind(client)` returns a `BoundSkills`. Each verb validates that the server +advertises the extension before sending; `list_skills` and `read_directory` +follow `nextCursor` to completion, so one call returns every page's results. """ from __future__ import annotations +from typing import TYPE_CHECKING + from mcp_types import ReadResourceResult, Resource +from mcp.client.extension import ClientExtension from mcp.client.session import ClientSession from mcp.shared.skills import ( EXTENSION_ID, @@ -37,94 +40,124 @@ ) from mcp.shared.skills import verify_skill_resource as verify_skill_resource -__all__ = ["get_skill", "list_skills", "read_directory", "read_skill_uri", "verify_skill_resource"] +if TYPE_CHECKING: + from mcp.client.client import Client +__all__ = ["BoundSkills", "Skills", "verify_skill_resource"] -def _require_extension(session: ClientSession, *, directory_read: bool = False) -> None: - capabilities = session.server_capabilities - settings = (capabilities.extensions or {}).get(EXTENSION_ID) if capabilities else None - if settings is None: - raise ValueError(f"server does not advertise the {EXTENSION_ID!r} extension") - if directory_read and not settings.get("directoryRead"): - raise ValueError(f"server does not advertise {EXTENSION_ID!r}'s directoryRead setting") +class Skills(ClientExtension): + """The client-side Skills extension: register, then `bind` for typed verbs. -async def list_skills(session: ClientSession, params: ListSkillsParams | None = None) -> list[Skill]: - """Call `skills/list`, following `nextCursor` to completion, and validate the result. - - Raises: - ValueError: If the server doesn't advertise the Skills extension, or - its response is not SEP-2640 conformant. - """ - _require_extension(session) - base = params if params is not None else ListSkillsParams() - cursor = base.cursor - skills: list[Skill] = [] - seen_cursors: set[str] = {cursor} if cursor is not None else set() - while True: - page = await session.send_request( - ListSkillsRequest(params=base.model_copy(update={"cursor": cursor})), ListSkillsResult - ) - validate_list_result(page) - skills.extend(page.skills) - if page.next_cursor is None: - return skills - if page.next_cursor in seen_cursors: - raise ValueError(f"server repeated skills/list pagination cursor {page.next_cursor!r}") - seen_cursors.add(page.next_cursor) - cursor = page.next_cursor - - -async def get_skill(session: ClientSession, uri: str) -> Skill: - """Call `skills/get` for `uri` and validate the result. - - Unlike `list_skills`, this succeeds for a skill absent from any listing — - per SEP-2640, a server MUST answer `skills/get` for every skill it serves. - - Raises: - ValueError: If the server doesn't advertise the Skills extension, its - response names a different skill, or the skill is not conformant. + Pass an instance to `Client(extensions=[Skills()])` — this advertises + `io.modelcontextprotocol/skills` under the client's capabilities — and call + `bind(client)` once the client is connected for a `BoundSkills` handle. """ - _require_extension(session) - result = await session.send_request(GetSkillRequest(params=GetSkillParams(uri=uri)), GetSkillResult) - if result.skill.uri != uri: - raise ValueError(f"server returned skill {result.skill.uri!r} for requested {uri!r}") - validate_skill(result.skill) - return result.skill + identifier = EXTENSION_ID -async def read_skill_uri(session: ClientSession, uri: str) -> ReadResourceResult: - """Read a skill file's content via `resources/read`. + def bind(self, client: Client) -> BoundSkills: + """Return the SEP-2640 verbs bound to `client`'s connected session. - A thin, discoverable alias: works for any `skill://` (or other-scheme) - file regardless of whether the skill was ever enumerated. Verify the - result against a held `Skill` entry with `verify_skill_resource` before - treating it as trusted content — this call does not verify anything itself. - """ - return await session.read_resource(uri) + Raises: + RuntimeError: If `client` has not entered its `async with` block yet. + """ + return BoundSkills(client.session) -async def read_directory(session: ClientSession, uri: str, params: ReadDirectoryParams | None = None) -> list[Resource]: - """Call `resources/directory/read` for `uri`, following `nextCursor` to completion. +class BoundSkills: + """The SEP-2640 verbs bound to one connected session. - Raises: - ValueError: If the server doesn't advertise the `directoryRead` - setting, or its response is not a valid child listing of `uri`. + Obtain it from `Skills.bind(client)`. `list_skills` and `read_directory` + follow `nextCursor` to completion; every method validates the server's + response against the SEP-2640 conformance rules before returning it. """ - _require_extension(session, directory_read=True) - base = params if params is not None else ReadDirectoryParams(uri=uri) - cursor = base.cursor - resources: list[Resource] = [] - seen_cursors: set[str] = {cursor} if cursor is not None else set() - while True: - page = await session.send_request( - ReadDirectoryRequest(params=base.model_copy(update={"uri": uri, "cursor": cursor})), ReadDirectoryResult - ) - validate_directory_result(uri, page) - resources.extend(page.resources) - if page.next_cursor is None: - return resources - if page.next_cursor in seen_cursors: - raise ValueError(f"server repeated resources/directory/read pagination cursor {page.next_cursor!r}") - seen_cursors.add(page.next_cursor) - cursor = page.next_cursor + + def __init__(self, session: ClientSession) -> None: + self._session = session + + def _require_extension(self, *, directory_read: bool = False) -> None: + capabilities = self._session.server_capabilities + settings = (capabilities.extensions or {}).get(EXTENSION_ID) if capabilities else None + if settings is None: + raise ValueError(f"server does not advertise the {EXTENSION_ID!r} extension") + if directory_read and not settings.get("directoryRead"): + raise ValueError(f"server does not advertise {EXTENSION_ID!r}'s directoryRead setting") + + async def list_skills(self, params: ListSkillsParams | None = None) -> list[Skill]: + """Call `skills/list`, following `nextCursor` to completion, and validate the result. + + Raises: + ValueError: If the server doesn't advertise the Skills extension, or + its response is not SEP-2640 conformant. + """ + self._require_extension() + base = params if params is not None else ListSkillsParams() + cursor = base.cursor + skills: list[Skill] = [] + seen_cursors: set[str] = {cursor} if cursor is not None else set() + while True: + page = await self._session.send_request( + ListSkillsRequest(params=base.model_copy(update={"cursor": cursor})), ListSkillsResult + ) + validate_list_result(page) + skills.extend(page.skills) + if page.next_cursor is None: + return skills + if page.next_cursor in seen_cursors: + raise ValueError(f"server repeated skills/list pagination cursor {page.next_cursor!r}") + seen_cursors.add(page.next_cursor) + cursor = page.next_cursor + + async def get_skill(self, uri: str) -> Skill: + """Call `skills/get` for `uri` and validate the result. + + Unlike `list_skills`, this succeeds for a skill absent from any listing — + per SEP-2640, a server MUST answer `skills/get` for every skill it serves. + + Raises: + ValueError: If the server doesn't advertise the Skills extension, its + response names a different skill, or the skill is not conformant. + """ + self._require_extension() + result = await self._session.send_request(GetSkillRequest(params=GetSkillParams(uri=uri)), GetSkillResult) + if result.skill.uri != uri: + raise ValueError(f"server returned skill {result.skill.uri!r} for requested {uri!r}") + validate_skill(result.skill) + return result.skill + + async def read_skill_uri(self, uri: str) -> ReadResourceResult: + """Read a skill file's content via `resources/read`. + + A thin, discoverable alias: works for any `skill://` (or other-scheme) + file regardless of whether the skill was ever enumerated. Verify the + result against a held `Skill` entry with `verify_skill_resource` before + treating it as trusted content — this call does not verify anything itself. + """ + return await self._session.read_resource(uri) + + async def read_directory(self, uri: str, params: ReadDirectoryParams | None = None) -> list[Resource]: + """Call `resources/directory/read` for `uri`, following `nextCursor` to completion. + + Raises: + ValueError: If the server doesn't advertise the `directoryRead` + setting, or its response is not a valid child listing of `uri`. + """ + self._require_extension(directory_read=True) + base = params if params is not None else ReadDirectoryParams(uri=uri) + cursor = base.cursor + resources: list[Resource] = [] + seen_cursors: set[str] = {cursor} if cursor is not None else set() + while True: + page = await self._session.send_request( + ReadDirectoryRequest(params=base.model_copy(update={"uri": uri, "cursor": cursor})), + ReadDirectoryResult, + ) + validate_directory_result(uri, page) + resources.extend(page.resources) + if page.next_cursor is None: + return resources + if page.next_cursor in seen_cursors: + raise ValueError(f"server repeated resources/directory/read pagination cursor {page.next_cursor!r}") + seen_cursors.add(page.next_cursor) + cursor = page.next_cursor diff --git a/tests/client/test_skills.py b/tests/client/test_skills.py index 9bda9e5834..c3fda84f68 100644 --- a/tests/client/test_skills.py +++ b/tests/client/test_skills.py @@ -1,13 +1,16 @@ -"""Tests for the client-side Skills convenience wrappers (SEP-2640, `mcp.client.skills`).""" +"""Tests for the client-side Skills extension (SEP-2640, `mcp.client.skills`).""" import hashlib +from collections.abc import AsyncIterator +from contextlib import asynccontextmanager from typing import Any import pytest from mcp_types import INVALID_PARAMS, Resource, TextResourceContents from mcp.client.client import Client -from mcp.client.skills import get_skill, list_skills, read_directory, read_skill_uri, verify_skill_resource +from mcp.client.skills import BoundSkills, verify_skill_resource +from mcp.client.skills import Skills as ClientSkills from mcp.server.context import ServerRequestContext from mcp.server.extension import Extension, MethodBinding from mcp.server.mcpserver import MCPServer @@ -134,44 +137,52 @@ def _server(*, with_directory_read: bool = False) -> MCPServer: return server +@asynccontextmanager +async def _skills(server: MCPServer) -> AsyncIterator[BoundSkills]: + """Register the client `Skills` extension, connect, and yield the bound verbs.""" + extension = ClientSkills() + async with Client(server, extensions=[extension]) as client: + yield extension.bind(client) + + async def test_list_skills_follows_next_cursor_to_completion() -> None: - async with Client(_server()) as client: - skills = await list_skills(client.session) - assert [s.uri for s in skills] == [_SKILL_URI, "skill://other/SKILL.md"] + async with _skills(_server()) as skills: + result = await skills.list_skills() + assert [s.uri for s in result] == [_SKILL_URI, "skill://other/SKILL.md"] async def test_list_skills_raises_on_a_server_that_repeats_its_cursor() -> None: server = MCPServer( "catalog", extensions=[Skills(list_skills=_repeating_cursor_list_handler(), get_skill=_get_skill)] ) - async with Client(server) as client: + async with _skills(server) as skills: with pytest.raises(ValueError, match="repeated"): - await list_skills(client.session) + await skills.list_skills() async def test_list_skills_requires_the_extension_to_be_advertised() -> None: """No `Skills` extension at all: the server never advertises `io.modelcontextprotocol/skills`.""" - async with Client(MCPServer("plain")) as client: + async with _skills(MCPServer("plain")) as skills: with pytest.raises(ValueError, match="does not advertise"): - await list_skills(client.session) + await skills.list_skills() async def test_get_skill_returns_the_matching_entry() -> None: - async with Client(_server()) as client: - skill = await get_skill(client.session, _SKILL_URI) + async with _skills(_server()) as skills: + skill = await skills.get_skill(_SKILL_URI) assert skill.uri == _SKILL_URI async def test_get_skill_propagates_the_servers_unknown_skill_error() -> None: - async with Client(_server()) as client: + async with _skills(_server()) as skills: with pytest.raises(MCPError) as exc_info: - await get_skill(client.session, "skill://missing/SKILL.md") + await skills.get_skill("skill://missing/SKILL.md") assert exc_info.value.code == INVALID_PARAMS async def test_read_skill_uri_reads_the_registered_resource() -> None: - async with Client(_server()) as client: - result = await read_skill_uri(client.session, _SKILL_URI) + async with _skills(_server()) as skills: + result = await skills.read_skill_uri(_SKILL_URI) contents = result.contents[0] assert isinstance(contents, TextResourceContents) assert contents.text == _SKILL_CONTENT @@ -179,17 +190,17 @@ async def test_read_skill_uri_reads_the_registered_resource() -> None: async def test_read_skill_uri_content_verifies_against_the_held_skill() -> None: """End-to-end: `skills/get`'s digest and `resources/read`'s bytes agree.""" - async with Client(_server()) as client: - skill = await get_skill(client.session, _SKILL_URI) - result = await read_skill_uri(client.session, _SKILL_URI) + async with _skills(_server()) as skills: + skill = await skills.get_skill(_SKILL_URI) + result = await skills.read_skill_uri(_SKILL_URI) contents = result.contents[0] assert isinstance(contents, TextResourceContents) verify_skill_resource(skill, _SKILL_URI, contents.text.encode()) async def test_read_directory_follows_next_cursor_to_completion() -> None: - async with Client(_server(with_directory_read=True)) as client: - resources = await read_directory(client.session, "skill://git-workflow/references") + async with _skills(_server(with_directory_read=True)) as skills: + resources = await skills.read_directory("skill://git-workflow/references") assert [r.uri for r in resources] == [ "skill://git-workflow/references/A.md", "skill://git-workflow/references/B.md", @@ -198,9 +209,9 @@ async def test_read_directory_follows_next_cursor_to_completion() -> None: async def test_read_directory_requires_the_directory_read_setting() -> None: """The extension is advertised, but without `directoryRead: true`.""" - async with Client(_server(with_directory_read=False)) as client: + async with _skills(_server(with_directory_read=False)) as skills: with pytest.raises(ValueError, match="directoryRead"): - await read_directory(client.session, "skill://git-workflow/references") + await skills.read_directory("skill://git-workflow/references") async def test_read_directory_raises_on_a_server_that_repeats_its_cursor() -> None: @@ -214,16 +225,16 @@ async def test_read_directory_raises_on_a_server_that_repeats_its_cursor() -> No ) ], ) - async with Client(server) as client: + async with _skills(server) as skills: with pytest.raises(ValueError, match="repeated"): - await read_directory(client.session, "skill://git-workflow/references") + await skills.read_directory("skill://git-workflow/references") async def test_get_skill_rejects_a_mismatched_uri_from_a_non_conformant_server() -> None: server = MCPServer("catalog", extensions=[_NonConformantGetSkill()]) - async with Client(server) as client: + async with _skills(server) as skills: with pytest.raises(ValueError, match="returned skill"): - await get_skill(client.session, "skill://other/SKILL.md") + await skills.get_skill("skill://other/SKILL.md") async def test_get_skill_round_trips_a_dynamic_skill() -> None: @@ -241,8 +252,8 @@ async def get_dynamic(ctx: ServerRequestContext[Any, Any], params: GetSkillParam return GetSkillResult(skill=dynamic) server = MCPServer("catalog", extensions=[Skills(list_skills=_paginated_list_handler(), get_skill=get_dynamic)]) - async with Client(server) as client: - skill = await get_skill(client.session, dynamic.uri) + async with _skills(server) as skills: + skill = await skills.get_skill(dynamic.uri) assert skill.resources == "dynamic" assert skill.frontmatter["name"] == "generated" @@ -250,9 +261,9 @@ async def get_dynamic(ctx: ServerRequestContext[Any, Any], params: GetSkillParam async def test_list_skills_starts_from_a_caller_supplied_cursor() -> None: """A host resuming from a saved cursor: `list_skills` begins at that cursor rather than the top, so only the pages after it come back (here, page 1's skill is skipped).""" - async with Client(_server()) as client: - skills = await list_skills(client.session, ListSkillsParams(cursor="page-2")) - assert [s.uri for s in skills] == ["skill://other/SKILL.md"] + async with _skills(_server()) as skills: + result = await skills.list_skills(ListSkillsParams(cursor="page-2")) + assert [s.uri for s in result] == ["skill://other/SKILL.md"] async def test_list_skills_detects_a_server_repeating_the_caller_supplied_cursor() -> None: @@ -266,9 +277,9 @@ async def handler(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) return ListSkillsResult(skills=[_skill()], next_cursor="resume-here") server = MCPServer("catalog", extensions=[Skills(list_skills=handler, get_skill=_get_skill)]) - async with Client(server) as client: + async with _skills(server) as skills: with pytest.raises(ValueError, match="repeated"): - await list_skills(client.session, ListSkillsParams(cursor="resume-here")) + await skills.list_skills(ListSkillsParams(cursor="resume-here")) assert calls == 1 @@ -290,9 +301,9 @@ async def handler(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) ) server = MCPServer("catalog", extensions=[Skills(list_skills=handler, get_skill=_get_skill)]) - async with Client(server) as client: + async with _skills(server) as skills: params = ListSkillsParams.model_validate({"_meta": {"progressToken": "t"}}) - await list_skills(client.session, params) + await skills.list_skills(params) # `_meta` crosses the wire camelCase (`progressToken`), but the server deserializes it back # through the meta model, which exposes the known field snake_case as `progress_token`; the # transport also enriches `_meta` with its own keys. What matters is the caller's token diff --git a/tests/docs_src/test_skills.py b/tests/docs_src/test_skills.py index cc68175db1..d5eecd9e57 100644 --- a/tests/docs_src/test_skills.py +++ b/tests/docs_src/test_skills.py @@ -5,7 +5,7 @@ from docs_src.skills import tutorial001 from mcp import Client -from mcp.client.skills import get_skill, list_skills, read_skill_uri, verify_skill_resource +from mcp.client.skills import Skills, verify_skill_resource from mcp.shared.exceptions import MCPError pytestmark = pytest.mark.anyio @@ -13,23 +13,26 @@ async def test_list_skills_returns_the_registered_skill() -> None: """tutorial001: `list_skills` returns the one skill the server declared.""" - async with Client(tutorial001.mcp) as client: - skills = await list_skills(client.session) - assert [s.uri for s in skills] == [tutorial001.SKILL_URI] + skills = Skills() + async with Client(tutorial001.mcp, extensions=[skills]) as client: + result = await skills.bind(client).list_skills() + assert [s.uri for s in result] == [tutorial001.SKILL_URI] async def test_get_skill_answers_by_uri() -> None: """tutorial001: `get_skill` returns the same entry `list_skills` does.""" - async with Client(tutorial001.mcp) as client: - skill = await get_skill(client.session, tutorial001.SKILL_URI) + skills = Skills() + async with Client(tutorial001.mcp, extensions=[skills]) as client: + skill = await skills.bind(client).get_skill(tutorial001.SKILL_URI) assert skill.frontmatter["name"] == "git-workflow" async def test_get_skill_rejects_an_unknown_uri() -> None: """tutorial001: `get_skill` raises `-32602` (Invalid params) for an unknown skill.""" - async with Client(tutorial001.mcp) as client: + skills = Skills() + async with Client(tutorial001.mcp, extensions=[skills]) as client: with pytest.raises(MCPError) as exc_info: - await get_skill(client.session, "skill://unknown/SKILL.md") + await skills.bind(client).get_skill("skill://unknown/SKILL.md") assert exc_info.value.code == INVALID_PARAMS @@ -47,9 +50,11 @@ async def test_the_skill_file_is_served_as_an_ordinary_resource() -> None: async def test_read_skill_uri_content_verifies_against_the_held_skill() -> None: """tutorial001_client: fetch the entry, read the file, verify the bytes against it - the digest/size check `verify_skill_resource` performs.""" - async with Client(tutorial001.mcp) as client: - skill = await get_skill(client.session, tutorial001.SKILL_URI) - result = await read_skill_uri(client.session, skill.uri) + skills = Skills() + async with Client(tutorial001.mcp, extensions=[skills]) as client: + catalog = skills.bind(client) + skill = await catalog.get_skill(tutorial001.SKILL_URI) + result = await catalog.read_skill_uri(skill.uri) contents = result.contents[0] assert isinstance(contents, TextResourceContents) verify_skill_resource(skill, skill.uri, contents.text.encode()) From 1ddace726f59691a525a57f1409dd07772a5441c Mon Sep 17 00:00:00 2001 From: vijay Date: Wed, 23 Sep 2026 22:54:28 +0530 Subject: [PATCH 21/25] removed restriction to allow only 512 files and 16 mb for each skill, it's not mandatory as per recent update --- src/mcp/client/skills.py | 35 ++++++++++++++++++++++++++--------- src/mcp/shared/skills.py | 31 +++++++++++++++++++------------ tests/shared/test_skills.py | 25 +++++++++++++++++-------- 3 files changed, 62 insertions(+), 29 deletions(-) diff --git a/src/mcp/client/skills.py b/src/mcp/client/skills.py index 451ebd3cfa..947e618559 100644 --- a/src/mcp/client/skills.py +++ b/src/mcp/client/skills.py @@ -9,9 +9,12 @@ for skill in await skills.bind(client).list_skills(): print(skill.uri, skill.frontmatter["description"]) -`bind(client)` returns a `BoundSkills`. Each verb validates that the server -advertises the extension before sending; `list_skills` and `read_directory` -follow `nextCursor` to completion, so one call returns every page's results. +`bind(client)` returns a `BoundSkills`. Its catalog verbs — `list_skills`, +`get_skill`, and `read_directory` — check that the server advertises the +extension and validate its response; `list_skills` and `read_directory` follow +`nextCursor` to completion, so one call returns every page's results. +`read_skill_uri` is a thin `resources/read` alias that does neither — verify its +result with `verify_skill_resource`. """ from __future__ import annotations @@ -68,9 +71,13 @@ def bind(self, client: Client) -> BoundSkills: class BoundSkills: """The SEP-2640 verbs bound to one connected session. - Obtain it from `Skills.bind(client)`. `list_skills` and `read_directory` - follow `nextCursor` to completion; every method validates the server's - response against the SEP-2640 conformance rules before returning it. + Obtain it from `Skills.bind(client)`. The catalog verbs — `list_skills`, + `get_skill`, and `read_directory` — check that the server advertises the + extension and validate its response against the SEP-2640 conformance rules + before returning; `list_skills` and `read_directory` also follow + `nextCursor` to completion. `read_skill_uri` is the exception: a thin + `resources/read` alias that neither checks advertisement nor validates — + pair it with `verify_skill_resource`. """ def __init__(self, session: ClientSession) -> None: @@ -88,8 +95,9 @@ async def list_skills(self, params: ListSkillsParams | None = None) -> list[Skil """Call `skills/list`, following `nextCursor` to completion, and validate the result. Raises: - ValueError: If the server doesn't advertise the Skills extension, or - its response is not SEP-2640 conformant. + ValueError: If the server doesn't advertise the Skills extension, its + response is not SEP-2640 conformant, or it repeats a pagination cursor. + MCPError: If the server returns an error response. """ self._require_extension() base = params if params is not None else ListSkillsParams() @@ -118,6 +126,8 @@ async def get_skill(self, uri: str) -> Skill: Raises: ValueError: If the server doesn't advertise the Skills extension, its response names a different skill, or the skill is not conformant. + MCPError: If the server returns an error response, such as `-32602` + for a URI it does not serve. """ self._require_extension() result = await self._session.send_request(GetSkillRequest(params=GetSkillParams(uri=uri)), GetSkillResult) @@ -133,6 +143,11 @@ async def read_skill_uri(self, uri: str) -> ReadResourceResult: file regardless of whether the skill was ever enumerated. Verify the result against a held `Skill` entry with `verify_skill_resource` before treating it as trusted content — this call does not verify anything itself. + + Raises: + MCPError: If the server returns an error response. + RuntimeError: If the server returns an `InputRequiredResult`; this + alias does not drive the input-required loop. """ return await self._session.read_resource(uri) @@ -141,7 +156,9 @@ async def read_directory(self, uri: str, params: ReadDirectoryParams | None = No Raises: ValueError: If the server doesn't advertise the `directoryRead` - setting, or its response is not a valid child listing of `uri`. + setting, its response is not a valid child listing of `uri`, or + it repeats a pagination cursor. + MCPError: If the server returns an error response. """ self._require_extension(directory_read=True) base = params if params is not None else ReadDirectoryParams(uri=uri) diff --git a/src/mcp/shared/skills.py b/src/mcp/shared/skills.py index 9242f55bc0..680ea6cca1 100644 --- a/src/mcp/shared/skills.py +++ b/src/mcp/shared/skills.py @@ -24,10 +24,17 @@ METHOD_READ_DIRECTORY = "resources/directory/read" MAX_RESOURCES_PER_SKILL = 512 -"""SEP-2640 per-skill resource-count limit, `SKILL.md` included.""" +"""SEP-2640 per-skill resource-count threshold (`SKILL.md` included). + +A SHOULD NOT limit, not a hard cap: the spec requires a host to support skills +*up to and including* 512 entries and permits it to support larger ones, so +`validate_skill` does not reject an over-count manifest.""" MAX_TOTAL_SIZE = 16 * 1024 * 1024 -"""SEP-2640 per-skill total-byte-size limit (16 MiB), summed over `resources[].size`.""" +"""SEP-2640 per-skill total-byte-size threshold (16 MiB), summed over `resources[].size`. + +A SHOULD NOT limit, not a hard cap (see `MAX_RESOURCES_PER_SKILL`); an over-size +manifest is not rejected.""" _NAME_RE = re.compile(r"^[a-z0-9]+(?:-[a-z0-9]+)*$") _DIGEST_RE = re.compile(r"^sha256:[0-9a-f]{64}$") @@ -180,9 +187,11 @@ def validate_skill(skill: Skill) -> None: """Validate `skill` against the SEP-2640 and Agent Skills conformance rules. Checks the frontmatter's `name`/`description` fields, that `resources` (when - not `"dynamic"`) is complete and within the `MAX_RESOURCES_PER_SKILL`/ - `MAX_TOTAL_SIZE` limits, and that every resource entry names a file within - the skill's own directory with a well-formed digest. + not `"dynamic"`) is complete — every entry names a file within the skill's + own directory, has a well-formed digest, and `SKILL.md` is present. The + 512-entry/16-MiB limits are SEP-2640 SHOULD NOT thresholds, not MUST NOT, so + an over-limit manifest is accepted (a conforming host must support up to the + limits and may support larger). Raises: ValueError: If `skill` violates any of the above. @@ -200,10 +209,7 @@ def validate_skill(skill: Skill) -> None: if skill.resources == "dynamic": return resources = skill.resources - if len(resources) > MAX_RESOURCES_PER_SKILL: - raise ValueError(f"skill {skill.uri!r} has {len(resources)} resources, exceeding {MAX_RESOURCES_PER_SKILL}") seen: set[str] = set() - total_size = 0 for resource in resources: _validate_resource_uri_in_skill(skill.uri, resource.uri) if resource.uri in seen: @@ -213,11 +219,8 @@ def validate_skill(skill: Skill) -> None: raise ValueError(f"skill {skill.uri!r} resource {resource.uri!r} has an invalid SHA-256 digest") if resource.size < 0: raise ValueError(f"skill {skill.uri!r} resource {resource.uri!r} has a negative size") - total_size += resource.size if skill.uri not in seen: raise ValueError(f"skill {skill.uri!r} resources does not include its own SKILL.md") - if total_size > MAX_TOTAL_SIZE: - raise ValueError(f"skill {skill.uri!r} has {total_size} bytes, exceeding {MAX_TOTAL_SIZE}") def validate_list_result(result: ListSkillsResult) -> None: @@ -250,7 +253,11 @@ def parse_directory_uri(uri: str) -> tuple[str, str, str]: def validate_directory_result(uri: str, result: ReadDirectoryResult) -> None: - """Validate that `result.resources` are exactly the direct children of `uri`. + """Validate that each entry in `result.resources` is a unique direct child of `uri`. + + Checks containment and shape only — that every listed resource is a direct + child of `uri` with a unique `uri` and `name`. It cannot confirm the listing + is exhaustive, since it has no independent view of the directory's contents. Raises: ValueError: If `uri` is malformed, or any entry is not a direct child, diff --git a/tests/shared/test_skills.py b/tests/shared/test_skills.py index a15a4f12b9..e8fe25219f 100644 --- a/tests/shared/test_skills.py +++ b/tests/shared/test_skills.py @@ -249,25 +249,34 @@ def test_validate_skill_rejects_malformed_digest_formats(digest: str) -> None: validate_skill(skill) -def test_validate_skill_rejects_more_than_512_resources() -> None: - """SEP-2640 Limits: 512 entries per skill, `SKILL.md` included.""" +def test_validate_skill_accepts_exactly_512_resources() -> None: + """SEP-2640 Limits: a host MUST support skills up to and including 512 entries + (`SKILL.md` counted), so a 512-entry manifest validates.""" + root = "skill://git-workflow/SKILL.md" + resources = [_resource(root)] + [_resource(f"skill://git-workflow/f{i}.md") for i in range(511)] + skill = Skill(uri=root, frontmatter={"name": "git-workflow", "description": "d"}, resources=resources) + validate_skill(skill) + + +def test_validate_skill_accepts_more_than_512_resources() -> None: + """SEP-2640 Limits: 512 is a SHOULD NOT threshold, not a hard cap — a host MAY support + larger skills, so an over-count manifest is not rejected.""" root = "skill://git-workflow/SKILL.md" resources = [_resource(root)] + [_resource(f"skill://git-workflow/f{i}.md") for i in range(512)] skill = Skill(uri=root, frontmatter={"name": "git-workflow", "description": "d"}, resources=resources) - with pytest.raises(ValueError, match="exceeding 512"): - validate_skill(skill) + validate_skill(skill) -def test_validate_skill_rejects_total_size_over_16mib() -> None: - """SEP-2640 Limits: 16 MiB total per skill, summed over `resources[].size`.""" +def test_validate_skill_accepts_total_size_over_16mib() -> None: + """SEP-2640 Limits: 16 MiB total is a SHOULD NOT threshold, not a hard cap — an over-size + manifest is not rejected.""" root = "skill://git-workflow/SKILL.md" skill = Skill( uri=root, frontmatter={"name": "git-workflow", "description": "d"}, resources=[_resource(root, size=16 * 1024 * 1024 + 1)], ) - with pytest.raises(ValueError, match="exceeding"): - validate_skill(skill) + validate_skill(skill) def test_validate_skill_accepts_dynamic_resources_without_further_checks() -> None: From 488e7062dea590198057e49507d217fc491bc851 Mon Sep 17 00:00:00 2001 From: vijay Date: Wed, 23 Sep 2026 23:42:26 +0530 Subject: [PATCH 22/25] removed separate skill validations as per review suggestion --- src/mcp/client/skills.py | 8 +- src/mcp/server/skills.py | 23 ++--- src/mcp/shared/skills.py | 128 ++++++++++++------------ tests/shared/test_skills.py | 190 +++++++++++++++++------------------- 4 files changed, 169 insertions(+), 180 deletions(-) diff --git a/src/mcp/client/skills.py b/src/mcp/client/skills.py index 947e618559..0321e5fca5 100644 --- a/src/mcp/client/skills.py +++ b/src/mcp/client/skills.py @@ -38,8 +38,6 @@ ReadDirectoryResult, Skill, validate_directory_result, - validate_list_result, - validate_skill, ) from mcp.shared.skills import verify_skill_resource as verify_skill_resource @@ -105,10 +103,11 @@ async def list_skills(self, params: ListSkillsParams | None = None) -> list[Skil skills: list[Skill] = [] seen_cursors: set[str] = {cursor} if cursor is not None else set() while True: + # `send_request` parses each page into `ListSkillsResult`, whose validators reject a + # non-conformant skill or a duplicate URI — no separate conformance call is needed. page = await self._session.send_request( ListSkillsRequest(params=base.model_copy(update={"cursor": cursor})), ListSkillsResult ) - validate_list_result(page) skills.extend(page.skills) if page.next_cursor is None: return skills @@ -130,10 +129,11 @@ async def get_skill(self, uri: str) -> Skill: for a URI it does not serve. """ self._require_extension() + # Parsing `GetSkillResult` validates the skill's own conformance; the requested-URI match + # is the one rule the skill body can't carry, so it stays an explicit check here. result = await self._session.send_request(GetSkillRequest(params=GetSkillParams(uri=uri)), GetSkillResult) if result.skill.uri != uri: raise ValueError(f"server returned skill {result.skill.uri!r} for requested {uri!r}") - validate_skill(result.skill) return result.skill async def read_skill_uri(self, uri: str) -> ReadResourceResult: diff --git a/src/mcp/server/skills.py b/src/mcp/server/skills.py index 0b94edbe63..a15fd51d76 100644 --- a/src/mcp/server/skills.py +++ b/src/mcp/server/skills.py @@ -35,6 +35,7 @@ async def get_skill(ctx, params): from mcp_types import CacheableResult from mcp_types.jsonrpc import INTERNAL_ERROR, INVALID_PARAMS from mcp_types.version import MODERN_PROTOCOL_VERSIONS +from pydantic import ValidationError from mcp.server.context import HandlerResult, ServerRequestContext from mcp.server.extension import Extension, MethodBinding @@ -53,8 +54,6 @@ async def get_skill(ctx, params): parse_directory_uri, skill_name_from_uri, validate_directory_result, - validate_list_result, - validate_skill, ) __all__ = ["Skills"] @@ -103,25 +102,27 @@ def methods(self) -> Sequence[MethodBinding]: return bindings async def _handle_list(self, ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> HandlerResult: - result = await self._list_skills(ctx, params) + # `ListSkillsResult`/`Skill` self-validate on construction, so a handler that builds a + # non-conformant listing raises `ValidationError` here — a server fault, surfaced as an + # Internal error rather than the framework's default Invalid params for a bad body. try: - validate_list_result(result) - except ValueError: + result = await self._list_skills(ctx, params) + except ValidationError: logger.exception("list_skills handler returned an invalid result") raise MCPError(code=INTERNAL_ERROR, message="Handler returned an invalid result") from None return _finalize_cacheable(result, ctx.protocol_version) async def _handle_get(self, ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> HandlerResult: _require_skill_md_uri(params.uri) - result = await self._get_skill(ctx, params) - if result.skill.uri != params.uri: - logger.error("get_skill handler returned %r for requested %r", result.skill.uri, params.uri) - raise MCPError(code=INTERNAL_ERROR, message="Handler returned an invalid result") + # `Skill` self-validates on construction (see `_handle_list`). try: - validate_skill(result.skill) - except ValueError: + result = await self._get_skill(ctx, params) + except ValidationError: logger.exception("get_skill handler returned an invalid result") raise MCPError(code=INTERNAL_ERROR, message="Handler returned an invalid result") from None + if result.skill.uri != params.uri: + logger.error("get_skill handler returned %r for requested %r", result.skill.uri, params.uri) + raise MCPError(code=INTERNAL_ERROR, message="Handler returned an invalid result") return _finalize_cacheable(result, ctx.protocol_version) async def _handle_read_directory( diff --git a/src/mcp/shared/skills.py b/src/mcp/shared/skills.py index 680ea6cca1..614bdf05aa 100644 --- a/src/mcp/shared/skills.py +++ b/src/mcp/shared/skills.py @@ -9,11 +9,11 @@ import hashlib import re -from typing import Any, Literal +from typing import Annotated, Any, Literal from urllib.parse import urlsplit from mcp_types import CacheableResult, PaginatedRequestParams, PaginatedResult, Request, RequestParams, Resource -from pydantic import BaseModel, ConfigDict +from pydantic import AfterValidator, BaseModel, ConfigDict, Field, model_validator from pydantic.alias_generators import to_camel EXTENSION_ID = "io.modelcontextprotocol/skills" @@ -27,8 +27,8 @@ """SEP-2640 per-skill resource-count threshold (`SKILL.md` included). A SHOULD NOT limit, not a hard cap: the spec requires a host to support skills -*up to and including* 512 entries and permits it to support larger ones, so -`validate_skill` does not reject an over-count manifest.""" +*up to and including* 512 entries and permits it to support larger ones, so a +`Skill` does not reject an over-count manifest.""" MAX_TOTAL_SIZE = 16 * 1024 * 1024 """SEP-2640 per-skill total-byte-size threshold (16 MiB), summed over `resources[].size`. @@ -40,6 +40,12 @@ _DIGEST_RE = re.compile(r"^sha256:[0-9a-f]{64}$") +def _check_digest(value: str) -> str: + if not _DIGEST_RE.fullmatch(value): + raise ValueError(f"invalid SHA-256 digest {value!r}; expected 'sha256:' + 64 lowercase hex characters") + return value + + class _SkillModel(BaseModel): """Base for Skills value types: matches `mcp_types`' internal `MCPModel` config. @@ -51,12 +57,16 @@ class _SkillModel(BaseModel): class SkillResource(_SkillModel): - """One file in a skill's manifest: `{uri, digest, size}`.""" + """One file in a skill's manifest: `{uri, digest, size}`. + + Shape rules are intrinsic: a `digest` that isn't `sha256:` + 64 lowercase + hex characters, or a negative `size`, is rejected at construction. + """ uri: str - digest: str + digest: Annotated[str, AfterValidator(_check_digest)] """SHA-256 digest of the file's raw bytes, formatted `sha256:{64 hex chars}`.""" - size: int + size: Annotated[int, Field(ge=0)] """Length in bytes of the file's raw content.""" @@ -68,13 +78,49 @@ class SkillResource(_SkillModel): class Skill(_SkillModel): - """An entry returned by `skills/list` or `skills/get`.""" + """An entry returned by `skills/list` or `skills/get`. + + SEP-2640 conformance is intrinsic: constructing (or parsing) a `Skill` + validates the frontmatter `name`/`description`, and — unless `resources` is + `"dynamic"` — that every entry names a file within the skill's own directory, + with no duplicates and `SKILL.md` present. The 512-entry/16-MiB limits are + SHOULD NOT thresholds, not MUST NOT, so an over-limit manifest is accepted. + """ uri: str """Resource URI of the skill's `SKILL.md`.""" frontmatter: Frontmatter resources: SkillResources + @model_validator(mode="after") + def _check_conformance(self) -> Skill: + name = skill_name_from_uri(self.uri) + frontmatter_name = self.frontmatter.get("name") + if ( + not isinstance(frontmatter_name, str) + or not _NAME_RE.fullmatch(frontmatter_name) + or len(frontmatter_name) > 64 + ): + raise ValueError(f"skill {self.uri!r} frontmatter name must be 1-64 lowercase, digits, or hyphens") + if frontmatter_name != name: + raise ValueError( + f"skill {self.uri!r} frontmatter name {frontmatter_name!r} does not match URI name {name!r}" + ) + description = self.frontmatter.get("description") + if not isinstance(description, str) or not (1 <= len(description) <= 1024): + raise ValueError(f"skill {self.uri!r} frontmatter description must contain 1 to 1024 characters") + if self.resources == "dynamic": + return self + seen: set[str] = set() + for resource in self.resources: + _validate_resource_uri_in_skill(self.uri, resource.uri) + if resource.uri in seen: + raise ValueError(f"skill {self.uri!r} lists resource {resource.uri!r} more than once") + seen.add(resource.uri) + if self.uri not in seen: + raise ValueError(f"skill {self.uri!r} resources does not include its own SKILL.md") + return self + class ListSkillsParams(PaginatedRequestParams): """Parameters for `skills/list`.""" @@ -83,6 +129,9 @@ class ListSkillsParams(PaginatedRequestParams): class ListSkillsResult(PaginatedResult, CacheableResult): """Result of `skills/list`. + Each skill self-validates; on top of that, constructing (or parsing) this + result rejects two entries that share a `uri`. + `ttl_ms`/`cache_scope` are SEP-2549 fields inherited from `CacheableResult`; unlike a core spec method, nothing sieves them off the wire for a pre-2026-07-28 connection automatically (see `mcp.server.skills`), so @@ -91,6 +140,15 @@ class ListSkillsResult(PaginatedResult, CacheableResult): skills: list[Skill] + @model_validator(mode="after") + def _check_unique_uris(self) -> ListSkillsResult: + seen: set[str] = set() + for skill in self.skills: + if skill.uri in seen: + raise ValueError(f"skills/list result lists skill {skill.uri!r} more than once") + seen.add(skill.uri) + return self + class GetSkillParams(RequestParams): """Parameters for `skills/get`.""" @@ -183,60 +241,6 @@ def _validate_resource_uri_in_skill(skill_uri: str, resource_uri: str) -> None: raise ValueError(f"resource URI {resource_uri!r} contains a traversal segment") -def validate_skill(skill: Skill) -> None: - """Validate `skill` against the SEP-2640 and Agent Skills conformance rules. - - Checks the frontmatter's `name`/`description` fields, that `resources` (when - not `"dynamic"`) is complete — every entry names a file within the skill's - own directory, has a well-formed digest, and `SKILL.md` is present. The - 512-entry/16-MiB limits are SEP-2640 SHOULD NOT thresholds, not MUST NOT, so - an over-limit manifest is accepted (a conforming host must support up to the - limits and may support larger). - - Raises: - ValueError: If `skill` violates any of the above. - """ - name = skill_name_from_uri(skill.uri) - frontmatter_name = skill.frontmatter.get("name") - if not isinstance(frontmatter_name, str) or not _NAME_RE.fullmatch(frontmatter_name) or len(frontmatter_name) > 64: - raise ValueError(f"skill {skill.uri!r} frontmatter name must be 1-64 lowercase, digits, or hyphens") - if frontmatter_name != name: - raise ValueError(f"skill {skill.uri!r} frontmatter name {frontmatter_name!r} does not match URI name {name!r}") - description = skill.frontmatter.get("description") - if not isinstance(description, str) or not (1 <= len(description) <= 1024): - raise ValueError(f"skill {skill.uri!r} frontmatter description must contain 1 to 1024 characters") - - if skill.resources == "dynamic": - return - resources = skill.resources - seen: set[str] = set() - for resource in resources: - _validate_resource_uri_in_skill(skill.uri, resource.uri) - if resource.uri in seen: - raise ValueError(f"skill {skill.uri!r} lists resource {resource.uri!r} more than once") - seen.add(resource.uri) - if not _DIGEST_RE.fullmatch(resource.digest): - raise ValueError(f"skill {skill.uri!r} resource {resource.uri!r} has an invalid SHA-256 digest") - if resource.size < 0: - raise ValueError(f"skill {skill.uri!r} resource {resource.uri!r} has a negative size") - if skill.uri not in seen: - raise ValueError(f"skill {skill.uri!r} resources does not include its own SKILL.md") - - -def validate_list_result(result: ListSkillsResult) -> None: - """Validate every skill in `result.skills` and reject duplicate URIs. - - Raises: - ValueError: If any skill is invalid, or two entries share a `uri`. - """ - seen: set[str] = set() - for skill in result.skills: - validate_skill(skill) - if skill.uri in seen: - raise ValueError(f"skills/list result lists skill {skill.uri!r} more than once") - seen.add(skill.uri) - - def parse_directory_uri(uri: str) -> tuple[str, str, str]: """Split a directory resource URI into `(scheme, netloc, path)`. diff --git a/tests/shared/test_skills.py b/tests/shared/test_skills.py index e8fe25219f..b564de032b 100644 --- a/tests/shared/test_skills.py +++ b/tests/shared/test_skills.py @@ -9,6 +9,7 @@ import pytest from mcp_types import Resource +from pydantic import ValidationError from mcp.shared.skills import ( ListSkillsParams, @@ -20,8 +21,6 @@ parse_directory_uri, skill_name_from_uri, validate_directory_result, - validate_list_result, - validate_skill, verify_skill_resource, ) @@ -95,18 +94,18 @@ def test_skill_with_dynamic_resources_round_trips_through_json_as_the_literal_st assert restored.resources == "dynamic" -def test_validate_skill_accepts_a_conformant_skill() -> None: - validate_skill(_skill()) +def test_skill_accepts_a_conformant_manifest() -> None: + skill = _skill() + assert isinstance(skill.resources, list) -def test_validate_skill_rejects_a_non_string_frontmatter_name() -> None: - skill = Skill( - uri="skill://git-workflow/SKILL.md", - frontmatter={"name": 1, "description": "d"}, - resources=[_resource("skill://git-workflow/SKILL.md")], - ) - with pytest.raises(ValueError, match="frontmatter name"): - validate_skill(skill) +def test_skill_rejects_a_non_string_frontmatter_name() -> None: + with pytest.raises(ValidationError, match="frontmatter name"): + Skill( + uri="skill://git-workflow/SKILL.md", + frontmatter={"name": 1, "description": "d"}, + resources=[_resource("skill://git-workflow/SKILL.md")], + ) @pytest.mark.parametrize( @@ -120,112 +119,98 @@ def test_validate_skill_rejects_a_non_string_frontmatter_name() -> None: "a" * 65, # exceeds the 64-char limit ], ) -def test_validate_skill_rejects_names_violating_the_agent_skills_grammar(name: str) -> None: +def test_skill_rejects_names_violating_the_agent_skills_grammar(name: str) -> None: """SEP-2640 defers naming to the Agent Skills spec: 1-64 chars, lowercase alphanumeric and hyphens, no leading/trailing/consecutive hyphens. A URI whose final path segment carries the bad name (so `frontmatter.name` can match it) still fails the name-grammar check first.""" uri = f"skill://acme/{name}/SKILL.md" - skill = Skill( - uri=uri, - frontmatter={"name": name, "description": "d"}, - resources=[SkillResource(uri=uri, digest=_DIGEST, size=4)], - ) - with pytest.raises(ValueError, match="frontmatter name"): - validate_skill(skill) + with pytest.raises(ValidationError, match="frontmatter name"): + Skill( + uri=uri, + frontmatter={"name": name, "description": "d"}, + resources=[SkillResource(uri=uri, digest=_DIGEST, size=4)], + ) -def test_validate_skill_rejects_an_invalid_resource_uri() -> None: +def test_skill_rejects_an_invalid_resource_uri() -> None: """A resource URI with a query component fails the same shape check as a skill URI.""" - skill = _skill(extra_resources=[_resource("skill://git-workflow/x.md?y=1")]) - with pytest.raises(ValueError, match="is invalid"): - validate_skill(skill) + with pytest.raises(ValidationError, match="is invalid"): + _skill(extra_resources=[_resource("skill://git-workflow/x.md?y=1")]) -def test_validate_skill_rejects_a_resource_with_the_same_authority_but_a_sibling_path() -> None: +def test_skill_rejects_a_resource_with_the_same_authority_but_a_sibling_path() -> None: """Same scheme+authority as the skill (so the URI passes the authority check) but a path outside the skill's own directory, per SEP-2640 Resources ('a file within the skill's directory').""" root = "skill://acme/billing/refunds/SKILL.md" - skill = Skill( - uri=root, - frontmatter={"name": "refunds", "description": "d"}, - resources=[_resource(root), _resource("skill://acme/billing/other/x.md")], - ) - with pytest.raises(ValueError, match="outside the skill root"): - validate_skill(skill) + with pytest.raises(ValidationError, match="outside the skill root"): + Skill( + uri=root, + frontmatter={"name": "refunds", "description": "d"}, + resources=[_resource(root), _resource("skill://acme/billing/other/x.md")], + ) -def test_validate_skill_rejects_a_traversal_segment_in_a_resource_uri() -> None: - skill = _skill(extra_resources=[_resource("skill://git-workflow/../evil.md")]) - with pytest.raises(ValueError, match="traversal segment"): - validate_skill(skill) +def test_skill_rejects_a_traversal_segment_in_a_resource_uri() -> None: + with pytest.raises(ValidationError, match="traversal segment"): + _skill(extra_resources=[_resource("skill://git-workflow/../evil.md")]) -def test_validate_skill_rejects_a_resource_uri_with_a_trailing_slash() -> None: +def test_skill_rejects_a_resource_uri_with_a_trailing_slash() -> None: """A manifest entry names a file, not a directory: a trailing slash is rejected.""" - skill = _skill(extra_resources=[_resource("skill://git-workflow/references/")]) - with pytest.raises(ValueError, match="names a directory"): - validate_skill(skill) + with pytest.raises(ValidationError, match="names a directory"): + _skill(extra_resources=[_resource("skill://git-workflow/references/")]) -def test_validate_skill_rejects_a_negative_resource_size() -> None: - root = "skill://git-workflow/SKILL.md" - skill = Skill( - uri=root, - frontmatter={"name": "git-workflow", "description": "d"}, - resources=[SkillResource(uri=root, digest=_DIGEST, size=-1)], - ) - with pytest.raises(ValueError, match="negative size"): - validate_skill(skill) +def test_skill_resource_rejects_a_negative_size() -> None: + with pytest.raises(ValidationError, match="greater than or equal to 0"): + SkillResource(uri="skill://git-workflow/SKILL.md", digest=_DIGEST, size=-1) -def test_validate_skill_rejects_frontmatter_name_uri_mismatch() -> None: +def test_skill_rejects_frontmatter_name_uri_mismatch() -> None: """SEP-2640 Resource Mapping: `frontmatter.name` MUST equal the URI-derived name.""" - skill = Skill( - uri="skill://git-workflow/SKILL.md", - frontmatter={"name": "other-name", "description": "d"}, - resources=[_resource("skill://git-workflow/SKILL.md")], - ) - with pytest.raises(ValueError, match="does not match URI name"): - validate_skill(skill) + with pytest.raises(ValidationError, match="does not match URI name"): + Skill( + uri="skill://git-workflow/SKILL.md", + frontmatter={"name": "other-name", "description": "d"}, + resources=[_resource("skill://git-workflow/SKILL.md")], + ) @pytest.mark.parametrize("description", ["", "x" * 1025]) -def test_validate_skill_rejects_out_of_range_description_length(description: str) -> None: - skill = Skill( - uri="skill://git-workflow/SKILL.md", - frontmatter={"name": "git-workflow", "description": description}, - resources=[_resource("skill://git-workflow/SKILL.md")], - ) - with pytest.raises(ValueError, match="description"): - validate_skill(skill) +def test_skill_rejects_out_of_range_description_length(description: str) -> None: + with pytest.raises(ValidationError, match="description"): + Skill( + uri="skill://git-workflow/SKILL.md", + frontmatter={"name": "git-workflow", "description": description}, + resources=[_resource("skill://git-workflow/SKILL.md")], + ) -def test_validate_skill_rejects_a_resource_outside_the_skill_root() -> None: +def test_skill_rejects_a_resource_outside_the_skill_root() -> None: """SEP-2640 Resources: every entry MUST name a file within the skill's own directory.""" - skill = _skill(extra_resources=[_resource("skill://other-skill/file.md")]) - with pytest.raises(ValueError, match="outside the skill root"): - validate_skill(skill) + with pytest.raises(ValidationError, match="outside the skill root"): + _skill(extra_resources=[_resource("skill://other-skill/file.md")]) -def test_validate_skill_rejects_missing_skill_md_entry() -> None: +def test_skill_rejects_missing_skill_md_entry() -> None: """SEP-2640 Resources: `resources` MUST include an entry for the skill's own `SKILL.md`.""" - skill = Skill( - uri="skill://git-workflow/SKILL.md", - frontmatter={"name": "git-workflow", "description": "d"}, - resources=[_resource("skill://git-workflow/references/GUIDE.md")], - ) - with pytest.raises(ValueError, match="does not include its own SKILL.md"): - validate_skill(skill) + with pytest.raises(ValidationError, match="does not include its own SKILL.md"): + Skill( + uri="skill://git-workflow/SKILL.md", + frontmatter={"name": "git-workflow", "description": "d"}, + resources=[_resource("skill://git-workflow/references/GUIDE.md")], + ) -def test_validate_skill_rejects_duplicate_resource_uris() -> None: +def test_skill_rejects_duplicate_resource_uris() -> None: root = "skill://git-workflow/SKILL.md" - skill = Skill( - uri=root, frontmatter={"name": "git-workflow", "description": "d"}, resources=[_resource(root), _resource(root)] - ) - with pytest.raises(ValueError, match="more than once"): - validate_skill(skill) + with pytest.raises(ValidationError, match="more than once"): + Skill( + uri=root, + frontmatter={"name": "git-workflow", "description": "d"}, + resources=[_resource(root), _resource(root)], + ) @pytest.mark.parametrize( @@ -239,35 +224,34 @@ def test_validate_skill_rejects_duplicate_resource_uris() -> None: "sha256:" + "g" * 64, # non-hex characters ], ) -def test_validate_skill_rejects_malformed_digest_formats(digest: str) -> None: +def test_skill_resource_rejects_malformed_digest_formats(digest: str) -> None: """SEP-2640 Integrity and verification: `sha256:{hex}` where `{hex}` is exactly 64 lowercase hexadecimal characters - each of these near-misses must still be rejected.""" - root = "skill://git-workflow/SKILL.md" - bad = SkillResource(uri=root, digest=digest, size=1) - skill = Skill(uri=root, frontmatter={"name": "git-workflow", "description": "d"}, resources=[bad]) - with pytest.raises(ValueError, match="digest"): - validate_skill(skill) + with pytest.raises(ValidationError, match="digest"): + SkillResource(uri="skill://git-workflow/SKILL.md", digest=digest, size=1) -def test_validate_skill_accepts_exactly_512_resources() -> None: +def test_skill_accepts_exactly_512_resources() -> None: """SEP-2640 Limits: a host MUST support skills up to and including 512 entries (`SKILL.md` counted), so a 512-entry manifest validates.""" root = "skill://git-workflow/SKILL.md" resources = [_resource(root)] + [_resource(f"skill://git-workflow/f{i}.md") for i in range(511)] skill = Skill(uri=root, frontmatter={"name": "git-workflow", "description": "d"}, resources=resources) - validate_skill(skill) + assert isinstance(skill.resources, list) + assert len(skill.resources) == 512 -def test_validate_skill_accepts_more_than_512_resources() -> None: +def test_skill_accepts_more_than_512_resources() -> None: """SEP-2640 Limits: 512 is a SHOULD NOT threshold, not a hard cap — a host MAY support larger skills, so an over-count manifest is not rejected.""" root = "skill://git-workflow/SKILL.md" resources = [_resource(root)] + [_resource(f"skill://git-workflow/f{i}.md") for i in range(512)] skill = Skill(uri=root, frontmatter={"name": "git-workflow", "description": "d"}, resources=resources) - validate_skill(skill) + assert isinstance(skill.resources, list) + assert len(skill.resources) == 513 -def test_validate_skill_accepts_total_size_over_16mib() -> None: +def test_skill_accepts_total_size_over_16mib() -> None: """SEP-2640 Limits: 16 MiB total is a SHOULD NOT threshold, not a hard cap — an over-size manifest is not rejected.""" root = "skill://git-workflow/SKILL.md" @@ -276,26 +260,26 @@ def test_validate_skill_accepts_total_size_over_16mib() -> None: frontmatter={"name": "git-workflow", "description": "d"}, resources=[_resource(root, size=16 * 1024 * 1024 + 1)], ) - validate_skill(skill) + assert isinstance(skill.resources, list) + assert skill.resources[0].size == 16 * 1024 * 1024 + 1 -def test_validate_skill_accepts_dynamic_resources_without_further_checks() -> None: +def test_skill_accepts_dynamic_resources_without_further_checks() -> None: """SEP-2640 Resources: `"dynamic"` offers no manifest to check against limits.""" skill = Skill( uri="skill://generated/SKILL.md", frontmatter={"name": "generated", "description": "d"}, resources="dynamic" ) - validate_skill(skill) + assert skill.resources == "dynamic" -def test_validate_list_result_rejects_duplicate_skill_uris_across_entries() -> None: - result = ListSkillsResult(skills=[_skill(), _skill()]) - with pytest.raises(ValueError, match="more than once"): - validate_list_result(result) +def test_list_result_rejects_duplicate_skill_uris_across_entries() -> None: + with pytest.raises(ValidationError, match="more than once"): + ListSkillsResult(skills=[_skill(), _skill()]) -def test_validate_list_result_accepts_an_empty_listing() -> None: +def test_list_result_accepts_an_empty_listing() -> None: """SEP-2640 Enumeration: `skills/list` MAY return an empty result.""" - validate_list_result(ListSkillsResult(skills=[])) + assert ListSkillsResult(skills=[]).skills == [] @pytest.mark.parametrize( From 7ca165a95bc68cc5a62613f2fdc2e8ffecb832c4 Mon Sep 17 00:00:00 2001 From: vijay Date: Thu, 24 Sep 2026 05:52:42 +0530 Subject: [PATCH 23/25] re validate the outbound payload for mutation --- src/mcp/server/skills.py | 9 +++++++-- tests/server/test_skills.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/mcp/server/skills.py b/src/mcp/server/skills.py index a15fd51d76..b4971528ca 100644 --- a/src/mcp/server/skills.py +++ b/src/mcp/server/skills.py @@ -104,9 +104,12 @@ def methods(self) -> Sequence[MethodBinding]: async def _handle_list(self, ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> HandlerResult: # `ListSkillsResult`/`Skill` self-validate on construction, so a handler that builds a # non-conformant listing raises `ValidationError` here — a server fault, surfaced as an - # Internal error rather than the framework's default Invalid params for a bad body. + # Internal error rather than the framework's default Invalid params for a bad body. The + # models stay mutable, so re-validate the outbound payload too: a handler that mutates a + # skill after building the result can't ship a non-conformant listing past this point. try: result = await self._list_skills(ctx, params) + ListSkillsResult.model_validate(result.model_dump()) except ValidationError: logger.exception("list_skills handler returned an invalid result") raise MCPError(code=INTERNAL_ERROR, message="Handler returned an invalid result") from None @@ -114,9 +117,11 @@ async def _handle_list(self, ctx: ServerRequestContext[Any, Any], params: ListSk async def _handle_get(self, ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> HandlerResult: _require_skill_md_uri(params.uri) - # `Skill` self-validates on construction (see `_handle_list`). + # `Skill` self-validates on construction, and the re-validation guards post-construction + # mutation, both as in `_handle_list`. try: result = await self._get_skill(ctx, params) + GetSkillResult.model_validate(result.model_dump()) except ValidationError: logger.exception("get_skill handler returned an invalid result") raise MCPError(code=INTERNAL_ERROR, message="Handler returned an invalid result") from None diff --git a/tests/server/test_skills.py b/tests/server/test_skills.py index 3f04e22217..f1a63d42b3 100644 --- a/tests/server/test_skills.py +++ b/tests/server/test_skills.py @@ -150,6 +150,23 @@ async def bad_list(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams assert exc_info.value.code == INTERNAL_ERROR +async def test_skills_list_rejects_a_skill_mutated_after_the_result_is_built() -> None: + """SDK-defined: the value models validate on construction but stay mutable. A handler that + builds a valid result and then mutates a skill in place must not slip a non-conformant + listing past the server — the outbound payload is re-validated, so this is an Internal error.""" + + async def mutating_list(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> ListSkillsResult: + result = ListSkillsResult(skills=[_git_workflow_skill()]) + result.skills[0].frontmatter["name"] = "not the uri name" + return result + + server = MCPServer("catalog", extensions=[Skills(list_skills=mutating_list, get_skill=_get_skill)]) + async with Client(server) as client: + with pytest.raises(MCPError) as exc_info: + await client.session.send_request(ListSkillsRequest(), ListSkillsResult) + assert exc_info.value.code == INTERNAL_ERROR + + async def test_skills_get_returns_the_matching_skill() -> None: async with Client(_server()) as client: result = await client.session.send_request( @@ -247,6 +264,23 @@ async def bad_skill(ctx: ServerRequestContext[Any, Any], params: GetSkillParams) assert exc_info.value.code == INTERNAL_ERROR +async def test_skills_get_rejects_a_skill_mutated_after_the_result_is_built() -> None: + """SDK-defined: as on `skills/list`, a handler that mutates the skill in place after building + the result can't ship a non-conformant body — the re-validated outbound payload fails it as an + Internal error.""" + + async def mutating_get(ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> GetSkillResult: + result = GetSkillResult(skill=_git_workflow_skill()) + result.skill.resources.clear() + return result + + server = MCPServer("catalog", extensions=[Skills(list_skills=_list_skills, get_skill=mutating_get)]) + async with Client(server) as client: + with pytest.raises(MCPError) as exc_info: + await client.session.send_request(GetSkillRequest(params=GetSkillParams(uri=_SKILL_URI)), GetSkillResult) + assert exc_info.value.code == INTERNAL_ERROR + + async def test_missing_uri_param_is_rejected_before_the_handler_runs() -> None: """SDK-defined: `uri` is a required field on `GetSkillParams`, so an omitted param is rejected by params validation - the handler never sees it.""" From 662ce9346821c66e116b2dc4e3a834f800797115 Mon Sep 17 00:00:00 2001 From: vijay Date: Thu, 24 Sep 2026 06:41:41 +0530 Subject: [PATCH 24/25] modified the docs and added check for de-dup across pages --- docs/advanced/skills.md | 7 +++++- src/mcp/client/skills.py | 29 +++++++++++++++++++------ src/mcp/server/skills.py | 6 ++++++ src/mcp/shared/skills.py | 43 ++++++++++++++++++++++++++++++------- tests/client/test_skills.py | 33 ++++++++++++++++++++++++++++ tests/server/test_skills.py | 1 + 6 files changed, 103 insertions(+), 16 deletions(-) diff --git a/docs/advanced/skills.md b/docs/advanced/skills.md index 62fbc2b986..dfaab9e2f8 100644 --- a/docs/advanced/skills.md +++ b/docs/advanced/skills.md @@ -84,7 +84,12 @@ a `ReadResourceResult` (text or blob contents) and validates nothing itself — never grant the frontmatter's `allowed-tools` field (or any other permission-widening field) without explicit per-skill user approval. - Both are host responsibilities the SDK cannot discharge for you — read the SEP's + A digest match confirms the *bytes*, not the *frontmatter*: it doesn't prove the + `frontmatter` the server advertised in `skills/get` matches the frontmatter inside the fetched + `SKILL.md`. If you act on `skill.frontmatter` — especially `allowed-tools` — parse the fetched + file and compare its frontmatter yourself. + + These are host responsibilities the SDK cannot discharge for you — read the SEP's [Security Implications](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2640) section before building a host on top of this extension. diff --git a/src/mcp/client/skills.py b/src/mcp/client/skills.py index 0321e5fca5..c6f646f4b8 100644 --- a/src/mcp/client/skills.py +++ b/src/mcp/client/skills.py @@ -94,7 +94,8 @@ async def list_skills(self, params: ListSkillsParams | None = None) -> list[Skil Raises: ValueError: If the server doesn't advertise the Skills extension, its - response is not SEP-2640 conformant, or it repeats a pagination cursor. + response is not SEP-2640 conformant, it repeats a pagination + cursor, or it lists the same skill URI on two pages. MCPError: If the server returns an error response. """ self._require_extension() @@ -102,17 +103,24 @@ async def list_skills(self, params: ListSkillsParams | None = None) -> list[Skil cursor = base.cursor skills: list[Skill] = [] seen_cursors: set[str] = {cursor} if cursor is not None else set() + seen_uris: set[str] = set() while True: # `send_request` parses each page into `ListSkillsResult`, whose validators reject a # non-conformant skill or a duplicate URI — no separate conformance call is needed. page = await self._session.send_request( ListSkillsRequest(params=base.model_copy(update={"cursor": cursor})), ListSkillsResult ) + # A server stuck repeating its cursor is a loop; catch that before the content checks. + if page.next_cursor is not None and page.next_cursor in seen_cursors: + raise ValueError(f"server repeated skills/list pagination cursor {page.next_cursor!r}") + # Per-page validation can't catch a URI repeated *across* pages, so track that here. + for skill in page.skills: + if skill.uri in seen_uris: + raise ValueError(f"server listed skill {skill.uri!r} on more than one skills/list page") + seen_uris.add(skill.uri) skills.extend(page.skills) if page.next_cursor is None: return skills - if page.next_cursor in seen_cursors: - raise ValueError(f"server repeated skills/list pagination cursor {page.next_cursor!r}") seen_cursors.add(page.next_cursor) cursor = page.next_cursor @@ -156,8 +164,8 @@ async def read_directory(self, uri: str, params: ReadDirectoryParams | None = No Raises: ValueError: If the server doesn't advertise the `directoryRead` - setting, its response is not a valid child listing of `uri`, or - it repeats a pagination cursor. + setting, its response is not a valid child listing of `uri`, it + repeats a pagination cursor, or it lists the same child on two pages. MCPError: If the server returns an error response. """ self._require_extension(directory_read=True) @@ -165,16 +173,23 @@ async def read_directory(self, uri: str, params: ReadDirectoryParams | None = No cursor = base.cursor resources: list[Resource] = [] seen_cursors: set[str] = {cursor} if cursor is not None else set() + seen_uris: set[str] = set() while True: page = await self._session.send_request( ReadDirectoryRequest(params=base.model_copy(update={"uri": uri, "cursor": cursor})), ReadDirectoryResult, ) + # A server stuck repeating its cursor is a loop; catch that before the content checks. + if page.next_cursor is not None and page.next_cursor in seen_cursors: + raise ValueError(f"server repeated resources/directory/read pagination cursor {page.next_cursor!r}") validate_directory_result(uri, page) + # `validate_directory_result` dedupes within a page; catch a child repeated across pages. + for resource in page.resources: + if resource.uri in seen_uris: + raise ValueError(f"server listed child {resource.uri!r} on more than one directory page") + seen_uris.add(resource.uri) resources.extend(page.resources) if page.next_cursor is None: return resources - if page.next_cursor in seen_cursors: - raise ValueError(f"server repeated resources/directory/read pagination cursor {page.next_cursor!r}") seen_cursors.add(page.next_cursor) cursor = page.next_cursor diff --git a/src/mcp/server/skills.py b/src/mcp/server/skills.py index b4971528ca..745af4cbd1 100644 --- a/src/mcp/server/skills.py +++ b/src/mcp/server/skills.py @@ -74,6 +74,12 @@ class Skills(Extension): setting and serves `resources/directory/read`; omitting it advertises neither. Handlers run per request, so a catalog that changes over time (or is too large to enumerate) can return a partial or empty listing. + + Handler error contract: raise `MCPError` to return a specific error to the + caller (e.g. `INVALID_PARAMS` from `get_skill` for a URI it doesn't serve). + Any other failure — a raised exception, or a result that isn't SEP-2640 + conformant — is treated as a server fault and reported as `INTERNAL_ERROR`, + never the caller's bad params. """ identifier = EXTENSION_ID diff --git a/src/mcp/shared/skills.py b/src/mcp/shared/skills.py index 614bdf05aa..12add5e15d 100644 --- a/src/mcp/shared/skills.py +++ b/src/mcp/shared/skills.py @@ -16,6 +16,32 @@ from pydantic import AfterValidator, BaseModel, ConfigDict, Field, model_validator from pydantic.alias_generators import to_camel +__all__ = [ + "EXTENSION_ID", + "METHOD_LIST", + "METHOD_GET", + "METHOD_READ_DIRECTORY", + "MAX_RESOURCES_PER_SKILL", + "MAX_TOTAL_SIZE", + "Frontmatter", + "SkillResource", + "SkillResources", + "Skill", + "ListSkillsParams", + "ListSkillsResult", + "GetSkillParams", + "GetSkillResult", + "ReadDirectoryParams", + "ReadDirectoryResult", + "ListSkillsRequest", + "GetSkillRequest", + "ReadDirectoryRequest", + "skill_name_from_uri", + "parse_directory_uri", + "validate_directory_result", + "verify_skill_resource", +] + EXTENSION_ID = "io.modelcontextprotocol/skills" """The Skills extension identifier, advertised under `ServerCapabilities.extensions`.""" @@ -83,8 +109,9 @@ class Skill(_SkillModel): SEP-2640 conformance is intrinsic: constructing (or parsing) a `Skill` validates the frontmatter `name`/`description`, and — unless `resources` is `"dynamic"` — that every entry names a file within the skill's own directory, - with no duplicates and `SKILL.md` present. The 512-entry/16-MiB limits are - SHOULD NOT thresholds, not MUST NOT, so an over-limit manifest is accepted. + with no duplicates and `SKILL.md` present. The `MAX_RESOURCES_PER_SKILL` + (512-entry) and `MAX_TOTAL_SIZE` (16-MiB) limits are SHOULD NOT thresholds, + not MUST NOT, so an over-limit manifest is accepted. """ uri: str @@ -287,16 +314,16 @@ def validate_directory_result(uri: str, result: ReadDirectoryResult) -> None: def verify_skill_resource(skill: Skill, uri: str, content: bytes) -> None: - """Verify `content` (the bytes read from `uri`) against `skill`'s held manifest. + """Check that `content` (the bytes read from `uri`) matches `skill`'s manifest entry. - Implements the SEP-2640 Integrity and verification requirement: a host - MUST verify a retrieved file's bytes against its manifest entry before - using them. Not applicable to a skill whose `resources` is `"dynamic"`, - which offers no digest to verify against. + Recomputes the size and SHA-256 digest of `content` and compares them to the + entry `skill` holds for `uri` — the byte-integrity check SEP-2640 requires + before a host trusts a fetched file. A `"dynamic"` skill carries no digests, + so it has nothing to verify against. Raises: ValueError: If `uri` is not one of `skill`'s resources, `skill.resources` - is `"dynamic"`, or `content` does not match the entry's `size`/`digest`. + is `"dynamic"`, or `content`'s size or digest doesn't match the entry. """ if skill.resources == "dynamic": raise ValueError(f"skill {skill.uri!r} has dynamic resources and cannot be integrity-verified") diff --git a/tests/client/test_skills.py b/tests/client/test_skills.py index c3fda84f68..623b4e1b92 100644 --- a/tests/client/test_skills.py +++ b/tests/client/test_skills.py @@ -160,6 +160,21 @@ async def test_list_skills_raises_on_a_server_that_repeats_its_cursor() -> None: await skills.list_skills() +async def test_list_skills_rejects_a_skill_repeated_across_pages() -> None: + """SEP-2640 pagination: a page's own validation dedupes within that page, so the client also + rejects a skill URI the server hands back on a second page rather than returning it twice.""" + + async def handler(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> ListSkillsResult: + if params.cursor is None: + return ListSkillsResult(skills=[_skill()], next_cursor="page-2") + return ListSkillsResult(skills=[_skill()], next_cursor=None) + + server = MCPServer("catalog", extensions=[Skills(list_skills=handler, get_skill=_get_skill)]) + async with _skills(server) as skills: + with pytest.raises(ValueError, match="more than one skills/list page"): + await skills.list_skills() + + async def test_list_skills_requires_the_extension_to_be_advertised() -> None: """No `Skills` extension at all: the server never advertises `io.modelcontextprotocol/skills`.""" async with _skills(MCPServer("plain")) as skills: @@ -230,6 +245,24 @@ async def test_read_directory_raises_on_a_server_that_repeats_its_cursor() -> No await skills.read_directory("skill://git-workflow/references") +async def test_read_directory_rejects_a_child_repeated_across_pages() -> None: + """The client dedupes directory children across pages, not just within a single page.""" + child = Resource(uri="skill://git-workflow/references/A.md", name="A.md") + + async def handler(ctx: ServerRequestContext[Any, Any], params: ReadDirectoryParams) -> ReadDirectoryResult: + if params.cursor is None: + return ReadDirectoryResult(resources=[child], next_cursor="page-2") + return ReadDirectoryResult(resources=[child], next_cursor=None) + + server = MCPServer( + "catalog", + extensions=[Skills(list_skills=_paginated_list_handler(), get_skill=_get_skill, read_directory=handler)], + ) + async with _skills(server) as skills: + with pytest.raises(ValueError, match="more than one directory page"): + await skills.read_directory("skill://git-workflow/references") + + async def test_get_skill_rejects_a_mismatched_uri_from_a_non_conformant_server() -> None: server = MCPServer("catalog", extensions=[_NonConformantGetSkill()]) async with _skills(server) as skills: diff --git a/tests/server/test_skills.py b/tests/server/test_skills.py index f1a63d42b3..2cc9d5e04b 100644 --- a/tests/server/test_skills.py +++ b/tests/server/test_skills.py @@ -271,6 +271,7 @@ async def test_skills_get_rejects_a_skill_mutated_after_the_result_is_built() -> async def mutating_get(ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> GetSkillResult: result = GetSkillResult(skill=_git_workflow_skill()) + assert isinstance(result.skill.resources, list) result.skill.resources.clear() return result From 083a5dc633c4c0fb678a3e2267d8c57d933facdd Mon Sep 17 00:00:00 2001 From: vijay Date: Thu, 24 Sep 2026 07:01:28 +0530 Subject: [PATCH 25/25] relaxed the de-dup failure --- src/mcp/client/skills.py | 35 +++++++++++++++++++---------------- src/mcp/server/skills.py | 5 ++--- tests/client/test_skills.py | 18 +++++++++--------- 3 files changed, 30 insertions(+), 28 deletions(-) diff --git a/src/mcp/client/skills.py b/src/mcp/client/skills.py index c6f646f4b8..79b3cafd23 100644 --- a/src/mcp/client/skills.py +++ b/src/mcp/client/skills.py @@ -92,10 +92,12 @@ def _require_extension(self, *, directory_read: bool = False) -> None: async def list_skills(self, params: ListSkillsParams | None = None) -> list[Skill]: """Call `skills/list`, following `nextCursor` to completion, and validate the result. + A skill a changing catalog surfaces on more than one page is returned once, in the order + first seen. + Raises: - ValueError: If the server doesn't advertise the Skills extension, its - response is not SEP-2640 conformant, it repeats a pagination - cursor, or it lists the same skill URI on two pages. + ValueError: If the server doesn't advertise the Skills extension, its response is not + SEP-2640 conformant, or it repeats a pagination cursor. MCPError: If the server returns an error response. """ self._require_extension() @@ -113,12 +115,12 @@ async def list_skills(self, params: ListSkillsParams | None = None) -> list[Skil # A server stuck repeating its cursor is a loop; catch that before the content checks. if page.next_cursor is not None and page.next_cursor in seen_cursors: raise ValueError(f"server repeated skills/list pagination cursor {page.next_cursor!r}") - # Per-page validation can't catch a URI repeated *across* pages, so track that here. + # A catalog that changes between page fetches can legitimately repeat a skill across + # pages; keep the first occurrence rather than treating it as an error. for skill in page.skills: - if skill.uri in seen_uris: - raise ValueError(f"server listed skill {skill.uri!r} on more than one skills/list page") - seen_uris.add(skill.uri) - skills.extend(page.skills) + if skill.uri not in seen_uris: + seen_uris.add(skill.uri) + skills.append(skill) if page.next_cursor is None: return skills seen_cursors.add(page.next_cursor) @@ -162,10 +164,12 @@ async def read_skill_uri(self, uri: str) -> ReadResourceResult: async def read_directory(self, uri: str, params: ReadDirectoryParams | None = None) -> list[Resource]: """Call `resources/directory/read` for `uri`, following `nextCursor` to completion. + A child a changing directory surfaces on more than one page is returned once, in the order + first seen. + Raises: - ValueError: If the server doesn't advertise the `directoryRead` - setting, its response is not a valid child listing of `uri`, it - repeats a pagination cursor, or it lists the same child on two pages. + ValueError: If the server doesn't advertise the `directoryRead` setting, its response + is not a valid child listing of `uri`, or it repeats a pagination cursor. MCPError: If the server returns an error response. """ self._require_extension(directory_read=True) @@ -183,12 +187,11 @@ async def read_directory(self, uri: str, params: ReadDirectoryParams | None = No if page.next_cursor is not None and page.next_cursor in seen_cursors: raise ValueError(f"server repeated resources/directory/read pagination cursor {page.next_cursor!r}") validate_directory_result(uri, page) - # `validate_directory_result` dedupes within a page; catch a child repeated across pages. + # A changing directory can legitimately repeat a child across pages; keep the first. for resource in page.resources: - if resource.uri in seen_uris: - raise ValueError(f"server listed child {resource.uri!r} on more than one directory page") - seen_uris.add(resource.uri) - resources.extend(page.resources) + if resource.uri not in seen_uris: + seen_uris.add(resource.uri) + resources.append(resource) if page.next_cursor is None: return resources seen_cursors.add(page.next_cursor) diff --git a/src/mcp/server/skills.py b/src/mcp/server/skills.py index 745af4cbd1..667aa39941 100644 --- a/src/mcp/server/skills.py +++ b/src/mcp/server/skills.py @@ -77,9 +77,8 @@ class Skills(Extension): Handler error contract: raise `MCPError` to return a specific error to the caller (e.g. `INVALID_PARAMS` from `get_skill` for a URI it doesn't serve). - Any other failure — a raised exception, or a result that isn't SEP-2640 - conformant — is treated as a server fault and reported as `INTERNAL_ERROR`, - never the caller's bad params. + A result that isn't SEP-2640 conformant is caught here and reported as + `INTERNAL_ERROR` — a server fault, not the caller's bad params. """ identifier = EXTENSION_ID diff --git a/tests/client/test_skills.py b/tests/client/test_skills.py index 623b4e1b92..13a259c191 100644 --- a/tests/client/test_skills.py +++ b/tests/client/test_skills.py @@ -160,9 +160,9 @@ async def test_list_skills_raises_on_a_server_that_repeats_its_cursor() -> None: await skills.list_skills() -async def test_list_skills_rejects_a_skill_repeated_across_pages() -> None: - """SEP-2640 pagination: a page's own validation dedupes within that page, so the client also - rejects a skill URI the server hands back on a second page rather than returning it twice.""" +async def test_list_skills_collapses_a_skill_repeated_across_pages() -> None: + """A catalog that changes between page fetches can surface the same skill on two pages; the + client returns it once (first occurrence) rather than failing or duplicating it.""" async def handler(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> ListSkillsResult: if params.cursor is None: @@ -171,8 +171,8 @@ async def handler(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) server = MCPServer("catalog", extensions=[Skills(list_skills=handler, get_skill=_get_skill)]) async with _skills(server) as skills: - with pytest.raises(ValueError, match="more than one skills/list page"): - await skills.list_skills() + result = await skills.list_skills() + assert [s.uri for s in result] == [_SKILL_URI] async def test_list_skills_requires_the_extension_to_be_advertised() -> None: @@ -245,8 +245,8 @@ async def test_read_directory_raises_on_a_server_that_repeats_its_cursor() -> No await skills.read_directory("skill://git-workflow/references") -async def test_read_directory_rejects_a_child_repeated_across_pages() -> None: - """The client dedupes directory children across pages, not just within a single page.""" +async def test_read_directory_collapses_a_child_repeated_across_pages() -> None: + """A changing directory can surface the same child on two pages; the client returns it once.""" child = Resource(uri="skill://git-workflow/references/A.md", name="A.md") async def handler(ctx: ServerRequestContext[Any, Any], params: ReadDirectoryParams) -> ReadDirectoryResult: @@ -259,8 +259,8 @@ async def handler(ctx: ServerRequestContext[Any, Any], params: ReadDirectoryPara extensions=[Skills(list_skills=_paginated_list_handler(), get_skill=_get_skill, read_directory=handler)], ) async with _skills(server) as skills: - with pytest.raises(ValueError, match="more than one directory page"): - await skills.read_directory("skill://git-workflow/references") + resources = await skills.read_directory("skill://git-workflow/references") + assert [r.uri for r in resources] == ["skill://git-workflow/references/A.md"] async def test_get_skill_rejects_a_mismatched_uri_from_a_non_conformant_server() -> None: