feat(skills): add skill-list, skill-install, skill-update CLI commands#14
feat(skills): add skill-list, skill-install, skill-update CLI commands#14maystudios wants to merge 1 commit intomainfrom
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a new “skills” feature to the CLI so users can list installed skills, install a specific bundled skill, and update (reinstall) all built-in skills from the package bundle.
Changes:
- Introduces
packages/cli/src/core/skills.tswithcmdSkillList,cmdSkillInstall, andcmdSkillUpdate. - Wires
skill list|install|updateinto the CLI command router (packages/cli/src/cli.ts). - Exports the new skill commands via
packages/cli/src/core/index.ts.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/cli/src/core/skills.ts | Implements skill list/install/update logic, bundle copying, and helpers. |
| packages/cli/src/core/index.ts | Re-exports the new skill command functions. |
| packages/cli/src/cli.ts | Registers a new skill command with list, install, and update subcommands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** Installed skills directory under the Claude config. */ | ||
| function skillsDir(): string { | ||
| return path.join(os.homedir(), '.claude', 'agents', 'skills'); | ||
| } |
There was a problem hiding this comment.
skillsDir() hard-codes ~/.claude/agents/skills, so skill list/install/update will ignore a local .claude install in the provided cwd (the CLI already supports --cwd, and other code prefers local-first, e.g. dashboard server resolution). This will make the new commands behave unexpectedly for local installs and also makes E2E testing unsafe (would modify the developer's real home). Consider resolving skills dir as path.join(cwd, '.claude', 'agents', 'skills') when it exists (or when cwd/.claude exists), and falling back to os.homedir() only if no local config is present.
|
|
||
| /** Bundled skills directory inside the npm package. */ | ||
| function bundledSkillsDir(): string { | ||
| return path.resolve(__dirname, 'assets', 'templates', 'skills'); |
There was a problem hiding this comment.
bundledSkillsDir() resolves path.resolve(__dirname, 'assets', 'templates', 'skills'). When this module is executed from dist/core/skills.js, __dirname will be dist/core, but the packaged assets live under dist/assets/..., so this path will not exist in the published build. Adjust the resolution to point at the package-level dist/assets/templates/skills (e.g., resolve relative to ..).
| return path.resolve(__dirname, 'assets', 'templates', 'skills'); | |
| return path.resolve(__dirname, '..', 'assets', 'templates', 'skills'); |
| const bundled = bundledSkillsDir(); | ||
| const srcDir = path.join(bundled, skillName); | ||
| const srcFile = path.join(srcDir, 'SKILL.md'); | ||
|
|
||
| if (!fs.existsSync(srcFile)) { | ||
| const available = listBundledSkillNames(bundled); | ||
| error(`Skill '${skillName}' not found in bundle. Available: ${available.join(', ') || 'none'}`); | ||
| } | ||
|
|
||
| const destDir = path.join(skillsDir(), skillName); | ||
| installSkillFromBundle(srcDir, destDir); | ||
|
|
There was a problem hiding this comment.
skillName is used directly in path.join() for both the bundle source and destination paths. Because path.join() will accept .. segments and absolute paths, a crafted skillName could read from / write to unintended filesystem locations (path traversal / arbitrary write), especially since destDir is computed from user input. Validate skillName (e.g., allow only [a-z0-9-]+), reject path separators/absolute paths, and/or verify path.resolve(...) stays within the expected base directories before copying.
|
|
||
| // Remove old version if present | ||
| if (fs.existsSync(destSkillDir)) { | ||
| fs.rmSync(destSkillDir, { recursive: true }); |
There was a problem hiding this comment.
fs.rmSync(destSkillDir, { recursive: true }) may fail on Windows or with read-only files (and elsewhere in the codebase rmSync calls often include force: true). Consider adding force: true (or using the existing safeRmDir helper pattern) so skill update is resilient and doesn't leave a partially-updated install.
| fs.rmSync(destSkillDir, { recursive: true }); | |
| fs.rmSync(destSkillDir, { recursive: true, force: true }); |
| const handleSkill: Handler = (args, cwd, raw) => { | ||
| const sub = args[1]; | ||
| const handlers: Record<string, () => void> = { | ||
| 'list': () => cmdSkillList(cwd, raw), | ||
| 'install': () => cmdSkillInstall(cwd, args[2], raw), | ||
| 'update': () => cmdSkillUpdate(cwd, raw), | ||
| }; | ||
| const handler = sub ? handlers[sub] : undefined; | ||
| if (handler) return handler(); | ||
| error('Unknown skill subcommand. Available: list, install, update'); | ||
| }; |
There was a problem hiding this comment.
New skill subcommands are registered here, but there are no E2E coverage additions. The repo already has CLI E2E tests that execute the built maxsim-tools.cjs (e.g. tests/e2e/tools.test.ts). Adding tests for skill list/install/update would help catch regressions in path resolution (local vs global config) and bundled asset lookups, and should run with an isolated HOME / temp config dir to avoid touching the developer's real ~/.claude.
| /** Skills installed by MAXSIM (not user-created). */ | ||
| const BUILT_IN_SKILLS = [ | ||
| 'tdd', | ||
| 'systematic-debugging', | ||
| 'verification-before-completion', | ||
| 'code-review', | ||
| 'simplify', | ||
| 'memory-management', | ||
| 'using-maxsim', | ||
| 'batch-execution', | ||
| 'subagent-driven-development', | ||
| 'writing-plans', | ||
| ] as const; |
There was a problem hiding this comment.
BUILT_IN_SKILLS introduces a second source of truth for “built-in skills”. There is already a separate built-in skills list in the installer logic (packages/cli/src/install/index.ts, used for cleanup before copying). Keeping these in sync manually is error-prone; consider centralizing the list (or deriving it from the bundled skills directory) so skill update and install/uninstall behavior stay consistent over time.
Summary
packages/cli/src/core/skills.tsmodule withcmdSkillList,cmdSkillInstall, andcmdSkillUpdatefunctionsskill list,skill install <name>, andskill updatesubcommands in the CLI tools router (cli.ts)packages/cli/src/core/index.tsTest plan
npx vitest run)npm run build)node dist/cli.cjs skill listlists installed skillsnode dist/cli.cjs skill install tddinstalls a skill from bundlenode dist/cli.cjs skill updatere-copies all built-in skills🤖 Generated with Claude Code