Repository navigation
Conversation
GET /api/skills/marketplace returned an empty catalog against the real OpenHands/extensions repository: _fetch_catalog_entries gave up when marketplaces/default.json was missing, before trying Marketplace.load, which discovers .plugin/marketplace.json. Try manifest discovery first and keep the explicit marketplace path as the fallback, as the plugin catalog already does. Drop the known-bug marker from F25.marketplace-public. Fixes #4665 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Review of #5767 — load skills marketplace from .plugin manifest first
Scope/ownership: The change lives in openhands-agent-server (skills catalog), which this repository owns, so it belongs here.
What I verified on head d407be87:
skills_service._fetch_catalog_entriesnow mirrorsplugins_service._fetch_plugin_catalog_entries:Marketplace.load(repo_path)first, and the explicitmarketplace_pathfile only as a fallback. The old early return whenmarketplaces/default.jsonwas missing is gone. This is the correct minimal fix for the reported symptom (empty Marketplace tab against the realOpenHands/extensionslayout, which ships.plugin/marketplace.json).- The fallback branch preserves explicit-path behavior; plugin-over-skill precedence, plugin filtering, the fresh
installedcheck, and REST schemas are untouched. - Focused tests:
uv run pytest tests/agent_server/test_skills_service.py tests/agent_server/test_skills_router.py -q-> 90 passed. The newTestFetchCatalogEntriescases cover the.plugin-only layout, the explicit-path fallback, and the empty-repo case. - Current-head checks are green (
agent-server-tests,sdk-tests,REST API (OpenAPI),security, etc.).
Material finding (scope / acceptance criteria): Linked issue #4665 (ready-for-dev, labeled architecture) lists five acceptance criteria. This head satisfies #1 (.plugin/marketplace.json layout), #2 (explicit path as fallback), and #5 (unchanged precedence/filtering/installed freshness/schemas). It does not satisfy:
- #3 "Skills and plugins within the TTL use the same fetched/parsed snapshot" — no shared snapshot; skills and plugins still fetch and parse the repository independently.
- #4 "Transient fetch/parse failures do not cache an empty catalog" —
service_get_marketplace_catalogstill caches whatever_fetch_catalog_entriesreturns for 300s, including an empty list (unchanged lines aroundentries = _fetch_catalog_entries(marketplace_path)/_catalog_cache = (now, entries)), andPOST /api/skills/syncdoes not clear it.
The PR body itself flags this ("Scope: minimal loader-order fix (this PR) or the shared marketplace snapshot the issue suggests… A maintainer should pick one of the two"), while the description still says Fixes #4665. Because the change addresses a narrower scenario than the linked issue, this needs a maintainer scope decision: either confirm the reduced scope and retarget the closing reference (e.g. Refs #4665, with the shared-snapshot / empty-cache work tracked as a follow-up — the author names F25.marketplace-stale-cache), or fold criteria #3/#4 into this PR.
The code change itself is correct and low-risk; I have no correctness, security, or compatibility finding against it. The only open item is the scope/acceptance gap above, which the repository review guide directs to human maintainer review.
🔄 CHANGES REQUESTED
3 participants
HUMAN:
This PR proposes a fix for a bug uncovered during the first full agentic run of
verify-agent-server.AGENT:
I'm an AI agent (Claude Code) helping Engel Nyst (@enyst) with project maintenance.
Requested by Engel · project thread
Decision needed
Scope: minimal loader-order fix (this PR) or the shared marketplace snapshot the issue suggests. The issue's "Suggested Implementation" and two of its acceptance criteria ask for a shared, success-only snapshot cache used by both the skills and plugins catalogs. This PR implements only the manifest-layout fix: it makes the skills catalog load the marketplace in the same order the plugin catalog already does (
Marketplace.loadfirst,marketplace_pathas fallback). I chose the smaller change because it fixes what a user sees (an empty Marketplace tab) with a few lines and no new shared state, and the cache behaviour is tracked separately on the map (F25.marketplace-stale-cache: the catalog caches empty results and sync does not clear it), to be fixed in its own PR.The alternative (one shared snapshot) would also satisfy "skills and plugins within the TTL use the same fetched/parsed snapshot" and "transient failures do not cache an empty catalog", at the cost of a larger refactor across
skills_service.pyandplugins_service.py. Another contributor's open PR, #4785, is linked to the issue; I did not read it, to keep this fix independent. A maintainer should pick one of the two (or merge this one and keep the shared snapshot as a follow-up).Why
GET /api/skills/marketplace(the Marketplace tab) returns{"skills": []}against the realOpenHands/extensionsrepository._fetch_catalog_entriesreturned early whenmarketplaces/default.jsonwas missing, before tryingMarketplace.load, which finds the repository's actual manifest at.plugin/marketplace.json. The repository shipsmarketplaces/openhands-extensions.jsonandlarge-codebase.json, notdefault.json.Summary
skills_service._fetch_catalog_entries: tryMarketplace.load(repo_path)first; read the explicitmarketplace_pathonly when discovery fails, and return[]only when neither works (mirrorsplugins_service._fetch_plugin_catalog_entries)..plugin/marketplace.jsonlayout withoutdefault.jsonlists entries (fails on main), the explicit-path fallback still works, and an empty repo still yields[].F25.marketplace-publicloses itsknown bugmarker; its prose and two Gotchas now describe the working behaviour.Issue Number
Fixes #4665
How to Test
uv run pytest tests/agent_server/test_skills_service.py tests/agent_server/test_skills_router.py -q: 90 passed (the new manifest test fails on main withMarketplace file not found: .../marketplaces/default.json).OpenHands/extensionsclone:control-agent-server map run --file F25 --only F25.marketplace-public --fresh --repeat 2 --checkout <main checkout>reportedxfailin both attempts (200 with{"skills": []}).xpassin both attempts; the catalog listed 73 entries with absolute sources inside the clone.control-agent-server map checkok, andmap run --file F25 --only F25.marketplace-public --freshreportspass.uv run pre-commit run --files <changed files>: clean.Video/Screenshots
n/a
Type
Notes
The 5-minute catalog cache (including caching an empty result, and not being cleared by
POST /api/skills/sync) is unchanged here; it is the separateF25.marketplace-stale-cachecase noted on the issue. REST schemas, plugin-over-skill name precedence and the freshinstalledcheck are unchanged.🤖 Generated with Claude Code
https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
Generated by Claude Code
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdkpython-node-runtimepython-node-runtimepython-node-runtimegolang: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:d407be8-pythonRun
All tags pushed for this build
About Multi-Architecture Support
d407be8-python) is a multi-arch manifest supporting both amd64 and arm64d407be8-python-amd64) are also available if needed