feat(plugin): Support multiple marketplace registrations with auto-load semantics#2495
feat(plugin): Support multiple marketplace registrations with auto-load semantics#2495jpshackelford wants to merge 23 commits intomainfrom
Conversation
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
- Document auto_load strategies (auto-load vs on-demand) - Explain plugin@marketplace reference syntax (matches Claude Code) - Add expandable example code block - Include MarketplaceRegistration field reference table Related: OpenHands/software-agent-sdk#2495
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean implementation with elegant design.
Key strengths:
- Well-structured data models (
MarketplaceRegistration,MarketplaceRegistry) with clear separation of concerns - Security improvement: path validation now uses normalization instead of simple
..checks, catching traversal tricks - Proper backward compatibility with clear deprecation path
- Comprehensive test coverage (21+ tests) using real marketplace directories, not excessive mocks
- Clean async architecture (router → service → event_service → conversation)
Linus would say: Data structures are right, no special cases, solves real problems. Ship it. 🚀
|
Thank you for this! Just trying to understand a detail. @OpenHands Read this PR and the linked issue, and understand them well. I think I saw in the linked issue that 4 things should be deprecated, but in this PR description I see 1 thing to deprecate. Verify what I just said. Are the others replaced with other implementations here, WDYT, should others be deprecated, or what seems to happen about that? Answer directly as a comment on the PR on github. |
|
I'm on it! enyst can track my progress at all-hands.dev |
|
@enyst I checked the linked issue, this PR, and the API-breakage workflow. You remembered correctly: #2494 lists 4 deprecations:
From the code in this PR, though, I only see #1 actually deprecated today:
The other 3 are not deprecated in the implementation yet:
So my read is: this PR introduces the new registration/resolution model ( So I think the accurate framing is probably:
On the griffe / Python API breakage workflow: it failed because the checker compares public SDK API against the PyPI baseline, saw the My take: for this specific case, that workflow is being too strict. The field is still there and still works; we are only marking it deprecated. I would not consider that alone a breaking Python API change that should require a minor bump. So I think either:
So short version: yes, the issue says 4; this PR currently implements 1 of those deprecations, while the other 3 still look live / not migrated yet. |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
OpenHands-GPT-5.4 here — @enyst @jpshackelford, now that the small Griffe/ First, the key observationRight now, the new model is only partially replacing the old one:
So the question is really: should this PR stay scoped to the part that is already implemented, or should it do a bit more so the new path is actually exercised end-to-end and more deprecations become justified? Alternative 1: narrow the PR so the claims match the codeWhat this meansKeep this PR focused on:
What I would do under this option
Pros
Cons
Alternative 2: do a bit more in this PR so the new path is used more broadlyWhat “a bit more” would need to meanIf we want to deprecate more than just the field, I think this PR needs to wire the new model into the real startup path. Concretely, I think that means:
What this would justify deprecatingUnder this option, I think these become more defensible:
What I still would not deprecate yetI would still be careful with:
Those are public SDK helpers and they are still the active implementation path today. Even if we extend this PR, I think the safer move is:
My recommendationI recommend Alternative 1 unless you want to explicitly grow the PR into an end-to-end migration PR. If the goal is to keep this PR tight, then I would:
If the goal is to make this PR the real migration point, then I would choose Alternative 2, but in that case I would do the extra wiring above before deprecating more. In particular, I would not deprecate Short version
|
…e support This PR introduces support for registering multiple marketplaces with explicit auto-load semantics, providing an alternative to the single marketplace_path approach in PR #13117. ## Changes ### New Models **MarketplaceRegistration** - Registration for a plugin marketplace: - name: Identifier for this marketplace registration - source: Marketplace source (github:owner/repo, git URL, or local path) - ref: Optional branch, tag, or commit - repo_path: Subdirectory path for monorepos - auto_load: 'all' to load plugins at conversation start, None for on-demand ### Updated Settings Model Added registered_marketplaces field to Settings (list of MarketplaceRegistration). This allows users to configure multiple marketplaces with different loading behaviors. ### Updated Skill Loading - skill_loader: Added registered_marketplaces parameter to pass marketplace registrations to the agent-server API - app_conversation_service_base: Added registered_marketplaces parameter to load_and_merge_all_skills method ### Tests - Added comprehensive tests for MarketplaceRegistration model - Added tests for Settings.registered_marketplaces field - Added tests for skill_loader marketplace handling ## Key Behaviors - Marketplace resolution composes instance → org → user (additive) - auto_load='all' loads all plugins at conversation start - auto_load=None registers marketplace for on-demand resolution - Path validation rejects absolute paths and directory traversal ## Dependencies This PR is designed to work with SDK PR #2495 which provides: - MarketplaceRegistry class for managing registered marketplaces - Plugin resolution via 'plugin-name@marketplace-name' syntax - Lazy fetching and caching of marketplace manifests ## Related - Alternative to #13117 (marketplace_path setting) - Leverages SDK PR OpenHands/software-agent-sdk#2495 - Enables #12916 (org-level default resources) - Aligns with #13188 (instance-default org proposal) - Supports #10947 (OpenHands configuration proposal)
neubig
left a comment
There was a problem hiding this comment.
Could you please fix CI and re-request review? Thanks!
… multiple marketplace support This introduces support for registering multiple marketplaces with explicit auto-load semantics, replacing the single marketplace_path string. Key changes: - Add MarketplaceRegistration model with name, source, ref, repo_path, and auto_load fields - Add MarketplaceRegistry class for lazy fetching, caching, and plugin resolution - Add registered_marketplaces field to AgentContext - Deprecate marketplace_path field (kept for backward compatibility) - Add comprehensive tests for the new functionality Plugin resolution supports: - Explicit marketplace qualifier: 'plugin-name@marketplace-name' - Search all registered marketplaces: 'plugin-name' (errors if ambiguous) Closes #2494
…plugin resolution flow Adds comprehensive integration tests that demonstrate: - Single and multiple marketplace registration - Auto-load vs registered-only marketplaces - Plugin resolution with explicit and implicit marketplace references - Ambiguous plugin name handling - Plugin not found error scenarios - Marketplace caching behavior - Prefetch functionality - Monorepo subdirectory support - Full plugin load flow from registration to Plugin.load() - Claude Code .claude-plugin directory fallback Co-authored-by: openhands <openhands@all-hands.dev>
Demonstrates the full marketplace registration and plugin resolution flow: - Registering multiple marketplaces with different auto-load settings - Resolving plugins with explicit and implicit marketplace references - Loading plugins from resolved sources - Error handling (not found, ambiguous, unregistered) - Prefetching for validation - Integration pattern with AgentContext Co-authored-by: openhands <openhands@all-hands.dev>
- Reduce to minimal happy path demonstration - Reuse existing 43_mixed_marketplace_skills marketplace - Remove verbose demos and generated directories Co-authored-by: openhands <openhands@all-hands.dev>
…example Shows the user-facing API: - Configure registered_marketplaces in AgentContext - Create Agent with agent_context - Use skills from marketplace in a Conversation Co-authored-by: openhands <openhands@all-hands.dev>
- Add load_plugin(plugin_ref) to BaseConversation abstract interface
- Implement in LocalConversation: creates MarketplaceRegistry from
agent_context.registered_marketplaces, resolves and loads plugin
- Implement in RemoteConversation: calls agent-server API endpoint
- Add POST /conversations/{id}/plugins/load endpoint to agent-server
- Add LoadPluginRequest model
- Add ConversationService.load_plugin() and EventService.load_plugin()
- Add tests for conversation.load_plugin() integration
- Update example to demonstrate the full API
Co-authored-by: openhands <openhands@all-hands.dev>
- Create marketplace and plugin on-the-fly instead of printing code - Shows the complete flow: register → load_plugin → use skill - Add .gitignore for generated demo_marketplace directory Co-authored-by: openhands <openhands@all-hands.dev>
- Fix line too long errors in agent_context.py, registry.py, local_conversation.py, and test files - Fix unused variable assignments in test_marketplace_registry_integration.py - Fix pyright errors: - Add null check for agent_context in local_conversation.py - Add null check for resolved_plugins in example file - Add missing load_plugin method to MockConversation in test file - Bump version from 1.14.0 to 1.15.0 for breaking API change (marketplace_path field marked as deprecated) - Apply ruff formatter fixes (import sorting, whitespace)
…kflow The API breakage check detecting the deprecation of marketplace_path is expected. Version bump will be applied through the Prepare Release workflow.
- Remove dynamic file creation from main.py - Add pre-created demo_marketplace/ directory with: - .plugin/marketplace.json - plugins/greeter/.plugin/plugin.json - plugins/greeter/skills/SKILL.md - Remove .gitignore that excluded demo_marketplace/ This makes the example cleaner and easier to understand.
Demonstrates both loading strategies in one example: - auto_marketplace/ with auto_load='all' - loaded at conversation start - demo_marketplace/ with auto_load=None - loaded on-demand This shows the full power of multiple marketplace registrations.
The plugin-name@marketplace-name format is consistent with Claude Code's plugin install syntax.
- Fix silent exception swallowing in registry.py:
- _resolve_from_all() now accumulates fetch errors and includes them in
PluginNotFoundError when all marketplaces fail
- list_plugins() now raises PluginResolutionError with details when all fail
- Improves debugging: users see actual errors instead of 'plugin not found'
- Simplify agent_context.py deprecation logic:
- Remove unnecessary effective_marketplace_path variable
- Remove misleading comment about backward compatibility
- Improve path validation security in types.py:
- Add _validate_repo_path() helper using os.path.normpath()
- Catches path traversal tricks like 'safe/../../../etc'
- Both MarketplaceRegistration and PluginSource use shared validation
- Fix state mutation atomicity in local_conversation.py:
- Update both self.agent and self._state.agent within same lock context
- Add comprehensive tests:
- TestErrorAccumulation: 3 tests for error accumulation behavior
- TestPathValidation: 5 tests for path security validation
- Fix E501 line too long in registry.py (90 > 88) - Fix ruff format issue in types.py (collapse multi-line string) Co-authored-by: openhands <openhands@all-hands.dev>
This commit implements Option 2 from the review discussion, completing the full migration path for registered_marketplaces: LocalConversation changes: - _ensure_plugins_loaded now honors registered_marketplaces with auto_load='all' - Automatically loads all plugins from marketplaces marked for auto-loading - Plugins from explicit specs load first, then marketplace auto-load plugins Agent-server changes: - Add SkillsRequest.registered_marketplaces field - Deprecate SkillsRequest.marketplace_path with warning - Update load_all_skills() to accept registered_marketplaces parameter - Add _load_marketplace_skills() helper to load skills from auto-load marketplaces - Marketplace-loaded skills have highest precedence in merge order SDK deprecations: - load_public_skills(marketplace_path=...) now emits DeprecationWarning - load_available_skills(marketplace_path=...) now emits DeprecationWarning - Both docstrings updated to indicate deprecation This fully implements the 4 deprecations outlined in issue #2494: 1. AgentContext.marketplace_path -> registered_marketplaces (done in original PR) 2. load_public_skills(marketplace_path=...) -> marketplace registry (this commit) 3. load_available_skills(marketplace_path=...) -> marketplace registry (this commit) 4. SkillsRequest.marketplace_path -> registered_marketplaces (this commit) Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
ae36a32 to
abb8b9a
Compare
|
@OpenHands Give us a view on how this PR works and tell WDYT. /codereview Post directly on github as a review with gh api. |
|
I'm on it! enyst can track my progress at all-hands.dev |
enyst
left a comment
There was a problem hiding this comment.
Thanks — I dug through the issue, the PR, and the updated SDK/agent-server paths. My read is that the overall direction is good: MarketplaceRegistration + MarketplaceRegistry is the right shape, the lazy-fetch/caching model is straightforward, and the plugin resolution flow (plugin vs plugin@marketplace) looks reasonable.
That said, I don't think the new registration model is fully the source of truth yet. I found a few correctness gaps where the legacy marketplace_path flow is still driving behavior, or where load_plugin() doesn't apply the full plugin semantics it advertises. Left the concrete spots inline.
Because this touches plugin loading / agent behavior, I'd want a maintainer to take the final call after those are sorted and the intended end-to-end semantics are tightened up. WDYT?
This comment was marked as duplicate.
This comment was marked as duplicate.
|
[Automatic Post]: It has been a while since there was any activity on this PR. @jpshackelford, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
1 similar comment
|
[Automatic Post]: It has been a while since there was any activity on this PR. @jpshackelford, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
…ading Address remaining reviewer feedback from PR review: Gap 1: AgentContext._load_auto_skills() now uses registered_marketplaces - When registered_marketplaces is set, skills are loaded from marketplaces with auto_load='all' via _load_skills_from_marketplaces() - User skills still loaded via legacy path (they are local files) - Legacy marketplace_path path only used when registered_marketplaces is empty - This makes registered_marketplaces the true source of truth at the AgentContext construction level, not just at conversation startup Gap 3: load_plugin() now fully applies plugin semantics - Merges plugin hooks into existing hook processor via _merge_plugin_hooks() - Registers plugin agent definitions via register_plugin_agents() - Skills and MCP config merging was already working - This matches what _ensure_plugins_loaded() does for startup plugins The implementation now honors the full plugin contract: - Skills are merged into agent_context - MCP config is merged into agent.mcp_config - Hooks are merged into the hook processor - Agent definitions are registered in the agent registry Co-authored-by: openhands <openhands@all-hands.dev>
|
All review feedback has been addressed and the 3 inline review threads are now resolved. CI Status: All required checks pass. The |
|
[Automatic Post]: This PR seems to be currently waiting for review. @enyst, could you please take a look when you have a chance? |
|
@OpenHands Give us a view on how this PR works and tell WDYT. /codereview Post directly on github as a review with gh api. |
|
I'm on it! enyst can track my progress at all-hands.dev |
enyst
left a comment
There was a problem hiding this comment.
Overall I like the direction here. MarketplaceRegistration + MarketplaceRegistry is a much clearer model than a single marketplace_path, and the lazy fetch/cache flow is straightforward to reason about. I also reran uv run pytest tests/sdk/plugin/test_marketplace_registry.py tests/sdk/plugin/test_marketplace_registry_integration.py tests/agent_server/test_skills_service.py -q locally and got 69 passing tests.
That said, I still see two correctness gaps in the current branch before I'd feel great about merging:
- runtime
conversation.load_plugin()does not rebuild agent tool state after merging plugin MCP config - agent-server
/skillsis only partially migrated toregistered_marketplacesand still ignoressource/reffor thepublicregistration path
Since this changes plugin / skill loading behavior, I'd still want a human maintainer to do a lightweight behavior/eval sanity check before final merge.
This review was generated by an AI assistant (OpenHands) on behalf of the user.
|
|
||
| # Update agent and state atomically | ||
| # Create new agent first, then update both references together | ||
| new_agent = self.agent.model_copy( |
There was a problem hiding this comment.
This still looks buggy once the conversation has already been initialized. load_plugin() swaps in new_agent = self.agent.model_copy(...), but model_copy() preserves private attrs, so the copied agent keeps _initialized=True and the old _tools map. In practice that means any MCP servers added by the newly loaded plugin never flow back through AgentBase._initialize(), so runtime-loaded MCP tools stay stale/unavailable. I think this needs to either reuse the same init path as startup or explicitly reset/rebuild the initialized tool state here, plus a regression test that loads a plugin after init and checks the new tool appears.`
There was a problem hiding this comment.
@OpenHands Is this behavior newly added in this PR?
There was a problem hiding this comment.
I'm on it! enyst can track my progress at all-hands.dev
There was a problem hiding this comment.
Yes — the problematic runtime behavior is newly introduced in this PR. The existing startup plugin-loading path also uses model_copy(), but it happens before agent initialization, so there isn't a stale initialized tool map yet. The new load_plugin() path can run after initialization, which is why it was preserving _initialized/_tools and leaving newly added MCP tools unavailable.
Fixed in 270226b by resetting and rebuilding the copied agent's runtime tool state when load_plugin() runs after startup, and I added a regression test that loads a marketplace plugin after initialization and checks the MCP init path runs again.
This reply was generated by an AI assistant (OpenHands) on behalf of the user.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
| marketplace_path = request.marketplace_path | ||
| if request.registered_marketplaces: | ||
| # New behavior: use registered_marketplaces | ||
| # For now, we extract marketplace_path from the first 'public' registration |
There was a problem hiding this comment.
This is still only a partial replacement for SkillsRequest.marketplace_path. When registered_marketplaces is provided, the router only extracts repo_path from the first public registration, and load_all_skills() still drives the base public catalog through load_available_skills(... marketplace_path=...). So source / ref from the registration are ignored here, and /skills can still return the default OpenHands/extensions data even when the caller registered a different public marketplace source/branch. I’d make /skills fully registry-driven once registrations are present, or explicitly narrow the API contract until source/ref are honored end-to-end.`
This comment was marked as duplicate.
This comment was marked as duplicate.
Rebuild the copied agent's runtime tool state when load_plugin() runs after conversation startup so new MCP tools become available immediately. Add a regression test covering marketplace-driven plugin loads after agent initialization. Co-authored-by: openhands <openhands@all-hands.dev>
Revert 270226b so the proposed fix remains visible in history for the PR author to evaluate. Co-authored-by: openhands <openhands@all-hands.dev>
|
[Automatic Post]: It has been a while since there was any activity on this PR. @jpshackelford, are you still working on it? If so, please go ahead, if not then please request review, close it, or request that someone else follow up. |
Summary
This PR introduces support for registering multiple marketplaces with explicit auto-load semantics, replacing the single
marketplace_pathstring.Closes #2494
Changes
New Models
MarketplaceRegistration- Registration for a plugin marketplace:New Classes
MarketplaceRegistry- Manages registered marketplaces with:plugin-name@marketplaceUpdated
AgentContextDeprecations
AgentContext.marketplace_pathAgentContext.registered_marketplacesBackward compatibility is maintained - if
marketplace_pathis set andregistered_marketplacesis empty, a deprecation warning is emitted and the old behavior is preserved.Plugin Resolution
Reference formats (same as Claude Code plugin syntax):
plugin-name@marketplace-name— Explicit marketplace qualifierplugin-name— Search registered marketplaces (error if ambiguous)Testing
All 21 tests pass.
Documentation
Related
marketplace_pathsupport, released in v1.13.0)marketplace_pathsetting)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:649cfd9-pythonRun
All tags pushed for this build
About Multi-Architecture Support
649cfd9-python) is a multi-arch manifest supporting both amd64 and arm64649cfd9-python-amd64) are also available if needed