Add Skills extension - #3485
Add Skills extension#3485vijaydeepsinha wants to merge 20 commits into
Skills extension#3485Conversation
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.
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+.
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.
Adds the Skills page under Advanced, with a runnable server/client example, and tests proving every claim the page makes against the real SDK.
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.
_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.
_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.
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.
- 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.
|
Assigned #3486 to @vijaydeepsinha and re-opened |
321ab53 to
b845490
Compare
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.
There was a problem hiding this comment.
All reported issues were addressed across 12 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…RAMS 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.
… 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.
…ages `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.
…ic 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.
3b09033 to
f1d2920
Compare
…ent 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.
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…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.
|
I think I'd like to figure out a nice way to isolate extension code a bit more explicitly first, although won't have time to properly look into this until next after the 27th. @Kludex do you have a chance to look? maybe an extensions/ folder somewhere? |
| ) | ||
| 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"] |
There was a problem hiding this comment.
I don't think this is the API that we want here.
The Client does accept a extensions parameter. Why can't we use that?
There's probably something smart we can do type wise to offer methods on the client's side... Something to explore:
async with Client(extensions={"skills": Skill()}) as client:
client.extensions["skills"].<autocomplete would work here>Can you check this, please?
There was a problem hiding this comment.
Probably not your fault, but we are doing a lot of validate_* calls everywhere. That a smell.
We should either leverage Pydantic a bit more (using Annotated), or not have them.
This point is only actionable if you can find ways to avoid this many validate_... Which you probably can 👀
There was a problem hiding this comment.
@Kludex But we have same pattern across the entire sdk, are we willing to deviate for this PR or pick refactoring as follow-up exercise.
Kludex
left a comment
There was a problem hiding this comment.
I don't like the fact that we are wrapping the client to expose methods, I think we can do better, but the PR itself is very self contained.
Can you please check my comments?
Also, can you please tell your agent to redo the skills.md as "Please rewrite the skills.md as if you were Sebastian Ramirez, matching the rest of the docs"?
Fixes #3486
Summary
Adds Python SDK support for SEP-2640 (Skills Extension):
skills/list,skills/get, andresources/directory/readas protocol primitives, SEP-2640 conformance validation, and capability negotiation. This SDK does not provide filesystem discovery, catalog indexing, or caching/refresh policy — those belong to a higher-level provider built on top of this.Motivation and Context
SEP-2640 defines a convention for serving Agent Skills over MCP using the Resources primitive. The Python SDK has no support for it today. This PR adds the extension using the SDK's existing
Extension/MethodBindingmechanism (the same one backing the shippedAppsextension, SEP-2133) — no schema or codegen changes, no new required dependencies.What's included
src/mcp/shared/skills.py— wire types (Skill,SkillResource, params/results), SEP-2640 conformance validation (name/URI/frontmatter consistency, digest format, resource-manifest completeness, the 512-entry/16 MiB limits), andverify_skill_resource(digest+size integrity check for content already read).src/mcp/server/skills.py— theSkillsextension: handler-based (list_skills,get_skill, optionalread_directory), validates results against SEP-2640 before they hit the wire, gates the SEP-2549ttlMs/cacheScopefields to protocol version 2026-07-28+.src/mcp/client/skills.py—list_skills/get_skill/read_directory(auto-paginating, with cursor-repeat detection),read_skill_uri(a thin, discoverableresources/readalias),verify_skill_resourcere-exported for client use.docs/advanced/skills.md+docs_src/skills/— a new doc page with a runnable example, explicitly scoping what the SDK does and doesn't do.Server usage
Client usage
Protocol version / compatibility notes
capabilities.extensions(SEP-2133) andttlMs/cacheScope(SEP-2549) are 2026-07-28+-only wire fields in this SDK's existing type surface — this is pre-existing, documented SDK behavior (docs/advanced/extensions.md), not something this PR changes.Skillsgates its own cache fields to match.-32602error returns HTTP 200 on the classic (pre-2026-07-28) wire and HTTP 400 on the modern (2026-07-28+) wire. This is existing, spec-mandated (SEP-2575) SDK-wide transport behavior — every handler in the SDK gets it automatically via the sharedERROR_CODE_HTTP_STATUStable; nothing Skills-specific.mkdocs.yml(one nav entry).How Has This Been Tested?
tests/{shared,server,client}/test_skills.py— 100% line+branch coverage on all three new modules (shared/skills.py,server/skills.py,client/skills.py), verified viacoverage report --fail-under=0. Coverage includes the SEP-2549 cache-attribute behavior onskills/listandskills/get(present on the 2026-07-28 wire, absent on the legacy wire), cursor-resume/pagination, request_metapropagation, and the URI reject rules (trailing-slash and dot-segment)../scripts/test→ 6000+ passed, 0 failed, 100% total coverage,strict-no-coverclean.ruff format/ruff check,pyright,markdownlint,mkdocs build --strict(Zensical), README-snippet check: all clean.uv run --python 3.10 pytest tests/*/test_skills.pypasses.Conformance
Ran the modelcontextprotocol/conformance PR #330 SEP-2640 scenarios end-to-end against a real server and client built on this implementation (server scenarios exercise this PR's server; client scenarios exercise this PR's client against a hostile server the harness stands up):
sep-2640-skills-enumeration(skills/list+skills/get)sep-2640-skills-manifest(SKILL.mdresource metadata)sep-2640-skills-directory(resources/directory/read)sep-2640-client-no-prefetchsep-2640-client-verify-digestsep-2640-client-verify-sizesep-2640-client-verify-frontmatter45 wire checks + 4 client checks, 0 failures, 0 warnings.
Also manually verified via a live server against a Postman collection covering both the session-based (2025-11-25) and stateless (2026-07-28) wires.
Breaking Changes
None. New files only; no existing public API is modified.
Deliberate scope exclusions (and why)
Skills,list_skills, andget_skill.SKILL.md's YAML frontmatter and compare it field-by-field against the held entry. This SDK does not ship that comparison, to avoid adding a new required YAML dependency to the core SDK for a check any host already has the means to do with whatever YAML library it uses elsewhere.verify_skill_resourcecovers the digest/size half (no new dependency needed for that). Documented explicitly indocs/advanced/skills.md's "What this SDK doesn't do".mcp-typesinstead ofmcp.shared:mcp-typesis generated from the official, versioned MCP JSON Schema; SEP-2640 is an extension, not core spec vocabulary, so its types are hand-written and live alongside the extension code — the same placement the existingAppsextension (SEP-2133) uses.Types of changes
Checklist
help wanted, or I'm a maintainer) — N/A while targeting my own fork; will file/link before retargeting to upstream