Add Skills Provider for exposing agent skills as MCP resources#2944
Add Skills Provider for exposing agent skills as MCP resources#2944
Conversation
marvin-context-protocol
Bot
added
feature
jlowin
force-pushed
the
skills-provider
branch
from
57b797a to
a00683d
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57b797af44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| Note: task_meta is ignored - this template doesn't support background tasks. | ||
| """ | ||
| # Call read() directly and convert to ResourceResult | ||
| result = await self.read(arguments=params) |
There was a problem hiding this comment.
The overridden _read explicitly ignores task_meta and directly returns a synchronous ResourceResult. When a client sends task metadata for resources/read (SEP-1686), the server passes that metadata into ResourceTemplate._read, which normally enforces task_config (e.g., raising for mode="forbidden" or returning a CreateTaskResult). Here, those checks are bypassed, so task-augmented requests to skill://…/{path*} will return an immediate result instead of an error or task handle, breaking the task contract and allowing callers to bypass mode="forbidden"/required behavior.
Useful? React with 👍 / 👎.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (16)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request introduces a comprehensive Skills Provider system for FastMCP, enabling exposure of skill directories as MCP resources. The implementation includes a core SkillProvider class for single-skill directories, a SkillsDirectoryProvider for aggregating multiple skills from one or more root directories, vendor-specific providers for Claude, Cursor, VSCode, and other tools, and supporting utilities for frontmatter parsing and file scanning. The change adds new public exports to the providers package and includes documentation and example code demonstrating server and client usage with sample skill definitions for PDF processing and code review tasks. Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
examples/skills/server.py (1)
23-46: Make optional provider examples copy‑paste ready.
Uncommenting Options 1/3 currently requires manual imports; a short note avoids NameError surprises.💡 Optional tweak
-# Option 1: Load a single skill -# mcp.add_provider(SkillProvider(Path.home() / ".claude/skills/pdf-processing")) +# Option 1: Load a single skill (add SkillProvider to the imports if you uncomment) +# mcp.add_provider(SkillProvider(Path.home() / ".claude/skills/pdf-processing")) -# Option 3: Load skills from a platform's default location -# mcp.add_provider(ClaudeSkillsProvider()) # ~/.claude/skills/ +# Option 3: Load skills from a platform's default location +# (add ClaudeSkillsProvider to the imports if you uncomment) +# mcp.add_provider(ClaudeSkillsProvider()) # ~/.claude/skills/docs/servers/providers/skills.mdx (1)
20-48: Convert Quick Start into a Steps-based, runnable procedure.This section reads as a procedure but isn’t structured with a
<Steps>component and doesn’t show a fully runnable flow with expected outcomes/error handling. Consider converting it to explicit steps and including a minimal runnable example (with basic error handling and expected output). As per coding guidelines, keep procedures in Steps and include runnable examples with outcomes.src/fastmcp/server/providers/skills/_common.py (1)
86-99: Make manifest ordering deterministic.
rglob()order can vary by filesystem, which can lead to unstable manifests and tests. Consider sorting by relative path before building the list.Proposed fix
- for file_path in skill_dir.rglob("*"): + for file_path in sorted(skill_dir.rglob("*")): if file_path.is_file(): rel_path = file_path.relative_to(skill_dir) files.append( SkillFileInfo( path=str(rel_path), size=file_path.stat().st_size, hash=compute_file_hash(file_path), ) )src/fastmcp/server/providers/skills/directory_provider.py (1)
105-114: Consider catching broader exceptions during provider creation.
SkillProvider.__init__can raise exceptions beyondFileNotFoundError(e.g.,PermissionError,OSErrorfor I/O issues, or exceptions fromparse_frontmatter). Currently onlyFileNotFoundErroris caught, which could cause a single problematic skill to prevent loading all subsequent skills in the loop.♻️ Suggested improvement
try: provider = SkillProvider( skill_path=skill_dir, main_file_name=self._main_file_name, supporting_files=self._supporting_files, ) self.providers.append(provider) seen_skill_names.add(skill_name) - except FileNotFoundError: + except (FileNotFoundError, PermissionError, OSError) as e: logger.exception(f"Failed to load skill: {skill_dir.name}")src/fastmcp/server/providers/skills/skill_provider.py (2)
68-80: Path traversal protection is correctly implemented.The security checks using
resolve()andis_relative_to()effectively prevent directory traversal attacks. However, the exception handling can be simplified since theValueErrorfromis_relative_to()is already meaningful.♻️ Optional simplification
# Security: ensure path doesn't escape skill directory - try: - full_path = full_path.resolve() - if not full_path.is_relative_to(self.skill_info.path): - raise ValueError(f"Path {file_path} escapes skill directory") - except ValueError as e: - raise ValueError(f"Invalid path: {e}") from e + full_path = full_path.resolve() + if not full_path.is_relative_to(self.skill_info.path): + raise ValueError(f"Path {file_path} escapes skill directory")
131-147: Consider extracting shared file-reading logic.The path validation and content reading logic is duplicated between
SkillFileTemplate.read()(lines 68-87) andSkillFileResource.read()(lines 133-147). A shared helper method could reduce duplication.
| class VSCodeSkillsProvider(SkillsDirectoryProvider): | ||
| """VS Code skills from ~/.copilot/skills/.""" | ||
|
|
||
| def __init__( | ||
| self, | ||
| reload: bool = False, | ||
| supporting_files: Literal["template", "resources"] = "template", | ||
| ) -> None: | ||
| root = Path.home() / ".copilot" / "skills" | ||
|
|
||
| super().__init__( | ||
| roots=[root], | ||
| reload=reload, | ||
| main_file_name="SKILL.md", | ||
| supporting_files=supporting_files, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/fastmcp/server/providers/skills/vendor_providers.pyRepository: jlowin/fastmcp
Length of output: 5001
VSCodeSkillsProvider and CopilotSkillsProvider use identical paths.
Both providers point to ~/.copilot/skills/ (lines 37 and 117). If both are added to a server, they'll create duplicate resources for the same directory. Either:
- Use a distinct path for VS Code (e.g.,
~/.vscode/skills/) - Document explicitly in the docstrings that VS Code shares GitHub Copilot's skills directory (clarify whether this is intentional)
Test Failure AnalysisSummary: Windows-specific test failures due to path separator inconsistencies. The code generates Windows-style backslashes ( Root Cause: In Suggested Solution: Normalize path separators to forward slashes in # In src/fastmcp/server/providers/skills/_common.py, line 94
path=str(rel_path).replace("\\\\", "/"), # Normalize to forward slashesOr use path=rel_path.as_posix(),This will make paths consistent across all platforms since MCP URIs use forward slashes. Detailed AnalysisPrimary Issue: Path SeparatorsThe tests are failing with assertions like: Expected: The issue originates in def scan_skill_files(skill_dir: Path) -> list[SkillFileInfo]:
files = []
for file_path in skill_dir.rglob("*"):
if file_path.is_file():
rel_path = file_path.relative_to(skill_dir)
files.append(
SkillFileInfo(
path=str(rel_path), # <-- Problem: Windows uses backslashes
size=file_path.stat().st_size,
hash=compute_file_hash(file_path),
)
)
return filesSecondary Issue: Absolute Path Conversion on WindowsTests that mock However, this is primarily a test issue, not a code issue. The path normalization fix above will resolve the main functionality problems. Failed Tests
Related Files
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/fastmcp/server/providers/skills/vendor_providers.py (1)
29-44:VSCodeSkillsProviderandCopilotSkillsProvidershare identical root path.Both providers point to
~/.copilot/skills/(lines 37 and 117). If both are added to a server, they'll expose duplicate resources for the same directory.Either use a distinct path for VS Code (e.g.,
~/.vscode/skills/) or document explicitly that this is intentional (VS Code shares GitHub Copilot's skills directory).Also applies to: 109-124
🧹 Nitpick comments (5)
src/fastmcp/server/providers/skills/_common.py (1)
33-74: Custom YAML parser has limited coverage — acceptable for simple frontmatter but may fail on edge cases.The parser handles single-level key-value pairs, quoted strings, and inline arrays, which is sufficient for typical frontmatter. However, it doesn't support:
- Multi-line values
- Nested objects
- Block arrays (YAML list syntax with
-)- Boolean/numeric type coercion
This is acceptable given the comment at line 53 noting "simple key: value parsing, no complex types." Consider documenting these limitations in the docstring or falling back to a YAML library if more complex frontmatter is expected.
src/fastmcp/server/providers/skills/skill_provider.py (2)
38-44: Synchronous file I/O in async method may block the event loop.
main_file_path.read_text()at line 44 performs synchronous file I/O within an async method. For small skill files this is likely acceptable, but for larger files or high-concurrency scenarios, this could block the event loop.Consider wrapping in
asyncio.to_thread()if performance becomes a concern, though this is optional for the typical use case.
307-358:versionparameter is accepted but unused.The
versionparameter in_get_resourceis never used, which the static analysis correctly flags (ARG002). Since versioning may be relevant for future resource management, consider either:
- Passing it to the base class method if applicable
- Documenting that versioning is not supported for skill resources
This is minor since the base
Provider._get_resourcesignature requires this parameter.src/fastmcp/server/providers/skills/directory_provider.py (1)
76-120: Synchronous discovery called from async context may block event loop.
_discover_skills()performs synchronous filesystem operations (root.exists(),root.iterdir(),main_file.exists()) and is called from async methods via_ensure_discovered(). Withreload=True, this runs on every request and could block the event loop.For directories with many skills, consider running discovery in a thread:
async def _ensure_discovered(self) -> None: if self._reload or not self._discovered: await asyncio.to_thread(self._discover_skills)This is a recommended improvement for production use with reload enabled.
src/fastmcp/server/providers/skills/__init__.py (1)
43-44: Clarify the purpose of the "backwards compatibility" alias.Since this is a new module being introduced in this PR, the "backwards compatibility alias" comment may be misleading. If
SkillsProviderwas used during development or in early documentation, consider adding a brief note explaining when/why this alias exists. Otherwise, if this is purely for API convenience (offering both singular and plural naming), consider updating the comment to reflect that intent.📝 Suggested comment clarification
-# Backwards compatibility alias +# Convenience alias: SkillsProvider is an alias for SkillsDirectoryProvider SkillsProvider = SkillsDirectoryProvider
| async def _read( # type: ignore[override] | ||
| self, | ||
| uri: str, | ||
| params: dict[str, Any], | ||
| task_meta: Any = None, | ||
| ) -> ResourceResult: | ||
| """Server entry point - read file directly without creating ephemeral resource. | ||
|
|
||
| Note: task_meta is ignored - this template doesn't support background tasks. | ||
| """ | ||
| # Call read() directly and convert to ResourceResult | ||
| result = await self.read(arguments=params) | ||
| return self.convert_result(result) |
There was a problem hiding this comment.
Ignoring task_meta bypasses task routing contract.
The _read override explicitly ignores task_meta (as noted in the docstring), which means requests with task metadata will receive immediate results instead of task handles. This bypasses the task contract where mode="required" should enforce background execution.
If background task support is intentionally unsupported for file templates, consider setting task_config explicitly to mode="forbidden" on the template to signal this clearly, rather than silently ignoring the parameter.
🧰 Tools
🪛 Ruff (0.14.13)
91-91: Unused method argument: uri
(ARG002)
93-93: Unused method argument: task_meta
(ARG002)
Test Failure AnalysisSummary: Tests are failing on Windows due to (1) not recognizing files as text/markdown on Windows, and (2) path mocking issues with Unix-style paths being converted to Windows drive-letter paths. Root Cause:
Suggested Solution: Fix #1: Ensure .md files are recognized as text/markdownIn import mimetypes
# Ensure .md is recognized as text/markdown on all platforms
mimetypes.add_type('text/markdown', '.md')This should be added near the top of the file, after the imports (around line 24). Fix #2: Fix path mocking in testsFor the path comparison tests, use Option A (Preferred - use tmp_path): def test_default_root_is_claude_skills_dir(self, tmp_path, monkeypatch):
monkeypatch.setattr(Path, "home", lambda: tmp_path)
provider = ClaudeSkillsProvider()
assert provider._roots == [tmp_path / ".claude" / "skills"]Option B (Use platform-aware mock): def test_default_root_is_claude_skills_dir(self, monkeypatch):
import sys
if sys.platform == "win32":
fake_home = Path("C:/fake/home")
else:
fake_home = Path("/fake/home")
monkeypatch.setattr(Path, "home", lambda: fake_home)
provider = ClaudeSkillsProvider()
assert provider._roots == [fake_home / ".claude" / "skills"]Detailed AnalysisFailure 1: BlobResourceContents vs TextResourceContentsFrom the logs: The issue is in mime_type, _ = mimetypes.guess_type(str(full_path))
if mime_type and mime_type.startswith("text/"):
return full_path.read_text()
else:
return full_path.read_bytes() # Returns bytes -> BlobResourceContentsOn Windows, Failure 2: Path Comparison IssuesFrom the logs: The test creates Related Files
Sources: |
Test Failure AnalysisSummary: Tests are failing on Windows due to (1) Root Cause:
Suggested Solution: Fix 1: Ensure .md files are recognized as text/markdownIn import mimetypes
# Ensure .md is recognized as text/markdown on all platforms
mimetypes.add_type('text/markdown', '.md')Fix 2: Fix path mocking in testsFor the path comparison tests, use def test_default_root_is_claude_skills_dir(self, tmp_path, monkeypatch):
monkeypatch.setattr(Path, "home", lambda: tmp_path)
provider = ClaudeSkillsProvider()
assert provider._roots == [tmp_path / ".claude" / "skills"]Detailed AnalysisFailure 1: BlobResourceContents vs TextResourceContentsFrom the logs, the test expects TextResourceContents but receives BlobResourceContents. The issue is in mime_type, _ = mimetypes.guess_type(str(full_path))
if mime_type and mime_type.startswith("text/"):
return full_path.read_text()
else:
return full_path.read_bytes() # Returns bytes which becomes BlobResourceContentsOn Windows, Failure 2: Path Comparison IssuesFrom the logs: The test creates Related Files
Sources: |
Two-layer architecture for skill discovery: - SkillProvider: handles a single skill folder - SkillsDirectoryProvider: scans directory, creates SkillProvider per folder - ClaudeSkillsProvider: convenience subclass for ~/.claude/skills/ Skills expose main file + manifest as resources, with supporting files accessible via ResourceTemplate (default) or as individual Resources (configurable via supporting_files parameter).
- SkillsDirectoryProvider now accepts roots: Path | list[Path] - Add vendor providers: Cursor, VS Code, Codex, Gemini, Goose, Copilot, OpenCode - Add documentation at docs/servers/providers/skills.mdx - Update examples to showcase vendor providers and multi-directory usage
- Use as_posix() for cross-platform path consistency in manifests - Sort rglob() results for deterministic ordering - Catch PermissionError and OSError in addition to FileNotFoundError - Fix documentation examples to use Path.home() instead of ~ strings - Fix examples to use roots= parameter instead of root=
jlowin
force-pushed
the
skills-provider
branch
from
a961d7a to
a6c1c7c
Compare
Test Failure AnalysisSummary: Tests are failing on Windows due to two platform-specific issues: path resolution differences and MIME type detection for text files. Root Cause:
Suggested Solution:
Detailed AnalysisFailed TestsPath resolution failures (7 tests):
Error pattern: MIME type detection failures (3 tests):
Error pattern: Code ReferencesPath resolution happens in self._roots = [Path(r).resolve() for r in roots]MIME type detection in mime_type, _ = mimetypes.guess_type(str(full_path))
if mime_type and mime_type.startswith("text/"):
return full_path.read_text()
else:
return full_path.read_bytes()Related Files
|
1 participant
Agent skills (directories with instruction files like
SKILL.md) are used by Claude Code, Cursor, VS Code Copilot, and other AI tools. This PR adds providers that expose these skill directories as MCP resources, enabling skill discovery and sharing across different tools and clients.Architecture
Two-layer design for flexibility:
Each skill exposes:
skill://name/SKILL.md- Main instruction fileskill://name/_manifest- JSON listing all files with hashesskill://name/{path}- Supporting files (via template or as resources)Vendor Providers
Platform-specific providers with locked default paths:
ClaudeSkillsProvider~/.claude/skills/CursorSkillsProvider~/.cursor/skills/VSCodeSkillsProvider~/.copilot/skills/CodexSkillsProvider~/.codex/skills/GeminiSkillsProvider~/.gemini/skills/GooseSkillsProvider~/.config/agents/skills/CopilotSkillsProvider~/.copilot/skills/OpenCodeSkillsProvider~/.config/opencode/skills/Configuration
The
supporting_filesparameter controls how non-main files are exposed:"template"(default): Hidden fromlist_resources(), accessed via ResourceTemplate"resources": All files visible as individual Resources