Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions docs/advanced/skills.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ There are three moves here:
* `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.
the string `"dynamic"` for content generated on demand. An optional `_meta` dict rides along
unchanged on both methods.
* `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.
Expand Down Expand Up @@ -117,7 +118,8 @@ setting.

## Protocol version and caching

In protocol version `2026-07-28` and later, `skills/list` and `skills/get` results carry the base
In protocol version `2026-07-28` and later, `skills/list`, `skills/get`, and
`resources/directory/read` 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 `"private"` when your handler leaves it unset, and omits both fields entirely on an
Expand Down
4 changes: 2 additions & 2 deletions src/mcp/server/skills.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ async def _handle_read_directory(
validate_directory_result(params.uri, result)
except ValueError:
raise MCPError(code=INTERNAL_ERROR, message="Handler returned an invalid result") from None
return result
return _finalize_cacheable(result, ctx.protocol_version)
Comment thread
Kludex marked this conversation as resolved.
Comment thread
Kludex marked this conversation as resolved.


def _require_skill_md_uri(uri: str) -> None:
Expand All @@ -159,7 +159,7 @@ def _require_directory_uri(uri: str) -> None:
def _finalize_cacheable(result: CacheableResult, protocol_version: str) -> HandlerResult:
"""Gate SEP-2549's `ttlMs`/`cacheScope` to protocol version 2026-07-28+.
`skills/list` and `skills/get` are extension methods, so — unlike a core spec
`skills/list`, `skills/get` and `resources/directory/read` 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"`, keeping user-specific entries out of shared caches.
Expand Down
10 changes: 8 additions & 2 deletions src/mcp/shared/skills.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ class Skill(_SkillModel):
"""Resource URI of the skill's `SKILL.md`."""
frontmatter: Frontmatter
resources: SkillResources
meta: dict[str, Any] | None = Field(alias="_meta", default=None)
"""Opaque `_meta` the server attaches to this entry."""

@model_validator(mode="after")
def _check_conformance(self) -> Skill:
Expand Down Expand Up @@ -191,8 +193,12 @@ class ReadDirectoryParams(PaginatedRequestParams):
"""URI of the directory resource whose direct children are listed."""


class ReadDirectoryResult(PaginatedResult):
"""Result of `resources/directory/read`."""
class ReadDirectoryResult(PaginatedResult, CacheableResult):
"""Result of `resources/directory/read`.

Like `ListSkillsResult`, this carries SEP-2549's `ttl_ms`/`cache_scope`, which
`mcp.server.skills` strips for a pre-2026-07-28 connection.
"""

resources: list[Resource]

Expand Down
24 changes: 24 additions & 0 deletions tests/client/extensions/test_skills.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,30 @@ async def test_get_skill_returns_the_matching_entry() -> None:
assert skill.uri == _SKILL_URI


async def test_list_and_get_skill_preserve_skill_meta() -> None:
Comment thread
Kludex marked this conversation as resolved.
"""ext-skills#157: a skill's optional `_meta` survives server -> wire -> client on both methods."""
meta = {"example.com/owner": "platform"}
skill_with_meta = Skill(
uri=_SKILL_URI,
frontmatter={"name": "git-workflow", "description": "d"},
resources=[SkillResource(uri=_SKILL_URI, digest=_SKILL_DIGEST, size=len(_SKILL_CONTENT))],
_meta=meta,
)

async def list_skills(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> ListSkillsResult:
return ListSkillsResult(skills=[skill_with_meta])

async def get_skill(ctx: ServerRequestContext[Any, Any], params: GetSkillParams) -> GetSkillResult:
return GetSkillResult(skill=skill_with_meta)

server = MCPServer("catalog", extensions=[Skills(list_skills=list_skills, get_skill=get_skill)])
async with _skills(server) as skills:
listed = await skills.list_skills()
fetched = await skills.get_skill(_SKILL_URI)
assert [s.meta for s in listed] == [meta]
assert fetched.meta == meta


async def test_get_skill_propagates_the_servers_unknown_skill_error() -> None:
async with _skills(_server()) as skills:
with pytest.raises(MCPError) as exc_info:
Expand Down
36 changes: 36 additions & 0 deletions tests/server/test_skills.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,20 @@ async def mutating_list(ctx: ServerRequestContext[Any, Any], params: ListSkillsP
assert exc_info.value.code == INTERNAL_ERROR


async def test_skills_list_sends_skill_meta_as_underscore_meta() -> None:
"""ext-skills#157: a skill's optional metadata crosses the wire as `_meta`, not `meta`."""

async def with_meta(ctx: ServerRequestContext[Any, Any], params: ListSkillsParams) -> ListSkillsResult:
skill = _git_workflow_skill().model_copy(update={"meta": {"example.com/owner": "platform"}})
return ListSkillsResult(skills=[skill])

server = MCPServer("catalog", extensions=[Skills(list_skills=with_meta, 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 {}
Comment on lines +186 to +192

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit (optional): maintainers get a _meta wire test that compares two hand-written literals instead of the value the handler sent. In tests/server/test_skills.py:186 the handler builds {"example.com/owner": "platform"} and tests/server/test_skills.py:193 asserts against a second copy of that literal. The repo's test-quality bar says pass-through values such as _meta are asserted against the same variable that was sent. Fix: bind the dict to a module- or test-level variable, pass that to model_copy, and assert extra["skills"][0]["_meta"] == meta against it. [also at: tests/server/test_skills.py:189 - nit: the _meta wire-key test compares the pass-through value to a retyped literal, so a fixer gets a test that only proves two literals match. tests/server/test_skills.py:184 builds the meta dict inline and tests/server/test_skills.py:189 asserts against a second copy of it.; tests/server/test_skills.py:191 - nit: maintainers get a pass-through _meta assertion that compares two hand-typed literals, which the repo's test-quality bar says proves nothing. tests/server/test_skills.py:184 builds the dict inline in the handler and tests/server/test_skills.py:191 re-types it in the assert.]

Why this was flagged

AGENTS.md line 68 delegates test review to .claude/skills/test-quality/SKILL.md, whose Assertions section says pass-through values (_meta named explicitly) are compared against the same variable you sent, because two literals only match because a human eyeballed them. tests/server/test_skills.py:186 builds the meta literal inside with_meta, and tests/server/test_skills.py:193 repeats the literal in the assertion. Nothing fails at runtime; the test still proves the _meta key name on the wire. The sibling client test at tests/client/extensions/test_skills.py:193 already uses a meta variable, so this is the one site in the diff that departs from the convention.

Verification: nit. AGENTS.md line 68 delegates tests to .claude/skills/test-quality/SKILL.md, whose lines 56-58 require pass-through values such as _meta be asserted against the same variable you sent. tests/server/test_skills.py line 186 builds the value inline and line 193 compares against a second hand-typed literal. Nothing fails at runtime, so this is a convention-only violation.

assert extra["skills"][0]["_meta"] == {"example.com/owner": "platform"}


async def test_skills_get_returns_the_matching_skill() -> None:
async with Client(_server()) as client:
result = await client.session.send_request(
Expand Down Expand Up @@ -321,6 +335,28 @@ async def test_directory_read_returns_the_handlers_children() -> None:
assert [r.uri for r in result.resources] == ["skill://git-workflow/references"]


async def test_directory_read_defaults_to_private_cache_scope_on_the_2026_07_28_wire() -> None:
"""ext-skills#157: `resources/directory/read` results carry SEP-2549's cache fields too."""
async with Client(_server(with_directory_read=True), mode="2026-07-28") as client:
raw = await client.session.send_request(
ReadDirectoryRequest(params=ReadDirectoryParams(uri="skill://git-workflow")), _RawResult
)
extra = raw.model_extra or {}
assert (extra["ttlMs"], extra["cacheScope"]) == (0, "private")


async def test_directory_read_omits_cache_fields_on_a_legacy_wire() -> None:
"""SEP-2549 fields are 2026-07-28+ only, and the runner's sieve skips extension methods,
so `Skills` strips them itself for an older connection."""
async with Client(_server(with_directory_read=True), mode="legacy") as client:
raw = await client.session.send_request(
ReadDirectoryRequest(params=ReadDirectoryParams(uri="skill://git-workflow")), _RawResult
)
extra = raw.model_extra or {}
assert "cacheScope" not in extra
assert "ttlMs" not in extra


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."""
Expand Down
Loading