Add client utilities for downloading skills#2948
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb5af39b7b
ℹ️ 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".
| file_path = skill_dir / file_info.path | ||
|
|
||
| # Create parent directories if needed | ||
| file_path.parent.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
Reject manifest paths that escape the target directory
The download loop trusts file_info.path from the server manifest and directly joins it with skill_dir. If a remote server returns paths like ../../.ssh/authorized_keys or absolute paths, this will write outside the intended skills folder (the subsequent mkdir/write_* calls will happily create or overwrite files). This is a security issue whenever these utilities are used against untrusted servers. Consider resolving the candidate path and enforcing is_relative_to(skill_dir) (and rejecting absolute paths) before creating directories or writing files.
Useful? React with 👍 / 👎.
|
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. WalkthroughAdds client-side skills utilities and examples for FastMCP. New module Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/fastmcp/utilities/skills.py (2)
176-179: Consider logging or warning when a file cannot be read.Silently continuing when
read_resourcereturns empty could hide server-side issues. While this keeps the function resilient, users may not realize files were skipped.Optional: Add logging for skipped files
+import logging + +logger = logging.getLogger(__name__) + # In download_skill function: if not result: + logger.warning(f"Could not read resource: {file_uri}") continue
231-239: Consider handling partial failures gracefully.If
download_skillraisesValueError(e.g., missing manifest),sync_skillsaborts entirely. Depending on intended behavior, you might want to catch and log errors for individual skills while continuing with others.Optional: Continue on individual skill failures
for skill in skills: try: path = await download_skill( client, skill.name, target_dir, overwrite=overwrite ) downloaded.append(path) except FileExistsError: # Skip existing skills when not overwriting continue + except ValueError as e: + # Log and skip skills that fail to download + logger.warning(f"Failed to download skill '{skill.name}': {e}") + continue
Adds client-side utilities for discovering and downloading skills from MCP servers that expose them via
SkillsProvider.The utilities work by reading the standard
skill://{name}/_manifestresources that SkillsProvider exposes - no special protocol needed.