Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
23 changes: 23 additions & 0 deletions tests/client/extensions/test_skills.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,29 @@ 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.
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
32 changes: 32 additions & 0 deletions tests/server/test_skills.py
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,18 @@ 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:
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 +333,26 @@ 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:
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