feat: add configurable marketplace_path setting for public skills loading

[neubig]

Summary

Adds the remaining marketplace_path plumbing needed to control which marketplace is used when loading public skills.

Closes #2297

Details

Overlap audit against main

A separate PR already landed the mixed-marketplace example and Marketplace.skills[] support on main, so this PR was reduced to the still-missing pieces only:

• AgentContext.marketplace_path
• load_available_skills(..., marketplace_path=...)
• load_public_skills(..., marketplace_path=...)
• agent-server /skills request plumbing for marketplace_path
• targeted tests for custom and invalid marketplace paths

Behavior

• Default behavior is unchanged when callers do not pass anything: public skills still use marketplaces/default.json
• Callers can pass a custom relative marketplace path such as marketplaces/custom.json
• Callers can pass None to load all public skills without marketplace filtering
• Invalid custom marketplace paths return no public skills instead of silently broadening to all skills

Testing

uv run ruff check \
  openhands-agent-server/openhands/agent_server/skills_router.py \
  openhands-agent-server/openhands/agent_server/skills_service.py \
  openhands-sdk/openhands/sdk/context/agent_context.py \
  openhands-sdk/openhands/sdk/context/skills/skill.py \
  tests/agent_server/test_skills_service.py \
  tests/sdk/context/skill/test_load_public_skills.py

uv run pytest tests/sdk/context/skill/test_load_public_skills.py \
  tests/agent_server/test_skills_service.py -q

Result:

52 passed in 0.14s

Evidence

Verification link: View conversation

Live marketplace-path demo against a temporary git repo:

$ HOME=/tmp/sdk-evidence-home uv run python - <<'PY'
from openhands.sdk.context.skills.skill import load_public_skills
repo_url = 'file:///tmp/marketplace-evidence-repo'
custom = sorted(
    skill.name
    for skill in load_public_skills(
        repo_url=repo_url,
        branch='main',
        marketplace_path='marketplaces/custom.json',
    )
)
all_skills = sorted(
    skill.name
    for skill in load_public_skills(
        repo_url=repo_url,
        branch='main',
        marketplace_path=None,
    )
)
print('custom_marketplace', custom)
print('all_public_skills', all_skills)
PY
custom_marketplace ['git', 'internal-only']
all_public_skills ['docker', 'git', 'internal-only']

This shows the branch's behavior directly:

• a custom marketplace path selects only the curated subset
• marketplace_path=None loads all public skills from the same repository

Checklist

• Overlap with main audited and redundant scope removed
• Targeted lint/tests pass
• Live CLI evidence added
• PR description matches the reduced diff

[github-actions]

API breakage checks (Griffe)

Result: Passed

Action log

[github-actions]

Agent server REST API breakage checks (OpenAPI)

Result: Failed

Log excerpt (first 1000 characters)

{"asctime": "2026-03-07 21:01:31,792", "levelname": "WARNING", "name": "openhands.agent_server.config", "filename": "config.py", "lineno": 173, "message": "\u26a0\ufe0f OH_SECRET_KEY was not defined. Secrets will not be persisted between restarts."}
::error title=openhands-agent-server REST API::Breaking REST API change detected without MINOR version bump (1.12.0 -> 1.12.0).

Breaking REST API changes detected compared to baseline release:
- the 'file' request property type/format changed from 'string'/'' to 'string'/'binary'
/home/runner/work/software-agent-sdk/software-agent-sdk/.venv/lib/python3.13/site-packages/litellm/llms/custom_httpx/async_client_cleanup.py:66: DeprecationWarning: There is no current event loop
  loop = asyncio.get_event_loop()

Action log

[github-actions]

Coverage Report •

File	Stmts	Miss	Cover	Missing
openhands-agent-server/openhands/agent_server
skills_service.py	133	35	73%	117, 151, 154–157, 160–162, 165–166, 169–173, 176–180, 182, 186–187, 189, 200–204, 341–342, 346–348
openhands-sdk/openhands/sdk/context
agent_context.py	127	6	95%	276–278, 307, 330, 336
openhands-sdk/openhands/sdk/context/skills
skill.py	433	35	91%	92–93, 248–249, 435–441, 444, 533–536, 711–712, 806–807, 838–839, 896–897, 965, 993, 1016, 1023–1024, 1068–1069, 1075–1076, 1082–1083
TOTAL	20182	5151	74%

[all-hands-bot]

🟡 Acceptable - Works but significant complexity concerns

This PR adds marketplace path configuration, but the implementation introduces substantial complexity that may not be justified by the use case. The core functionality works, but there are important questions about design choices and breaking changes.

Key concerns:

1. 🟠 Cache directory rename breaks existing installations
2. 🟠 load_public_skills() function complexity increased significantly
3. 🟡 Cross-repo marketplace loading - unclear if this solves a real problem
4. 🟡 Multiple helper functions for parsing different path formats

Questions:

• What is the actual use case for cross-repo marketplace references?
• Is the added complexity (cross-repo loading, multiple cache management) solving a real problem or a theoretical one?

See inline comments for specific issues.

[enyst]

@OpenHands Do a /codereview-roasted on this PR. Note that we know and intend the change in public skills location, it’s FINE, ignore that one. Post your review as a top-level comment to the PR.

[openhands-ai]

I'm on it! enyst can track my progress at all-hands.dev

[enyst]

/codereview-roasted (ignoring the intended public-skills location change)

Taste Rating

🔴 Needs improvement

[CRITICAL ISSUES]

• [openhands-sdk/openhands/sdk/context/skills/skill.py, Lines 1045–1059] Correctness / data model mismatch: you parse plugin.source into (repo_spec, skill_name) but then ignore the parsed skill_name and always try to load plugin.name from disk. That only works if plugin.name == basename(source) forever. If a marketplace ever uses a human-friendly name/alias, or if a cross-repo source points at a different directory name, this will silently fail to load the skill.
  Fix: treat source as the canonical locator for filesystem lookup; keep plugin.name only as display/ID if you need it.

• [openhands-sdk/openhands/sdk/context/skills/skill.py, Line 950] Correctness: source.lstrip("./") is not “remove the ./ prefix”. It strips any . or / characters until it hits something else. ../foo becomes foo, .../foo becomes foo, etc. That’s not parsing, that’s string-mangling.
  Fix: use removeprefix("./") (and consider explicitly rejecting ../ rather than silently rewriting it).

• [openhands-sdk/openhands/sdk/context/skills/skill.py, Lines 1016–1023] + [openhands-sdk/openhands/sdk/context/skills/utils.py, Lines 385–401] Behavior surprise: when marketplace_path includes owner/repo:... but no @branch, parse_marketplace_path() returns branch="main" and load_public_skills() overwrites the caller-provided branch argument. So load_public_skills(branch="dev", marketplace_path="someone/repo:marketplaces/x.json") still loads main.
  Fix: either clearly document “branch is ignored when repo is embedded in marketplace_path”, or (better taste) only override branch when an explicit @branch is present.

[IMPROVEMENT OPPORTUNITIES]

• [openhands-sdk/openhands/sdk/context/skills/skill.py, Lines 954–1116] Complexity: load_public_skills() is now doing: parsing input, git cache management, marketplace parsing, multi-repo loading, and dedupe. You started extracting helpers, but the function is still a “do everything” blob. This is how bugs creep in.
  Suggestion: split into 2–3 smaller functions (resolve_marketplace_repo(), load_skills_from_repo(), etc.) so the control flow is obvious.

• [openhands-sdk/openhands/sdk/context/skills/utils.py, Lines 339–363] Cache key collisions / path hygiene: get_cache_name_from_url() can collide across hosts (github vs gitlab same owner/repo) and can produce names with characters like @ / : for SSH-ish URLs. That’s not security-critical, but it’s a correctness/maintenance footgun.
  Suggestion: normalize via urllib.parse + conservative filename sanitization (or hash the URL and keep a readable prefix).

[TESTING GAPS]

• [tests/sdk/context/skill/test_load_public_skills.py, Lines 794–870] The new test covers “custom marketplace file in the same repo” and marketplace_path=None, which is good. But you introduced cross-repo loading and source parsing and there’s no test coverage for the parts most likely to break.
  • Add a test where marketplace plugins include a cross-repo source and update_skills_repository() is patched to return different repo dirs based on URL.
  • Add a test where plugin.name != basename(plugin.source) (even if you think it’ll never happen). If it’s truly not supported, then delete parse_skill_source() and the cross-repo doc claims — don’t ship dead parsing code.

VERDICT

❌ Needs rework before this is safe to merge. The feature goal is fine; the current “name vs source” ambiguity is not.

KEY INSIGHT: you added a second identifier (source) but still behave like name is the only truth. Pick one canonical locator and make the rest derived.

[openhands-ai]

I retrieved PR #2253’s metadata and diff via the GitHub CLI, then performed a /codereview-roasted focusing on correctness, breakage risk, complexity, and test coverage (while explicitly ignoring the intended “public skills location” change per your instruction).

I posted the review as a top-level comment on the PR. During posting, my first attempt got mangled by shell interpolation (backticks/$()), so I patched the same comment via the GitHub API to restore the intended content.

Top-level review comment link:
#2253 (comment)

[all-hands-bot]

🟡 Acceptable - Core functionality works, but extracting unused data violates good taste.

Key Insight: The marketplace source field is extracted but never used anywhere—both call sites immediately discard it. Either use it or don't extract it.

Review Decision: COMMENT (not APPROVE) because:

1. Changes skill loading behavior (per eval/benchmark risk policy)
2. Unnecessary complexity in data extraction
3. PR is marked DRAFT

[github-actions]

📁 PR Artifacts Notice

This PR contains a .pr/ directory with PR-specific documents. This directory will be automatically removed when the PR is approved.

For fork PRs: Manual removal is required before merging.

[all-hands-bot]

🟡 Acceptable - Code works, but use case unclear

Assessment: The code is technically sound after previous simplification rounds. Earlier complexity issues (cross-repo loading, unused data extraction) have been properly addressed.

Key Observations:

1. Pragmatism Question: The PR exposes marketplace_path as configurable but doesn't explain what real problem this solves. Is this for actual user requests, internal OpenHands workflows, or theoretical flexibility? Adding configurability without a concrete use case goes against the "solve real problems" principle.

2. Breaking Change: Cache directory rename (public-skills → openhands-extensions) is acknowledged and acceptable per resolved threads.

3. Implementation: The simplified version is clean:

   • Helper functions are reasonable extractions
   • Return type change from set[str] to list[str] preserves marketplace order (good choice)
   • Test coverage adequate

Review Decision: 💬 COMMENT (not APPROVE) because:

• PR changes skill loading behavior (falls under eval/benchmark risk policy)
• PR is marked DRAFT
• Use case justification unclear

Recommendation: Before merging, clarify the use case in the PR description and run lightweight evals to ensure no regression in agent behavior.

---

KEY INSIGHT: Technically solid implementation, but needs clearer motivation for why this configurability is necessary.

[neubig]

Addressed some review concerns!

[all-hands-bot]

🟡 Acceptable - Core functionality works after simplification rounds. One minor style issue in .pr/ artifacts.

Review Decision: COMMENT per eval/benchmark policy - this changes skill loading behavior (filters which skills are loaded into agent context). A human maintainer should verify with lightweight evals before merging.

Key Observations:

• ✅ Solves real use case (neubig/workflow multi-repo pattern)
• ✅ Helper functions are reasonable extractions
• ✅ Good test coverage for new parameter
• ⚠️ Minor: .pr/ demo script uses sys.path.insert hack (acceptable for temporary artifacts)

Verdict: Worth merging pending eval verification.

[all-hands-bot]

🟡 Acceptable - Implementation is solid after review rounds, but has one blocking issue (breaking API change without deprecation) and requires eval verification per policy.

[enyst]

OpenHands-GPT-5.2 here.

Style and Formatting

• No issues.

Clarity and Readability

• No issues.

Security and Common Bug Patterns

• No issues.

Testing and Behavior Verification
[openhands-sdk/openhands/sdk/context/skills/skill.py, Lines 1012–1104] 🧪 The PR advertises Claude Code compatibility for plugins[], but _load_skill_from_source only checks for SKILL.md at the plugin root (or <name>.md). Standard Claude plugin layouts keep skills under skills/<skill>/SKILL.md, so plugin entries that point at plugin roots will load zero skills. Either load skills via load_skills_from_dir/plugin manifest or clarify that plugins[] entries must point directly at skill directories. A test covering a plugin with skills/ layout would catch this.

[enyst]

@OpenHands Understand this PR and look at this comment #2253 (review)

Read agentskills.io spec and the claude code plugin spec, to understand the context we want to keep.

Also, find the latest issue posted by xingyaoww and closed, about plugins and skills, and read it in full.

Then analyze the behavior of this PR and tell us what it does and how it does it.

Take a deep breath and publish it as a review using gh api (not as a comment, as a review it's allowed)

[openhands-ai]

I'm on it! enyst can track my progress at all-hands.dev

[enyst]

Review — behavior vs specs

What this PR does

• Introduces a configurable marketplace (URL/path) that replaces the old repo_url/branch inputs for load_public_skills, with a new DEFAULT_MARKETPLACE pointing to https://raw.githubusercontent.com/OpenHands/extensions/main/marketplaces/default.json.
• Updates AgentContext + agent-server skill loading to thread that marketplace value through (load_available_skills(..., marketplace=...)).
• Extends the marketplace schema with an OpenHands-only skills[] array and metadata.skillRoot, alongside Claude Code–compatible plugins[] and metadata.pluginRoot.
• Changes cached repo naming from a fixed public-skills/ to a derived owner-repo/ (e.g., openhands-extensions/), causing a one-time re-clone.
• Makes sync_public_skills a no-op (skills are fetched on demand).

AgentSkills spec alignment (agentskills.io)

• ✅ AgentSkills expects each skill to be a directory containing SKILL.md, with optional scripts/, references/, and assets/. The new skills[] loader treats each entry as a skill directory and loads SKILL.md from that path, which matches the spec and keeps the progressive disclosure model intact.
• ✅ skillRoot matches the idea of a canonical skills directory; relative skill paths are resolved under it.

Claude Code plugin marketplace spec alignment (code.claude.com)

• ✅ metadata.pluginRoot is supported and applied to relative plugin source paths.
• ⚠️ Plugin layout mismatch: Claude Code’s marketplace examples show plugins with skills/<skill>/SKILL.md under the plugin root. This PR’s plugin loader path (_load_skill_from_source) only checks for SKILL.md directly at the plugin root (or <name>.md), and does not traverse skills/ within the plugin. As a result, a standard Claude Code plugin entry (source: ./plugins/review-plugin) yields zero skills unless the plugin is “flat” (root-level SKILL.md) or the marketplace source points directly at a skill subdirectory.
  • This mirrors the earlier review note: plugin entries that follow the official Claude Code layout won’t load any skills in this PR.

Related closed issue (#2272 by xingyaoww)

• The issue identifies that root-level SKILL.md in anthropics/skills isn’t discovered by the plugin loader (it only scans skills/).
• This PR’s marketplace loading path does check the plugin root for SKILL.md, so marketplace-based loading would work for a flat skill. However, it does not change the actual plugin loader (Plugin.get_all_skills), so the issue remains unsolved for installed plugins—and it still doesn’t handle the canonical skills/<skill>/SKILL.md layout.

Behavioral details (important edge cases)

• Raw GitHub URLs are parsed and cloned to enable relative source paths to resolve, aligning with Claude’s warning that relative paths only work with git-based marketplaces.
• For non-GitHub HTTP URLs, the code fetches only the JSON (no repo clone). Relative source strings therefore fail (base_dir=None), again matching Claude’s “URL-based marketplaces can’t resolve relative paths.”

Potential follow-ups

1. If Claude Code compatibility for plugins[] is a goal, the loader should either:
   • scan plugin_root/skills/**/SKILL.md (similar to load_skills_from_dir), or
   • instantiate the plugin and call its _load_skills (which already knows the skills/ layout).
2. Consider a test that uses the official plugin layout (plugins/<plugin>/skills/<skill>/SKILL.md) to guard against regressions.
3. The PR description mentions marketplace_path, but the actual API is marketplace — worth reconciling to avoid confusion.

Overall, the skills[] extension aligns well with AgentSkills, but Claude Code plugin compatibility is currently partial because the loader assumes a flat SKILL.md under each plugin source rather than skills/<skill>/.

[neubig]

I'm sorry, I'm going to work on this more.

[neubig]

Moved to draft - merge conflicts

This PR has merge conflicts (mergeable=false). Please rebase or merge from main to resolve.

Also missing: Conversation verification link in Evidence section.

[all-hands-bot]

🟡 Acceptable - Pragmatic solution after multiple simplification rounds. Requires human eval verification per policy.

Taste Rating: This PR solves a real problem (custom marketplace support for multi-repo patterns). Previous iterations removed cross-repo complexity and over-engineering, which is good. Current implementation is straightforward.

Key Insight: The marketplace abstraction cleanly separates "what to load" (marketplace JSON) from "where to load from" (repo cloning), eliminating the need for complex cross-repo logic.

Review Decision: COMMENT per eval/benchmark policy - this changes skill loading behavior (which skills are loaded and how they are resolved). A human maintainer should verify with lightweight evals before merging.

[enyst]

Thank you!