feat(skills): redesign skills catalog — full-page editor, repo import, reliable materialization - #148
Merged
Merged
Conversation
…port, reliable materialization UX: the skill-pack editor was a modal with a NESTED catalog modal. It's now a full page (routes/skill-packs/new + $packId render a shared SkillPackEditor: left = form, right = embedded catalog panel — no nested modals). The list page is list-only and navigates to those routes. Bulk import: convex/skills.ts importSkillRepo lists a repo's skills via the GitHub trees API, fetches + caches each SKILL.md (+ the repo's AGENTS.md / CLAUDE.md), indexes them, and returns them to drop into a pack. The editor adds an 'import a GitHub repo' input (e.g. greensock/gsap-skills) and four pre-built templates. Repo input is validated/host-pinned (no SSRF); './..' rejected. Reliability (the deep dive): the ACP gateway used to SILENTLY SKIP skills whose SKILL.md wasn't cached, with no fallback — so a freshly-added skill didn't reach Claude Code until ensureSkillDetails happened to finish. Fix: extracted the GitHub fetch into app/services/skill_content.py (fetch_skill_md), refactored chat.py to share it, and the gateway now back-fills missing SKILL.md on demand — bounded by an 8s budget + a 20-skill cap so it can never stall provisioning, and authenticated via GITHUB_TOKEN (5000/hr vs 60/hr). Distinct skills sharing a trailing id no longer collide on one ~/.claude/skills dir. Adversarially verified (multi-agent): chat.py refactor is faithful; nested-modal gone; gsap path confirmed against the live repo; fixed the two operational majors (unbounded back-fill, unauthenticated fetch) the review found. Tests: new test_skill_content.py + slug/cap tests; Convex 203, FastAPI 350, frontend 235; tsc clean (no new errors).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds on Skill Packs (#147). Three things: fix the catalog UX, add bulk/repo/template import, and make skills actually reliable across both harness types.
1. UX — no more nested modals
The skill-pack editor was a modal that opened the catalog in a second modal. It's now a full page:
/skill-packs/newand/skill-packs/$packIdrender a sharedSkillPackEditorwith the form on the left and the catalog embedded on the right — no nesting. The list page is list-only and navigates to those routes. (Only the skill preview and delete remain as single, non-stacked dialogs.)2. Bulk add — repos & templates
importSkillRepolists a repo's skills via the GitHub trees API (skills/<id>/SKILL.mdetc.), fetches + caches each SKILL.md and the repo's top-level AGENTS.md / CLAUDE.md, indexes them, and returns them to drop into the pack. The editor has anowner/repoinput (e.g.greensock/gsap-skills→ 8 GSAP skills + its .md in one click)../..rejected).3. Reliability (the deep dive) — skills now actually work for Claude Code
The deep dive found a real HIGH bug: the ACP gateway silently skipped any skill whose SKILL.md wasn't cached yet, with no fallback — so a freshly-added skill didn't reach Claude Code until the frontend's
ensureSkillDetailshappened to finish (a race). The default loop already had a 3-stage GitHub fallback; the gateway didn't.Fix: extracted the GitHub fetch into
app/services/skill_content.py(fetch_skill_md), refactoredchat.pyto share it (behavior preserved — verified line-by-line), and the gateway now back-fills missing SKILL.md on demand. It's bounded by an 8s budget + 20-skill cap so it can never stall provisioning, and authenticated via GITHUB_TOKEN (5000/hr vs 60/hr). Distinct skills sharing a trailing id no longer collide on one~/.claude/skillsdir.Verification
3-dimension adversarial review:
POST /sessions) and an unauthenticated gateway fetch (60/hr shared) — both fixed (budget+cap; GITHUB_TOKEN).confirmed-correct— nested-modal gone, editor remounts per pack (no stale state), save/import/list all correct.confirmed-correct— the silent-skip bug is genuinely fixed (no remaining silent-failure path), default loop intact, import idempotent, and the gsap repo layout verified against the live API (8 skills + AGENTS.md/CLAUDE.md).Tests: new
test_skill_content.py(resolver + auth headers) + slug-collision/back-fill-cap tests. Convex 203, FastAPI 350, frontend 235; tsc clean (no new errors).Notes
GITHUB_TOKENon the FastAPI backend env to lift the gateway's GitHub rate limit (the Convex side already uses it).