Repository navigation
Add SkillCatalog and FilesystemSkillProvider for serving skills from a directory - #3795
vijaydeepsinha wants to merge 20 commits into
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 13 files
Reply to a comment to ask cubic a question or push back. It learns from your replies.
Re-trigger cubic
There was a problem hiding this comment.
4 issues found across 13 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/server/skills/test_filesystem.py">
<violation number="1" location="tests/server/skills/test_filesystem.py:327">
P2: This fixture is a regular file, so it only tests the mocked `fstat` rejection and cannot catch removal of `O_NONBLOCK`, which would make opening a real FIFO block. Keep a POSIX FIFO test alongside this portable descriptor test.</violation>
</file>
<file name="docs/advanced/skills.md">
<violation number="1" location="docs/advanced/skills.md:258">
P2: This paragraph presents SSE as a supported transport for the refresh timer, but the transport docs call SSE superseded. Remove the SSE-specific transport guidance rather than adding new usage documentation for it.
(Based on your team's feedback about avoiding new SSE usage documentation.)</violation>
</file>
<file name="docs/advanced/extensions.md">
<violation number="1" location="docs/advanced/extensions.md:139">
P3: `resources()` is not called for every lookup: direct server resources win immediately, and an earlier dynamic provider can satisfy the URI before this extension runs. Qualify this as lookups that reach the provider.</violation>
</file>
<file name="src/mcp/server/mcpserver/resources/resource_manager.py">
<violation number="1" location="src/mcp/server/mcpserver/resources/resource_manager.py:38">
P2: This cache keeps the old URI key when a provider reuses its tuple but mutates a contained `Resource.uri`, so lookups by the new URI fail and the old URI still resolves. Track the URIs used to build the index and rebuild it when any contained URI changes.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| def by_uri(self) -> Mapping[str, Resource]: | ||
| resources = self._provider() | ||
| if resources is self._indexed: |
There was a problem hiding this comment.
P2: This cache keeps the old URI key when a provider reuses its tuple but mutates a contained Resource.uri, so lookups by the new URI fail and the old URI still resolves. Track the URIs used to build the index and rebuild it when any contained URI changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/mcp/server/mcpserver/resources/resource_manager.py, line 38:
<comment>This cache keeps the old URI key when a provider reuses its tuple but mutates a contained `Resource.uri`, so lookups by the new URI fail and the old URI still resolves. Track the URIs used to build the index and rebuild it when any contained URI changes.</comment>
<file context>
@@ -25,12 +25,34 @@
+
+ def by_uri(self) -> Mapping[str, Resource]:
+ resources = self._provider()
+ if resources is self._indexed:
+ return self._by_uri
+ by_uri: dict[str, Resource] = {}
</file context>
There was a problem hiding this comment.
Valid point, but it can only happen if a custom extension deliberately changes a resource's uri in place inside a tuple it keeps returning. It can't happen with refresh: every reload builds a new tuple of new resources, so the index is rebuilt. I've documented the rule in add_resource_provider and the extensions guide (return a new tuple instead of mutating) and added a unit test for this case as well.
Fixes #3683
Adds an optional helper that serves a directory of skills through the existing
Skillsextension, without changing the protocol or theSkillsAPI.mcp/server/skills.pybecomes a package;from mcp.server.skills import Skillsand the handler aliases still work.Motivation and Context
Serving skills from disk currently means writing handlers and registering every file as a resource by hand. This adds:
SkillProvider: a small protocol for listing and reading files.FilesystemSkillProvider: a local-directory provider that skips symlinks and junctions and runs blocking I/O in a worker thread.SkillCatalog: aSkillsextension that scans a provider, validates each skill, and answersskills/list,skills/getand optionallyresources/directory/read. Manifest digests always match whatresources/readserves, and a file that changed after the last scan is refused.SkillCatalog.reload(): rescans the provider and swaps the new catalog in all at once, returning the skills added, removed and changed. If the scan fails, the previous catalog keeps serving; concurrent calls run one after another.auto_refresh=True(off by default) withrefresh_interval(seconds, default 60): reloads on a timer while the server runs, serving the bytes it scanned so manifests and content stay consistent between refreshes. A failed refresh is logged and the previous catalog keeps serving.Optional pagination (
page_size) and SEP-2549 cache hints (ttl_ms,cache_scope) are supported.To let a catalog change while the server runs,
Extensiongains two optional hooks, both off by default:resources_are_dynamic = True: the server asksresources()again on each listing and lookup instead of copying it once. Returning the same tuple until something changes lets lookups reuse a URI index. Resources registered directly on the server still win URI collisions.lifespan(): an async context manager entered inside the application's lifespan, used for the refresh timer.Server usage
How Has This Been Tested?
All run locally on macOS
./scripts/test, twice: 6,377 passed, 100.00% statement and branch coverage,strict-no-coverclean.lowest-directresolution on 3.11, andMCP_EXAMPLES_SMOKE=1on 3.12: all passed.ruff format,ruff check,pyright,pre-commiton the changed files, README snippet check,gen_surface_types.py --check, the standalonemcp-typesimport check, and the Node workflow tests: all clean.scripts/docs/build.sh: every language built with 0 warnings.2025-11-25and the2026-07-28protocol versions: list/get/read, pagination, directory reads enabled and disabled, and auto-refresh picking up skills added and removed on disk without a restart.Conformance
CI's pinned harness (
@modelcontextprotocol/conformance@0.2.0-alpha.11), all suites, run exactly asconformance.ymldoes:2025-11-252026-07-28The baseline check passes on every leg.
The pinned harness has no SEP-2640 scenarios, so I also ran them from the conformance repository (modelcontextprotocol/conformance#330), against servers built with
SkillCatalog+FilesystemSkillProvider, one withauto_refresh=Trueand one withpage_size=2:2026-07-282025-11-25(--force)sep-2640-skills-enumerationsep-2640-skills-manifestsep-2640-skills-directoryThe four
sep-2640-client-*scenarios pass 1/1 each.Breaking Changes
None. Existing public signatures and defaults are unchanged; the new
SkillCatalogoptions and theExtensionhooks are optional and off by default.Types of changes
Checklist
Additional context
resources/list_changedafter a reload could be a follow-up if wanted.