feat(rmcp): implement SEP-2640 MCP Skills Extension

[branben]

Motivation and Context

SEP-2640 (Skills Extension for MCP) was merged Sep 13. This PR adds the Rust SDK
reference implementation: skills/list, skills/get, and resources/directory/read
methods, plus the #[skill] macro trio that lets server authors declare skill
endpoints the same way they declare tool endpoints.

How Has This Been Tested?

cargo check --all-features passes clean. 26 conformance tests pass in
crates/rmcp/tests/test_skills_conformance.rs (including 3 new RED->GREEN tests
for spec conformance gaps found in review). 10 macro tests pass.
FileSystemSkillServer demo server runs end-to-end with fixtures under
fixtures/skills-dir/. Full suite: 940 passed, 0 failed.

Breaking Changes

None. All new methods are additive. Existing code is unaffected.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would change existing functionality)
• Documentation update

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling (catch-all match arm for future SEP methods)
• I have added or updated documentation as needed

Review Fixes (this push)

Three spec conformance gaps found and fixed during review:

1. Macro skills_get gate — replaced 12-line directory_form || skill_md_form
   check with 2-line equality gate (request.uri != route.attr.uri) on both macro
   and Router paths. Closes two holes: non-SKILL.md canonical URIs were listed but
   not gettable; nested SKILL.md URIs returned the wrong skill's entry.

2. SkillsGetResult cache hints — added ttl_ms/cache_scope fields and
   included SkillsGetResult in fill_missing_cache_hints match. Spec:419 requires
   these on every skills/get response.

3. #[skill] macro compile-time URI validation — added /SKILL.md suffix check
   so non-SKILL.md URIs fail at compile time, not at runtime.

[alexhancock]

Thanks for looking into this. It needs refinement in a few areas

A list I found from an AI assisted review:

• No extension declaration: servers never advertise io.modelcontextprotocol/skills, directoryRead, or the resources capability, so compliant clients won't call any skill methods.
• resources treated as optional: entries can omit it, and the demo leaves out SKILL.md and subdirectory files. The spec requires a complete list.
• Wrong directory-read wire format: it returns children with isDirectory instead of resources with mimeType: "inode/directory".
• Directory read misbehaves: Router always returns an empty list, the demo lists the parent folder, and missing paths don't return -32602.
• skills/get too loose: it accepts any file URI inside a skill, returns -32601 instead of -32602 when nothing matches, and the macro version drops the frontmatter and resources.
• URI parsing bug: parse_skill_uri gets the skill path and name wrong for supporting files in subfolders.
• Frontmatter not copied exactly: the demo's flat-YAML parser mangles nested fields like metadata.
• Requests in the wrong direction: the skill requests were also added to ServerRequest, the server-to-client list.
• Dead macros: the skill macros are never exported, a crate-wide allow(dead_code) hides this, and their generated code wouldn't compile.

[alexhancock]

Why are we adding this?

[branben]

I set a slash goal for this implementation and instead of actually reading the logs my AI assisted review for this looked to simply use this as a temporary bandaid instead of literally using clippy and forgot to remove it. This should be a note that simply "passing tests" isn't enough! I've gone ahead and removed this in an upcoming commit

the skill macros were pub fn inside a private module

[alexhancock]

This is the wrong direction no? ServerRequest holds requests the server makes to the client.

[branben]

I misunderstood this and mentally had it the opposite! noted, and adjusted in my upcoming commit

client logic ended up in the wrong union

[branben]

Thanks for looking into this. It needs refinement in a few areas

A list I found from an AI assisted review:

• No extension declaration: servers never advertise io.modelcontextprotocol/skills, directoryRead, or the resources capability, so compliant clients won't call any skill methods.
• resources treated as optional: entries can omit it, and the demo leaves out SKILL.md and subdirectory files. The spec requires a complete list.
• Wrong directory-read wire format: it returns children with isDirectory instead of resources with mimeType: "inode/directory".
• Directory read misbehaves: Router always returns an empty list, the demo lists the parent folder, and missing paths don't return -32602.
• skills/get too loose: it accepts any file URI inside a skill, returns -32601 instead of -32602 when nothing matches, and the macro version drops the frontmatter and resources.
• URI parsing bug: parse_skill_uri gets the skill path and name wrong for supporting files in subfolders.
• Frontmatter not copied exactly: the demo's flat-YAML parser mangles nested fields like metadata.
• Requests in the wrong direction: the skill requests were also added to ServerRequest, the server-to-client list.
• Dead macros: the skill macros are never exported, a crate-wide allow(dead_code) hides this, and their generated code wouldn't compile.

Hey Alex, appreciate you checking this one out! I gave a pass at your feedback and I've got a follow up commit for each bullet. I'm listing it as "addressed in code" not "resolved". While working with AI on this solution I determined I needed a more genuine end to end test, so I built a real MCP consumer around a skill I currently have as a reference.

Here's the documentation with a timeline

I listed all the areas my agent deviated during these commits, why it failed and what I did as an alternative.

[branben]

follow up testing revealed some more defects that I've addressed in these commits:

• the Router's resources/directory/read returned a hardcoded empty list instead of delegating to the skill route's declared files

• directory-form URIs (skill://alpha) were unroutable, fill_missing_cache_hints skipped the two SEP-2640 skill result types

• the FS demo server never advertised the skills extension.

[HALT]

Each fix has a dedicated regression test that was verified RED before the fix and GREEN after, plus 5 new tests across unit and integration levels. I've also updated the linked documentation above

[DaleSeo]

The D2 test below quotes the spec — "clients descend by calling the method again on a child directory" — but a child-directory URI like skill://pdf-processing/templates matches the route by prefix and then fails this equality gate with -32602, so the subdirectories this very listing advertises as inode/directory can never be read. Could the gate treat a URI as a directory when some declared file lives under it, and list relative to the requested URI (the same gate in the skill_handler.rs macro expansion would need the same change)?

Suggested change

	// Spec: the method applies only to directory resources. The
	// skill root (`skill://<skill-path>`) is a directory; a file
	// URI such as `skill://<skill-path>/SKILL.md` is not, and
	// MUST return -32602.
	let directory_form =
	format!("skill://{}", route.skill_path().unwrap_or_default());
	if request.params.uri != directory_form {
	return Err(crate::ErrorData::invalid_params(
	format!("not a directory: {}", request.params.uri),
	None,
	));
	}
	let resources: Vec<crate::model::Resource> = match &route.attr.resources {
	Some(crate::model::skills::SkillResources::FileList(files)) => {
	// Spec, Directory Listing: "The result contains every
	// direct child of the directory." SKILL.md is a file
	// directly inside the skill root, so it IS a direct
	// child and MUST be listed (it is also in the skill's
	// own `resources` manifest).
	let root = route
	.attr
	.uri
	.strip_suffix("/SKILL.md")
	.or_else(|| route.attr.uri.strip_suffix("SKILL.md"))
	.unwrap_or(&route.attr.uri);
	let prefix = format!("{}/", root);
	// Spec: the method applies only to directory resources. The
	// skill root (`skill://<skill-path>`) is a directory, and so
	// is any subdirectory that declared files live under; a file
	// URI such as `skill://<skill-path>/SKILL.md` is not, and
	// MUST return -32602.
	let directory_form =
	format!("skill://{}", route.skill_path().unwrap_or_default());
	let subdir_prefix = format!("{}/", request.params.uri);
	let is_directory = request.params.uri == directory_form
	|| route
	.attr
	.resources
	.as_ref()
	.and_then(|r| r.as_files())
	.is_some_and(|files| {
	files.iter().any(|f| f.uri.starts_with(&subdir_prefix))
	});
	if !is_directory {
	return Err(crate::ErrorData::invalid_params(
	format!("not a directory: {}", request.params.uri),
	None,
	));
	}
	let resources: Vec<crate::model::Resource> = match &route.attr.resources {
	Some(crate::model::skills::SkillResources::FileList(files)) => {
	// Spec, Directory Listing: "The result contains every
	// direct child of the directory." SKILL.md is a file
	// directly inside the skill root, so it IS a direct
	// child and MUST be listed (it is also in the skill's
	// own `resources` manifest).
	let root = request.params.uri.as_str();
	let prefix = format!("{}/", root);

[branben]

treat a URI as a subdirectory when a declared file lives under it. This logic should go in both router.rs and the macro expansion!

[DaleSeo]

A server-side error, like -32602 for a missing skill, comes through here as Err(ServiceError::McpError(...)). The catch-all turns it into UnexpectedResponse, so we lose the real error.

Suggested change

	let result = self
	.send_request(ClientRequest::SkillsGetRequest(SkillsGetRequest::new(
	params,
	)))
	.await;
	match result {
	Ok(ServerResult::SkillsGetResult(result)) => Ok(result),
	_ => Err(ServiceError::UnexpectedResponse),
	}
	let result = self
	.send_request(ClientRequest::SkillsGetRequest(SkillsGetRequest::new(
	params,
	)))
	.await?;
	match result {
	ServerResult::SkillsGetResult(result) => Ok(result),
	_ => Err(ServiceError::UnexpectedResponse),
	}

[DaleSeo]

Same issue as with skills_get above. A genuine server error is being turned into UnexpectedResponse instead of being passed through.

Suggested change

	let result = self
	.send_request(ClientRequest::ResourcesDirectoryReadRequest(
	ResourcesDirectoryReadRequest::new(params),
	))
	.await;
	match result {
	Ok(ServerResult::ResourcesDirectoryReadResult(result)) => Ok(result),
	_ => Err(ServiceError::UnexpectedResponse),
	}
	let result = self
	.send_request(ClientRequest::ResourcesDirectoryReadRequest(
	ResourcesDirectoryReadRequest::new(params),
	))
	.await?;
	match result {
	ServerResult::ResourcesDirectoryReadResult(result) => Ok(result),
	_ => Err(ServiceError::UnexpectedResponse),
	}

[branben]

match only on the Ok variant, so real service errors are actually propagating

[DaleSeo]

SkillsGetRequest delegates to the inner service when the router has no answer, but SkillsListRequest is intercepted unconditionally — a wrapped service's own skills_list implementation becomes unreachable. Should an empty skill_router delegate instead?

Suggested change

	ClientRequest::SkillsListRequest(_) => {
	// Spec: the final `<skill-path>` segment of `uri` MUST equal
	// `frontmatter.name`. Entries that violate it are invalid, so
	// they are not served.
	let skills: Vec<_> = self
	ClientRequest::SkillsListRequest(request) => {
	if self.skill_router.map.is_empty() {
	return self
	.service
	.handle_request(ClientRequest::SkillsListRequest(request), context)
	.await;
	}
	// Spec: the final `<skill-path>` segment of `uri` MUST equal
	// `frontmatter.name`. Entries that violate it are invalid, so
	// they are not served.
	let skills: Vec<_> = self

[DaleSeo]

The macro generates a working resources_directory_read, but advertises the extension without directoryRead.

Suggested change

	capability_calls.push(quote! { .enable_skills_extension_without_directory_read() });
	capability_calls.push(quote! { .enable_skills_extension() });

[DaleSeo]

dynamic is accepted but silently ignored by the let _ = dynamic; below. Also, the note about retaining it for source compatibility doesn't apply to a macro introduced in this PR.

Suggested change

	/// Whether this skill generates its content dynamically (no fixed file list).
	pub dynamic: bool,

[branben]

+1
dropping the dead param because it doesn't apply here

[DaleSeo]

This doc tells readers to use skills_list for pagination, but this is skills_list. The helper that handles pagination is list_all_skills.

Suggested change

	/// Send one `skills/list` request and return the list result, without
	/// pagination support. For paginated listing that follows `nextCursor`,
	/// use [`Peer::skills_list`] with the desired pagination params.
	/// Send one `skills/list` request and return a single page of results.
	/// Use [`Peer::list_all_skills`] to follow `nextCursor` across pages.

[DaleSeo]

"metatarsal" is not a cache scope (the valid values are "public"/"private").

Suggested change

	/// "_meta": { "ttlMs": 600000, "cacheScope": "metatarsal" }
	/// "_meta": { "ttlMs": 600000, "cacheScope": "public" }

[DaleSeo]

Just for simplicity

Suggested change

	//! Skill handler support — analogous to `tool.rs` for tools.
	//!
	//! Pedagogy: The skill call context is like a hall pass. When a skill
	//! request comes in, the router checks the gradebook, finds the matching
	//! route, and hands the handler a "hall pass" (SkillCallContext) that
	//! says "you are allowed to run skill X with URI Y."
	//! Skill handler support — analogous to `tool.rs` for tools.

[DaleSeo]

The comment above quotes the spec, which says the enclosing skill's resources list them too. But this skips all nested SKILL.md files, so their manifests aren't included in the enclosing skill's resources.

Suggested change

	if name == SKILL_FILE {
	continue;
	}

[alexhancock]

Thanks for submitting, but a lot is missing from this. The feedback I sent earlier wasn't addressed + other things are wrong after more recent changes. I'm going to close this one out now.

[branben]

Thanks for submitting, but a lot is missing from this. The feedback I sent earlier wasn't addressed + other things are wrong after more recent changes. I'm going to close this one out now.

Sorry for the late response! I seperated each of your bullets into individual commits (force pushed away from ff211dd), however Dale's review is new, is there any area in my follow up commits at 60fc35a (head) that I might've missed from your feedback?