Skip to content

Conversation

@neubig
Copy link
Contributor

@neubig neubig commented Dec 22, 2025

Summary

This PR adds support for the SKILL.md file convention and skill name validation according to the AgentSkills standard.

Depends on PR #1479 (AgentSkills fields)

Changes

1. New find_skill_md() Function

Locates SKILL.md files in a directory (case-insensitive):

from openhands.sdk.context.skills import find_skill_md

skill_md = find_skill_md(Path("skills/pdf-tools"))
# Returns Path to SKILL.md if found, None otherwise

2. New validate_skill_name() Function

Validates skill names according to AgentSkills spec:

from openhands.sdk.context.skills import validate_skill_name

errors = validate_skill_name("my-skill", directory_name="my-skill")
# Returns list of validation errors (empty if valid)

Validation rules:

  • 1-64 characters
  • Lowercase alphanumeric + hyphens only (a-z, 0-9, -)
  • Must not start or end with hyphen
  • Must not contain consecutive hyphens (--)
  • Name must match directory name (if provided)

3. Updated Skill.load()

Now accepts optional directory_name parameter for SKILL.md directory format:

skill = Skill.load(
    skill_md,
    skill_dir,
    directory_name="pdf-tools"  # NEW: Enables name validation
)

When directory_name is provided:

  • Uses directory name as skill name (ignoring frontmatter name)
  • Validates name against AgentSkills spec
  • Raises SkillValidationError if name is invalid

4. Updated load_skills_from_dir()

Now automatically discovers SKILL.md directories:

skills/
├── pdf-tools/
│   └── SKILL.md    # Loaded as "pdf-tools" skill
├── code-review/
│   └── SKILL.md    # Loaded as "code-review" skill
└── legacy.md       # Still supported (flat file format)

Backward Compatibility

  • All changes are backward compatible
  • Existing flat .md files continue to work
  • directory_name parameter is optional
  • Name validation only applies to SKILL.md directory format

Testing

Added 27 new tests covering:

  • find_skill_md() functionality
  • validate_skill_name() validation rules
  • Skill.load() with directory_name parameter
  • load_skills_from_dir() with SKILL.md directories

Related Issues

Closes #1475

Part of #1473 (Support AgentSkills standard)

Diff from Previous PR

View changes from PR #1479


Agent Server images for this PR

GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server

Variants & Base Images

Variant Architectures Base Image Docs / Tags
java amd64, arm64 eclipse-temurin:17-jdk Link
python amd64, arm64 nikolaik/python-nodejs:python3.12-nodejs22 Link
golang amd64, arm64 golang:1.21-bookworm Link

Pull (multi-arch manifest)

# Each variant is a multi-arch manifest supporting both amd64 and arm64
docker pull ghcr.io/openhands/agent-server:8aacbfc-python

Run

docker run -it --rm \
  -p 8000:8000 \
  --name agent-server-8aacbfc-python \
  ghcr.io/openhands/agent-server:8aacbfc-python

All tags pushed for this build

ghcr.io/openhands/agent-server:8aacbfc-golang-amd64
ghcr.io/openhands/agent-server:8aacbfc-golang_tag_1.21-bookworm-amd64
ghcr.io/openhands/agent-server:8aacbfc-golang-arm64
ghcr.io/openhands/agent-server:8aacbfc-golang_tag_1.21-bookworm-arm64
ghcr.io/openhands/agent-server:8aacbfc-java-amd64
ghcr.io/openhands/agent-server:8aacbfc-eclipse-temurin_tag_17-jdk-amd64
ghcr.io/openhands/agent-server:8aacbfc-java-arm64
ghcr.io/openhands/agent-server:8aacbfc-eclipse-temurin_tag_17-jdk-arm64
ghcr.io/openhands/agent-server:8aacbfc-python-amd64
ghcr.io/openhands/agent-server:8aacbfc-nikolaik_s_python-nodejs_tag_python3.12-nodejs22-amd64
ghcr.io/openhands/agent-server:8aacbfc-python-arm64
ghcr.io/openhands/agent-server:8aacbfc-nikolaik_s_python-nodejs_tag_python3.12-nodejs22-arm64
ghcr.io/openhands/agent-server:8aacbfc-golang
ghcr.io/openhands/agent-server:8aacbfc-java
ghcr.io/openhands/agent-server:8aacbfc-python

About Multi-Architecture Support

  • Each variant tag (e.g., 8aacbfc-python) is a multi-arch manifest supporting both amd64 and arm64
  • Docker automatically pulls the correct architecture for your platform
  • Individual architecture tags (e.g., 8aacbfc-python-amd64) are also available if needed

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>
@github-actions
Copy link
Contributor

github-actions bot commented Dec 22, 2025

Coverage

Coverage Report •
FileStmtsMissCoverMissing
openhands-sdk/openhands/sdk/context/skills
   skill.py32825921%45–50, 63, 65–67, 69–70, 72–73, 78–79, 81, 176–182, 188–192, 205, 208, 212, 224–225, 230, 237, 265, 268, 271–273, 275, 279, 282–284, 287–289, 291–293, 296, 299, 302–305, 311, 314, 322, 327–329, 335, 337–343, 346, 355–356, 365–368, 381–388, 398–399, 401, 407–408, 410, 419–422, 424–427, 435–437, 445–447, 462–468, 480–488, 501–512, 536–537, 543–546, 571–572, 574–576, 578–580, 582, 585–589, 592–593, 596–597, 600–601, 603, 607, 628–629, 631–634, 636–638, 641–645, 647, 651–652, 654, 657, 675–676, 678–679, 682, 687–689, 692, 694–696, 699–703, 705, 710–711, 715, 718, 732–734, 752, 754–758, 765, 772–776, 781–783, 785, 800, 802, 804–808, 811–814, 848, 850, 852–853, 855–857, 860–863, 866, 868, 871–873, 877–881, 883–884, 886, 889
TOTAL13493615854% 

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>
@neubig
Copy link
Contributor Author

neubig commented Dec 22, 2025

@OpenHands think carefully about the load_skills_from_dir function. it has become very complex, could the logic be simplified and/or modularized?

@openhands-ai
Copy link

openhands-ai bot commented Dec 22, 2025

I'm on it! neubig can track my progress at 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>
@openhands-ai
Copy link

openhands-ai bot commented Dec 22, 2025

Summary

I've refactored the load_skills_from_dir function as requested to simplify and modularize its logic. The changes have been pushed to the feat/skill-md-convention branch.

Changes Made

The original function (~120 lines) was doing multiple things: file discovery, skill loading, and categorization. I extracted four focused helper functions:

  1. _find_third_party_files(repo_root) - Finds .cursorrules, AGENTS.md, CLAUDE.md, etc. in the repository root with case-insensitive matching

  2. _find_skill_md_directories(skill_dir) - Finds AgentSkills-style directories containing SKILL.md files, returns tuples of (path, directory_name, directory_path)

  3. _find_regular_md_files(skill_dir, exclude_dirs) - Finds regular .md skill files, excluding README.md, SKILL.md, and files in SKILL.md directories

  4. _load_skill_safe(path, skill_dir, ...) - Loads a skill with consistent error handling, eliminating the repeated try/except blocks

Result

The main load_skills_from_dir function is now a simple orchestrator:

  • Calls file discovery functions
  • Loads each file using the safe loader
  • Categorizes into repo_skills/knowledge_skills
  • Returns results

Verification

  • ✅ All pre-commit checks pass (ruff format, ruff lint, pycodestyle, pyright, import rules)
  • ✅ All 69 skill-related tests pass
  • ✅ Changes are backward compatible (same public API)
  • ✅ No extraneous changes - only the skill.py file was modified

View full conversation

@neubig neubig marked this pull request as ready for review December 22, 2025 22:56
@neubig neubig requested a review from xingyaoww December 22, 2025 22:57
@openhands-ai
Copy link

openhands-ai bot commented Dec 22, 2025

Looks like there are a few issues preventing this PR from being merged!

  • GitHub Actions are failing:
    • Run tests
    • Agent Server

If you'd like me to help, just leave a comment, like

@OpenHands please fix the failing actions on PR #1480 at branch `feat/skill-md-convention`

Feel free to include any additional details that might help me get this PR into a better state.

You can manage your notification settings

@xingyaoww
Copy link
Collaborator

@OpenHands /codereview-roasted

@openhands-ai
Copy link

openhands-ai bot commented Dec 23, 2025

I'm on it! xingyaoww can track my progress at all-hands.dev

@openhands-ai
Copy link

openhands-ai bot commented Dec 23, 2025

Code Review Summary for PR #1480

I provided a Linus Torvalds-style roasted code review for the feat/skill-md-convention branch. Here's the assessment:

🔴 Taste Rating: Needs Improvement


Critical Issues (Must Fix)

  1. Data Structure Smell (Lines 471-488): _find_skill_md_directories returns list[tuple[Path, str, Path]] where the third element (directory_path) is redundant - it's just skill_md.parent. This leads to confusing index-based access like entry[2].

  2. Noisy Wrapper Function (Lines 515-546): _load_skill_safe wraps exceptions just to prepend the path, but Skill.load() already has path context. This creates double-wrapped error messages.


Improvement Opportunities

  1. Unnecessary Nested Function (Lines 578-582): The add_skill closure is 4 lines of trivial logic that should be inlined.

  2. Incomplete Case-Insensitive Search (Lines 462-468): Only checks 3 variants (original, lower, upper) but misses mixed-case like "Agents.MD".

  3. Confusing API Design (Lines 245-246): Having both directory_name and validate_name as separate parameters is confusing. If directory_name is provided, validation should happen automatically.


Test Issues

  • Magic number assertions (len(...) == 1) are fragile. Should check for error content instead.

Verdict

Needs rework - The refactoring extracted functions but didn't simplify the data flow. The redundant tuple field and wrapper function noise should be addressed before merging.

Key Insight

Good refactoring eliminates complexity; this one just moved it around and added a redundant tuple field. The data structure choice (tuple[Path, str, Path]) is the root cause of the awkwardness.


No code changes were made - this was a review-only request.

View full conversation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support SKILL.md file convention and name validation

4 participants