diff --git a/CHANGELOG.md b/CHANGELOG.md index 584864b0..8435c0e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +- Added `skill remove-registry` with project-scoped config removal and global removal that also deletes the registry's cached repository. - Bias the brainstorm skill toward minimal solutions: baseline-first divergence, speculative-generality and verified-claim checks, and deletion cost in comparisons. ## [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 new file mode 100644 index 00000000..2d089e66 --- /dev/null +++ b/docs/ai/design/2026-08-22-feature-skill-remove-registry.md @@ -0,0 +1,67 @@ +--- +phase: design +title: Skill Registry Removal Design +description: Scoped config removal with guarded global cache deletion +--- + +# Skill Registry Removal Design + +## Architecture Overview + +```mermaid +flowchart LR + CLI[remove-registry command] --> Validate[validateRegistryId] + 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 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 + +```ts +type SkillRegistryRemoveStatus = 'removed' | 'not-registered'; +interface SkillRegistryRemoveMutation { + registries: Record; + status: SkillRegistryRemoveStatus; +} +``` + +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.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 + +| Component | Change | +|---|---| +| `util/skill-registry.ts` | Pure removal planner and types | +| `Config.ts`, `GlobalConfig.ts` | Scoped removal writers | +| `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. +- 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. + +## Non-Functional Requirements + +- Work is constant-time apart from recursive global cache deletion. +- Writes retain existing config safety conventions. +- Invalid IDs cannot influence filesystem paths. +- 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 new file mode 100644 index 00000000..7266ae9d --- /dev/null +++ b/docs/ai/implementation/2026-08-22-feature-skill-remove-registry.md @@ -0,0 +1,69 @@ +--- +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, 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 leave the seed-backed discovery index unchanged. +- Never call registry fetch/update during removal. +- 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). +- 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 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 + +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. 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 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 + +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'`. + +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 new file mode 100644 index 00000000..7538f5a6 --- /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] Simplified scoped command and guarded global cache deletion implemented with tests. +- [x] Documentation and full local validation gates completed. +- [x] Initial feature commit published in PR #196; review simplification prepared for the same PR. + +## 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: Command and cache behavior + +- [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. +- [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. The command uses existing config managers and `SKILL_CACHE_DIR`; no index API or external service is required. + +## Timeline & Estimates + +Single feature iteration: implementation and targeted tests, documentation, full gates, review/publish. + +## Risks & Mitigation + +- 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/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 new file mode 100644 index 00000000..ed3aabe8 --- /dev/null +++ b/docs/ai/requirements/2026-08-22-feature-skill-remove-registry.md @@ -0,0 +1,49 @@ +--- +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 without editing configuration manually. + +## Goals & Objectives + +- Add `skill remove-registry ` beside `add-registry`. +- 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: 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 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 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. +- 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 + +- 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 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 new file mode 100644 index 00000000..987dd9b4 --- /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, 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 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 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] 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] 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 (unit-tested in `SkillManager.removeRegistryCache`). +- [x] The discovery index is not touched during removal. + +## 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 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 simplification results from 2026-08-22: + +- 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,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`. + +## Manual Testing + +Inspect CLI help and exact output assertions; no browser/device checks apply. + +## Performance Testing + +No load test is required; the only size-dependent operation is recursive global cache deletion. + +## 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..c9ac9ead 100644 --- a/packages/cli/src/__tests__/commands/skill.test.ts +++ b/packages/cli/src/__tests__/commands/skill.test.ts @@ -3,6 +3,9 @@ import { Command } from 'commander'; import { registerSkillCommand } from '../../commands/skill.js'; import { ui } from '../../util/terminal-ui.js'; +const mockRemoveCache = vi.hoisted(() => vi.fn()); + + const mockAddSkill = vi.fn(); const mockListGlobalSkills = vi.fn(); const mockListSkills = vi.fn(); @@ -11,13 +14,16 @@ const mockCacheRegistry = vi.fn(); const mockUpdateSkillIndexForRegistry = 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 +31,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 +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(), @@ -62,14 +70,70 @@ describe('skill command', () => { mockRemoveSkill.mockImplementation(async () => undefined); mockCacheRegistry.mockImplementation(async () => undefined); mockUpdateSkillIndexForRegistry.mockImplementation(async () => undefined); + mockRemoveCache.mockResolvedValue(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 keeps its cache', 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(ui.success).toHaveBeenCalledWith('Removed project skill registry "example/skills".'); + expect(mockRemoveCache).not.toHaveBeenCalled(); + }); + + 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('example/skills'); + expect(ui.success).toHaveBeenCalledWith('Removed global skill registry "example/skills".'); + }); + + 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(ui.error).toHaveBeenCalledWith('Failed to remove registry: Registry "codeaholicguy/ai-devkit" is built in and cannot be unregistered.'); + expect(mockProjectRemoveSkillRegistry).not.toHaveBeenCalled(); + }); + + 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 (try --global).'); + }); + + 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', '--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 () => { + const program = new Command(); registerSkillCommand(program); + 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 () => { const program = new Command(); registerSkillCommand(program); @@ -194,7 +258,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..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/__tests__/util/skill-registry.test.ts b/packages/cli/src/__tests__/util/skill-registry.test.ts new file mode 100644 index 00000000..76d2ba3e --- /dev/null +++ b/packages/cli/src/__tests__/util/skill-registry.test.ts @@ -0,0 +1,45 @@ +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', + }); + 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..c5180dba 100644 --- a/packages/cli/src/commands/skill.ts +++ b/packages/cli/src/commands/skill.ts @@ -93,6 +93,36 @@ export function registerSkillCommand(program: Command): void { } })); + skillCommand + .command('remove-registry ') + .description('Unregister a third-party skill registry') + .option('-g, --global', 'Remove from global config and delete the cached registry') + .action(withErrorHandler('remove registry', async ( + id: string, + options: { global?: boolean }, + ) => { + validateRegistryId(id); + if (id === BUILTIN_SKILL_REGISTRY) { + throw new Error(`Registry "${id}" is built in and cannot be unregistered.`); + } + + 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).`); + } + + await configManager.removeSkillRegistry(id); + if (options.global) { + await new SkillManager(new ConfigManager()).removeRegistryCache(id); + } + + const scope = options.global ? 'global' : 'project'; + ui.success(`Removed ${scope} skill registry "${id}".`); + })); + skillCommand .command('list') .description('List installed project skills, or global skills with --global') 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/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(); diff --git a/packages/cli/src/util/skill-registry.ts b/packages/cli/src/util/skill-registry.ts index 3107776c..a8331b11 100644 --- a/packages/cli/src/util/skill-registry.ts +++ b/packages/cli/src/util/skill-registry.ts @@ -11,6 +11,13 @@ export interface SkillRegistryMutation { status: SkillRegistryAddStatus; } +export type SkillRegistryRemoveStatus = 'removed' | 'not-registered'; + +export interface SkillRegistryRemoveMutation { + registries: Record; + status: SkillRegistryRemoveStatus; +} + export function planSkillRegistryAdd( registries: Record, id: string, @@ -36,3 +43,16 @@ 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' }; + } + + delete nextRegistries[id]; + return { registries: nextRegistries, status: 'removed' }; +} diff --git a/web/content/docs/7-skills.md b/web/content/docs/7-skills.md index b76c2e2e..83d5e915 100644 --- a/web/content/docs/7-skills.md +++ b/web/content/docs/7-skills.md @@ -192,6 +192,19 @@ 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 +``` + +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. + +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` List skills installed in the current project, or inspect skills installed across known global environment paths.