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
…-public Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S978x74H7Drsnw7bzoGWc8
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 skills catalog lives in openhands-agent-server, which this repository owns. In scope, no cross-repo move needed.
Code change verified on head 901c3f8b.
skills_service._fetch_catalog_entriesnow callsMarketplace.load(repo_path)first and reads the explicitmarketplace_pathfile only when discovery raisesFileNotFoundError/ValueError, returning[]only when neither works (skills_service.py:747-759). This mirrorsplugins_service._fetch_plugin_catalog_entries(plugins_service.py:344-376) and removes the earlymarketplaces/default.jsonexistence gate that blanked the catalog. This is the correct minimal fix for the reported symptom.- The fallback branch, plugin-over-skill precedence, plugin filtering, the fresh
installedcheck, and the REST schemas are unchanged. - Focused tests pass in the workspace:
uv run pytest tests/agent_server/test_skills_service.py tests/agent_server/test_skills_router.py -q-> 95 passed. The newTestFetchCatalogEntriescases cover the.plugin-only layout, the explicit-path fallback, and the empty-repo case. - All 51 check runs on
901c3f8bare green.
Acceptance-criteria status against issue #4665 (currently open, ready-for-dev, labeled architecture, priority:low). The issue lists five criteria:
- #1
.plugin/marketplace.jsonlayout — met. - #2 explicit marketplace path as fallback — met.
- #3 "Skills and plugins within the TTL use the same fetched/parsed snapshot" — not met. Skills and plugins still fetch and parse the repository independently, each with its own cache (
_catalog_cacheatskills_service.py:673,_plugin_catalog_cacheatplugins_service.py:308). - #4 "Transient fetch/parse failures do not cache an empty catalog" — already satisfied on this head: the PR base includes merged #5903, so
service_get_marketplace_catalogcaches only non-empty results (skills_service.py:712-715) andsync_public_skillsclears the cache (skills_service.py:491). The earlier review's criterion-#4 concern no longer applies to the current head. - #5 precedence, filtering, installed-state freshness, schemas unchanged — met.
So this head delivers a narrower scenario than the linked issue (criterion #3 unmet) while the description still says Fixes #4665. The PR body flags exactly this and asks a maintainer to choose between the minimal loader-order fix and the shared snapshot described in the issue's "Suggested Implementation" (an alternative is open PR #4785). An acceptance gap against the linked issue is a maintainer scope decision under this repository's review guide, so approval is deferred to a human maintainer.
No correctness, security, or compatibility defect was found in the code change itself; the only open item is the acceptance scope above.
Recommendation: confirm the reduced scope and retarget the closing reference (for example Refs #4665, tracking the shared-snapshot criterion #3 as a follow-up), or fold criterion #3 into this PR.
🔄 CHANGES REQUESTED
Co-authored-by: openhands <openhands@all-hands.dev>
|
📁 PR Artifacts Notice This PR contains a |
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 touches openhands-agent-server/openhands/agent_server/skills_service.py plus its tests and the verify-agent-server feature map. The skills catalog belongs to the Agent Server, which this repository owns; no cross-repo move is needed.
Code change verified on head ae35866fe (fresh fetch; prior reviews covered d407be8 and 901c3f8b, and this head is new).
- The diff against
mainis exactly three files._fetch_catalog_entriesnow callsMarketplace.load(repo_path)first — which discovers.plugin/marketplace.json/.claude-plugin/marketplace.json— and reads the explicitmarketplace_pathfile only when discovery raisesFileNotFoundError/ValueErrorand the file exists, returning[]only when neither works. This matchesplugins_service._fetch_plugin_catalog_entriesline for line and removesmain's earlymarketplaces/default.jsonexistence gate that blanked the catalog on the realOpenHands/extensionslayout. This is the correct minimal fix for the reported symptom. - Fallback error handling is unchanged from base (same inner
json.JSONDecodeError/ValidationError/OSErrorcatch), plugin-over-skill name precedence, the freshinstalledcheck, and all REST schemas are untouched — no OpenAPI drift (Check OpenAPI SchemaandREST API (OpenAPI)are green on this head). - Cache semantics are unchanged by this PR, and I re-verified them on the base (
901c3f8bc, first parent of this merge):service_get_marketplace_catalogalready caches only non-empty results andsync_public_skillsalready invalidates the catalog cache. Criterion #4 of the linked issue ("transient failures do not cache an empty catalog") is already satisfied on this head; the first bot review's concern there is stale for the current code. - Focused tests in the workspace:
uv run pytest tests/agent_server/test_skills_service.py tests/agent_server/test_skills_router.py -q→ 101 passed, including the three newTestFetchCatalogEntriescases (.plugin-only layout, explicit-path fallback, empty repo). - All check runs on
ae35866feare green, includingagent-server-tests,sdk-tests,integration-test,pre-commit, andsecurity. - One deliberate behavior change worth naming (not a defect): for a repository that ships both a manifest and the explicit file, the manifest now wins where
mainused the explicit file. That is exactly the "manifest discovery first, explicit path only as fallback" ordering the linked issue asks for, and it keeps skills consistent with the plugin catalog.
Acceptance-criteria status against linked issue #4665 (currently open, ready-for-dev, labeled bug/architecture/priority:low):
- #1
.plugin/marketplace.jsonlayout — met. - #2 explicit path as fallback — met.
- #4 transient failures not cached as empty — already met on this head (see above).
- #5 precedence/filtering/installed freshness/schemas unchanged — met.
- #3 "Skills and plugins within the TTL use the same fetched/parsed snapshot" — not met: skills and plugins still fetch and parse independently with separate caches (
_catalog_cache,_plugin_catalog_cache).
This head therefore delivers a narrower fix than the issue's suggested shared, success-only snapshot, while the PR description still says Fixes #4665. The gap is transparent — the PR body flags it and asks a maintainer to choose between this minimal loader-order fix and the shared-snapshot design (alternative PR #4785 remains open) — and the issue's author/maintainer notes the same in the issue thread. No one has recorded that maintainer decision on the issue or PR yet.
What's needed to proceed: a maintainer scope decision — either confirm the reduced scope and retarget the closing reference (e.g. Refs #4665, tracking the shared-snapshot criterion #3 and the F25.marketplace-stale-cache follow-up separately), or fold criterion #3 into this PR. No correctness, security, or compatibility defect was found in the changed code; the open item is purely this scope/acceptance decision, so approval is deferred to a human maintainer.
🔄 CHANGES REQUESTED
HUMAN:
This PR proposes a fix for a bug uncovered during the first full agentic run of
verify-agent-server.AGENT:
Why
The skills catalog returned empty before attempting the
.plugin/marketplace.jsonmanifest used by OpenHands/extensions.Summary
Try normal marketplace discovery before checking the explicit legacy path, matching the plugin catalog’s ordering.
Source: skills_service.py.
Issue Number
Fixes #4665
How to Test
Previously recorded verification
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
This is the narrow manifest-order fix. It does not implement the issue’s broader shared, success-only snapshot cache: the five-minute cache can still retain an empty result, and sync does not invalidate it (
F25.marketplace-stale-cache). Maintainer choice remains whether to take this partial fix or expand the cache design. Schema, plugin-over-skill precedence and installed checks are unchanged. Independent comparison: #4785.Visual explanation by Codex using HumanLayer show-me.
🐳 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:ae35866-pythonRun
All tags pushed for this build
About Multi-Architecture Support
ae35866-python) is a multi-arch manifest supporting both amd64 and arm64ae35866-python-amd64) are also available if needed