fix(cli): install all 7 skills via uipro init, not just the orchestrator (#362)#387
Conversation
`uipro init` rendered only the orchestrator (ui-ux-pro-max) and never delivered the 6 sibling skills (banner-design, brand, design, design-system, slides, ui-styling), so users got 1 of 7 skills (nextlevelbuilder#362). - sync-assets.mjs: bundle the 6 sub-skills into cli/assets/skills/ as static copies (source of truth: .claude/skills/), with sync + check coverage. Excludes ui-styling/canvas-fonts (~5.8MB of TTF) and __pycache__/.pyc cruft — a skill registers from its SKILL.md, not its fonts — so the bundle adds ~0.9MB, not ~6.6MB. - template.ts: after rendering the orchestrator, install each bundled sub-skill as a sibling. The skills parent is derived from the platform's skillPath (skills/ for most, prompts/ for copilot, steering/ for kiro) rather than hardcoded. - uninstall.ts: remove the sub-skills too. Verified: check:assets in sync, tsc passes, and a per-platform install harness delivers all 7 skills to the correct parent dir with no fonts. Closes nextlevelbuilder#362
|
@mrgoonie another one bites the dust. Get these PRs while they're hot. This one is a bit heavier than, but builds on my previous npm fix. |
mrgoonie
left a comment
There was a problem hiding this comment.
Blocking this one for the asset-sync regression.
npm run check:assets fails on this PR:
CLI assets are out of sync with src/ui-ux-pro-max:
- extra asset file: scripts/__pycache__/core.cpython-312.pyc
- extra asset file: scripts/__pycache__/design_system.cpython-312.pyc
The new exclusion logic filters sourceFiles, but targetFiles = await listFiles(targetDir) is not filtered in checkAssets(), so existing/generated cli/assets/scripts/__pycache__/*.pyc files are still treated as extra assets. That means the required check:assets gate fails locally even though the PR is supposed to harden against Python build cruft.
Please filter target files with the same isExcludedAssetFile predicate, or remove/avoid those generated files before comparing. After that, rerun at least:
cd cli
npm run check:assets
npm run typecheckThe overall direction is right for #362, but this cannot merge while the asset-sync check fails.
check:assets filtered sourceFiles with isExcludedAssetFile but not targetFiles, so a stray cli/assets/scripts/__pycache__/*.pyc (generated by a local Python run) was reported as an "extra asset file" and failed the gate. Apply the same predicate to targetFiles in both the dirsToSync and sub-skill loops. Verified: check:assets now passes with __pycache__/*.pyc present in the target tree; typecheck passes.
|
Good catch -- ;) fixed in You're right: Reproduced your failure (compiled cd cli
npm run check:assets # CLI assets are in sync. (exit 0, with __pycache__/*.pyc present)
npm run typecheck # tsc --noEmit (exit 0)Diff is the two |
mrgoonie
left a comment
There was a problem hiding this comment.
Summary: This PR fixes #362 by bundling the six sibling skills into cli/assets/skills/ and installing them next to the generated ui-ux-pro-max orchestrator. The earlier check:assets blocker is fixed at head 8a97d6817100ab56d6e7fab7501f6556ef301d85, and npm run check:assets now passes locally.
Risk level: Medium — packaging/install path change across all supported AI targets, with 100+ bundled asset files.
Mandatory gates:
- Duplicate / prior implementation: clear — PR #385 fixes Claude plugin registration, not
uipro init; issue #362 remains the matching CLI install gap. - Project standards: docs/config found — platform install paths come from
cli/assets/templates/platforms/*.json; asset sync is enforced bycli/scripts/sync-assets.mjs. - Strategic necessity: clear value —
uipro initmust deliver the advertised seven-skill bundle, otherwise the CLI install channel stays materially broken.
Findings:
Important
uipro uninstallstill removes from<folder>/skills/<name>for every AI type, but this PR installs sibling skills using each platform config’sskillPathparent. That means Copilot installs to.github/prompts/*and Kiro installs to.kiro/steering/*viageneratePlatformFiles(), butremoveSkillDir()only tries.github/skills/*and.kiro/skills/*. The PR description says uninstall removes sub-skills too, but for those platforms the new sibling skills (and the orchestrator, pre-existing) will be left behind. Please make uninstall derive the removal parent fromloadPlatformConfig(aiType).folderStructure.skillPath(or equivalent shared helper) instead of the legacy hardcodedskillssegment, then verify at least Copilot and Kiro uninstall paths.
Suggestion
- Local
npm run typecheckcould not be completed in this cron workspace becausetscis not installed (sh: 1: tsc: not found). This is an environment/dependency limitation here, not a PR finding;npm run check:assetsdid pass locally and the GitHub asset-sync check is green.
Verdict: Request changes
The core direction is right and the asset-sync regression is fixed. Please patch the uninstall path logic before merge so the CLI does not install seven skills but fail to remove them for non-skills/ platforms.
…ed skills/ removeSkillDir() hardcoded <folder>/skills/<name>, but the installer places skills under each platform config's skillPath parent — copilot in .github/prompts/, kiro in .kiro/steering/. So uninstall left those platforms' skills (orchestrator + sub-skills) behind. Derive the install parent from loadPlatformConfig(aiType).folderStructure (same source the installer uses), and keep the legacy <folder>/skills/ cleanup (incl. .shared/) for older installs. Deduped via a Set. Verified: typecheck passes; an install+uninstall harness removes all 7 skills with zero leftovers for claude (.claude/skills), copilot (.github/prompts) and kiro (.kiro/steering).
|
Noted. Fixed in
const { folderStructure } = await loadPlatformConfig(aiType);
parents.add(join(folderStructure.root, dirname(folderStructure.skillPath)));
// + legacy <folder>/skills (incl. .shared) for older installs, deduped via SetSo uninstall now targets each platform's real install dir -- Verified:
( |
…ped ckm- names nextlevelbuilder#385 merged to main and removed the ckm- prefix from the six .claude/skills/*/SKILL.md name fields. This branch's bundled copies under cli/assets/skills/ still carried the old ckm- names, so after the PR merges with main the source no longer matched the bundle and the check-asset-sync CI gate failed (stale asset file: skills/*/SKILL.md). Merge main and regenerate the bundle so cli/assets/skills matches the current .claude/skills source of truth. check:assets and typecheck pass.
mrgoonie
left a comment
There was a problem hiding this comment.
Re-reviewed head f7fd4dfdb4f0b0cb1f5b638d5b83376434d6c422. The uninstall blocker from the previous review is fixed: removal now derives the real parent from platform skillPath and still cleans legacy <folder>/skills paths.
Validation run locally:
npm --prefix cli run check:assets✅npm --prefix cli run typecheck✅- Manual compiled-CLI harness:
init --ai claude --forceinstalls 7 skills under.claude/skills, and uninstall removes all 7. - Manual compiled-CLI harness:
init --ai copilot --forceinstalls the orchestrator plus sibling sub-skills under.github/prompts, and uninstall removes all 7 from the real Copilot parent. - Manual compiled-CLI harness:
init --ai kiro --forceinstalls all 7 under.kiro/steering, and uninstall removes all 7.
Duplicate/prior-work check: PR #385 fixed Claude plugin registration, PR #386 fixed stale pointer dirs, and #355 fixed asset sync; none resolves issue #362's CLI install gap. This PR is the right narrow implementation for #362.
Approved. Merge is still subject to the repository's required checks and branch protection.
What & why
uipro initinstalls only 1 of 7 skills (#362). The installer renders just the orchestrator (ui-ux-pro-max) from templates and copiesdata/scriptsinto it — the 6 sibling skills (banner-design,brand,design,design-system,slides,ui-styling) are never bundled or installed.sync-assets.mjsonly mirroreddata/scripts/templates, socli/assets/had noskills/at all.Change (lean bundle)
1. Bundle the 6 sub-skills —
sync-assets.mjsnow mirrors.claude/skills/{6}→cli/assets/skills/(source of truth =.claude/skills/), withsync:assets+check:assetscoverage.ui-styling/canvas-fonts/(~5.8 MB of TTF) and__pycache__/.pyc/.coveragecruft. A skill registers from itsSKILL.md, not its fonts — so the bundle adds ~0.9 MB, not ~6.6 MB. (Font-dependent canvas rendering inui-stylingwould fetch fonts separately; out of scope here.)data/scripts/templatessync against stray local.pycfiles.2. Install all 7 —
template.tsinstalls each bundled sub-skill as a sibling of the orchestrator. The skills parent is derived from the platform'sskillPath(dirname), so it's correct per-platform —skills/for most,prompts/for copilot,steering/for kiro — not hardcoded.3. Uninstall —
uninstall.tsnow removes the sub-skills too.Verification
check:assetsin sync,tsc --noEmitpasses..claude/skills/(7), copilot →.github/prompts/(7), kiro →.kiro/steering/(7);brand/SKILL.mdpresent, no fonts in the output.Notes
Closes #362