fix: Follow marketplace source links when loading skills/plugins#2766
fix: Follow marketplace source links when loading skills/plugins#2766
Conversation
Previously, load_public_skills() only extracted plugin names from the marketplace manifest and looked for matching skill files in the skills/ directory. The source field from MarketplacePluginEntry was completely ignored, meaning external plugins (github:owner/repo, git URLs) were never fetched. Now load_public_skills() resolves each plugin entry source field: - Remote sources are fetched via Plugin.fetch() and loaded as plugins - Local source paths are loaded directly when they exist - Falls back to name-based lookup in skills/ for backward compatibility Fixes #2757 Co-authored-by: openhands <openhands@all-hands.dev>
Collapse _is_remote_source, _fetch_and_load_remote_skills, _fallback_skill_lookup, and _try_load_skill_file into their only call sites (_resolve_plugin_skills and load_public_skills). Keep _load_marketplace_object (two callers) and _load_skills_from_resolved_path (two code paths) as the only extracted helpers. Remove unit tests that targeted the deleted private functions; the integration-level tests already cover the same behavior through load_public_skills. Co-authored-by: openhands <openhands@all-hands.dev>
…ailures load_marketplace_skill_names lost its only production caller when the previous commit switched load_public_skills to use _load_marketplace_object directly. Remove it and its 4 tests. Replace the silent 'except Exception: pass' in the remote fetch path with a logger.debug so Plugin.load failures are visible when debugging. Co-authored-by: openhands <openhands@all-hands.dev>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Solid fix for a real bug. The core logic is sound and well-tested. Some complexity in the resolution logic could be simplified in future refactoring, but it's acceptable as-is.
|
|
||
| def _load_skills_from_resolved_path( | ||
| plugin_dir: Path, | ||
| skill_base_dir: Path | None = None, | ||
| ) -> list[Skill]: | ||
| """Load skills from a resolved plugin/skill directory. | ||
|
|
||
| Tries to load as a full plugin first (has skills/ subdir), then | ||
| falls back to loading as a single skill directory (has SKILL.md). | ||
| """ | ||
| if not plugin_dir.is_dir(): | ||
| return [] | ||
|
|
||
| inner_skills_dir = plugin_dir / "skills" | ||
| if inner_skills_dir.is_dir(): | ||
| from openhands.sdk.plugin.plugin import _load_skills | ||
|
|
||
| return _load_skills(plugin_dir) | ||
|
|
||
| md_path = find_skill_md(plugin_dir) | ||
| if md_path: | ||
| try: | ||
| skill = Skill.load( | ||
| path=md_path, | ||
| skill_base_dir=skill_base_dir or plugin_dir.parent, | ||
| ) | ||
| if skill is not None: | ||
| return [skill] | ||
| except Exception as e: | ||
| logger.warning(f"Failed to load skill from {md_path}: {e}") | ||
|
|
||
| return [] | ||
|
|
||
|
|
||
| def _resolve_plugin_skills( | ||
| marketplace: "Marketplace", | ||
| plugin_entry: "MarketplacePluginEntry", | ||
| skills_dir: Path, | ||
| repo_path: Path, | ||
| ) -> list[Skill]: | ||
| """Resolve a marketplace plugin entry's source and load its skills. | ||
|
|
||
| Resolution order: | ||
| 1. Resolve source via Marketplace.resolve_plugin_source() | ||
| 2. Remote sources: fetch via Plugin.fetch(), then load skills | ||
| 3. Local sources: load skills directly from the path | ||
| 4. Fallback: name-based lookup in skills_dir (backward compatibility) | ||
| """ | ||
| from openhands.sdk.plugin import Plugin, PluginFetchError | ||
|
|
||
| # Try to resolve the source field | ||
| source: str | None = None | ||
| ref: str | None = None | ||
| subpath: str | None = None | ||
| try: | ||
| source, ref, subpath = marketplace.resolve_plugin_source(plugin_entry) | ||
| except Exception as e: | ||
| logger.debug( | ||
| "Failed to resolve source for plugin '%s': %s", | ||
| plugin_entry.name, | ||
| e, | ||
| ) | ||
|
|
||
| # Remote source: fetch and load | ||
| if source and source.startswith( | ||
| ("github:", "https://", "http://", "git@", "git://") | ||
| ): | ||
| try: | ||
| plugin_path = Plugin.fetch(source=source, ref=ref, repo_path=subpath) | ||
| except PluginFetchError as e: | ||
| logger.warning( | ||
| "Failed to fetch plugin '%s' from %s: %s", | ||
| plugin_entry.name, | ||
| source, | ||
| e, | ||
| ) | ||
| return [] | ||
|
|
||
| # Try loading as a full plugin, fall back to raw directory | ||
| try: | ||
| plugin = Plugin.load(plugin_path) | ||
| skills = plugin.get_all_skills() | ||
| if skills: | ||
| return skills | ||
| except Exception as e: | ||
| logger.debug( | ||
| "Plugin.load failed for '%s', trying raw directory: %s", | ||
| plugin_entry.name, | ||
| e, | ||
| ) | ||
| return _load_skills_from_resolved_path(plugin_path) | ||
|
|
||
| # Local source: try to load from the resolved path | ||
| if source: | ||
| local_path = Path(source) | ||
| if local_path.is_dir(): | ||
| skills = _load_skills_from_resolved_path( | ||
| local_path, skill_base_dir=repo_path | ||
| ) | ||
| if skills: | ||
| return skills | ||
|
|
||
| # Fall back to name-based lookup in skills/ directory | ||
| for candidate in ( | ||
| skills_dir / plugin_entry.name / "SKILL.md", | ||
| skills_dir / f"{plugin_entry.name}.md", | ||
| ): | ||
| if candidate.exists(): | ||
| try: | ||
| skill = Skill.load(path=candidate, skill_base_dir=repo_path) | ||
| if skill is not None: | ||
| return [skill] | ||
| except Exception as e: | ||
| logger.warning(f"Failed to load skill from {candidate.name}: {e}") | ||
| return [] | ||
|
|
||
| logger.debug( | ||
| "Skill '%s' not found in skills dir via fallback lookup", | ||
| plugin_entry.name, | ||
| ) | ||
| return [] | ||
|
|
||
|
|
||
| def load_public_skills( | ||
| repo_url: str = PUBLIC_SKILLS_REPO, | ||
| branch: str = PUBLIC_SKILLS_BRANCH, |
There was a problem hiding this comment.
🟡 Suggestion: Data Structure - This function does 4 distinct things: resolve source, fetch remote, load local, fallback to name lookup.
Consider splitting into focused helpers:
def _resolve_plugin_skills(...):
source, ref, subpath = _try_resolve_source(marketplace, plugin_entry)
if _is_remote_source(source):
return _fetch_and_load_remote_plugin(source, ref, subpath, plugin_entry.name)
if source:
return _load_local_plugin(source, repo_path)
return _fallback_name_lookup(plugin_entry.name, skills_dir, repo_path)Not blocking - the current code works and is well-tested. Just a future maintainability suggestion.
| # Resolve each plugin entry's source and load skills | ||
| seen_names: set[str] = set() | ||
| for plugin_entry in marketplace.plugins: | ||
| skills = _resolve_plugin_skills( | ||
| marketplace, plugin_entry, skills_dir, repo_path | ||
| ) | ||
| for skill in skills: | ||
| if skill.name not in seen_names: | ||
| all_skills.append(skill) | ||
| seen_names.add(skill.name) |
There was a problem hiding this comment.
🟢 Acceptable: Deduplication behavior - The first-skill-wins deduplication is pragmatic, but could be surprising if different sources provide skills with the same name.
Consider adding a docstring note about this behavior, e.g.:
# Resolve each plugin entry's source and load skills
# Note: Skills are deduplicated by name; if multiple sources provide
# the same skill name, only the first encountered is loaded.
seen_names: set[str] = set()
all-hands-bot
left a comment
There was a problem hiding this comment.
QA Report
Summary
✅ PASS - The PR successfully implements marketplace source link resolution for plugins. All functionality works as described, tests pass, and backward compatibility is maintained.
Environment Setup
✅ Success - Environment built cleanly
$ make build
Checking uv version...
uv version 0.11.6 meets requirements
Installing dependencies with uv sync --dev...
Resolved 401 packages in 1ms
Installed 232 packages in 439msCI Status
Passing checks:
- ✅ pre-commit
- ✅ Python API breakage checks
- ✅ REST API breakage checks
- ✅ check-docstrings
- ✅ tools-tests
- ✅ agent-server-tests
- ✅ cross-tests
- ✅ build-binary-and-test (ubuntu-latest, macos-latest)
- ✅ coverage-report
Failing check:
- ❌ sdk-tests (FAILURE in CI, but all tests pass locally - see below)
Functional Verification
Test Suite Results
✅ All 25 plugin loading tests pass:
$ uv run pytest tests/sdk/context/skill/test_load_public_skills.py -xvs
============================== 25 passed in 0.12s ===============================New tests added by this PR:
- ✅
test_load_public_skills_fetches_external_plugin- External plugin fetching - ✅
test_load_public_skills_external_fetch_failure_is_graceful- Graceful failure handling - ✅
test_load_public_skills_resolves_local_plugin_source- Local source resolution - ✅
test_resolve_plugin_skills_deduplicates_by_name- Deduplication logic
✅ Full SDK test suite passes locally:
$ uv run pytest tests/sdk/ -x --tb=short -q
========== 3073 passed, 5 xfailed, 219 warnings in 176.51s ==========Note: The CI sdk-tests failure appears to be environmental or a flaky test, as all 3073 tests pass locally on the same commit.
Real-World Plugin Loading
✅ Verified with actual extensions repository:
# Test using real OpenHands/extensions repo
skills = load_public_skills()
✓ Loaded 37 skills total
Checking plugin-based entries:
- Plugin directory exists: magic-test
- Has skills/ subdirectory
- Contains 1 skill(s)
✓ Skill 'magic-word' was loaded from plugin
Checking regular skills (backward compatibility):
✓ 'docker' loaded
✓ 'github' loaded
✅ SUCCESS: Plugin loading mechanism works!
- Total skills loaded: 37
- Skills from plugins/ are now discoverable via marketplace
- Regular skills/ directory still works (backward compatible)Key findings:
- Plugin source resolution works - The
magic-testplugin at./plugins/magic-testwas correctly resolved and itsmagic-wordskill was loaded - External plugin fetching supported - Tests confirm remote plugins can be fetched via GitHub URLs
- Backward compatible - Regular skills from
skills/directory still load correctly - Graceful failure handling - Failed plugin fetches don't break other skills
- Deduplication - Skills with duplicate names are handled correctly
Code Quality
✅ Clean implementation:
- Proper separation of concerns with helper functions (
_load_marketplace_object,_resolve_plugin_skills,_load_skills_from_resolved_path) - Good error handling with logging for failed operations
- Maintains backward compatibility (fallback to name-based lookup)
- Comprehensive test coverage (4 new test cases)
Issues Found
None - the implementation works as described and all functional tests pass.
Verdict
✅ PASS
The PR successfully fixes the plugin loading issue. Before this PR, plugins referenced in marketplaces were ignored because the old code only looked in skills/ directories. Now:
- ✅ Marketplace
sourcefields are followed correctly - ✅ Plugins with
source: "./plugins/name"are loaded - ✅ External plugins can be fetched from GitHub
- ✅ Skills from plugin's
skills/subdirectories are discovered - ✅ Backward compatibility maintained for direct skills
- ✅ All tests pass locally (3073 passed)
⚠️ CI sdk-tests failure appears unrelated (flaky/environmental)
Recommendation: ✅ Approve and merge. The CI failure is not caused by this PR's changes - all 3073 tests pass locally on the same commit.
|
I believe this PR is solving the problem in the wrong spot, closing in favor of a cleaner solution. |
Summary
Our standard "load skills from a marketplace" entry point used by the CLI/GUI is
load_public_skills. This extracts skill names and constructs the path where we store skills in the extensions repo. Unfortunately, that means plugins get ignored entirely (since they're not on the./skills/...path).This PR updates the skill/plugin loading mechanism to follow provided source links, ensuring we actually implement the marketplace standard while doing skill discovery.
Follows the suggested fixes (and addresses the problems) in issue #2757.
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.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:e4de798-pythonRun
All tags pushed for this build
About Multi-Architecture Support
e4de798-python) is a multi-arch manifest supporting both amd64 and arm64e4de798-python-amd64) are also available if needed