feat(delegate): File-based agent definitions with markdown + YAML frontmatter#2183
feat(delegate): File-based agent definitions with markdown + YAML frontmatter#2183VascoSch92 wants to merge 15 commits intomainfrom
Conversation
API breakage checks (Griffe)Result: Passed |
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
0d6d277 to
e0fb41b
Compare
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean refactoring with solid architecture.
Key strengths:
- Right data structures: clean separation (schema, load, registry)
- Solves real problem: multi-source agent registration with clear priority rules
- No unnecessary complexity: straightforward loading and deduplication logic
- Well-tested: comprehensive coverage of priority logic, edge cases, and error paths
Architecture is sound:
- Moving agent definitions to SDK makes sense (tools should use SDK, not define core abstractions)
- File-based loading with
.agents/priority over.openhands/is pragmatic - Error handling is adequate (try/except with logging in load.py:136-139)
One future improvement: Integration test for LocalConversation._ensure_plugins_loaded() would verify the auto-registration wiring, but unit coverage is thorough enough for merge.
Verdict: ✅ Worth merging
Key insight: The 6-level priority system (programmatic > plugin > project .agents > project .openhands > user .agents > user .openhands) is complex but justified - it solves real composition needs without forcing special cases. The deduplication in _load_agents_from_dirs() handles it cleanly.
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean refactoring with solid architecture.
Key strengths:
- Right data structures: clean separation (schema, load, registry)
- Solves real problem: multi-source agent registration with clear priority rules
- No unnecessary complexity: straightforward loading and deduplication logic
- Well-tested: comprehensive coverage of priority logic, edge cases, and error paths
Architecture is sound:
- Moving agent definitions to SDK makes sense (tools should use SDK, not define core types)
- Priority hierarchy is clear and well-documented: programmatic > plugin > file-based (project > user)
- Thread-safe with minimal locking overhead
✅ Worth merging
|
Note that the docs tests are not passing because of missing documentations for |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as duplicate.
This comment was marked as duplicate.
|
(HUMAN: pasting the result here, but please feel free to smack it when it's over the top on nothing really - I feel that might be the case) Taste Rating: Needs improvementThis PR is trying to solve a real problem (declarative subagents), but it does it with a couple of “global state + first-wins” shortcuts that will absolutely bite you in any long-running process (agent-server), and it breaks existing public imports without a compatibility shim. [CRITICAL ISSUES] (must fix)1) You broke userspace: removed public APIs without a compatibility layerTwo concrete breakages:
If you want to refactor internals, fine, but keep the old import paths working (even if they just re-export and warn) or bump versions and formally deprecate. Right now it’s “surprise, your code doesn’t import anymore”. 2) Global registry +
|
Review of the roasted-review1) You broke userspace: removed public APIs without a compatibility layer 2) Global registry + register_*_if_absent() destroys your priority model and breaks “reload on new conversation” 3) The “default agent” used for delegation changed semantics (likely a regression) 4) One of the registry tests is structurally wrong (so it doesn’t test what it claims) 5) Tool naming / UX mismatch is unresolved 6) The “example usage” in the new registry module is stale 7) Loader error handling is “log-and-ignore everything” |
This comment was marked as resolved.
This comment was marked as resolved.
I mean an example script in |
Can we decide what is public API now, and add it to the SDK's |
Added :-) |
Summary
This PR introduces the ability to define and load agents via Markdown files and performs a structural refactor to centralize agent-loading logic. (ref #2049 )
Key Features
Architectural Refactor
Moved all logic related to agent discovery and instantiation from plugin and tools.delegate into a dedicated subagents module.
Remarks
This PR focuses on the core infrastructure. Several advanced features for Markdown-based agents are currently unsupported and are being tracked in the primary Epic #2054.
docs
OpenHands/docs#358
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:8e214f1-pythonRun
All tags pushed for this build
About Multi-Architecture Support
8e214f1-python) is a multi-arch manifest supporting both amd64 and arm648e214f1-python-amd64) are also available if needed