Unify skills modules: move context/skills to sdk/skills#2774
Conversation
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean module consolidation that eliminates confusion without breaking anything.
Analysis: This refactor solves a real problem (two separate skills modules creating API confusion) with the right approach: file consolidation + standard Python deprecation pattern. Backward compatibility is maintained perfectly via __getattr__, all internal imports are updated consistently, and the migration path is crystal clear.
Key insight: Sometimes the best refactor is the one that makes things simpler without causing pain. This is that refactor.
✅ Verdict: Worth merging - solid engineering fundamentals, no issues found.
all-hands-bot
left a comment
There was a problem hiding this comment.
QA Report: Skills Module Unification
✅ PASS
This refactor successfully unifies the skills modules from openhands.sdk.context.skills to openhands.sdk.skills with full backward compatibility and proper deprecation warnings.
Environment Setup
✓ Build successful using make build
- Dependencies installed via
uv sync --dev - 232 packages installed
- Pre-commit hooks configured
CI & Test Status
CI Checks: 18/27 passing (remaining are Docker builds in progress)
✓ Critical checks passing:
- SDK tests
- Agent server tests
- Tools tests
- Cross tests
- Pre-commit checks
- Python API breakage checks
- REST API breakage checks
Test Suite: All 224 skills-related tests pass in 1.05s
uv run pytest tests/sdk/context/skill/ tests/sdk/skills/ tests/sdk/subagent/test_subagent_registry.py -v
# Result: 224 passed, 1 warning in 1.05sFunctional Verification
✓ Test 1: Old Import Path Shows Deprecation Warning
Command:
import warnings
warnings.filterwarnings('always', category=DeprecationWarning)
from openhands.sdk.context.skills import Skill, load_skills_from_dir, KeywordTriggerResult: ✓ Proper deprecation warnings emitted:
DeprecatedWarning: Importing 'Skill' from 'openhands.sdk.context.skills' is deprecated as of 1.16.0
and will be removed in 1.20.0. Use 'from openhands.sdk.skills import Skill' instead.
Verification:
- Warning includes deprecation version (1.16.0)
- Warning includes removal version (1.20.0)
- Warning provides migration guidance
- Objects correctly resolve to new location:
openhands.sdk.skills.skill.Skill
✓ Test 2: New Import Path Works Without Warning
Command:
import warnings
warnings.filterwarnings('error', category=DeprecationWarning) # Warnings become errors
from openhands.sdk.skills import Skill, load_skills_from_dir, KeywordTrigger, TaskTrigger
from openhands.sdk.skills import SkillResources, to_prompt, discover_skill_resources
from openhands.sdk.skills import load_project_skills, load_user_skills, load_public_skillsResult: ✓ All imports succeed with no warnings
- Script uses
warnings.filterwarnings('error')so any deprecation warning would raise an exception - All 10 imports work cleanly
✓ Test 3: Real Usage - Skills Loading Example
Command:
cd examples/05_skills_and_plugins/01_loading_agentskills
uv run python main.pyResult: ✓ Example runs successfully
- Skills loaded using new import path
from openhands.sdk.skills import discover_skill_resources()found scripts, references, and assetsload_skills_from_dir()loaded 2 skills from directory- Skills correctly categorized as AgentSkills SKILL.md format
- Skill metadata parsed correctly (name, description, license, compatibility, resources)
- Agent initialized successfully with loaded skills
Output excerpt:
Loaded skills from directory:
- Repo skills: []
- Knowledge skills: []
- Agent skills (SKILL.md): ['rot13-encryption', 'code-style-guide']
Discovered resources in rot13-encryption/:
- scripts: ['encrypt.sh']
- references: ['examples.md']
- assets: []
✓ Test 4: Comprehensive Test Suite
Command:
uv run pytest tests/sdk/context/skill/ tests/sdk/skills/ tests/sdk/subagent/test_subagent_registry.py -vResult: ✓ All 224 tests pass
- Coverage includes: skill loading, validation, resource discovery, triggers, serialization, AgentSkills fields, project/user/public skills, MCP integration, subagent registry
- Only 1 warning (unrelated deprecation for 'default' agent name)
- Test execution time: 1.05s
Issues Found
None.
Verdict
✅ PASS
The refactor successfully:
- Moves skills implementation to canonical location
openhands.sdk.skills - Maintains backward compatibility via deprecation shim
- Provides clear migration path with proper deprecation warnings
- Updates all internal imports to use new path
- Passes all tests with no regressions
- Works in real usage scenarios
Recommendation: Ready to merge.
enyst
left a comment
There was a problem hiding this comment.
Thank you! Yes this makes more sense 😅
This change consolidates the two skill modules into a single canonical location at openhands.sdk.skills. Changes: - Move skill implementation files from context/skills/ to sdk/skills/ - Create comprehensive exports in sdk/skills/__init__.py - Add deprecation warnings for imports from context.skills (deprecated since 1.16.0, removal planned for 1.20.0) - Update all internal imports to use new canonical path - Update examples and tests to use new import paths - Fix circular import by using direct submodule imports in plugin.py The old import path (openhands.sdk.context.skills) still works but emits deprecation warnings. Users should migrate to openhands.sdk.skills. Migration guide: # Old (deprecated): from openhands.sdk.context.skills import Skill, load_skills_from_dir # New: from openhands.sdk.skills import Skill, load_skills_from_dir Co-authored-by: openhands <openhands@all-hands.dev>
Update patch target from deprecated openhands.sdk.context.skills.skill to new canonical path openhands.sdk.skills.skill Co-authored-by: openhands <openhands@all-hands.dev>
Move tests from tests/sdk/context/skill/ to tests/sdk/skills/ to align with the new canonical module location at openhands.sdk.skills. Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
1df1877 to
bd3fc64
Compare
Why
The SDK currently has two separate skill modules:
openhands.sdk.context.skills- core skill model, loading, and trigger typesopenhands.sdk.skills- skill fetch/install functionalityThis split creates confusion about where skill-related code should live and makes the API harder to understand.
This PR addresses this problem by unifying all skills-related code to
openhands.sdk.skills. The original imports still stand, but with a deprecation warning.Summary
openhands/sdk/context/skills/toopenhands/sdk/skills/as the canonical locationsdk/skills/__init__.pyfor all skill-related functionalityopenhands.sdk.context.skills) withdeprecated_in="1.16.0"andremoved_in="1.20.0"Issue Number
N/A - This is a codebase cleanup/refactoring task.
How to Test
The changes have been verified by running:
uv run pytest tests/sdk/context/skill/ tests/sdk/skills/ tests/sdk/subagent/test_subagent_registry.py -v ======================== 224 passed, 1 warning in 0.75s ========================Video/Screenshots
N/A - This is a refactoring change verified through tests.
Type
Notes
Migration Guide
Exports available from
openhands.sdk.skillsSkill,SkillResources,SkillInfoBaseTrigger,KeywordTrigger,TaskTriggerSkillKnowledge,InputMetadata,SkillResponse,SkillContentResponseload_skills_from_dir,load_project_skills,load_user_skills,load_public_skills,load_available_skillsinstall_skill,uninstall_skill,list_installed_skills,load_installed_skills,get_installed_skill,enable_skill,disable_skill,update_skill,get_installed_skills_dirdiscover_skill_resources,validate_skill_name,to_promptThis PR was created by an AI assistant (OpenHands) on behalf of the user.
@csmith49 can click here to continue refining the PR
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.13-nodejs22-slimgolang: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:ac22dda-pythonRun
All tags pushed for this build
About Multi-Architecture Support
ac22dda-python) is a multi-arch manifest supporting both amd64 and arm64ac22dda-python-amd64) are also available if needed