feat(skills): carry a category on every catalog skill - #425
Conversation
The agent-canvas /skills page renders categories as a facet rail, and the previous values produced an unusable distribution: productivity held 16 skills, the top three buckets held 71% of all skills, and four buckets held exactly one skill. Skill entries now use nine balanced categories; plugin entries keep their existing values because they serve Claude Code marketplace browsing.
buildCatalog now joins the `category` from marketplace skill entries onto the generated catalog, keyed by skill directory name. Unknown values and cross-manifest disagreements fail the build; skills with no marketplace entry warn and fall back to "other". SKILL_CATEGORY_IDS is exported so consumers drive their UI from the package instead of re-declaring the list.
TestCodegenEndToEnd builds catalogs from ephemeral tmp_path fixtures, but buildCatalog's marketplacesDir now defaults to the real marketplaces/ directory. A future fixture named after a real skill (docker, github, linear, etc.) would silently pick up its real category or trip the real conflict/unknown-category checks. Each fixture call site now passes an explicit empty marketplaces temp dir so fixtures stay isolated, and buildCatalog's doc comment calls out the need for callers to do this. Also tightened test_conflicting_categories_across_manifests_throws to assert on the conflict message itself, not just that stderr happens to mention "docker".
The skill-category docblock was duplicated near-verbatim in the build script and index.d.ts, and its add-javadoc/github-pr-review rationale annotated marketplace JSON entries that live in neither file — 15c3dd3's message already carries it. buildCatalog's six-line warning about fixture isolation restated 93e24ec's message, which the parameter itself and the tests already convey. The four copies of the "empty marketplaces dir" comment collapse into an empty_marketplaces helper whose docstring states the isolation rule once. Also adds the one comment that was genuinely missing: why flarglebargle is excluded from test_uncovered_skills_land_in_other.
…-for-canvas-rail # Conflicts: # skills/index.js
|
@OpenHands Do a /codereview on this PR please. Check out also the linked PR from openhands/openhands so you understand the whole, and review both. Post your feedback using gh api and enyst token. |
|
I'm on it! enyst can track my progress at all-hands.dev |
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
🟢 Good taste
LGTM. I reviewed this together with OpenHands/OpenHands#16124. The catalog is the right owner for the taxonomy; the generator validates unknown and conflicting values, preserves the unrelated plugin-category semantics, gives every emitted skill a deterministic fallback, and keeps the generated JS/types/docs aligned. The generator was a clean no-op locally and the category smoke check passed.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
The change extends a generated public data contract without changing runtime execution or security-sensitive paths. Validation and fallback behavior limit malformed-catalog risk.
VERDICT:
✅ Worth merging: The producer contract is coherent and ready for the consumer PR.
KEY INSIGHT:
The taxonomy has one validated source of truth and reaches consumers through generated artifacts rather than duplicated frontend constants.
This review was generated by an AI agent (OpenHands) on behalf of enyst.
|
Reviewed and posted feedback on both PRs using the
Validation: extensions catalog generation was a clean no-op and the Node category smoke check passed. Local pytest execution was unavailable because |
3 participants
Why
The OpenHands Skills page (
/skills) shows every skill in one flat grid. With 58 public skills inthis repo alone, search is the only way to narrow it down, which only helps when you already know
what you're looking for. See OpenHands/OpenHands#16123 for the full motivation.
To group skills by topic, the UI needs a topical axis, and the catalog is the only place that can
own it. Marketplace entries already carry a
category, but the values were ad-hoc and inconsistentacross manifests (
development,productivity,integration,monitoring,documentation, …),and they were never exported from the generated catalog, so the frontend could not read them.
Summary
categoryon every skill entry acrossmarketplaces/*.jsononto a fixed nine-valuetaxonomy:
automations,environment,code-hosting,agent-authoring,code-quality,integrations,writing,design,other.scripts/build-skills-catalog.mjsnow reads those manifests, validates each skill's categoryagainst the taxonomy, fails the build on an unknown or conflicting value, and emits
categoryon every
SKILLS_CATALOGentry plus aSKILL_CATEGORY_IDSexport. Skills with no marketplaceentry fall back to
otherand are listed in a build warning.SkillCategoryId/SKILL_CATEGORY_IDSfromskills/index.d.ts.Issue Number
OpenHands/OpenHands#16123
How to Test
node scripts/build-skills-catalog.mjs # regenerates skills/index.js; should be a no-op on this branch python3 -m pytest tests/test_skills_catalog.py -qVerify the generated catalog carries the field:
node -e "import('./skills/index.js').then(m => console.log(m.SKILLS_CATALOG.slice(0,3).map(s => [s.name, s.category])))"Negative paths covered by the test suite: an unknown category value and the same skill given
conflicting categories in two manifests both throw at build time.
Result of
python3 -m pytest tests/test_skills_catalog.py -q: 38 passed.Video/Screenshots
n/a — build-time/catalog change with no UI of its own. The consuming UI is shown in
OpenHands/OpenHands#16124.
Notes
categoryon skill entries now means something different fromcategoryon plugin entries.Only entries whose
sourcestarts with./skills/are re-keyed onto this taxonomy; pluginentries keep their existing free-form values for Claude Code marketplace browsing. If anything
downstream groups skill entries by the old values, this changes what it sees. Flagging for
reviewers who know the marketplace consumers better than I do — happy to move this onto a
separate
skill_categoryfield instead if the shared key is a problem.qa-changes,release-notes) have no marketplace entry and so land inother.Adding entries would mean creating
.plugin/plugin.jsonplus vendor symlinks, i.e. publishingthem as Codex/Claude Code plugins — out of scope here, and the fallback is deliberate rather
than accidental.
buildCatalog()takes an optionalmarketplacesDirso fixture-based tests can point at an emptydirectory instead of inheriting categories from this repo's real manifests.
otherfor any category it doesn'tknow, so a newer catalog against an older frontend renders rather than breaking.