feat: add to_prompt() for XML skill prompt generation #1483
+162
−0
Conversation
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
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>
- Add find_mcp_config() function to locate .mcp.json files - Add expand_mcp_variables() for variable expansion (, default) - Add load_mcp_config() to parse and validate .mcp.json files - Add mcp_config_path field to Skill model - Update Skill.load() to auto-load .mcp.json from SKILL.md directories - .mcp.json takes precedence over mcp_tools frontmatter - Support variable expansion - Export new functions from __init__.py - Add 19 unit tests for new functionality Closes #1476 Co-authored-by: openhands <openhands@all-hands.dev>
- Add SkillResources model to track resource directories - Add discover_skill_resources() function to scan for resources - Add RESOURCE_DIRECTORIES constant for standard directory names - Add resources field to Skill model - Update Skill.load() to auto-discover resources from SKILL.md directories - Export new classes and functions from __init__.py - Add 21 unit tests for new functionality Closes #1477 Co-authored-by: openhands <openhands@all-hands.dev>
- Enhance SkillValidationError with errors list - Add validate_skill() function for skill directory validation - Add to_prompt() function for XML prompt generation - Export new functions from __init__.py - Add 24 unit tests for new functionality Closes #1478 Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
Coverage Report •
|
||||||||||||||||||||
Reduce test code by 74% while maintaining essential coverage: - test_agentskills_fields.py: 299 → 73 lines - test_skill_md_convention.py: 405 → 86 lines - test_mcp_json_config.py: 301 → 89 lines - test_resource_directories.py: 300 → 83 lines - test_validation_prompt.py: 332 → 101 lines Total: 1637 → 432 lines Co-authored-by: openhands <openhands@all-hands.dev>
Reduce test code while maintaining essential coverage. Co-authored-by: openhands <openhands@all-hands.dev>
Reduce test code while maintaining essential coverage. Co-authored-by: openhands <openhands@all-hands.dev>
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>
- Merged field validators from main (_parse_allowed_tools, _convert_metadata_values) - Kept new fields (mcp_config_path, resources) from the PR branch - Updated tests to use Pydantic ValidationError for description validation - Removed redundant _parse_agentskills_fields method (functionality moved to validators) 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 |
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>
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>
Contributor
Author
|
@OpenHands resolve merge conflicts with main |
|
I'm on it! neubig can track my progress at all-hands.dev |
Resolved merge conflicts: - openhands-sdk/openhands/sdk/context/skills/skill.py: Integrated resource directories and MCP config support with main's refactored Skill.load() API - tests/sdk/context/skill/test_agentskills_fields.py: Used main's test structure - tests/sdk/context/skill/test_skill_md_convention.py: Updated imports to use public functions (find_skill_md, validate_skill_name) - tests/sdk/context/skill/test_mcp_json_config.py: Updated to use new API - tests/sdk/context/skill/test_resource_directories.py: Updated to use new API Co-authored-by: openhands <openhands@all-hands.dev>
Merged changes from main branch (fd98c16) which refactored Skill.load() to use helper methods (_load_agentskills_skill, _load_legacy_openhands_skill, _create_skill_from_metadata) and added agent_skills as a third category. Key changes: - Integrated validation and prompt utilities with main's refactored structure - Made find_skill_md and validate_skill_name public functions (exported from __init__.py) - Added mcp_config_path and resources fields to Skill model - Updated _load_agentskills_skill to load MCP config and resources - Updated _create_skill_from_metadata to accept mcp_tools, mcp_config_path, resources - Updated tests to use new API (removed directory_name parameter) Co-authored-by: openhands <openhands@all-hands.dev>
Summary: Merge Conflicts ResolvedI successfully resolved the merge conflicts between the Conflicts Resolved
Key Integration Changes
Verification
The PR #1483 is now updated and ready for review. |
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>
|
I'm on it! neubig can track my progress at all-hands.dev |
Co-authored-by: openhands <openhands@all-hands.dev>
SummaryI've successfully completed all tasks for PR #1483: ✅ Checklist
Changes SummaryThe PR adds validation and prompt generation utilities for skills:
CI StatusAll checks passed:
PR link: #1483 |
YAGNI - validation is already handled by Skill.load() and Pydantic validators. Keep only to_prompt() for XML prompt generation. Co-authored-by: openhands <openhands@all-hands.dev>
neubig
changed the title
Remove SkillValidationError enhancement and validate_skill_name export. These will be part of a separate PR for Pydantic validation improvements. Co-authored-by: openhands <openhands@all-hands.dev>
neubig
changed the title
Co-authored-by: openhands <openhands@all-hands.dev>
xingyaoww
reviewed
Jan 6, 2026
Contributor
Author
|
@OpenHands reflect xingyaoww's two comments |
|
I'm on it! neubig can track my progress at all-hands.dev |
- Show 'no available skills' message when skills list is empty - Add truncation indicator with character count when description exceeds limit - Include source file path in truncation message for reference - Add max_description_length parameter for configurability Co-authored-by: openhands <openhands@all-hands.dev>
SummaryI've addressed xingyaoww's two review comments on PR #1483: Changes Made1. Empty skills message (Comment on line 801)
2. Truncation indicator with source reference (Comment on line 813)
Example OutputFor a skill with a 250-character description and source path: <skill name="test">AAAA... [50 characters truncated. View /path/to/skill.md for complete information]</skill>Files Changed
Verification
|
xingyaoww
reviewed
Jan 6, 2026
xingyaoww
reviewed
Jan 6, 2026
Contributor
Author
|
@OpenHands reflect xingyaoww's most recent comments. |
|
I'm on it! neubig can track my progress at all-hands.dev |
…truncated Address xingyaoww's review comments: 1. Use Python standard library xml.sax.saxutils.escape for XML escaping instead of custom implementation 2. Count content after the description line as truncated characters when using content fallback (not just when description exceeds max length) Co-authored-by: openhands <openhands@all-hands.dev>
SummaryI addressed xingyaoww's most recent review comments on PR #1483: Changes Made
Files Changed
Verification
The changes have been pushed to |
Remove trivial wrapper function and use xml_escape directly. Co-authored-by: openhands <openhands@all-hands.dev>
xingyaoww
reviewed
Jan 6, 2026
Comment on lines
+811
to
+824
| # Extract first non-empty, non-header line from content as fallback | ||
| # Track position to calculate truncated content after the description | ||
| chars_before_desc = 0 | ||
| for line in skill.content.split("\n"): | ||
| stripped = line.strip() | ||
| # Skip markdown headers and empty lines | ||
| if not stripped or stripped.startswith("#"): | ||
| chars_before_desc += len(line) + 1 # +1 for newline | ||
| continue | ||
| description = stripped | ||
| # Calculate remaining content after this line as truncated | ||
| desc_end_pos = chars_before_desc + len(line) | ||
| content_truncated = max(0, len(skill.content) - desc_end_pos) | ||
| break |
There was a problem hiding this comment.
I'm on it! xingyaoww can track my progress at all-hands.dev
There was a problem hiding this comment.
Summary
I provided a Linus Torvalds-style code review for PR #1483 (feat: add to_prompt() for XML skill prompt generation) as requested with the /codereview-roasted command.
Review Verdict: 🟡 Acceptable - Worth merging
Key Findings:
-
Core logic is sound - The XML generation works correctly and uses the standard library (
xml.sax.saxutils.escape) for escaping, which is good. -
Main improvement opportunity (Lines 809-824): The manual character position tracking for content fallback is fragile and over-engineered. The code tracks
chars_before_desc,desc_end_pos, andcontent_truncatedthrough manual byte counting - this is error-prone and could be simplified to just detect "has more content" rather than counting exact characters. -
Over-engineering concern (Lines 827-840): The exact truncation character counts (e.g., "47 characters truncated") solve an imaginary problem. Users don't care about exact counts - they just need to know there's more content and where to find it.
Suggested simplification: Replace the character-counting logic with a simpler "are there more meaningful lines?" check, and simplify the truncation message to something like "... [see /skills/test.md for full content]".
No code changes were made - this was a review-only request. The feedback has been provided above for the PR author to consider.
Contributor
Author
There was a problem hiding this comment.
@xingyaoww Would you like to make this change? Or keep as is? I kinda feel as-is might be OK
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
This PR adds the
to_prompt()function for generating XML prompt blocks from skills, following the AgentSkills standard.Depends on PR #1482 (resource directories support)
View changes from PR #1482
Changes
New
to_prompt()FunctionGenerates XML prompt block for available skills:
Output:
Features:
descriptionfield if availableBackward Compatibility
Testing
Added 3 tests covering:
Related Issues
Closes #1478
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:aaf0950-pythonRun
All tags pushed for this build
About Multi-Architecture Support
aaf0950-python) is a multi-arch manifest supporting both amd64 and arm64aaf0950-python-amd64) are also available if needed