From 7c5f180a91e8b520ae4a1f0e1bbaf5882c09b66a Mon Sep 17 00:00:00 2001 From: Hoang Nguyen Date: Sat, 22 Aug 2026 14:15:58 +0000 Subject: [PATCH 1/4] feat(cli): remove skill registry --- CHANGELOG.md | 4 + ...026-08-22-feature-skill-remove-registry.md | 71 +++++++++++ ...026-08-22-feature-skill-remove-registry.md | 59 +++++++++ ...026-08-22-feature-skill-remove-registry.md | 55 +++++++++ ...026-08-22-feature-skill-remove-registry.md | 51 ++++++++ ...026-08-22-feature-skill-remove-registry.md | 72 +++++++++++ .../cli/src/__tests__/commands/skill.test.ts | 113 +++++++++++++++++- packages/cli/src/__tests__/lib/Config.test.ts | 26 ++++ .../src/__tests__/lib/GlobalConfig.test.ts | 22 ++++ .../src/__tests__/lib/SkillManager.test.ts | 27 +++++ .../src/__tests__/util/skill-registry.test.ts | 46 +++++++ packages/cli/src/commands/skill.ts | 65 +++++++++- packages/cli/src/constants.ts | 77 ++++++++++++ packages/cli/src/lib/Config.ts | 13 +- packages/cli/src/lib/GlobalConfig.ts | 18 ++- packages/cli/src/lib/SkillIndex.ts | 11 ++ packages/cli/src/lib/SkillManager.ts | 4 + packages/cli/src/util/skill-registry.ts | 22 ++++ web/content/docs/7-skills.md | 15 +++ 19 files changed, 766 insertions(+), 5 deletions(-) create mode 100644 docs/ai/design/2026-08-22-feature-skill-remove-registry.md create mode 100644 docs/ai/implementation/2026-08-22-feature-skill-remove-registry.md create mode 100644 docs/ai/planning/2026-08-22-feature-skill-remove-registry.md create mode 100644 docs/ai/requirements/2026-08-22-feature-skill-remove-registry.md create mode 100644 docs/ai/testing/2026-08-22-feature-skill-remove-registry.md create mode 100644 packages/cli/src/__tests__/util/skill-registry.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b1c6ffa..edceba4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,10 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +- Added `skill remove-registry` with scoped config removal, offline index cleanup, default-registry protection, and cache/install preservation. + ## [0.54.0] - 2026-08-21 - [8284e76](https://github.com/codeaholicguy/ai-devkit/pull/194) Indexed cached registry skills. diff --git a/docs/ai/design/2026-08-22-feature-skill-remove-registry.md b/docs/ai/design/2026-08-22-feature-skill-remove-registry.md new file mode 100644 index 00000000..5517c0c5 --- /dev/null +++ b/docs/ai/design/2026-08-22-feature-skill-remove-registry.md @@ -0,0 +1,71 @@ +--- +phase: design +title: Skill Registry Removal Design +description: Safe scoped config removal and local index cleanup +--- + +# Skill Registry Removal Design + +## Architecture Overview + +```mermaid +flowchart LR + CLI[remove-registry command] --> Validate[validateRegistryId] + Validate --> Read[read project/global/default maps] + Read --> Plan[planSkillRegistryRemove] + Plan --> Config[selected ConfigManager] + Config --> Index[local focused index cleanup] + Index -. no access .-> Network[(Network)] + Index -. preserved .-> Cache[(Registry cache)] + Config -. untouched .-> Installs[(Installed skills)] +``` + +The command resolves precedence and messages. Config managers persist the pure planner result. `SkillIndex` performs a focused local inverse of registry indexing. + +## Data Models + +```ts +type SkillRegistryRemoveStatus = 'removed' | 'not-registered'; +interface SkillRegistryRemoveMutation { + registries: Record; + status: SkillRegistryRemoveStatus; + removedUrl?: string; +} +``` + +The planner tests own-property presence, copies the input, and omits only the selected ID. + +## API Design + +- `skill remove-registry [-g|--global]` +- `planSkillRegistryRemove(registries, id)` is pure. +- Project/global config managers expose `removeSkillRegistry(id)`. +- `SkillManager.removeSkillIndexForRegistry(id)` delegates to local filtering of `skills[].registry` and `meta.registryHeads[id]`. +- Exact output follows the approved exploration, including shadow reports and partial-success repair guidance. + +## Component Breakdown + +| Component | Change | +|---|---| +| `util/skill-registry.ts` | Pure removal planner and types | +| `Config.ts`, `GlobalConfig.ts` | Scoped removal writers | +| `SkillIndex.ts`, `SkillManager.ts` | Local focused index cleanup | +| `commands/skill.ts` | Command and scope/default protection | +| Tests/docs | Behavior, exact copy, and follow-ups | + +## Design Decisions + +- Mutate only the selected scope. +- Config plus focused index cleanup avoids stale search results. +- Never use the network; a lower source repopulates on later refresh. +- Built-in/default sources are read-only, while configured shadows remain removable. +- Preserve cache to protect symlink-backed installs across projects. +- Keep `remove-registry` paired with `add-registry`; defer registry-group migration. +- Defer `--purge-cache`; a future version must require `--yes` in non-TTY use, protect effective/built-in sources, and never remove installed skills. + +## Non-Functional Requirements + +- Local complexity is linear in index size. +- Writes retain existing config/index safety conventions. +- Invalid IDs cannot influence filesystem paths. +- Failures distinguish pre-write rejection from repairable post-write index failure. diff --git a/docs/ai/implementation/2026-08-22-feature-skill-remove-registry.md b/docs/ai/implementation/2026-08-22-feature-skill-remove-registry.md new file mode 100644 index 00000000..b73dc6de --- /dev/null +++ b/docs/ai/implementation/2026-08-22-feature-skill-remove-registry.md @@ -0,0 +1,59 @@ +--- +phase: implementation +title: Skill Registry Removal Implementation +description: Implementation record for skill remove-registry +--- + +# Skill Registry Removal Implementation + +## Development Setup + +Use repository Node/npm. Run `npm ci` and `npm run build` before full gates. + +## Code Structure + +Changes are confined to CLI registry utilities, config managers, skill index/manager, command registration, their tests, and user/lifecycle documentation. + +## Implementation Notes + +- Follow red-green-refactor for planner, config, index, and command slices. +- Reuse validation and add-registry scope conventions. +- Keep the planner copy-on-write and I/O-free. +- Remove only the selected map entry and clean index data locally. +- Never call registry fetch/update during removal. +- Preserve cache and installed skills and say so in successful output. +- No `--purge-cache`, `--yes`, or registry-group migration in v1. +- Implemented the pure own-property removal planner and project/global persistence methods. Targeted planner/config suites pass (71 tests). +- Implemented focused index filtering and the scoped command. The complete shipped default-registry ID snapshot is embedded for deterministic offline protection; configured shadows remain removable. + +## Integration Points + +The command reads project/global registries and default metadata, writes one config manager, then invokes focused derived-index cleanup. Lower-precedence shadows are reported and repopulate on a later refresh. + +## Error Handling + +Reject invalid IDs and default-only registries before writes. Wrong-scope errors explain the correct flag. Missing IDs list sorted project/global registrations. After config write, index failure reports partial success and the rebuild command. + +## Performance Considerations + +Config maps are small; focused cleanup is a linear local index pass with no network latency. + +## Security Notes + +Validate IDs before use. Never mutate built-in/default data, cache directories, or installed skill paths. + +## Validation Evidence + +Fresh validation on 2026-08-22 completed after resuming the interrupted session: + +- `npm run build`: exit 0; Nx built all 6 projects. +- `npm test`: exit 0; all 6 projects passed (1,962 tests across 140 files). +- `npm run lint`: exit 0; all 6 projects passed with 4 existing unused-catch warnings and no errors. +- `npx ai-devkit@latest lint`: exit 0. +- `npx ai-devkit@latest lint --feature skill-remove-registry`: exit 0. +- Targeted Vitest command for planner, config, index/manager, and command suites: exit 0; 184 tests across 5 files. +- Planner-module coverage: 100% statements, branches, functions, and lines (12/12 statements, 11/11 branches, 2/2 functions, 12/12 lines). +- `node dist/cli.js skill remove-registry --help`: exit 0; exposes `-g, --global` and standard help only. +- `git diff --check`: exit 0. + +Optional task tracing was unavailable: `npx ai-devkit@latest task list --name skill-remove-registry --json` returned `error: unknown command 'task'`. diff --git a/docs/ai/planning/2026-08-22-feature-skill-remove-registry.md b/docs/ai/planning/2026-08-22-feature-skill-remove-registry.md new file mode 100644 index 00000000..e4413869 --- /dev/null +++ b/docs/ai/planning/2026-08-22-feature-skill-remove-registry.md @@ -0,0 +1,55 @@ +--- +phase: planning +title: Skill Registry Removal Plan +description: TDD implementation and validation tasks +--- + +# Skill Registry Removal Plan + +## Milestones + +- [x] Planner and persistence behavior implemented with tests. +- [x] Command and local index behavior implemented with exact-copy tests. +- [x] Documentation and full local validation gates completed. +- [ ] Commit and PR publication completed. + +## Task Breakdown + +### Phase 1: Pure behavior and persistence + +- [x] Add failing planner tests covering every branch and immutability. +- [x] Implement `planSkillRegistryRemove`; final coverage gate remains. +- [x] Add failing project/global config preservation tests, then removal methods. + +### Phase 2: Index and command + +- [x] Add failing focused-index tests for filtering, sibling preservation, missing index, and metadata. +- [x] Implement local-only index cleanup and SkillManager delegation. +- [x] Add failing command tests for validation, scopes, shadows, defaults, messages, index failure, and no network. +- [x] Implement `remove-registry` beside `add-registry` without purge flags. + +### Phase 3: Integration and polish + +- [x] Update user docs, changelog, implementation, and testing records. +- [x] Run targeted tests/coverage, build, full workspace tests, lint, and lifecycle lint. +- [ ] Create a conventional commit and open a PR when requested. + +## Dependencies + +Planner precedes persistence; persistence/index APIs precede command integration. Existing add-registry patterns and the approved exploration are authoritative. No external service is required. + +## Timeline & Estimates + +Single feature iteration: implementation and targeted tests, documentation, full gates, review/publish. + +## Risks & Mitigation + +- Stale discovery: focused local cleanup after config write. +- Wrong-scope deletion: inspect both maps, mutate one, emit actionable hints. +- Broken installed skills: never delete cache or installations. +- Default deletion: only configured shadows reach removal. +- Partial failure: print repair instructions without unsafe rollback. + +## Resources Needed + +Existing CLI/config/index modules, Vitest suites, approved exploration, lifecycle skills, npm workspace tooling, and GitHub CLI. diff --git a/docs/ai/requirements/2026-08-22-feature-skill-remove-registry.md b/docs/ai/requirements/2026-08-22-feature-skill-remove-registry.md new file mode 100644 index 00000000..d55ec6a5 --- /dev/null +++ b/docs/ai/requirements/2026-08-22-feature-skill-remove-registry.md @@ -0,0 +1,51 @@ +--- +phase: requirements +title: Skill Registry Removal Requirements +description: Add the safe inverse of skill add-registry +--- + +# Skill Registry Removal Requirements + +## Problem Statement + +Users can register project or global skill registries with `skill add-registry`, but cannot unregister them. Manual config edits can leave stale search-index entries and make scope precedence unclear. + +## Goals & Objectives + +- Add `skill remove-registry ` beside `add-registry`. +- Default to project scope; use `-g`/`--global` for global scope. +- Remove only the selected config entry and locally clean stale index data without network access. +- Preserve cache repositories and installed skills. +- Protect built-in/default registries while allowing a user shadow to be removed. + +Non-goals: cache purge, installed-skill removal, registry-group command migration, or changes to unrelated update behavior. + +## User Stories & Use Cases + +- Remove a project registry without affecting a same-ID global registration. +- Remove only a global registration with `--global`. +- When removing a shadow, report which lower-precedence source remains active. +- Give automation deterministic exact messages without prompts or network traffic. +- Give actionable wrong-scope hints or a sorted registry inventory. + +## Success Criteria + +- Reuse `validateRegistryId` before config/index work. +- A pure copy-on-write planner returns `removed` or `not-registered`, including the removed URL. +- Config writers preserve unrelated keys and registry entries. +- Removed-only registry entries leave the local index; sibling data remains intact. +- If another source remains, invalidate the ID locally for later refresh. +- Built-in/default-only sources cannot be removed; user shadows can. +- Tests give the planner 100% coverage and cover scope, messages, preservation, and no-network behavior. +- User docs and changelog describe the command and safe boundary. + +## Constraints & Assumptions + +- Configuration is written before derived-index cleanup. Index failure reports the rebuild command and does not roll config back. +- Built-in/default registry metadata is read-only. Removal never fetches registry data. +- Cache and installations are always untouched in v1. +- `--purge-cache` is deferred with future safety semantics documented. + +## Questions & Open Items + +All material questions are resolved by the approved design. Follow-ups: guarded `--purge-cache` and coordinated registry-group aliases/migration. diff --git a/docs/ai/testing/2026-08-22-feature-skill-remove-registry.md b/docs/ai/testing/2026-08-22-feature-skill-remove-registry.md new file mode 100644 index 00000000..f6eb896b --- /dev/null +++ b/docs/ai/testing/2026-08-22-feature-skill-remove-registry.md @@ -0,0 +1,72 @@ +--- +phase: testing +title: Skill Registry Removal Testing +description: Coverage and validation strategy for skill remove-registry +--- + +# Skill Registry Removal Testing + +## Test Coverage Goals + +- 100% statements, branches, functions, and lines for `planSkillRegistryRemove`. +- Critical command, persistence, index, and failure paths covered. +- Full workspace test and lint gates remain green. + +## Unit Tests + +### Pure planner + +- [x] Present/absent IDs return correct status and removed URL. +- [x] Input is immutable; siblings, empty maps, and inherited properties behave correctly. +- [x] Coverage demonstrates 100% statements, branches, functions, and lines for the planner module. + +### Config and local index + +- [x] Project/global removers preserve unrelated registries and config keys. +- [x] Existing missing/malformed-config guarantees remain intact in the full config suites. +- [x] Index cleanup drops only target skills/head, preserves siblings, and no-ops when absent. + +## Integration Tests + +- [x] Default removes only project; `-g`/`--global` remove only global. +- [x] Cross-scope and built-in/default shadows report the revealed source. +- [x] Wrong-scope, protected, and missing errors use exact copy and sorted inventories. +- [x] Invalid IDs cause no reads/writes. +- [x] Index failure reports partial success and rebuild guidance. +- [x] Registry network fetch/update is mocked and asserted unused. + +## End-to-End Tests + +- [x] CLI help exposes only the scope option beside `add-registry`. +- [x] Full workspace build/tests/lint and lifecycle lint pass. +- [x] Adjacent add-registry behavior remains green. + +## Test Data + +Use command mocks and temporary filesystem fixtures. Seed mixed registry entries and heads to prove sibling preservation. + +## Test Reporting & Coverage + +Run targeted Vitest suites and planner coverage, then repository-native full test/lint gates. Record fresh results during completion. + +Fresh results from 2026-08-22: + +- Targeted suites: 5 files passed, 184 tests passed. +- Planner module: 100% statements (12/12), branches (11/11), functions (2/2), and lines (12/12). +- Workspace build: 6 projects passed. +- Workspace tests: 6 projects passed; 140 files and 1,962 tests passed. +- Workspace lint: 6 projects passed with no errors; 4 unrelated existing warnings were reported. +- Base and feature lifecycle lint: passed. +- Built CLI help: exit 0 and lists `-g, --global` plus standard `--help`, with no purge option. + +## Manual Testing + +Inspect CLI help and exact output assertions; no browser/device checks apply. + +## Performance Testing + +No load test is required; the operation is bounded local filtering. + +## Bug Tracking + +Fix regressions before review and document intentional follow-ups. diff --git a/packages/cli/src/__tests__/commands/skill.test.ts b/packages/cli/src/__tests__/commands/skill.test.ts index cbdb1f5f..4e3501e7 100644 --- a/packages/cli/src/__tests__/commands/skill.test.ts +++ b/packages/cli/src/__tests__/commands/skill.test.ts @@ -9,15 +9,19 @@ const mockListSkills = vi.fn(); const mockRemoveSkill = vi.fn(); const mockCacheRegistry = vi.fn(); const mockUpdateSkillIndexForRegistry = vi.fn(); +const mockRemoveSkillIndexForRegistry = vi.fn(); const mockProjectGetSkillRegistries = vi.fn(); const mockProjectAddSkillRegistry = vi.fn(); +const mockProjectRemoveSkillRegistry = vi.fn(); const mockGlobalGetSkillRegistries = vi.fn(); const mockGlobalAddSkillRegistry = vi.fn(); +const mockGlobalRemoveSkillRegistry = vi.fn(); vi.mock('../../lib/Config.js', () => ({ ConfigManager: vi.fn(function () { return { getSkillRegistries: (...args: unknown[]) => mockProjectGetSkillRegistries(...args), addSkillRegistry: (...args: unknown[]) => mockProjectAddSkillRegistry(...args), + removeSkillRegistry: (...args: unknown[]) => mockProjectRemoveSkillRegistry(...args), }; }), })); @@ -25,6 +29,7 @@ vi.mock('../../lib/GlobalConfig.js', () => ({ GlobalConfigManager: vi.fn(function () { return { getSkillRegistries: (...args: unknown[]) => mockGlobalGetSkillRegistries(...args), addSkillRegistry: (...args: unknown[]) => mockGlobalAddSkillRegistry(...args), + removeSkillRegistry: (...args: unknown[]) => mockGlobalRemoveSkillRegistry(...args), }; }), })); @@ -36,6 +41,7 @@ vi.mock('../../lib/SkillManager.js', () => ({ removeSkill: (...args: unknown[]) => mockRemoveSkill(...args), cacheRegistry: (...args: unknown[]) => mockCacheRegistry(...args), updateSkillIndexForRegistry: (...args: unknown[]) => mockUpdateSkillIndexForRegistry(...args), + removeSkillIndexForRegistry: (...args: unknown[]) => mockRemoveSkillIndexForRegistry(...args), updateSkills: vi.fn(), findSkills: vi.fn(), rebuildIndex: vi.fn(), @@ -62,14 +68,116 @@ describe('skill command', () => { mockRemoveSkill.mockImplementation(async () => undefined); mockCacheRegistry.mockImplementation(async () => undefined); mockUpdateSkillIndexForRegistry.mockImplementation(async () => undefined); + mockRemoveSkillIndexForRegistry.mockImplementation(async () => undefined); mockProjectGetSkillRegistries.mockResolvedValue({}); mockProjectAddSkillRegistry.mockResolvedValue({}); mockGlobalGetSkillRegistries.mockResolvedValue({}); mockGlobalAddSkillRegistry.mockResolvedValue({}); + mockProjectRemoveSkillRegistry.mockResolvedValue({}); + mockGlobalRemoveSkillRegistry.mockResolvedValue({}); vi.spyOn(process, 'exit').mockImplementation((() => undefined) as any); vi.spyOn(process.stderr, 'write').mockImplementation((() => true) as any); }); + it('removes a project registry by default and preserves cache/installations', async () => { + mockProjectGetSkillRegistries.mockResolvedValue({ 'example/skills': 'url' }); + const program = new Command(); registerSkillCommand(program); + + await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'example/skills']); + + expect(mockProjectRemoveSkillRegistry).toHaveBeenCalledWith('example/skills'); + expect(mockGlobalRemoveSkillRegistry).not.toHaveBeenCalled(); + expect(mockRemoveSkillIndexForRegistry).toHaveBeenCalledWith('example/skills'); + expect(ui.success).toHaveBeenCalledWith('Removed project skill registry "example/skills".'); + expect(ui.info).toHaveBeenCalledWith('Cached repository preserved at ~/.ai-devkit/skills/example/skills because installed skills may depend on it.'); + expect(ui.info).toHaveBeenCalledWith('Installed skills were not removed.'); + }); + + it.each(['-g', '--global'])('removes only the global registry with %s', async flag => { + mockGlobalGetSkillRegistries.mockResolvedValue({ 'example/skills': 'url' }); + const program = new Command(); registerSkillCommand(program); + await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'example/skills', flag]); + expect(mockGlobalRemoveSkillRegistry).toHaveBeenCalledWith('example/skills'); + expect(mockProjectRemoveSkillRegistry).not.toHaveBeenCalled(); + expect(ui.success).toHaveBeenCalledWith('Removed global skill registry "example/skills".'); + }); + + it('reports a remaining global shadow after project removal', async () => { + mockProjectGetSkillRegistries.mockResolvedValue({ 'example/skills': 'project-url' }); + mockGlobalGetSkillRegistries.mockResolvedValue({ 'example/skills': 'global-url' }); + const program = new Command(); registerSkillCommand(program); + await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'example/skills']); + expect(ui.success).toHaveBeenCalledWith('Removed project registry "example/skills"; the global registration remains active.'); + expect(mockRemoveSkillIndexForRegistry).toHaveBeenCalledWith('example/skills'); + }); + + it('reports a remaining project registration after global removal', async () => { + mockProjectGetSkillRegistries.mockResolvedValue({ 'example/skills': 'project-url' }); + mockGlobalGetSkillRegistries.mockResolvedValue({ 'example/skills': 'global-url' }); + const program = new Command(); registerSkillCommand(program); + await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'example/skills', '--global']); + expect(ui.success).toHaveBeenCalledWith('Removed global registry "example/skills"; the project registration remains active.'); + }); + + it('removes a built-in shadow and reports the default is active again', async () => { + mockProjectGetSkillRegistries.mockResolvedValue({ 'codeaholicguy/ai-devkit': 'shadow-url' }); + const program = new Command(); registerSkillCommand(program); + await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'codeaholicguy/ai-devkit']); + expect(mockProjectRemoveSkillRegistry).toHaveBeenCalled(); + expect(ui.success).toHaveBeenCalledWith('Removed project registry "codeaholicguy/ai-devkit"; the built-in/default registry remains active.'); + }); + + it('removes a default-registry shadow and reports the default is active again', async () => { + mockProjectGetSkillRegistries.mockResolvedValue({ 'anthropics/skills': 'shadow-url' }); + const program = new Command(); registerSkillCommand(program); + await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'anthropics/skills']); + expect(ui.success).toHaveBeenCalledWith('Removed project registry "anthropics/skills"; the built-in/default registry remains active.'); + }); + + it('protects the built-in registry when no user shadow exists', async () => { + const program = new Command(); registerSkillCommand(program); + await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'codeaholicguy/ai-devkit']); + expect(ui.error).toHaveBeenCalledWith('Failed to remove registry: Registry "codeaholicguy/ai-devkit" is built in and cannot be unregistered.'); + expect(mockProjectRemoveSkillRegistry).not.toHaveBeenCalled(); + }); + + it('protects a registry provided by the bundled default registry', async () => { + const program = new Command(); registerSkillCommand(program); + await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'anthropics/skills']); + expect(ui.error).toHaveBeenCalledWith('Failed to remove registry: Registry "anthropics/skills" is provided by the default registry and cannot be unregistered.'); + expect(mockProjectRemoveSkillRegistry).not.toHaveBeenCalled(); + }); + + it('reports partial success when local index cleanup fails', async () => { + mockProjectGetSkillRegistries.mockResolvedValue({ 'example/skills': 'url' }); + mockRemoveSkillIndexForRegistry.mockRejectedValue(new Error('write failed')); + const program = new Command(); registerSkillCommand(program); + await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'example/skills']); + expect(ui.error).toHaveBeenCalledWith('Failed to remove registry: Registry was removed from project config, but the skill index could not be updated. Run "ai-devkit skill rebuild-index".'); + }); + + it('points to the other scope when the target scope is missing', async () => { + mockGlobalGetSkillRegistries.mockResolvedValue({ 'example/skills': 'url' }); + const program = new Command(); registerSkillCommand(program); + await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'example/skills']); + expect(ui.error).toHaveBeenCalledWith('Failed to remove registry: Registry "example/skills" is not registered in project config. It is registered globally; re-run with --global.'); + }); + + it('lists sorted registrations when the ID is missing everywhere', async () => { + mockProjectGetSkillRegistries.mockResolvedValue({ 'b/two': '2', 'a/one': '1' }); + mockGlobalGetSkillRegistries.mockResolvedValue({ 'c/three': '3' }); + const program = new Command(); registerSkillCommand(program); + await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'x/missing']); + expect(ui.error).toHaveBeenCalledWith('Failed to remove registry: Registry "x/missing" is not registered.\nRegistered project registries: a/one, b/two\nRegistered global registries: c/three'); + }); + + it('validates removal IDs before reading either scope', async () => { + const program = new Command(); registerSkillCommand(program); + await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'invalid']); + expect(mockProjectGetSkillRegistries).not.toHaveBeenCalled(); + expect(mockGlobalGetSkillRegistries).not.toHaveBeenCalled(); + }); + it('adds an opaque registry URL to project config by default', async () => { const program = new Command(); registerSkillCommand(program); @@ -194,7 +302,10 @@ describe('skill command', () => { expect(addRegistryCommand?.usage()).toContain(''); expect(addRegistryCommand?.helpInformation()).toContain('-g, --global'); expect(addRegistryCommand?.helpInformation()).toContain('-f, --force'); - expect(skillCommand?.commands.some(command => command.name() === 'remove-registry')).toBe(false); + const removeRegistryCommand = skillCommand?.commands.find(command => command.name() === 'remove-registry'); + expect(removeRegistryCommand?.usage()).toContain(''); + expect(removeRegistryCommand?.helpInformation()).toContain('-g, --global'); + expect(removeRegistryCommand?.helpInformation()).not.toContain('purge'); expect(skillCommand?.commands.some(command => command.name() === 'list-registries')).toBe(false); }); diff --git a/packages/cli/src/__tests__/lib/Config.test.ts b/packages/cli/src/__tests__/lib/Config.test.ts index fe227ee3..bcc44f84 100644 --- a/packages/cli/src/__tests__/lib/Config.test.ts +++ b/packages/cli/src/__tests__/lib/Config.test.ts @@ -791,6 +791,32 @@ describe('ConfigManager', () => { }); }); + describe('removeSkillRegistry', () => { + it('removes one registry while preserving sibling configuration', async () => { + const config: DevKitConfig = { + version: '1.0.0', environments: ['cursor'], phases: [], + skills: [{ registry: 'target/skills', name: 'keep-installed' }], + registries: { 'target/skills': 'target-url', 'keep/skills': 'keep-url' }, + createdAt: '2024-01-01T00:00:00.000Z', + }; + (mockFs.pathExists as any).mockResolvedValue(true); + (mockFs.readJson as any).mockResolvedValue(config); + + const result = await configManager.removeSkillRegistry('target/skills'); + + expect(result).toEqual(expect.objectContaining({ + environments: ['cursor'], skills: config.skills, + registries: { 'keep/skills': 'keep-url' }, + })); + }); + + it('rejects removal when project config is missing', async () => { + (mockFs.pathExists as any).mockResolvedValue(false); + await expect(configManager.removeSkillRegistry('target/skills')).rejects.toThrow('Config file not found'); + expect(mockFs.writeJson).not.toHaveBeenCalled(); + }); + }); + describe('getMemoryDbPath', () => { it('returns undefined when config does not exist', async () => { (mockFs.pathExists as any).mockResolvedValue(false); diff --git a/packages/cli/src/__tests__/lib/GlobalConfig.test.ts b/packages/cli/src/__tests__/lib/GlobalConfig.test.ts index d742f1f1..779b0c46 100644 --- a/packages/cli/src/__tests__/lib/GlobalConfig.test.ts +++ b/packages/cli/src/__tests__/lib/GlobalConfig.test.ts @@ -174,6 +174,28 @@ describe('GlobalConfigManager', () => { }); }); + describe('removeSkillRegistry', () => { + it('removes one registry while preserving unrelated global config', async () => { + (mockFs.pathExists as any).mockResolvedValue(true); + (mockFs.readJson as any).mockResolvedValue({ + plugins: ['memory-dashboard'], + registries: { 'target/skills': 'target-url', 'keep/skills': 'keep-url' }, + }); + + const result = await configManager.removeSkillRegistry('target/skills'); + + expect(result).toEqual({ + plugins: ['memory-dashboard'], registries: { 'keep/skills': 'keep-url' }, + }); + }); + + it('does not create a global config for a missing registry', async () => { + (mockFs.pathExists as any).mockResolvedValue(false); + expect(await configManager.removeSkillRegistry('target/skills')).toEqual({}); + expect(mockFs.writeJson).not.toHaveBeenCalled(); + }); + }); + describe('getPlugins', () => { it('should return empty list when no config exists', async () => { (mockFs.pathExists as any).mockResolvedValue(false); diff --git a/packages/cli/src/__tests__/lib/SkillManager.test.ts b/packages/cli/src/__tests__/lib/SkillManager.test.ts index 7a753703..f9ddd31a 100644 --- a/packages/cli/src/__tests__/lib/SkillManager.test.ts +++ b/packages/cli/src/__tests__/lib/SkillManager.test.ts @@ -1473,6 +1473,33 @@ describe("SkillManager", () => { }); }); + it('removes only one registry from the local skill index', async () => { + const fetchSpy = vi.spyOn(globalThis, 'fetch'); + (mockedFs.pathExists as any).mockResolvedValue(true); + (mockedFs.readJson as any).mockResolvedValue(mockSkillIndex); + + await skillManager.removeSkillIndexForRegistry('anthropics/skills'); + + expect(mockedFs.writeJson).toHaveBeenCalledWith( + expect.stringContaining('skills.json'), + expect.objectContaining({ + meta: expect.objectContaining({ registryHeads: { 'vercel-labs/agent-skills': 'def456' } }), + skills: [expect.objectContaining({ registry: 'vercel-labs/agent-skills' })], + }), + { spaces: 2 }, + ); + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + it('does not create a skill index when none exists', async () => { + (mockedFs.pathExists as any).mockResolvedValue(false); + + await skillManager.removeSkillIndexForRegistry('anthropics/skills'); + + expect(mockedFs.readJson).not.toHaveBeenCalled(); + expect(mockedFs.writeJson).not.toHaveBeenCalled(); + }); + it("should throw error if keyword is empty", async () => { await expect(skillManager.findSkills("")).rejects.toThrow("Keyword is required"); await expect(skillManager.findSkills(" ")).rejects.toThrow("Keyword is required"); diff --git a/packages/cli/src/__tests__/util/skill-registry.test.ts b/packages/cli/src/__tests__/util/skill-registry.test.ts new file mode 100644 index 00000000..76ad7eed --- /dev/null +++ b/packages/cli/src/__tests__/util/skill-registry.test.ts @@ -0,0 +1,46 @@ +import { planSkillRegistryAdd, planSkillRegistryRemove } from '../../util/skill-registry.js'; + +describe('planSkillRegistryAdd', () => { + it('covers existing add planner states used by the shared module', () => { + expect(planSkillRegistryAdd({}, 'new/skills', 'url')).toEqual({ + registries: { 'new/skills': 'url' }, status: 'added', + }); + const existing = { 'new/skills': 'url' }; + expect(planSkillRegistryAdd(existing, 'new/skills', 'url')).toEqual({ registries: existing, status: 'already-registered' }); + expect(planSkillRegistryAdd(existing, 'new/skills', 'new-url', { force: true })).toEqual({ + registries: { 'new/skills': 'new-url' }, status: 'updated', + }); + expect(() => planSkillRegistryAdd(existing, 'new/skills', 'new-url')).toThrow('Use --force'); + }); +}); + +describe('planSkillRegistryRemove', () => { + it('removes an own registry entry without mutating the input', () => { + const registries = { 'target/skills': 'target-url', 'keep/skills': 'keep-url' }; + + expect(planSkillRegistryRemove(registries, 'target/skills')).toEqual({ + registries: { 'keep/skills': 'keep-url' }, + status: 'removed', + removedUrl: 'target-url', + }); + expect(registries).toEqual({ 'target/skills': 'target-url', 'keep/skills': 'keep-url' }); + }); + + it('returns a copied map when the registry is not registered', () => { + const registries = { 'keep/skills': 'keep-url' }; + const result = planSkillRegistryRemove(registries, 'missing/skills'); + + expect(result).toEqual({ registries, status: 'not-registered' }); + expect(result.registries).not.toBe(registries); + }); + + it('does not treat an inherited registry as registered', () => { + const registries = Object.create({ 'shadow/skills': 'inherited-url' }) as Record; + registries['keep/skills'] = 'keep-url'; + + expect(planSkillRegistryRemove(registries, 'shadow/skills')).toEqual({ + registries: { 'keep/skills': 'keep-url' }, + status: 'not-registered', + }); + }); +}); diff --git a/packages/cli/src/commands/skill.ts b/packages/cli/src/commands/skill.ts index e0be7872..2643bec3 100644 --- a/packages/cli/src/commands/skill.ts +++ b/packages/cli/src/commands/skill.ts @@ -3,12 +3,12 @@ import chalk from 'chalk'; import { ConfigManager } from '../lib/Config.js'; import { GlobalConfigManager } from '../lib/GlobalConfig.js'; import { SkillManager } from '../lib/SkillManager.js'; -import { BUILTIN_SKILL_NAMES, BUILTIN_SKILL_REGISTRY } from '../constants.js'; +import { BUILTIN_SKILL_NAMES, BUILTIN_SKILL_REGISTRY, DEFAULT_SKILL_REGISTRY_IDS } from '../constants.js'; import { ui } from '../util/terminal-ui.js'; import { withErrorHandler } from '../util/errors.js'; import { truncate, getErrorMessage } from '../util/text.js'; import { validateRegistryId } from '../util/skill.js'; -import { planSkillRegistryAdd } from '../util/skill-registry.js'; +import { planSkillRegistryAdd, planSkillRegistryRemove } from '../util/skill-registry.js'; export function registerSkillCommand(program: Command): void { const skillCommand = program @@ -93,6 +93,67 @@ export function registerSkillCommand(program: Command): void { } })); + skillCommand + .command('remove-registry ') + .description('Unregister a third-party skill registry') + .option('-g, --global', 'Remove from global config (~/.ai-devkit/.ai-devkit.json)') + .action(withErrorHandler('remove registry', async ( + id: string, + options: { global?: boolean }, + ) => { + validateRegistryId(id); + const projectConfig = new ConfigManager(); + const globalConfig = new GlobalConfigManager(); + const [projectRegistries, globalRegistries] = await Promise.all([ + projectConfig.getSkillRegistries(), + globalConfig.getSkillRegistries(), + ]); + const targetRegistries = options.global ? globalRegistries : projectRegistries; + const otherRegistries = options.global ? projectRegistries : globalRegistries; + const mutation = planSkillRegistryRemove(targetRegistries, id); + + if (mutation.status === 'not-registered') { + if (Object.prototype.hasOwnProperty.call(otherRegistries, id)) { + throw new Error(options.global + ? `Registry "${id}" is not registered globally. It is registered in project config; omit --global.` + : `Registry "${id}" is not registered in project config. It is registered globally; re-run with --global.`); + } + if (id === BUILTIN_SKILL_REGISTRY) { + throw new Error(`Registry "${id}" is built in and cannot be unregistered.`); + } + if (DEFAULT_SKILL_REGISTRY_IDS.has(id)) { + throw new Error(`Registry "${id}" is provided by the default registry and cannot be unregistered.`); + } + const projectList = Object.keys(projectRegistries).sort().join(', ') || '(none)'; + const globalList = Object.keys(globalRegistries).sort().join(', ') || '(none)'; + throw new Error( + `Registry "${id}" is not registered.\nRegistered project registries: ${projectList}\nRegistered global registries: ${globalList}`, + ); + } + + const targetConfig = options.global ? globalConfig : projectConfig; + await targetConfig.removeSkillRegistry(id); + try { + await new SkillManager(projectConfig).removeSkillIndexForRegistry(id); + } catch { + const scope = options.global ? 'global config' : 'project config'; + throw new Error(`Registry was removed from ${scope}, but the skill index could not be updated. Run "ai-devkit skill rebuild-index".`); + } + + const scope = options.global ? 'global' : 'project'; + if (!options.global && Object.prototype.hasOwnProperty.call(globalRegistries, id)) { + ui.success(`Removed project registry "${id}"; the global registration remains active.`); + } else if (options.global && Object.prototype.hasOwnProperty.call(projectRegistries, id)) { + ui.success(`Removed global registry "${id}"; the project registration remains active.`); + } else if (DEFAULT_SKILL_REGISTRY_IDS.has(id)) { + ui.success(`Removed ${scope} registry "${id}"; the built-in/default registry remains active.`); + } else { + ui.success(`Removed ${scope} skill registry "${id}".`); + } + ui.info(`Cached repository preserved at ~/.ai-devkit/skills/${id} because installed skills may depend on it.`); + ui.info('Installed skills were not removed.'); + })); + skillCommand .command('list') .description('List installed project skills, or global skills with --global') diff --git a/packages/cli/src/constants.ts b/packages/cli/src/constants.ts index 020d09a6..c3100760 100644 --- a/packages/cli/src/constants.ts +++ b/packages/cli/src/constants.ts @@ -3,6 +3,83 @@ */ export const BUILTIN_SKILL_REGISTRY = 'codeaholicguy/ai-devkit'; +/** Registry IDs shipped in the default registry snapshot used for offline protection. */ +export const DEFAULT_SKILL_REGISTRY_IDS = new Set([ + BUILTIN_SKILL_REGISTRY, + 'CloudAI-X/claude-workflow-v2', + 'HeyVincent-ai/agent-skills', + 'SawyerHood/dev-browser', + 'Shopify/Shopify-AI-Toolkit', + 'WordPress/agent-skills', + 'addyosmani/agent-skills', + 'addyosmani/web-quality-skills', + 'adithya-s-k/manim_skill', + 'affaan-m/everything-claude-code', + 'analogjs/angular-skills', + 'antfu/skills', + 'anthropics/skills', + 'apify/agent-skills', + 'atxp-dev/cli', + 'boristane/agent-skills', + 'brianlovin/claude-config', + 'browser-use/browser-use', + 'callstackincubator/agent-skills', + 'cloudai-x/threejs-skills', + 'cloudflare/skills', + 'coreyhaines31/marketingskills', + 'dbt-labs/dbt-agent-skills', + 'dgreenheck/webgpu-claude-skill', + 'figma/mcp-server-guide', + 'firecrawl/cli', + 'forrestchang/andrej-karpathy-skills', + 'github/awesome-copilot', + 'giuseppe-trisciuoglio/developer-kit', + 'google-gemini/gemini-skills', + 'google-labs-code/stitch-skills', + 'google/skills', + 'huggingface/skills', + 'hyf0/vue-skills', + 'ibelick/ui-skills', + 'inference-sh/skills', + 'intellectronica/agent-skills', + 'itsmostafa/aws-agent-skills', + 'jeffallan/claude-skills', + 'jezweb/claude-skills', + 'jimliu/baoyu-skills', + 'kepano/obsidian-skills', + 'lackeyjb/playwright-skill', + 'mcollina/skills', + 'microsoft/agent-skills', + 'microsoft/playwright-cli', + 'muratcankoylan/Agent-Skills-for-Context-Engineering', + 'napoleond/clawdirect', + 'obra/superpowers', + 'onmax/nuxt-skills', + 'othmanadi/planning-with-files', + 'pluginagentmarketplace/custom-plugin-java', + 'remotion-dev/skills', + 'resciencelab/opc-skills', + 'resend/react-email', + 'samber/cc-skills-golang', + 'sickn33/antigravity-awesome-skills', + 'simonwong/agent-skills', + 'softaworks/agent-toolkit', + 'stripe/ai', + 'subsy/ralph-tui', + 'supabase/agent-skills', + 'superdesigndev/superdesign-skill', + 'vercel-labs/agent-browser', + 'vercel-labs/agent-skills', + 'vercel-labs/next-skills', + 'vercel-labs/skills', + 'vercel/ai', + 'vercel/turborepo', + 'vuejs-ai/skills', + 'vueuse/skills', + 'waynesutton/convexskills', + 'zackkorman/skills', +]); + /** * Canonical list of built-in skills that ship with AI DevKit. Keep in sync * with the skills published under the {@link BUILTIN_SKILL_REGISTRY} diff --git a/packages/cli/src/lib/Config.ts b/packages/cli/src/lib/Config.ts index 3c89b10b..6ef7db91 100644 --- a/packages/cli/src/lib/Config.ts +++ b/packages/cli/src/lib/Config.ts @@ -3,7 +3,7 @@ import * as path from 'path'; import { DevKitConfig, Phase, EnvironmentCode, ConfigSkill, DEFAULT_DOCS_DIR, DEFAULT_PHASES } from '../types.js'; import { filterStringRecord } from '../util/config.js'; import { ConfigNotFoundError } from '../util/errors.js'; -import { AddSkillRegistryOptions, planSkillRegistryAdd } from '../util/skill-registry.js'; +import { AddSkillRegistryOptions, planSkillRegistryAdd, planSkillRegistryRemove } from '../util/skill-registry.js'; import packageJson from '../../package.json' with { type: 'json' }; const CONFIG_FILE_NAME = '.ai-devkit.json'; @@ -189,4 +189,15 @@ export class ConfigManager { return this.update({ registries: mutation.registries }); } + + async removeSkillRegistry(id: string): Promise { + const config = await this.read(); + if (!config) { + throw new ConfigNotFoundError('Config file not found. Run ai-devkit init first.'); + } + const mutation = planSkillRegistryRemove(filterStringRecord(config.registries), id); + return mutation.status === 'removed' + ? this.update({ registries: mutation.registries }) + : config; + } } diff --git a/packages/cli/src/lib/GlobalConfig.ts b/packages/cli/src/lib/GlobalConfig.ts index 35d52432..cd01902d 100644 --- a/packages/cli/src/lib/GlobalConfig.ts +++ b/packages/cli/src/lib/GlobalConfig.ts @@ -4,7 +4,7 @@ import * as path from 'path'; import { GlobalDevKitConfig } from '../types.js'; import { filterStringRecord } from '../util/config.js'; import { CliError } from '../util/errors.js'; -import { AddSkillRegistryOptions, planSkillRegistryAdd } from '../util/skill-registry.js'; +import { AddSkillRegistryOptions, planSkillRegistryAdd, planSkillRegistryRemove } from '../util/skill-registry.js'; import { ui } from '../util/terminal-ui.js'; export class GlobalConfigManager { @@ -55,6 +55,22 @@ export class GlobalConfigManager { }); } + async removeSkillRegistry(id: string): Promise { + const configExists = await this.exists(); + const config = await this.read(); + if (configExists && !config) { + throw new CliError( + `Cannot update global config because the existing file could not be read: ${this.getGlobalConfigPath()}`, + 'GLOBAL_CONFIG_UNREADABLE', + { configPath: this.getGlobalConfigPath() }, + ); + } + const existingConfig = config ?? {}; + const mutation = planSkillRegistryRemove(filterStringRecord(existingConfig.registries), id); + if (mutation.status === 'not-registered') return existingConfig; + return this.write({ ...existingConfig, registries: mutation.registries }); + } + async getPlugins(): Promise { const config = await this.read(); return normalizePlugins(config?.plugins); diff --git a/packages/cli/src/lib/SkillIndex.ts b/packages/cli/src/lib/SkillIndex.ts index 0673ab2f..dc42b9ec 100644 --- a/packages/cli/src/lib/SkillIndex.ts +++ b/packages/cli/src/lib/SkillIndex.ts @@ -90,6 +90,17 @@ export class SkillIndex { await fs.writeJson(SKILL_INDEX_PATH, nextIndex, { spaces: 2 }); } + async removeRegistry(registryId: string): Promise { + if (!await fs.pathExists(SKILL_INDEX_PATH)) return; + const existingIndex: SkillIndexData = await fs.readJson(SKILL_INDEX_PATH); + const registryHeads = { ...existingIndex.meta.registryHeads }; + delete registryHeads[registryId]; + await fs.writeJson(SKILL_INDEX_PATH, { + meta: { ...existingIndex.meta, updatedAt: Date.now(), registryHeads }, + skills: existingIndex.skills.filter(skill => skill.registry !== registryId), + }, { spaces: 2 }); + } + private async ensureSkillIndex(forceRefresh = false): Promise { const indexExists = await fs.pathExists(SKILL_INDEX_PATH); diff --git a/packages/cli/src/lib/SkillManager.ts b/packages/cli/src/lib/SkillManager.ts index c665b657..d5836fb8 100644 --- a/packages/cli/src/lib/SkillManager.ts +++ b/packages/cli/src/lib/SkillManager.ts @@ -367,6 +367,10 @@ export class SkillManager { return this.index.updateRegistryFromCache(registryId); } + async removeSkillIndexForRegistry(registryId: string): Promise { + return this.index.removeRegistry(registryId); + } + private async resolveProjectEnvironments(): Promise { ui.info('Loading project configuration...'); let config = await this.configManager.read(); diff --git a/packages/cli/src/util/skill-registry.ts b/packages/cli/src/util/skill-registry.ts index 3107776c..87bc3bfa 100644 --- a/packages/cli/src/util/skill-registry.ts +++ b/packages/cli/src/util/skill-registry.ts @@ -11,6 +11,14 @@ export interface SkillRegistryMutation { status: SkillRegistryAddStatus; } +export type SkillRegistryRemoveStatus = 'removed' | 'not-registered'; + +export interface SkillRegistryRemoveMutation { + registries: Record; + status: SkillRegistryRemoveStatus; + removedUrl?: string; +} + export function planSkillRegistryAdd( registries: Record, id: string, @@ -36,3 +44,17 @@ export function planSkillRegistryAdd( status: existingUrl === undefined ? 'added' : 'updated', }; } + +export function planSkillRegistryRemove( + registries: Record, + id: string, +): SkillRegistryRemoveMutation { + const nextRegistries = { ...registries }; + if (!Object.prototype.hasOwnProperty.call(registries, id)) { + return { registries: nextRegistries, status: 'not-registered' }; + } + + const removedUrl = registries[id]; + delete nextRegistries[id]; + return { registries: nextRegistries, status: 'removed', removedUrl }; +} diff --git a/web/content/docs/7-skills.md b/web/content/docs/7-skills.md index b76c2e2e..1eaf8e25 100644 --- a/web/content/docs/7-skills.md +++ b/web/content/docs/7-skills.md @@ -192,6 +192,21 @@ ai-devkit skill add-registry my-org/skills https://github.com/my-org/new-skills. Registry IDs use the `organization/repository` format. Each segment may contain letters, numbers, underscores, and hyphens. +### `ai-devkit skill remove-registry` + +Unregister a third-party skill registry from the current project, or from global configuration with `--global`: + +```bash +ai-devkit skill remove-registry my-org/skills +ai-devkit skill remove-registry my-org/skills --global +``` + +The command removes only the selected scope's configuration entry and immediately filters that registry from the local search index. It never uses the network. Cached repositories and installed skills are preserved because installed skills in this or another project may depend on that cache. Built-in and default registries cannot be unregistered, but a project or global registration that shadows one can be removed to reveal the default again. + +If the same ID is registered in another scope, that registration remains active. The command reports the remaining source and the next registry refresh can repopulate its index entries. + +> **Follow-up:** v1 intentionally has no `--purge-cache` option. A future cache-cleanup workflow must use that narrow name, require explicit `--yes` in non-interactive terminals, protect built-in or still-effective registries, and never remove installed skills. A future registry command group should migrate `add-registry` and `remove-registry` together rather than changing one name independently. + ### `ai-devkit skill list` List skills installed in the current project, or inspect skills installed across known global environment paths. From 50eed033cdfea8d7ec224d71819cd9ef2f4ea422 Mon Sep 17 00:00:00 2001 From: Hoang Nguyen Date: Sat, 22 Aug 2026 14:57:58 +0000 Subject: [PATCH 2/4] refactor(cli): simplify registry removal --- CHANGELOG.md | 2 +- ...026-08-22-feature-skill-remove-registry.md | 40 +++++----- ...026-08-22-feature-skill-remove-registry.md | 28 ++++--- ...026-08-22-feature-skill-remove-registry.md | 30 +++---- ...026-08-22-feature-skill-remove-registry.md | 40 +++++----- ...026-08-22-feature-skill-remove-registry.md | 32 ++++---- .../cli/src/__tests__/commands/skill.test.ts | 80 +++++-------------- .../src/__tests__/lib/SkillManager.test.ts | 27 ------- .../src/__tests__/util/skill-registry.test.ts | 1 - packages/cli/src/commands/skill.ts | 79 +++++++----------- packages/cli/src/constants.ts | 77 ------------------ packages/cli/src/lib/SkillIndex.ts | 11 --- packages/cli/src/lib/SkillManager.ts | 4 - packages/cli/src/util/skill-registry.ts | 4 +- web/content/docs/7-skills.md | 6 +- 15 files changed, 140 insertions(+), 321 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index edceba4d..b0624aaa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -- Added `skill remove-registry` with scoped config removal, offline index cleanup, default-registry protection, and cache/install preservation. +- Added `skill remove-registry` with project-scoped config removal and global removal that also deletes the registry's cached repository. ## [0.54.0] - 2026-08-21 diff --git a/docs/ai/design/2026-08-22-feature-skill-remove-registry.md b/docs/ai/design/2026-08-22-feature-skill-remove-registry.md index 5517c0c5..c2253e85 100644 --- a/docs/ai/design/2026-08-22-feature-skill-remove-registry.md +++ b/docs/ai/design/2026-08-22-feature-skill-remove-registry.md @@ -1,7 +1,7 @@ --- phase: design title: Skill Registry Removal Design -description: Safe scoped config removal and local index cleanup +description: Scoped config removal with guarded global cache deletion --- # Skill Registry Removal Design @@ -11,16 +11,15 @@ description: Safe scoped config removal and local index cleanup ```mermaid flowchart LR CLI[remove-registry command] --> Validate[validateRegistryId] - Validate --> Read[read project/global/default maps] - Read --> Plan[planSkillRegistryRemove] - Plan --> Config[selected ConfigManager] - Config --> Index[local focused index cleanup] - Index -. no access .-> Network[(Network)] - Index -. preserved .-> Cache[(Registry cache)] - Config -. untouched .-> Installs[(Installed skills)] + Validate --> Read[read selected config map] + Read --> Guard[own-property guard] + Guard --> Config[selected ConfigManager] + Config -->|global only| Contain[resolve and contain cache path] + Contain --> Cache[remove registry cache directory] + Config -. unchanged .-> Index[(Seed-backed discovery index)] ``` -The command resolves precedence and messages. Config managers persist the pure planner result. `SkillIndex` performs a focused local inverse of registry indexing. +The command validates and guards the selected scope, while config managers persist the pure planner result. Project removal stops after config mutation. Global removal additionally deletes the contained registry cache path. The discovery index is intentionally unchanged because its seed contains unconfigured registries by design. ## Data Models @@ -29,7 +28,6 @@ type SkillRegistryRemoveStatus = 'removed' | 'not-registered'; interface SkillRegistryRemoveMutation { registries: Record; status: SkillRegistryRemoveStatus; - removedUrl?: string; } ``` @@ -40,8 +38,8 @@ The planner tests own-property presence, copies the input, and omits only the se - `skill remove-registry [-g|--global]` - `planSkillRegistryRemove(registries, id)` is pure. - Project/global config managers expose `removeSkillRegistry(id)`. -- `SkillManager.removeSkillIndexForRegistry(id)` delegates to local filtering of `skills[].registry` and `meta.registryHeads[id]`. -- Exact output follows the approved exploration, including shadow reports and partial-success repair guidance. +- `--global` resolves `~/.ai-devkit/skills/`, verifies containment under the cache root, and recursively removes it. +- Missing selected-scope entries return the concise `try --global` error. ## Component Breakdown @@ -49,23 +47,21 @@ The planner tests own-property presence, copies the input, and omits only the se |---|---| | `util/skill-registry.ts` | Pure removal planner and types | | `Config.ts`, `GlobalConfig.ts` | Scoped removal writers | -| `SkillIndex.ts`, `SkillManager.ts` | Local focused index cleanup | -| `commands/skill.ts` | Command and scope/default protection | +| `commands/skill.ts` | Scope guard and contained global cache deletion | | Tests/docs | Behavior, exact copy, and follow-ups | ## Design Decisions - Mutate only the selected scope. -- Config plus focused index cleanup avoids stale search results. -- Never use the network; a lower source repopulates on later refresh. -- Built-in/default sources are read-only, while configured shadows remain removable. -- Preserve cache to protect symlink-backed installs across projects. +- Do not clean the index: seed catalog entries are valid even without local registry configuration. +- Never use the network during removal. +- Protect the built-in ID explicitly; default sources are structurally protected by absence from user config maps. +- Preserve cache for project removal; delete the selected cached repository for global removal. - Keep `remove-registry` paired with `add-registry`; defer registry-group migration. -- Defer `--purge-cache`; a future version must require `--yes` in non-TTY use, protect effective/built-in sources, and never remove installed skills. ## Non-Functional Requirements -- Local complexity is linear in index size. -- Writes retain existing config/index safety conventions. +- Work is constant-time apart from recursive global cache deletion. +- Writes retain existing config safety conventions. - Invalid IDs cannot influence filesystem paths. -- Failures distinguish pre-write rejection from repairable post-write index failure. +- Resolved cache targets must be strict descendants of `SKILL_CACHE_DIR`. diff --git a/docs/ai/implementation/2026-08-22-feature-skill-remove-registry.md b/docs/ai/implementation/2026-08-22-feature-skill-remove-registry.md index b73dc6de..b8c3f741 100644 --- a/docs/ai/implementation/2026-08-22-feature-skill-remove-registry.md +++ b/docs/ai/implementation/2026-08-22-feature-skill-remove-registry.md @@ -12,35 +12,36 @@ Use repository Node/npm. Run `npm ci` and `npm run build` before full gates. ## Code Structure -Changes are confined to CLI registry utilities, config managers, skill index/manager, command registration, their tests, and user/lifecycle documentation. +Changes are confined to CLI registry utilities, config managers, command registration, their tests, and user/lifecycle documentation. The follow-up removes the earlier index/manager additions. ## Implementation Notes - Follow red-green-refactor for planner, config, index, and command slices. - Reuse validation and add-registry scope conventions. - Keep the planner copy-on-write and I/O-free. -- Remove only the selected map entry and clean index data locally. +- Remove only the selected map entry and leave the seed-backed discovery index unchanged. - Never call registry fetch/update during removal. -- Preserve cache and installed skills and say so in successful output. -- No `--purge-cache`, `--yes`, or registry-group migration in v1. +- Preserve cache for project removal; delete the contained registry cache path for global removal. - Implemented the pure own-property removal planner and project/global persistence methods. Targeted planner/config suites pass (71 tests). -- Implemented focused index filtering and the scoped command. The complete shipped default-registry ID snapshot is embedded for deterministic offline protection; configured shadows remain removable. +- Removed focused index filtering because `SEED_INDEX_URL` intentionally catalogs unconfigured registries, making removed-registry entries equivalent to normal seed entries. +- Removed the frozen default-registry ID snapshot. Defaults are structurally protected because the planner and command only act on own properties in the selected user config map. +- Removed redundant command-level planner execution; config managers remain the single persistence planning layer. ## Integration Points -The command reads project/global registries and default metadata, writes one config manager, then invokes focused derived-index cleanup. Lower-precedence shadows are reported and repopulate on a later refresh. +The command reads only the selected config map and delegates removal to its manager. For global removal, it resolves the cache root and target, verifies that the target is a strict descendant, writes global config, and recursively removes that cache directory. ## Error Handling -Reject invalid IDs and default-only registries before writes. Wrong-scope errors explain the correct flag. Missing IDs list sorted project/global registrations. After config write, index failure reports partial success and the rebuild command. +Reject invalid IDs and the built-in registry before writes. A missing own property returns `Registry is not registered (try --global).`. Unsafe resolved cache paths are rejected before config mutation or recursive deletion. ## Performance Considerations -Config maps are small; focused cleanup is a linear local index pass with no network latency. +Config maps are small. Project removal is constant-time apart from config I/O; global cache deletion is proportional to the cached repository size. ## Security Notes -Validate IDs before use. Never mutate built-in/default data, cache directories, or installed skill paths. +Validate IDs before path construction. Resolve both cache root and target, require a strict contained target, and only then permit recursive removal. Never traverse installed-skill paths. ## Validation Evidence @@ -57,3 +58,12 @@ Fresh validation on 2026-08-22 completed after resuming the interrupted session: - `git diff --check`: exit 0. Optional task tracing was unavailable: `npx ai-devkit@latest task list --name skill-remove-registry --json` returned `error: unknown command 'task'`. + +Review follow-up validation on 2026-08-22: + +- `npm run build`: exit 0; Nx built all 6 projects. +- `npm test`: exit 0; all 6 projects passed (1,954 tests across 140 files). +- `npm run lint`: exit 0; all 6 projects passed with 4 existing unused-catch warnings and no errors. +- Targeted Vitest command for command, planner, config, and manager suites: exit 0; 176 tests across 5 files. +- Planner-module coverage: 100% statements, branches, functions, and lines (11/11 statements, 11/11 branches, 2/2 functions, 11/11 lines). +- Feature lifecycle lint and built `remove-registry --help`: exit 0. diff --git a/docs/ai/planning/2026-08-22-feature-skill-remove-registry.md b/docs/ai/planning/2026-08-22-feature-skill-remove-registry.md index e4413869..7538f5a6 100644 --- a/docs/ai/planning/2026-08-22-feature-skill-remove-registry.md +++ b/docs/ai/planning/2026-08-22-feature-skill-remove-registry.md @@ -9,9 +9,9 @@ description: TDD implementation and validation tasks ## Milestones - [x] Planner and persistence behavior implemented with tests. -- [x] Command and local index behavior implemented with exact-copy tests. +- [x] Simplified scoped command and guarded global cache deletion implemented with tests. - [x] Documentation and full local validation gates completed. -- [ ] Commit and PR publication completed. +- [x] Initial feature commit published in PR #196; review simplification prepared for the same PR. ## Task Breakdown @@ -21,22 +21,23 @@ description: TDD implementation and validation tasks - [x] Implement `planSkillRegistryRemove`; final coverage gate remains. - [x] Add failing project/global config preservation tests, then removal methods. -### Phase 2: Index and command +### Phase 2: Command and cache behavior -- [x] Add failing focused-index tests for filtering, sibling preservation, missing index, and metadata. -- [x] Implement local-only index cleanup and SkillManager delegation. -- [x] Add failing command tests for validation, scopes, shadows, defaults, messages, index failure, and no network. -- [x] Implement `remove-registry` beside `add-registry` without purge flags. +- [x] Add command tests for validation, selected-scope guards, project cache preservation, and global cache deletion. +- [x] Implement `remove-registry` beside `add-registry` with resolved-path containment. +- [x] Remove focused index cleanup because seed catalog entries are valid without local configuration. +- [x] Remove the frozen default-registry ID snapshot and rely on config-map structure. ### Phase 3: Integration and polish - [x] Update user docs, changelog, implementation, and testing records. - [x] Run targeted tests/coverage, build, full workspace tests, lint, and lifecycle lint. -- [ ] Create a conventional commit and open a PR when requested. +- [x] Create a conventional commit and open PR #196. +- [x] Validate and prepare the reviewed simplification follow-up for commit and push. ## Dependencies -Planner precedes persistence; persistence/index APIs precede command integration. Existing add-registry patterns and the approved exploration are authoritative. No external service is required. +Planner precedes persistence. The command uses existing config managers and `SKILL_CACHE_DIR`; no index API or external service is required. ## Timeline & Estimates @@ -44,12 +45,11 @@ Single feature iteration: implementation and targeted tests, documentation, full ## Risks & Mitigation -- Stale discovery: focused local cleanup after config write. -- Wrong-scope deletion: inspect both maps, mutate one, emit actionable hints. -- Broken installed skills: never delete cache or installations. -- Default deletion: only configured shadows reach removal. -- Partial failure: print repair instructions without unsafe rollback. +- Unsafe recursive deletion: validate the registry ID and require the resolved target to remain inside the cache root. +- Wrong-scope deletion: read and mutate only the selected config map. +- Seed catalog inconsistency: leave the discovery index unchanged, matching its unconfigured-registry semantics. +- Default deletion: defaults are absent from user config maps and fail the own-property guard. ## Resources Needed -Existing CLI/config/index modules, Vitest suites, approved exploration, lifecycle skills, npm workspace tooling, and GitHub CLI. +Existing CLI/config/cache modules, Vitest suites, lifecycle skills, npm workspace tooling, and GitHub CLI. diff --git a/docs/ai/requirements/2026-08-22-feature-skill-remove-registry.md b/docs/ai/requirements/2026-08-22-feature-skill-remove-registry.md index d55ec6a5..ed3aabe8 100644 --- a/docs/ai/requirements/2026-08-22-feature-skill-remove-registry.md +++ b/docs/ai/requirements/2026-08-22-feature-skill-remove-registry.md @@ -8,44 +8,42 @@ description: Add the safe inverse of skill add-registry ## Problem Statement -Users can register project or global skill registries with `skill add-registry`, but cannot unregister them. Manual config edits can leave stale search-index entries and make scope precedence unclear. +Users can register project or global skill registries with `skill add-registry`, but cannot unregister them without editing configuration manually. ## Goals & Objectives - Add `skill remove-registry ` beside `add-registry`. -- Default to project scope; use `-g`/`--global` for global scope. -- Remove only the selected config entry and locally clean stale index data without network access. -- Preserve cache repositories and installed skills. -- Protect built-in/default registries while allowing a user shadow to be removed. +- Default to project-only removal and preserve its cached repository. +- Use `-g`/`--global` to remove the global entry and recursively delete that registry's cache directory. +- Leave the discovery index unchanged because seed entries do not imply local registration. +- Protect the built-in registry explicitly; default registries remain protected structurally because they are absent from user config maps. -Non-goals: cache purge, installed-skill removal, registry-group command migration, or changes to unrelated update behavior. +Non-goals: discovery-index cleanup, installed-skill traversal/removal, registry-group command migration, or changes to unrelated update behavior. ## User Stories & Use Cases - Remove a project registry without affecting a same-ID global registration. -- Remove only a global registration with `--global`. -- When removing a shadow, report which lower-precedence source remains active. -- Give automation deterministic exact messages without prompts or network traffic. -- Give actionable wrong-scope hints or a sorted registry inventory. +- Remove a global registration and its cache with `--global`. +- Give automation deterministic output without prompts or network traffic. +- Reject missing registrations with a concise `try --global` hint. ## Success Criteria -- Reuse `validateRegistryId` before config/index work. -- A pure copy-on-write planner returns `removed` or `not-registered`, including the removed URL. +- Reuse `validateRegistryId` before config work or path deletion. +- A pure copy-on-write planner returns `removed` or `not-registered` with the next registry map. - Config writers preserve unrelated keys and registry entries. -- Removed-only registry entries leave the local index; sibling data remains intact. -- If another source remains, invalidate the ID locally for later refresh. -- Built-in/default-only sources cannot be removed; user shadows can. -- Tests give the planner 100% coverage and cover scope, messages, preservation, and no-network behavior. +- Removal leaves the seed-backed discovery index unchanged. +- Global cache deletion resolves the target and proves it is contained inside `SKILL_CACHE_DIR` before recursive removal. +- The built-in source cannot be removed; defaults absent from the selected config map fail the own-property guard. +- Tests cover planner behavior, both scopes, cache deletion, validation order, and exact messages. - User docs and changelog describe the command and safe boundary. ## Constraints & Assumptions -- Configuration is written before derived-index cleanup. Index failure reports the rebuild command and does not roll config back. -- Built-in/default registry metadata is read-only. Removal never fetches registry data. -- Cache and installations are always untouched in v1. -- `--purge-cache` is deferred with future safety semantics documented. +- Removal never fetches registry or index data. +- Project removal never deletes cache data. +- Global removal deletes `~/.ai-devkit/skills/` after containment validation; it does not traverse installed-skill locations. ## Questions & Open Items -All material questions are resolved by the approved design. Follow-ups: guarded `--purge-cache` and coordinated registry-group aliases/migration. +All material questions are resolved by the approved simplification. A coordinated registry-group alias/migration remains a possible follow-up. diff --git a/docs/ai/testing/2026-08-22-feature-skill-remove-registry.md b/docs/ai/testing/2026-08-22-feature-skill-remove-registry.md index f6eb896b..cf295c94 100644 --- a/docs/ai/testing/2026-08-22-feature-skill-remove-registry.md +++ b/docs/ai/testing/2026-08-22-feature-skill-remove-registry.md @@ -9,31 +9,31 @@ description: Coverage and validation strategy for skill remove-registry ## Test Coverage Goals - 100% statements, branches, functions, and lines for `planSkillRegistryRemove`. -- Critical command, persistence, index, and failure paths covered. +- Critical command, persistence, cache containment, and failure paths covered. - Full workspace test and lint gates remain green. ## Unit Tests ### Pure planner -- [x] Present/absent IDs return correct status and removed URL. +- [x] Present/absent IDs return correct status and registry maps. - [x] Input is immutable; siblings, empty maps, and inherited properties behave correctly. - [x] Coverage demonstrates 100% statements, branches, functions, and lines for the planner module. -### Config and local index +### Config and cache behavior - [x] Project/global removers preserve unrelated registries and config keys. - [x] Existing missing/malformed-config guarantees remain intact in the full config suites. -- [x] Index cleanup drops only target skills/head, preserves siblings, and no-ops when absent. +- [x] Project removal preserves cache; global removal deletes only the resolved registry cache path. ## Integration Tests - [x] Default removes only project; `-g`/`--global` remove only global. -- [x] Cross-scope and built-in/default shadows report the revealed source. -- [x] Wrong-scope, protected, and missing errors use exact copy and sorted inventories. -- [x] Invalid IDs cause no reads/writes. -- [x] Index failure reports partial success and rebuild guidance. -- [x] Registry network fetch/update is mocked and asserted unused. +- [x] Built-in removal is rejected before config mutation. +- [x] Missing selected-scope registrations use the concise `try --global` error. +- [x] Invalid IDs cause no config reads or cache deletion. +- [x] Global cache deletion targets a strict descendant of the cache root. +- [x] The discovery index is not touched during removal. ## End-to-End Tests @@ -43,21 +43,21 @@ description: Coverage and validation strategy for skill remove-registry ## Test Data -Use command mocks and temporary filesystem fixtures. Seed mixed registry entries and heads to prove sibling preservation. +Use command mocks and temporary filesystem fixtures. Seed mixed registry maps to prove sibling preservation and assert the global cache target path. ## Test Reporting & Coverage Run targeted Vitest suites and planner coverage, then repository-native full test/lint gates. Record fresh results during completion. -Fresh results from 2026-08-22: +Fresh simplification results from 2026-08-22: -- Targeted suites: 5 files passed, 184 tests passed. -- Planner module: 100% statements (12/12), branches (11/11), functions (2/2), and lines (12/12). +- Targeted suites: 5 files passed, 176 tests passed. +- Planner module: 100% statements (11/11), branches (11/11), functions (2/2), and lines (11/11). - Workspace build: 6 projects passed. -- Workspace tests: 6 projects passed; 140 files and 1,962 tests passed. +- Workspace tests: 6 projects passed; 140 files and 1,954 tests passed. - Workspace lint: 6 projects passed with no errors; 4 unrelated existing warnings were reported. - Base and feature lifecycle lint: passed. -- Built CLI help: exit 0 and lists `-g, --global` plus standard `--help`, with no purge option. +- Built CLI help: exit 0 and lists `-g, --global` plus standard `--help`. ## Manual Testing @@ -65,7 +65,7 @@ Inspect CLI help and exact output assertions; no browser/device checks apply. ## Performance Testing -No load test is required; the operation is bounded local filtering. +No load test is required; the only size-dependent operation is recursive global cache deletion. ## Bug Tracking diff --git a/packages/cli/src/__tests__/commands/skill.test.ts b/packages/cli/src/__tests__/commands/skill.test.ts index 4e3501e7..0ff515e7 100644 --- a/packages/cli/src/__tests__/commands/skill.test.ts +++ b/packages/cli/src/__tests__/commands/skill.test.ts @@ -3,13 +3,16 @@ import { Command } from 'commander'; import { registerSkillCommand } from '../../commands/skill.js'; import { ui } from '../../util/terminal-ui.js'; +const mockRemoveCache = vi.hoisted(() => vi.fn()); + +vi.mock('fs-extra', () => ({ default: { remove: mockRemoveCache } })); + const mockAddSkill = vi.fn(); const mockListGlobalSkills = vi.fn(); const mockListSkills = vi.fn(); const mockRemoveSkill = vi.fn(); const mockCacheRegistry = vi.fn(); const mockUpdateSkillIndexForRegistry = vi.fn(); -const mockRemoveSkillIndexForRegistry = vi.fn(); const mockProjectGetSkillRegistries = vi.fn(); const mockProjectAddSkillRegistry = vi.fn(); const mockProjectRemoveSkillRegistry = vi.fn(); @@ -41,7 +44,6 @@ vi.mock('../../lib/SkillManager.js', () => ({ removeSkill: (...args: unknown[]) => mockRemoveSkill(...args), cacheRegistry: (...args: unknown[]) => mockCacheRegistry(...args), updateSkillIndexForRegistry: (...args: unknown[]) => mockUpdateSkillIndexForRegistry(...args), - removeSkillIndexForRegistry: (...args: unknown[]) => mockRemoveSkillIndexForRegistry(...args), updateSkills: vi.fn(), findSkills: vi.fn(), rebuildIndex: vi.fn(), @@ -68,7 +70,7 @@ describe('skill command', () => { mockRemoveSkill.mockImplementation(async () => undefined); mockCacheRegistry.mockImplementation(async () => undefined); mockUpdateSkillIndexForRegistry.mockImplementation(async () => undefined); - mockRemoveSkillIndexForRegistry.mockImplementation(async () => undefined); + mockRemoveCache.mockResolvedValue(undefined); mockProjectGetSkillRegistries.mockResolvedValue({}); mockProjectAddSkillRegistry.mockResolvedValue({}); mockGlobalGetSkillRegistries.mockResolvedValue({}); @@ -79,7 +81,7 @@ describe('skill command', () => { vi.spyOn(process.stderr, 'write').mockImplementation((() => true) as any); }); - it('removes a project registry by default and preserves cache/installations', async () => { + it('removes a project registry by default and keeps its cache', async () => { mockProjectGetSkillRegistries.mockResolvedValue({ 'example/skills': 'url' }); const program = new Command(); registerSkillCommand(program); @@ -87,88 +89,41 @@ describe('skill command', () => { expect(mockProjectRemoveSkillRegistry).toHaveBeenCalledWith('example/skills'); expect(mockGlobalRemoveSkillRegistry).not.toHaveBeenCalled(); - expect(mockRemoveSkillIndexForRegistry).toHaveBeenCalledWith('example/skills'); expect(ui.success).toHaveBeenCalledWith('Removed project skill registry "example/skills".'); - expect(ui.info).toHaveBeenCalledWith('Cached repository preserved at ~/.ai-devkit/skills/example/skills because installed skills may depend on it.'); - expect(ui.info).toHaveBeenCalledWith('Installed skills were not removed.'); + expect(mockRemoveCache).not.toHaveBeenCalled(); }); - it.each(['-g', '--global'])('removes only the global registry with %s', async flag => { + it.each(['-g', '--global'])('removes the global registry and its cache with %s', async flag => { mockGlobalGetSkillRegistries.mockResolvedValue({ 'example/skills': 'url' }); const program = new Command(); registerSkillCommand(program); await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'example/skills', flag]); expect(mockGlobalRemoveSkillRegistry).toHaveBeenCalledWith('example/skills'); expect(mockProjectRemoveSkillRegistry).not.toHaveBeenCalled(); + expect(mockRemoveCache).toHaveBeenCalledWith(expect.stringMatching(/\.ai-devkit\/skills\/example\/skills$/)); expect(ui.success).toHaveBeenCalledWith('Removed global skill registry "example/skills".'); }); - it('reports a remaining global shadow after project removal', async () => { - mockProjectGetSkillRegistries.mockResolvedValue({ 'example/skills': 'project-url' }); - mockGlobalGetSkillRegistries.mockResolvedValue({ 'example/skills': 'global-url' }); - const program = new Command(); registerSkillCommand(program); - await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'example/skills']); - expect(ui.success).toHaveBeenCalledWith('Removed project registry "example/skills"; the global registration remains active.'); - expect(mockRemoveSkillIndexForRegistry).toHaveBeenCalledWith('example/skills'); - }); - - it('reports a remaining project registration after global removal', async () => { - mockProjectGetSkillRegistries.mockResolvedValue({ 'example/skills': 'project-url' }); - mockGlobalGetSkillRegistries.mockResolvedValue({ 'example/skills': 'global-url' }); - const program = new Command(); registerSkillCommand(program); - await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'example/skills', '--global']); - expect(ui.success).toHaveBeenCalledWith('Removed global registry "example/skills"; the project registration remains active.'); - }); - - it('removes a built-in shadow and reports the default is active again', async () => { + it('always protects the built-in registry', async () => { mockProjectGetSkillRegistries.mockResolvedValue({ 'codeaholicguy/ai-devkit': 'shadow-url' }); - const program = new Command(); registerSkillCommand(program); - await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'codeaholicguy/ai-devkit']); - expect(mockProjectRemoveSkillRegistry).toHaveBeenCalled(); - expect(ui.success).toHaveBeenCalledWith('Removed project registry "codeaholicguy/ai-devkit"; the built-in/default registry remains active.'); - }); - - it('removes a default-registry shadow and reports the default is active again', async () => { - mockProjectGetSkillRegistries.mockResolvedValue({ 'anthropics/skills': 'shadow-url' }); - const program = new Command(); registerSkillCommand(program); - await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'anthropics/skills']); - expect(ui.success).toHaveBeenCalledWith('Removed project registry "anthropics/skills"; the built-in/default registry remains active.'); - }); - - it('protects the built-in registry when no user shadow exists', async () => { const program = new Command(); registerSkillCommand(program); await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'codeaholicguy/ai-devkit']); expect(ui.error).toHaveBeenCalledWith('Failed to remove registry: Registry "codeaholicguy/ai-devkit" is built in and cannot be unregistered.'); expect(mockProjectRemoveSkillRegistry).not.toHaveBeenCalled(); }); - it('protects a registry provided by the bundled default registry', async () => { - const program = new Command(); registerSkillCommand(program); - await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'anthropics/skills']); - expect(ui.error).toHaveBeenCalledWith('Failed to remove registry: Registry "anthropics/skills" is provided by the default registry and cannot be unregistered.'); - expect(mockProjectRemoveSkillRegistry).not.toHaveBeenCalled(); - }); - - it('reports partial success when local index cleanup fails', async () => { - mockProjectGetSkillRegistries.mockResolvedValue({ 'example/skills': 'url' }); - mockRemoveSkillIndexForRegistry.mockRejectedValue(new Error('write failed')); - const program = new Command(); registerSkillCommand(program); - await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'example/skills']); - expect(ui.error).toHaveBeenCalledWith('Failed to remove registry: Registry was removed from project config, but the skill index could not be updated. Run "ai-devkit skill rebuild-index".'); - }); - - it('points to the other scope when the target scope is missing', async () => { + it('suggests --global when the project registration is missing', async () => { mockGlobalGetSkillRegistries.mockResolvedValue({ 'example/skills': 'url' }); const program = new Command(); registerSkillCommand(program); await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'example/skills']); - expect(ui.error).toHaveBeenCalledWith('Failed to remove registry: Registry "example/skills" is not registered in project config. It is registered globally; re-run with --global.'); + expect(ui.error).toHaveBeenCalledWith('Failed to remove registry: Registry example/skills is not registered (try --global).'); }); - it('lists sorted registrations when the ID is missing everywhere', async () => { - mockProjectGetSkillRegistries.mockResolvedValue({ 'b/two': '2', 'a/one': '1' }); - mockGlobalGetSkillRegistries.mockResolvedValue({ 'c/three': '3' }); + it('reports a missing global registration without reading project config', async () => { const program = new Command(); registerSkillCommand(program); - await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'x/missing']); - expect(ui.error).toHaveBeenCalledWith('Failed to remove registry: Registry "x/missing" is not registered.\nRegistered project registries: a/one, b/two\nRegistered global registries: c/three'); + await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'x/missing', '--global']); + expect(ui.error).toHaveBeenCalledWith('Failed to remove registry: Registry x/missing is not registered (try --global).'); + expect(mockProjectGetSkillRegistries).not.toHaveBeenCalled(); + expect(mockRemoveCache).not.toHaveBeenCalled(); }); it('validates removal IDs before reading either scope', async () => { @@ -176,6 +131,7 @@ describe('skill command', () => { await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'invalid']); expect(mockProjectGetSkillRegistries).not.toHaveBeenCalled(); expect(mockGlobalGetSkillRegistries).not.toHaveBeenCalled(); + expect(mockRemoveCache).not.toHaveBeenCalled(); }); it('adds an opaque registry URL to project config by default', async () => { diff --git a/packages/cli/src/__tests__/lib/SkillManager.test.ts b/packages/cli/src/__tests__/lib/SkillManager.test.ts index f9ddd31a..7a753703 100644 --- a/packages/cli/src/__tests__/lib/SkillManager.test.ts +++ b/packages/cli/src/__tests__/lib/SkillManager.test.ts @@ -1473,33 +1473,6 @@ describe("SkillManager", () => { }); }); - it('removes only one registry from the local skill index', async () => { - const fetchSpy = vi.spyOn(globalThis, 'fetch'); - (mockedFs.pathExists as any).mockResolvedValue(true); - (mockedFs.readJson as any).mockResolvedValue(mockSkillIndex); - - await skillManager.removeSkillIndexForRegistry('anthropics/skills'); - - expect(mockedFs.writeJson).toHaveBeenCalledWith( - expect.stringContaining('skills.json'), - expect.objectContaining({ - meta: expect.objectContaining({ registryHeads: { 'vercel-labs/agent-skills': 'def456' } }), - skills: [expect.objectContaining({ registry: 'vercel-labs/agent-skills' })], - }), - { spaces: 2 }, - ); - expect(fetchSpy).not.toHaveBeenCalled(); - }); - - it('does not create a skill index when none exists', async () => { - (mockedFs.pathExists as any).mockResolvedValue(false); - - await skillManager.removeSkillIndexForRegistry('anthropics/skills'); - - expect(mockedFs.readJson).not.toHaveBeenCalled(); - expect(mockedFs.writeJson).not.toHaveBeenCalled(); - }); - it("should throw error if keyword is empty", async () => { await expect(skillManager.findSkills("")).rejects.toThrow("Keyword is required"); await expect(skillManager.findSkills(" ")).rejects.toThrow("Keyword is required"); diff --git a/packages/cli/src/__tests__/util/skill-registry.test.ts b/packages/cli/src/__tests__/util/skill-registry.test.ts index 76ad7eed..76d2ba3e 100644 --- a/packages/cli/src/__tests__/util/skill-registry.test.ts +++ b/packages/cli/src/__tests__/util/skill-registry.test.ts @@ -21,7 +21,6 @@ describe('planSkillRegistryRemove', () => { expect(planSkillRegistryRemove(registries, 'target/skills')).toEqual({ registries: { 'keep/skills': 'keep-url' }, status: 'removed', - removedUrl: 'target-url', }); expect(registries).toEqual({ 'target/skills': 'target-url', 'keep/skills': 'keep-url' }); }); diff --git a/packages/cli/src/commands/skill.ts b/packages/cli/src/commands/skill.ts index 2643bec3..76ad4b52 100644 --- a/packages/cli/src/commands/skill.ts +++ b/packages/cli/src/commands/skill.ts @@ -1,14 +1,17 @@ import { Command } from 'commander'; import chalk from 'chalk'; +import fs from 'fs-extra'; +import * as path from 'path'; import { ConfigManager } from '../lib/Config.js'; import { GlobalConfigManager } from '../lib/GlobalConfig.js'; import { SkillManager } from '../lib/SkillManager.js'; -import { BUILTIN_SKILL_NAMES, BUILTIN_SKILL_REGISTRY, DEFAULT_SKILL_REGISTRY_IDS } from '../constants.js'; +import { SKILL_CACHE_DIR } from '../lib/SkillRegistry.js'; +import { BUILTIN_SKILL_NAMES, BUILTIN_SKILL_REGISTRY } from '../constants.js'; import { ui } from '../util/terminal-ui.js'; import { withErrorHandler } from '../util/errors.js'; import { truncate, getErrorMessage } from '../util/text.js'; import { validateRegistryId } from '../util/skill.js'; -import { planSkillRegistryAdd, planSkillRegistryRemove } from '../util/skill-registry.js'; +import { planSkillRegistryAdd } from '../util/skill-registry.js'; export function registerSkillCommand(program: Command): void { const skillCommand = program @@ -96,62 +99,42 @@ export function registerSkillCommand(program: Command): void { skillCommand .command('remove-registry ') .description('Unregister a third-party skill registry') - .option('-g, --global', 'Remove from global config (~/.ai-devkit/.ai-devkit.json)') + .option('-g, --global', 'Remove from global config and delete the cached registry') .action(withErrorHandler('remove registry', async ( id: string, options: { global?: boolean }, ) => { validateRegistryId(id); - const projectConfig = new ConfigManager(); - const globalConfig = new GlobalConfigManager(); - const [projectRegistries, globalRegistries] = await Promise.all([ - projectConfig.getSkillRegistries(), - globalConfig.getSkillRegistries(), - ]); - const targetRegistries = options.global ? globalRegistries : projectRegistries; - const otherRegistries = options.global ? projectRegistries : globalRegistries; - const mutation = planSkillRegistryRemove(targetRegistries, id); - - if (mutation.status === 'not-registered') { - if (Object.prototype.hasOwnProperty.call(otherRegistries, id)) { - throw new Error(options.global - ? `Registry "${id}" is not registered globally. It is registered in project config; omit --global.` - : `Registry "${id}" is not registered in project config. It is registered globally; re-run with --global.`); - } - if (id === BUILTIN_SKILL_REGISTRY) { - throw new Error(`Registry "${id}" is built in and cannot be unregistered.`); - } - if (DEFAULT_SKILL_REGISTRY_IDS.has(id)) { - throw new Error(`Registry "${id}" is provided by the default registry and cannot be unregistered.`); - } - const projectList = Object.keys(projectRegistries).sort().join(', ') || '(none)'; - const globalList = Object.keys(globalRegistries).sort().join(', ') || '(none)'; - throw new Error( - `Registry "${id}" is not registered.\nRegistered project registries: ${projectList}\nRegistered global registries: ${globalList}`, - ); + if (id === BUILTIN_SKILL_REGISTRY) { + throw new Error(`Registry "${id}" is built in and cannot be unregistered.`); } - const targetConfig = options.global ? globalConfig : projectConfig; - await targetConfig.removeSkillRegistry(id); - try { - await new SkillManager(projectConfig).removeSkillIndexForRegistry(id); - } catch { - const scope = options.global ? 'global config' : 'project config'; - throw new Error(`Registry was removed from ${scope}, but the skill index could not be updated. Run "ai-devkit skill rebuild-index".`); + const configManager = options.global + ? new GlobalConfigManager() + : new ConfigManager(); + const registries = await configManager.getSkillRegistries(); + if (!Object.prototype.hasOwnProperty.call(registries, id)) { + throw new Error(`Registry ${id} is not registered (try --global).`); } - const scope = options.global ? 'global' : 'project'; - if (!options.global && Object.prototype.hasOwnProperty.call(globalRegistries, id)) { - ui.success(`Removed project registry "${id}"; the global registration remains active.`); - } else if (options.global && Object.prototype.hasOwnProperty.call(projectRegistries, id)) { - ui.success(`Removed global registry "${id}"; the project registration remains active.`); - } else if (DEFAULT_SKILL_REGISTRY_IDS.has(id)) { - ui.success(`Removed ${scope} registry "${id}"; the built-in/default registry remains active.`); - } else { - ui.success(`Removed ${scope} skill registry "${id}".`); + let cachePath: string | undefined; + if (options.global) { + const cacheRoot = path.resolve(SKILL_CACHE_DIR); + cachePath = path.resolve(cacheRoot, id); + const relativeCachePath = path.relative(cacheRoot, cachePath); + const escapesCacheRoot = relativeCachePath === '..' + || relativeCachePath.startsWith(`..${path.sep}`) + || path.isAbsolute(relativeCachePath); + if (!relativeCachePath || escapesCacheRoot) { + throw new Error(`Refusing to remove cache outside ${cacheRoot}.`); + } } - ui.info(`Cached repository preserved at ~/.ai-devkit/skills/${id} because installed skills may depend on it.`); - ui.info('Installed skills were not removed.'); + + await configManager.removeSkillRegistry(id); + if (cachePath) await fs.remove(cachePath); + + const scope = options.global ? 'global' : 'project'; + ui.success(`Removed ${scope} skill registry "${id}".`); })); skillCommand diff --git a/packages/cli/src/constants.ts b/packages/cli/src/constants.ts index c3100760..020d09a6 100644 --- a/packages/cli/src/constants.ts +++ b/packages/cli/src/constants.ts @@ -3,83 +3,6 @@ */ export const BUILTIN_SKILL_REGISTRY = 'codeaholicguy/ai-devkit'; -/** Registry IDs shipped in the default registry snapshot used for offline protection. */ -export const DEFAULT_SKILL_REGISTRY_IDS = new Set([ - BUILTIN_SKILL_REGISTRY, - 'CloudAI-X/claude-workflow-v2', - 'HeyVincent-ai/agent-skills', - 'SawyerHood/dev-browser', - 'Shopify/Shopify-AI-Toolkit', - 'WordPress/agent-skills', - 'addyosmani/agent-skills', - 'addyosmani/web-quality-skills', - 'adithya-s-k/manim_skill', - 'affaan-m/everything-claude-code', - 'analogjs/angular-skills', - 'antfu/skills', - 'anthropics/skills', - 'apify/agent-skills', - 'atxp-dev/cli', - 'boristane/agent-skills', - 'brianlovin/claude-config', - 'browser-use/browser-use', - 'callstackincubator/agent-skills', - 'cloudai-x/threejs-skills', - 'cloudflare/skills', - 'coreyhaines31/marketingskills', - 'dbt-labs/dbt-agent-skills', - 'dgreenheck/webgpu-claude-skill', - 'figma/mcp-server-guide', - 'firecrawl/cli', - 'forrestchang/andrej-karpathy-skills', - 'github/awesome-copilot', - 'giuseppe-trisciuoglio/developer-kit', - 'google-gemini/gemini-skills', - 'google-labs-code/stitch-skills', - 'google/skills', - 'huggingface/skills', - 'hyf0/vue-skills', - 'ibelick/ui-skills', - 'inference-sh/skills', - 'intellectronica/agent-skills', - 'itsmostafa/aws-agent-skills', - 'jeffallan/claude-skills', - 'jezweb/claude-skills', - 'jimliu/baoyu-skills', - 'kepano/obsidian-skills', - 'lackeyjb/playwright-skill', - 'mcollina/skills', - 'microsoft/agent-skills', - 'microsoft/playwright-cli', - 'muratcankoylan/Agent-Skills-for-Context-Engineering', - 'napoleond/clawdirect', - 'obra/superpowers', - 'onmax/nuxt-skills', - 'othmanadi/planning-with-files', - 'pluginagentmarketplace/custom-plugin-java', - 'remotion-dev/skills', - 'resciencelab/opc-skills', - 'resend/react-email', - 'samber/cc-skills-golang', - 'sickn33/antigravity-awesome-skills', - 'simonwong/agent-skills', - 'softaworks/agent-toolkit', - 'stripe/ai', - 'subsy/ralph-tui', - 'supabase/agent-skills', - 'superdesigndev/superdesign-skill', - 'vercel-labs/agent-browser', - 'vercel-labs/agent-skills', - 'vercel-labs/next-skills', - 'vercel-labs/skills', - 'vercel/ai', - 'vercel/turborepo', - 'vuejs-ai/skills', - 'vueuse/skills', - 'waynesutton/convexskills', - 'zackkorman/skills', -]); - /** * Canonical list of built-in skills that ship with AI DevKit. Keep in sync * with the skills published under the {@link BUILTIN_SKILL_REGISTRY} diff --git a/packages/cli/src/lib/SkillIndex.ts b/packages/cli/src/lib/SkillIndex.ts index dc42b9ec..0673ab2f 100644 --- a/packages/cli/src/lib/SkillIndex.ts +++ b/packages/cli/src/lib/SkillIndex.ts @@ -90,17 +90,6 @@ export class SkillIndex { await fs.writeJson(SKILL_INDEX_PATH, nextIndex, { spaces: 2 }); } - async removeRegistry(registryId: string): Promise { - if (!await fs.pathExists(SKILL_INDEX_PATH)) return; - const existingIndex: SkillIndexData = await fs.readJson(SKILL_INDEX_PATH); - const registryHeads = { ...existingIndex.meta.registryHeads }; - delete registryHeads[registryId]; - await fs.writeJson(SKILL_INDEX_PATH, { - meta: { ...existingIndex.meta, updatedAt: Date.now(), registryHeads }, - skills: existingIndex.skills.filter(skill => skill.registry !== registryId), - }, { spaces: 2 }); - } - private async ensureSkillIndex(forceRefresh = false): Promise { const indexExists = await fs.pathExists(SKILL_INDEX_PATH); diff --git a/packages/cli/src/lib/SkillManager.ts b/packages/cli/src/lib/SkillManager.ts index d5836fb8..c665b657 100644 --- a/packages/cli/src/lib/SkillManager.ts +++ b/packages/cli/src/lib/SkillManager.ts @@ -367,10 +367,6 @@ export class SkillManager { return this.index.updateRegistryFromCache(registryId); } - async removeSkillIndexForRegistry(registryId: string): Promise { - return this.index.removeRegistry(registryId); - } - private async resolveProjectEnvironments(): Promise { ui.info('Loading project configuration...'); let config = await this.configManager.read(); diff --git a/packages/cli/src/util/skill-registry.ts b/packages/cli/src/util/skill-registry.ts index 87bc3bfa..a8331b11 100644 --- a/packages/cli/src/util/skill-registry.ts +++ b/packages/cli/src/util/skill-registry.ts @@ -16,7 +16,6 @@ export type SkillRegistryRemoveStatus = 'removed' | 'not-registered'; export interface SkillRegistryRemoveMutation { registries: Record; status: SkillRegistryRemoveStatus; - removedUrl?: string; } export function planSkillRegistryAdd( @@ -54,7 +53,6 @@ export function planSkillRegistryRemove( return { registries: nextRegistries, status: 'not-registered' }; } - const removedUrl = registries[id]; delete nextRegistries[id]; - return { registries: nextRegistries, status: 'removed', removedUrl }; + return { registries: nextRegistries, status: 'removed' }; } diff --git a/web/content/docs/7-skills.md b/web/content/docs/7-skills.md index 1eaf8e25..83d5e915 100644 --- a/web/content/docs/7-skills.md +++ b/web/content/docs/7-skills.md @@ -201,11 +201,9 @@ ai-devkit skill remove-registry my-org/skills ai-devkit skill remove-registry my-org/skills --global ``` -The command removes only the selected scope's configuration entry and immediately filters that registry from the local search index. It never uses the network. Cached repositories and installed skills are preserved because installed skills in this or another project may depend on that cache. Built-in and default registries cannot be unregistered, but a project or global registration that shadows one can be removed to reveal the default again. +Without `--global`, the command removes only the project configuration entry and keeps the cached repository. With `--global`, it removes the global configuration entry and recursively deletes that registry's cache directory under `~/.ai-devkit/skills/`. Registry IDs are validated and the resolved cache path must remain inside the skills cache root before deletion. -If the same ID is registered in another scope, that registration remains active. The command reports the remaining source and the next registry refresh can repopulate its index entries. - -> **Follow-up:** v1 intentionally has no `--purge-cache` option. A future cache-cleanup workflow must use that narrow name, require explicit `--yes` in non-interactive terminals, protect built-in or still-effective registries, and never remove installed skills. A future registry command group should migrate `add-registry` and `remove-registry` together rather than changing one name independently. +The local discovery index is not modified. It is seeded with skills from registries that are not configured locally, so entries for a removed registry remain valid catalog entries. Default registries are structurally protected because they do not live in project or global configuration maps; the built-in `codeaholicguy/ai-devkit` registry is explicitly protected. ### `ai-devkit skill list` From 1805a4e7ae71b67f9448295c892256f4fcfb950b Mon Sep 17 00:00:00 2001 From: Hoang Nguyen Date: Sat, 22 Aug 2026 15:16:44 +0000 Subject: [PATCH 3/4] refactor(cli): move registry cache removal into SkillManager Commands stay orchestration-only: validate, guard, delegate, render. The containment check and fs removal now live beside the rest of the cache-path logic in SkillManager, with unit tests for the happy path and escape refusal. --- .../cli/src/__tests__/commands/skill.test.ts | 4 ++-- .../cli/src/__tests__/lib/SkillManager.test.ts | 17 +++++++++++++++++ packages/cli/src/commands/skill.ts | 18 ++---------------- packages/cli/src/lib/SkillManager.ts | 17 +++++++++++++++++ 4 files changed, 38 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/__tests__/commands/skill.test.ts b/packages/cli/src/__tests__/commands/skill.test.ts index 0ff515e7..c9ac9ead 100644 --- a/packages/cli/src/__tests__/commands/skill.test.ts +++ b/packages/cli/src/__tests__/commands/skill.test.ts @@ -5,7 +5,6 @@ import { ui } from '../../util/terminal-ui.js'; const mockRemoveCache = vi.hoisted(() => vi.fn()); -vi.mock('fs-extra', () => ({ default: { remove: mockRemoveCache } })); const mockAddSkill = vi.fn(); const mockListGlobalSkills = vi.fn(); @@ -44,6 +43,7 @@ vi.mock('../../lib/SkillManager.js', () => ({ removeSkill: (...args: unknown[]) => mockRemoveSkill(...args), cacheRegistry: (...args: unknown[]) => mockCacheRegistry(...args), updateSkillIndexForRegistry: (...args: unknown[]) => mockUpdateSkillIndexForRegistry(...args), + removeRegistryCache: (...args: unknown[]) => mockRemoveCache(...args), updateSkills: vi.fn(), findSkills: vi.fn(), rebuildIndex: vi.fn(), @@ -99,7 +99,7 @@ describe('skill command', () => { await program.parseAsync(['node', 'test', 'skill', 'remove-registry', 'example/skills', flag]); expect(mockGlobalRemoveSkillRegistry).toHaveBeenCalledWith('example/skills'); expect(mockProjectRemoveSkillRegistry).not.toHaveBeenCalled(); - expect(mockRemoveCache).toHaveBeenCalledWith(expect.stringMatching(/\.ai-devkit\/skills\/example\/skills$/)); + expect(mockRemoveCache).toHaveBeenCalledWith('example/skills'); expect(ui.success).toHaveBeenCalledWith('Removed global skill registry "example/skills".'); }); diff --git a/packages/cli/src/__tests__/lib/SkillManager.test.ts b/packages/cli/src/__tests__/lib/SkillManager.test.ts index 7a753703..3c47e2cb 100644 --- a/packages/cli/src/__tests__/lib/SkillManager.test.ts +++ b/packages/cli/src/__tests__/lib/SkillManager.test.ts @@ -1641,4 +1641,21 @@ describe("SkillManager", () => { expect(result).toBe(repoPath); }); }); + + describe("removeRegistryCache", () => { + it("removes the contained registry cache directory", async () => { + await skillManager.removeRegistryCache("example/skills"); + + expect(mockedFs.remove).toHaveBeenCalledWith( + path.join(os.homedir(), ".ai-devkit", "skills", "example", "skills"), + ); + }); + + it("refuses paths that escape the cache root", async () => { + await expect( + skillManager.removeRegistryCache("../escaped"), + ).rejects.toThrow(/outside/); + expect(mockedFs.remove).not.toHaveBeenCalled(); + }); + }); }); diff --git a/packages/cli/src/commands/skill.ts b/packages/cli/src/commands/skill.ts index 76ad4b52..c5180dba 100644 --- a/packages/cli/src/commands/skill.ts +++ b/packages/cli/src/commands/skill.ts @@ -1,11 +1,8 @@ import { Command } from 'commander'; import chalk from 'chalk'; -import fs from 'fs-extra'; -import * as path from 'path'; import { ConfigManager } from '../lib/Config.js'; import { GlobalConfigManager } from '../lib/GlobalConfig.js'; import { SkillManager } from '../lib/SkillManager.js'; -import { SKILL_CACHE_DIR } from '../lib/SkillRegistry.js'; import { BUILTIN_SKILL_NAMES, BUILTIN_SKILL_REGISTRY } from '../constants.js'; import { ui } from '../util/terminal-ui.js'; import { withErrorHandler } from '../util/errors.js'; @@ -117,22 +114,11 @@ export function registerSkillCommand(program: Command): void { throw new Error(`Registry ${id} is not registered (try --global).`); } - let cachePath: string | undefined; + await configManager.removeSkillRegistry(id); if (options.global) { - const cacheRoot = path.resolve(SKILL_CACHE_DIR); - cachePath = path.resolve(cacheRoot, id); - const relativeCachePath = path.relative(cacheRoot, cachePath); - const escapesCacheRoot = relativeCachePath === '..' - || relativeCachePath.startsWith(`..${path.sep}`) - || path.isAbsolute(relativeCachePath); - if (!relativeCachePath || escapesCacheRoot) { - throw new Error(`Refusing to remove cache outside ${cacheRoot}.`); - } + await new SkillManager(new ConfigManager()).removeRegistryCache(id); } - await configManager.removeSkillRegistry(id); - if (cachePath) await fs.remove(cachePath); - const scope = options.global ? 'global' : 'project'; ui.success(`Removed ${scope} skill registry "${id}".`); })); diff --git a/packages/cli/src/lib/SkillManager.ts b/packages/cli/src/lib/SkillManager.ts index c665b657..435af81b 100644 --- a/packages/cli/src/lib/SkillManager.ts +++ b/packages/cli/src/lib/SkillManager.ts @@ -367,6 +367,23 @@ export class SkillManager { return this.index.updateRegistryFromCache(registryId); } + /** + * Remove a registry's cached repository from the skill cache directory. + * Refuses paths that would escape the cache root. + */ + async removeRegistryCache(registryId: string): Promise { + const cacheRoot = path.resolve(SKILL_CACHE_DIR); + const cachePath = path.resolve(cacheRoot, registryId); + const relativeCachePath = path.relative(cacheRoot, cachePath); + const escapesCacheRoot = relativeCachePath === '..' + || relativeCachePath.startsWith(`..${path.sep}`) + || path.isAbsolute(relativeCachePath); + if (!relativeCachePath || escapesCacheRoot) { + throw new Error(`Refusing to remove cache outside ${cacheRoot}.`); + } + await fs.remove(cachePath); + } + private async resolveProjectEnvironments(): Promise { ui.info('Loading project configuration...'); let config = await this.configManager.read(); From f23acc68ae132a13eeaa2c5bfc0d0dbd102ac276 Mon Sep 17 00:00:00 2001 From: Hoang Nguyen Date: Sat, 22 Aug 2026 15:19:38 +0000 Subject: [PATCH 4/4] docs(skill): align lifecycle docs with SkillManager cache removal --- docs/ai/design/2026-08-22-feature-skill-remove-registry.md | 2 +- .../implementation/2026-08-22-feature-skill-remove-registry.md | 2 +- docs/ai/testing/2026-08-22-feature-skill-remove-registry.md | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/ai/design/2026-08-22-feature-skill-remove-registry.md b/docs/ai/design/2026-08-22-feature-skill-remove-registry.md index c2253e85..2d089e66 100644 --- a/docs/ai/design/2026-08-22-feature-skill-remove-registry.md +++ b/docs/ai/design/2026-08-22-feature-skill-remove-registry.md @@ -38,7 +38,7 @@ The planner tests own-property presence, copies the input, and omits only the se - `skill remove-registry [-g|--global]` - `planSkillRegistryRemove(registries, id)` is pure. - Project/global config managers expose `removeSkillRegistry(id)`. -- `--global` resolves `~/.ai-devkit/skills/`, verifies containment under the cache root, and recursively removes it. +- `SkillManager.removeRegistryCache(id)` resolves `~/.ai-devkit/skills/`, verifies containment under the cache root, and recursively removes it; the command only delegates to it. - Missing selected-scope entries return the concise `try --global` error. ## Component Breakdown diff --git a/docs/ai/implementation/2026-08-22-feature-skill-remove-registry.md b/docs/ai/implementation/2026-08-22-feature-skill-remove-registry.md index b8c3f741..7266ae9d 100644 --- a/docs/ai/implementation/2026-08-22-feature-skill-remove-registry.md +++ b/docs/ai/implementation/2026-08-22-feature-skill-remove-registry.md @@ -29,7 +29,7 @@ Changes are confined to CLI registry utilities, config managers, command registr ## Integration Points -The command reads only the selected config map and delegates removal to its manager. For global removal, it resolves the cache root and target, verifies that the target is a strict descendant, writes global config, and recursively removes that cache directory. +The command reads only the selected config map and delegates removal to its manager. For global removal, it additionally calls `SkillManager.removeRegistryCache(id)`, which resolves the cache root and target, verifies that the target is a strict descendant, and recursively removes that cache directory. The command layer contains no filesystem logic. ## Error Handling diff --git a/docs/ai/testing/2026-08-22-feature-skill-remove-registry.md b/docs/ai/testing/2026-08-22-feature-skill-remove-registry.md index cf295c94..987dd9b4 100644 --- a/docs/ai/testing/2026-08-22-feature-skill-remove-registry.md +++ b/docs/ai/testing/2026-08-22-feature-skill-remove-registry.md @@ -32,7 +32,7 @@ description: Coverage and validation strategy for skill remove-registry - [x] Built-in removal is rejected before config mutation. - [x] Missing selected-scope registrations use the concise `try --global` error. - [x] Invalid IDs cause no config reads or cache deletion. -- [x] Global cache deletion targets a strict descendant of the cache root. +- [x] Global cache deletion targets a strict descendant of the cache root (unit-tested in `SkillManager.removeRegistryCache`). - [x] The discovery index is not touched during removal. ## End-to-End Tests