-
Notifications
You must be signed in to change notification settings - Fork 100
feat: support .mcp.json for MCP server configuration #1481
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Add support for AgentSkills standard fields (https://agentskills.io/specification): - description: Brief description of what the skill does - license: License under which the skill is distributed - compatibility: Environment requirements or compatibility notes - metadata: Arbitrary key-value metadata for extensibility - allowed_tools: List of pre-approved tools for the skill Also adds skills-ref as an optional dependency for future validation and prompt generation utilities. Closes #1474 Co-authored-by: openhands <openhands@all-hands.dev>
The skills-ref library will be added when validation and prompt generation utilities are implemented (issue #1478). Co-authored-by: openhands <openhands@all-hands.dev>
- Add find_skill_md() function to locate SKILL.md files (case-insensitive) - Add validate_skill_name() function for AgentSkills spec validation - Update load_skills_from_dir() to support skill-name/SKILL.md directories - Add directory_name and validate_name parameters to Skill.load() - Export new functions from __init__.py - Add 27 unit tests for new functionality Closes #1475 Co-authored-by: openhands <openhands@all-hands.dev>
Coverage Report •
|
||||||||||||||||||||
Reduce test code while maintaining essential coverage. Co-authored-by: openhands <openhands@all-hands.dev>
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
Resolved merge conflicts in: - openhands-sdk/openhands/sdk/context/skills/skill.py - tests/sdk/context/skill/test_agentskills_fields.py The resolution keeps both: 1. Pydantic field validators for allowed_tools and metadata from main 2. Skill name validation logic from this branch 3. SKILL.md convention support from this branch Co-authored-by: openhands <openhands@all-hands.dev>
Extract helper functions to simplify the load_skills_from_dir function: - _find_third_party_files: Find .cursorrules, AGENTS.md, etc. in repo root - _find_skill_md_directories: Find AgentSkills-style SKILL.md directories - _find_regular_md_files: Find regular .md files excluding SKILL.md dirs - _load_skill_safe: Load skills with consistent error handling This improves code readability and maintainability by following the single responsibility principle. Each helper function handles one specific aspect of skill discovery or loading. Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
- Remove redundant tuple element from _find_skill_md_directories (directory_path can be derived from skill_md.parent) - Replace _load_skill_safe wrapper with _load_and_categorize that combines loading and categorization in one function - Auto-validate skill name when directory_name is provided (removed separate validate_name parameter) - Fix case-insensitive search for third-party files to iterate over all files instead of checking specific variants - Update tests to check for specific error messages instead of magic number assertions Co-authored-by: openhands <openhands@all-hands.dev>
SKILL.md directories should always be categorized as knowledge_skills (progressive loading), not repo_skills (permanent context), even when they have no triggers defined. This addresses enyst's feedback that AgentSkills are fundamentally different from permanent OH skills like repo.md - they should use progressive loading while permanent skills should use AGENTS.md. Changes: - Modified _load_and_categorize to always put SKILL.md files in knowledge_skills when directory_name is provided - Added test_skill_md_always_knowledge_skill to verify the behavior Co-authored-by: openhands <openhands@all-hands.dev>
The file_content parameter was only used in unit tests. Tests have been updated to use pytest's tmp_path fixture and write content to actual temp files instead. Co-authored-by: openhands <openhands@all-hands.dev>
…gacy formats Split load() into: - _load_agentskills_skill(): For SKILL.md files (AgentSkills format) - _load_legacy_openhands_skill(): For legacy OpenHands skill files - _create_skill_from_metadata(): Shared helper for Skill object creation Co-authored-by: openhands <openhands@all-hands.dev>
- load_skills_from_dir() now returns 3 dictionaries: repo_skills, knowledge_skills, agent_skills - AgentSkills (SKILL.md directories) are categorized into agent_skills (separate from OpenHands skills) - Updated all callers to handle the new return type - Updated tests to verify the new categorization This addresses enyst's review comment about keeping AgentSkills separate from OpenHands skills, as they follow different standards and loading patterns. Co-authored-by: openhands <openhands@all-hands.dev>
|
@OpenHands pull in the changes from #1480, re-do the PR description to say it is based on 1480, and only search for and load |
|
I'm on it! neubig can track my progress at all-hands.dev |
b3345e4 to
1bc5f51
Compare
SummaryI've completed all the requested changes for PR #1481: Checklist
Changes Made (1 commit on top of PR #1480)Files modified:
Key implementation details:
All 91 tests pass. PR updated at: #1481 |
1bc5f51 to
ffb6c6a
Compare
Add support for .mcp.json files in AgentSkills directories (SKILL.md format),
following the AgentSkills standard for MCP server configuration.
Changes:
- Add _find_mcp_config() to locate .mcp.json files in skill directories
- Add _expand_mcp_variables() for variable expansion (${VAR}, ${VAR:-default})
- Add _load_mcp_config() to load and validate .mcp.json files
- Update _load_agentskills_skill() to load .mcp.json (agent_skills only)
- Update _load_legacy_openhands_skill() to load mcp_tools from frontmatter
MCP loading rules:
- AgentSkills (SKILL.md): ONLY use .mcp.json, ignore mcp_tools frontmatter
- Legacy skills (.md): ONLY use mcp_tools frontmatter, no .mcp.json support
Co-authored-by: openhands <openhands@all-hands.dev>
ffb6c6a to
e22f6c5
Compare
|
@OpenHands resolve merge conflicts with main, favoring the changes made on main |
|
I'm on it! neubig can track my progress at all-hands.dev |
Resolved merge conflicts by keeping the .mcp.json loading functionality from this branch while incorporating main's changes (PR #1480). Key changes preserved: - AgentSkills (SKILL.md) load .mcp.json with variable expansion - Legacy skills load mcp_tools from frontmatter only - _find_mcp_config, _expand_mcp_variables, _load_mcp_config functions Co-authored-by: openhands <openhands@all-hands.dev>
3191785 to
bfc17a7
Compare
Use directory_name consistently as in main branch. Co-authored-by: openhands <openhands@all-hands.dev>
|
|
||
| return cls._create_skill_from_metadata(agent_name, content, path, metadata_dict) | ||
| # Load MCP configuration from .mcp.json (agent_skills ONLY use .mcp.json) | ||
| mcp_tools: dict | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any mcp_tools here?
- we are here in the method loading
AgentSkills - while it seems that
mcp_toolsis a field in "repo.md" -type Skills?if not isinstance(mcp_tools, dict | None):
Summary
This PR adds support for
.mcp.jsonfiles in AgentSkills directories (SKILL.md format), following the AgentSkills standard for MCP server configuration.NOTE: until 1480 is merged, read the diff below instead of the "files changed" tab.
Based on PR #1480 (SKILL.md convention support) - View diff from #1480
Changes
MCP Loading Rules
.mcp.jsonfiles, ignoremcp_toolsfrontmattermcp_toolsfrom frontmatter, no.mcp.jsonsupportNew Private Functions
_find_mcp_config()- Locates.mcp.jsonfiles in a skill directory_expand_mcp_variables()- Expands variables in MCP configuration:${VAR}- Environment variable or provided variable${VAR:-default}- With default value if not found${SKILL_ROOT}- Automatically set to the skill directory path_load_mcp_config()- Loads and validates.mcp.jsonfilesExample
.mcp.json{ "mcpServers": { "pdf-processor": { "command": "${SKILL_ROOT}/scripts/server.py", "args": ["--port", "${PORT:-8080}"] } } }Testing
Verified with a realistic test skill directory:
Test output:
Added 6 tests covering:
.mcp.jsonwith variable expansionmcp_toolsfrontmattermcp_toolsfrom frontmatter.mcp.jsonin directoryload_skills_from_dir()with mixed skill typesBackward Compatibility
mcp_toolsfrontmatter continue to work.mcp.jsonis only loaded for AgentSkills (SKILL.md directory format)Related Issues
Closes #1476
Part of #1473 (Support AgentSkills standard)
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:9669acc-pythonRun
All tags pushed for this build
About Multi-Architecture Support
9669acc-python) is a multi-arch manifest supporting both amd64 and arm649669acc-python-amd64) are also available if needed