Feature/issue 66 skill resource loading prototype - #116
Open
tobi-oye wants to merge 8 commits into
Open
Conversation
…discovery/loading) Builds a from-scratch MCP server + client per the merged SEP-2640 draft (resources + skill://index.json, no custom methods), exercising flat and nested skill paths, relative sub-resource references, and the "enumeration is optional but direct read must still work" baseline. Findings recorded in docs/experimental-findings.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Inline the four skill fixture files into server.mjs (the prototype demonstrates the transport binding, not filesystem loading) and drop the prototype README, whose content is already in the PR description. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Records what implementing skill:// discovery in VS Code proper surfaced: the client already had the full progressive-disclosure loop and only lacked an MCP source, its mcp-resource:// FS provider means the loading half needs no code at all, and a new skill source touches more type surfaces than expected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
experimental-ext-skills is a working-group docs repo, so a runnable Node server/client pair did not belong in it. The actual client-side implementation for issue modelcontextprotocol#66 now lives in VS Code, and what belongs here is the write-up of what that surfaced. Folds the still-transferable observations from the removed prototype (URI- vs metadata-based identification per modelcontextprotocol#54, and the index digest doc inconsistency) into the VS Code findings entry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Records what surfaced once the implementation was run from a source build against the Hugging Face MCP server, rather than only type-checked: the index wire format had already moved past the checked-in draft, per-context-computation discovery is an easy accidental DoS, a new skill source touches four type surfaces that must agree, and cross-server name collisions are unspecified by the SEP. None of the three defects were caught by tsc or the unit tests. Also corrects the previous entry, which said the work was not verified against a running server — it now is, for discovery. Model invocation remains undemonstrated and is called out as such. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Cut ~half the words to match the length of neighbouring entries and make it reviewable at a glance. Same findings, less prose. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…he digest point Issue modelcontextprotocol#66 asks explicitly for a URI- vs metadata-based identification evaluation, so that belongs in the entry. The digest note is reframed from a doc nitpick into the actionable version: the live server sends a digest and this client ignores it, so nothing verifies skill integrity. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an
experimental-findings.mdentry for issue #66 — implementingskill://discovery in VS Code and running it against a live MCP server.Docs-only. The implementation lives in tobi-oye/vscode#1; this is a docs repo, so only the write-up belongs here.
Motivation and Context
Issue #66 asks for a prototype of skill resource loading in a major open-source client. This is that prototype, plus what it taught us.
The short version: VS Code already had almost everything needed. It has the full progressive-disclosure loop — discover skills, put just their names and descriptions in the model's context, give the model a tool to load the full text on demand. The only missing piece was that a skill could never come from an MCP server. And the loading half needed no new code at all, because
mcp-resource://is already registered with VS Code's filesystem service.So the work was small. The findings are the valuable part.
Screenshots
How Has This Been Tested?
Against the live Hugging Face MCP server (
https://huggingface.co/mcp, anonymous) from a source build of VS Code:tsc --noEmitover VS Code'ssrc/— cleanskill://→mcp-resource://round trip and index parsing against verbatim live server outputNot tested: the model actually choosing to load an MCP-served skill. Source builds of VS Code can't reach the Copilot service, and the available model was a small auto-routed one. Called out as open in the entry — discovery is verified by observation, loading only by mechanism.
Breaking Changes
None. Docs-only.
Types of changes
Checklist
Additional context
Four findings that seem worth other implementers' time.
1. "Where a skill came from" is written down in more than one place.
VS Code encodes it in four: a storage enum, a parallel string union, an ext-host protocol DTO, and a proposed extension API type. All four must agree. I updated two and missed two — which produced a runtime crash that broke chat entirely, while
tscstayed perfectly green. Budget for more than a one-line enum change.2. A server doesn't have to list its skills, and it's easy to forget.
The SEP makes enumeration optional: a skill is readable by URL whether or not it appears in any index. A client that only reads
skill://index.jsonwill silently see nothing on a server that doesn't publish one. Try both the index andresources/list, and never read "index missing or empty" as "this server has no skills."3. Skill names are one global namespace, and the SEP doesn't say who wins a clash.
If a local skill and an MCP skill are both called
deploy, one disappears. Same if two connected servers both servedeploy. The SEP ties a skill's URL to its name but says nothing about precedence, so every host invents its own rule. I chose local-over-MCP — a server shouldn't be able to shadow a skill on your disk — but server-vs-server ends up as whatever order discovery happened to run in. This seems worth stating explicitly in the SEP.4. Re-running discovery on every context computation is an accidental DoS.
findAgentSkills()runs each time the model's context is rebuilt, and discovery costs two round trips per server. A naive version issued 20 index reads for a single chat turn. That's the same shape as the incident behind hf-mcp-server's client denylist (#164, ~100k req/min). Cache the promise, not just the result, and key it on connection state — keying on server id alone pins an empty result from a lookup made while the server was stopped.