A reviewer left 9 comments on PR #1286. This is what each one meant and what we changed.
The MCP protocol lets an AI assistant talk to a "server" that offers it abilities — tools it can call, files it can read, and now skills: reusable instruction bundles the assistant can look up and follow. This PR adds that skills feature to the Rust SDK people use to build those servers.
It has two halves: the runtime (the code that actually answers skill requests over the wire) and macros — Rust shorthand that writes the boring plumbing for you, so a server author types three lines instead of sixty. The reviewer found problems in both halves.
#[skill]; the macro expands it into full working code at compile time. If a macro is never "exported", nobody can use it — question #1.skill://acme/billing/refunds/templates/x.md. The tricky part: you can't tell where the "skill name" ends and the "file path" begins just by looking.allow(dead_code) being added?“Why are we adding this?”
Short answer: it was a band-aid that hid a real bug, and we removed it.
Rust normally warns you when code is never used ("dead code"). That warning is useful — it's how you notice something you wrote isn't wired up. allow(dead_code) switches that warning off for the whole file.
It got added back in September in a commit called "chore(rmcp): fix clippy warnings and formatting" — the goal was to make a lint check go green, not to solve a design problem. What it actually did was silence the exact warning that would have caught the reviewer's other finding: the three new skill macros were never exported, so nobody could call them. The warning was pointing at a real bug; the suppression made it invisible.
The fix (commits f51601e and fcaef81) removes the suppression and exports the three macros properly. Once they're exported and used, there's no dead code left to warn about, so the suppression isn't needed.
8c7465c — "chore(rmcp): fix clippy warnings and formatting". One line, to quiet a lint.pub fn skill* exports existed at review time. The three macros were unreachable, and their generated code wouldn't have compiled even if it had been reachable.f51601e — suppression deleted; fcaef81 exports the macros, so the underlying warning has nothing left to flag.“This is the wrong direction no? ServerRequest holds requests the server makes to the client.”
Short answer: the reviewer is right, and we fixed it.
The SDK keeps two lists of message types. One list is "requests the server sends to the client" (for example: "please ask the human a question"). The other is "requests the client sends to the server" (for example: "list your skills").
The three new skill requests — skills/list, skills/get, resources/directory/read — are things the client asks the server. They were mistakenly added to both lists, including the server-to-client one where they don't belong.
That's not just tidiness: the server-to-client list is what a client's message router matches on. A request in the wrong list means the type system would let you send a "list your skills" message as the server, which the spec doesn't allow. The fix (commit 752882d) removes them from the wrong list, leaving them only where they belong.
ServerRequest — requests the server makes to the client.ClientRequest — requests the client makes to the server.752882d — removed all three from ServerRequest across 4 files; they now live only in ClientRequest.Plain: before a client asks for anything, the server announces what it can do — like a menu. The menu never listed "skills", so a well-behaved client would never ask. Fixed by declaring the io.modelcontextprotocol/skills extension in the capabilities map (not a top-level field — see deviation 3).
Plain: when a client asks "what's in this skill?", the reply left out the skill's own SKILL.md (the main instruction file) and any subfolders. The spec says the list must be complete. Fixed so the whole set comes back.
Plain: when listing a folder, the code returned a custom children structure. The spec says folders should come back as normal "resources" tagged with a folder marker (mimeType: "inode/directory"). Clients written to the spec wouldn't parse the old shape. Fixed to match the spec.
Plain: (a) one code path always returned an empty list; (b) asking for a folder that doesn't exist returned a success with nothing in it, instead of a proper "not found" error. Fixed: the empty path is wired up, and a missing folder now returns error code -32602.
Plain: three problems. It accepted any file address inside a skill rather than the skill itself. It returned the wrong error code ("method not found" instead of "invalid argument") when nothing matched. And the macro version threw away the skill's metadata after reading it. All three fixed.
Plain: each skill starts with a small block of settings (its "frontmatter"). The code used a home-made reader that flattened nested settings into a mess. Replaced with a proper reader (serde_yaml) that preserves the block exactly as written.
Plain: a skill address like skill://acme/billing/refunds/templates/x.md packs a skill name and a file path into one string, with no separator between them. Skill names can be several segments deep, so there's no way to split the address and know where the name ends. An initial attempt split at the first slash — wrong, it broke nested skills — so it was reverted.
acme/billing/refunds into just acme — silently wrong.394627d.wde-mcp-server, the WDE skill exposed over MCP) was built and driven with live requests. It immediately exposed 2 bugs in its own handlers, 1 compile-break from our DirectoryEntry→Resource rename, and 1 coverage lie in this very test suite. The suite went 10/10 → 12/12 with the two methods the header claimed but never called.ServerCapabilities.skills field + SkillsCapability struct + const SKILLS: bool in the builder macro.ServerCapabilitiesBuilderState changed every existing generic instantiation — cargo-semver-checks flagged it as a major breaking change. CI is configured for minor.enable_skills_extension() (which writes to the extensions map) instead — matching how enable_tasks() works. The macro calls .enable_skills_extension() + .enable_resources(). No public API change. Serena-verified: SkillsCapability, enable_skills_with, and the skills field no longer exist.[SkillEntry] and [SkillRouter] were unresolved (types live in rmcp, not rmcp-macros). Changed to plain text. .enable_skills() corrected to .enable_skills_extension().Plain: the best test of "does this actually work" is a real program that uses it, not a test that pretends to. So the WDE skill was turned into a real MCP server and driven with live requests over HTTP.
initialize returned capabilities.extensions["io.modelcontextprotocol/skills"] — the Option D change confirmed against a real client, not a snapshot. skills/list, skills/get, and tools/list all returned real data.DirectoryEntry → Resource rename (defect 6) broke the consumer at compile time. Every in-tree caller was updated; the one real external consumer was not, because no test watches for it.resources/directory/read returned empty (the file map was keyed by the SKILL.md address, but the request carries the skill root) and resources/read returned -32601 (never implemented). Both fixed in the consumer.resources/directory/read as covered. No test called it. Two real tests added; suite 10/10 → 12/12. Teeth proven by forcing the handler to return empty (RED: left: 0, right: 2), then restoring (GREEN).