feat(sdk/subagent): Support skills in file-based Agents#2260
feat(sdk/subagent): Support skills in file-based Agents#2260VascoSch92 merged 5 commits intomainfrom
Conversation
API breakage checks (Griffe)Result: Failed Log excerpt (first 1000 characters) |
Agent server REST API breakage checks (OpenAPI)Result: Passed |
all-hands-bot
left a comment
There was a problem hiding this comment.
Solid engineering. Clean implementation with comprehensive tests. Eager skill resolution at factory creation time is the right design choice - fail fast rather than at runtime. No critical or important issues found. ✅
Coverage Report •
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste. Eager skill resolution at factory creation time is the right design choice—fail fast with clear errors rather than runtime surprises. Clean refactoring, comprehensive tests, simple priority system (dict update FTW). Solid engineering. ✅
|
The answer from my OH :-): For instance, I would prefer not to add public skills yet, as it isn't clear how we should load them (should we load everything or just the specific ones provided?). In any case, we can handle that later; it will only be a two-line change. Is that okay with you? |
|
Thank you, sorry for the misunderstanding. I don't have an opinion on whether we should load public skills yet, I would just love to see that we (try to?) use the same behavior as we have for the main agent, unless we know it provably shouldn't apply. I'm a bit uncertain if it should apply here... but the result seems weird? Why would the main agent not load skills by default (the flag is false), but the sub-agents will? |
Sorry, I really don't understand. Can you point me to where this is happening? The priority order in resolving Skills is the same used in the SDK for the main agents. We just not load public & plugin skills (at least not yet) |
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste. Eager skill resolution at factory creation time is the right design choice—fail fast with clear errors rather than runtime surprises. The consolidation of skill loading into load_available_skills eliminates duplication while maintaining clear precedence (public → user → project via dict updates). Clean refactoring, comprehensive tests. Solid engineering. ✅
|
@enyst Refactored to centralized everything. Now the PR is doing a little bit too much things :-) |
enyst
left a comment
There was a problem hiding this comment.
Thank you! I was wondering if it will look readable (hey, not all DRY does!), but IMHO it is much better. We were at the 3rd spot in the code with similar stuff happening 🫠
I left a little comment on defaults, because we’re trying, as much as possible, to have a clear public API without side-effects on user machine (ish) and so using the SDK directly should require the user to explicitly opt in those that have boolean flags. Otherwise LGTM, thank you!
Summary
(ref #2186)
skillsfield toAgentDefinition, allowing subagents to reference skills by name in markdown frontmatter (comma-separated or YAML list)agent_definition_to_factoryresolves skill names toSkillobjects at factory creation time usingload_project_skillsandload_user_skills(project takes priority over user)schema.pyto extract frontmatter parsing into dedicated helpers (_extract_color,_extract_tools,_extract_skills) and promoteKNOWN_FIELDSto a module-level constantExample
Checklist
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:5aa1d20-pythonRun
All tags pushed for this build
About Multi-Architecture Support
5aa1d20-python) is a multi-arch manifest supporting both amd64 and arm645aa1d20-python-amd64) are also available if needed