From def998a52a5202728123567e52f9f62db6a368d3 Mon Sep 17 00:00:00 2001 From: Hoang Nguyen Date: Tue, 8 Sep 2026 21:14:22 +0200 Subject: [PATCH] refactor(cli): reorganize skill services --- ...7-feature-skill-module-service-refactor.md | 102 +++ ...7-feature-skill-module-service-refactor.md | 99 +++ ...7-feature-skill-module-service-refactor.md | 65 ++ ...7-feature-skill-module-service-refactor.md | 77 +++ ...7-feature-skill-module-service-refactor.md | 77 +++ .../cli/src/__tests__/commands/init.test.ts | 20 +- .../cli/src/__tests__/commands/skill.test.ts | 174 +++-- .../services/install/install.service.test.ts | 18 +- .../services/setup/setup-builtins.test.ts | 6 +- .../skill/index/skill-index.service.test.ts | 611 +++++++++++++++++ .../registry-skill-discovery.test.ts} | 2 +- .../registry/skill-registry-source.test.ts} | 2 +- .../registry/skill-registry.service.test.ts} | 139 +++- .../skill/skill-builtins.test.ts} | 14 +- .../skill/skill-validation.test.ts} | 2 +- .../skill/skill.service.test.ts} | 632 ++---------------- .../services/status/status.service.test.ts | 2 +- packages/cli/src/commands/init.ts | 2 +- packages/cli/src/commands/skill.ts | 118 ++-- packages/cli/src/lib/Config.ts | 2 +- packages/cli/src/lib/GlobalConfig.ts | 2 +- packages/cli/src/lib/InitTemplate.ts | 2 +- .../src/services/install/install.service.ts | 6 +- .../cli/src/services/setup/setup.service.ts | 8 +- .../skill/index/skill-index.repository.ts | 35 + .../skill/index/skill-index.service.ts} | 98 +-- .../installer/skill-installer.service.ts} | 312 +++------ .../registry/registry-skill-discovery.ts} | 14 +- .../skill/registry/skill-registry-source.ts} | 2 +- .../skill/registry/skill-registry.service.ts} | 101 ++- .../skill/skill-builtins.ts} | 6 +- .../src/services/skill/skill-description.ts | 27 + .../skill/skill-validation.ts} | 29 +- .../cli/src/services/skill/skill.service.ts | 99 +++ .../cli/src/services/skill/skill.types.ts | 35 + .../cli/src/services/status/status.service.ts | 2 +- 36 files changed, 1840 insertions(+), 1102 deletions(-) create mode 100644 docs/ai/design/2026-09-07-feature-skill-module-service-refactor.md create mode 100644 docs/ai/implementation/2026-09-07-feature-skill-module-service-refactor.md create mode 100644 docs/ai/planning/2026-09-07-feature-skill-module-service-refactor.md create mode 100644 docs/ai/requirements/2026-09-07-feature-skill-module-service-refactor.md create mode 100644 docs/ai/testing/2026-09-07-feature-skill-module-service-refactor.md create mode 100644 packages/cli/src/__tests__/services/skill/index/skill-index.service.test.ts rename packages/cli/src/__tests__/{util/local-registry.test.ts => services/skill/registry/registry-skill-discovery.test.ts} (97%) rename packages/cli/src/__tests__/{util/skill-registry.test.ts => services/skill/registry/skill-registry-source.test.ts} (98%) rename packages/cli/src/__tests__/{lib/SkillRegistry.test.ts => services/skill/registry/skill-registry.service.test.ts} (62%) rename packages/cli/src/__tests__/{lib/BuiltinSkills.test.ts => services/skill/skill-builtins.test.ts} (85%) rename packages/cli/src/__tests__/{util/skill.test.ts => services/skill/skill-validation.test.ts} (99%) rename packages/cli/src/__tests__/{lib/SkillManager.test.ts => services/skill/skill.service.test.ts} (63%) create mode 100644 packages/cli/src/services/skill/index/skill-index.repository.ts rename packages/cli/src/{lib/SkillIndex.ts => services/skill/index/skill-index.service.ts} (79%) rename packages/cli/src/{lib/SkillManager.ts => services/skill/installer/skill-installer.service.ts} (68%) rename packages/cli/src/{util/local-registry.ts => services/skill/registry/registry-skill-discovery.ts} (85%) rename packages/cli/src/{util/skill-registry.ts => services/skill/registry/skill-registry-source.ts} (98%) rename packages/cli/src/{lib/SkillRegistry.ts => services/skill/registry/skill-registry.service.ts} (75%) rename packages/cli/src/{lib/BuiltinSkills.ts => services/skill/skill-builtins.ts} (91%) create mode 100644 packages/cli/src/services/skill/skill-description.ts rename packages/cli/src/{util/skill.ts => services/skill/skill-validation.ts} (63%) create mode 100644 packages/cli/src/services/skill/skill.service.ts create mode 100644 packages/cli/src/services/skill/skill.types.ts diff --git a/docs/ai/design/2026-09-07-feature-skill-module-service-refactor.md b/docs/ai/design/2026-09-07-feature-skill-module-service-refactor.md new file mode 100644 index 00000000..5c63d09e --- /dev/null +++ b/docs/ai/design/2026-09-07-feature-skill-module-service-refactor.md @@ -0,0 +1,102 @@ +--- +phase: design +title: System Design & Architecture +description: Define the technical architecture, components, and data models +--- + +# System Design & Architecture + +## Architecture Overview +**What is the high-level system structure?** + +```mermaid +graph TD + SkillCommand[commands/skill.ts] --> SkillService[services/skill/skill.service.ts] + SetupService[services/setup/setup.service.ts] --> SkillService + InstallService[services/install/install.service.ts] --> SkillService + + SkillService --> Installer[installer/skill-installer.service.ts] + SkillService --> Registry[registry/skill-registry.service.ts] + SkillService --> Index[index/skill-index.service.ts] + + Installer --> Validation[skill-validation.ts] + Installer --> Description[skill-description.ts] + + Registry --> Sources[registry/skill-registry-source.ts] + Registry --> Discovery[registry/registry-skill-discovery.ts] + Registry --> Git[util/git.ts] + Registry --> Config[lib/Config.ts + lib/GlobalConfig.ts] + + Index --> Repository[index/skill-index.repository.ts] + Index --> Discovery + Index --> GitHub[util/github.ts] + Index --> Description +``` + +The feature keeps the repository's existing TypeScript/ESM stack and existing command/service direction. The skill module becomes a feature-owned service area under `services/skill`. + +## Data Models +**What data do we need to manage?** + +- `SkillEntry`: searchable skill index item with `name`, `registry`, `path`, `description`, and `lastIndexed`. +- `SkillIndexData`: persisted search index with metadata and skill entries. +- `SkillRegistryData`: merged registry map keyed by registry id. +- `InstalledSkill` and `GlobalInstalledSkill`: list output models used by commands. +- `AddSkillOptions` and `RemoveSkillOptions`: install/remove scope and environment options. +- `UpdateSummary` and `UpdateResult`: registry update result models. + +## API Design +**How do components communicate?** + +`SkillService` is the public application boundary for skill workflows: + +```ts +addSkill(registryId: string, skillName: string, options?: AddSkillOptions): Promise<'installed' | 'matched'> +addSkills(registryId: string, skillNames: string[], options?: AddSkillOptions): Promise<'installed' | 'matched'> +listInstallableSkills(registryId: string): Promise +removeSkill(skillName: string, options?: RemoveSkillOptions): Promise +listSkills(): Promise +listGlobalSkills(envCodes?: string[]): Promise +addRegistry(id: string, source: string, options?: AddSkillRegistryCommandOptions): Promise +removeRegistry(id: string, options?: RemoveSkillRegistryCommandOptions): Promise<'project' | 'global'> +updateSkills(registryId?: string): Promise +findSkills(keyword: string, options?: { refresh?: boolean }): Promise +rebuildIndex(outputPath?: string): Promise +``` + +Command rendering and interactive skill selection remain in `commands/skill.ts`. Services expose installable skill choices and install explicit skill names. Services may still use existing terminal UI for long-running status messages where current behavior already does so. + +## Component Breakdown +**What are the major building blocks?** + +- `skill.service.ts`: coordinates top-level skill use cases and owns dependency construction. +- `registry/skill-registry.service.ts`: fetches/merges registries, prepares Git/local registry repositories, adds/removes registry config, updates caches, and removes registry cache. +- `registry/registry-skill-discovery.ts`: discovers valid skills from registry directories and enforces local registry containment/metadata bounds. +- `installer/skill-installer.service.ts`: resolves install targets, lists installable skills, and installs/removes/lists installed skills for project/global targets. +- `index/skill-index.service.ts`: finds skills, rebuilds the index, updates one registry in the index, removes registry entries, and decides when to refresh or use stale data. +- `index/skill-index.repository.ts`: reads/writes/checks the persisted JSON index. +- Shared root files: + - `skill-validation.ts` + - `skill-description.ts` + - `skill-builtins.ts` + - `skill.types.ts` + +## Design Decisions +**Why did we choose this approach?** + +- Use `services/skill` instead of `lib`/`util` because skill behavior is a feature module, not generic infrastructure. +- Use one public `SkillService` plus three collaborator services because a file-per-use-case structure is too verbose for this module. +- Keep shared skill-domain primitives at the `services/skill` root because validation, description parsing, and built-ins are used across registry, installer, and index. +- Use explicit filenames for searchability. +- Use dot suffixes only for architectural roles, such as `.service.ts`, `.repository.ts`, and `.types.ts`. +- Keep checkbox selection in the command because selection, cancellation, and choice label formatting are CLI UI concerns. +- Keep installer target resolution inside `skill-installer.service.ts` because it is short and installer-specific. +- Do not keep `SkillManager` as a long-term compatibility wrapper because the type is internal to `packages/cli`. + +## Non-Functional Requirements +**How should the system perform?** + +- Preserve existing index caching, seed index fallback, registry update behavior, and concurrency. +- Preserve local registry containment checks and metadata size limits. +- Avoid broad behavior changes while moving code. +- Keep testability high by placing pure helpers in small files and persistence behind a repository. diff --git a/docs/ai/implementation/2026-09-07-feature-skill-module-service-refactor.md b/docs/ai/implementation/2026-09-07-feature-skill-module-service-refactor.md new file mode 100644 index 00000000..98b3d8f2 --- /dev/null +++ b/docs/ai/implementation/2026-09-07-feature-skill-module-service-refactor.md @@ -0,0 +1,99 @@ +--- +phase: implementation +title: Implementation Guide +description: Technical implementation notes, patterns, and code guidelines +--- + +# Implementation Guide + +## Development Setup +**How do we get started?** + +- Active worktree: `.worktrees/feature-skill-module-service-refactor` +- Branch: `feature-skill-module-service-refactor` +- Dependency bootstrap: `npm ci` + +## Code Structure +**How is the code organized?** + +Target structure: + +```text +packages/cli/src/services/skill/ + skill.service.ts + skill.types.ts + skill-validation.ts + skill-description.ts + skill-builtins.ts + + registry/ + skill-registry.service.ts + skill-registry-source.ts + registry-skill-discovery.ts + + installer/ + skill-installer.service.ts + + index/ + skill-index.service.ts + skill-index.repository.ts +``` + +Naming convention: +- dot suffixes for architectural roles: `.service.ts`, `.repository.ts`, `.types.ts`. +- hyphenated descriptive names for domain helpers. + +## Implementation Notes +**Key technical details to remember:** + +### Core Features +- Moved code first with minimal behavior changes. +- Added `SkillService` as the import boundary for skill workflows. +- Kept command table/status rendering in `commands/skill.ts`. +- Moved interactive skill selection into `commands/skill.ts`; installer services now list installable choices and install explicit skill names. +- Consolidated registry directory scanning in `registry/registry-skill-discovery.ts`. +- Inlined installer target resolution into `installer/skill-installer.service.ts`. +- Removed old skill-specific files from `lib` and `util` after imports/tests were updated. +- Moved skill-specific tests under `src/__tests__/services/skill`. + +### Patterns & Best Practices +- Preserve existing ESM `.js` import specifiers. +- Prefer explicit names for searchability. +- Keep service collaborators injectable where tests need mocks. + +## Integration Points +**How do pieces connect?** + +- `commands/skill.ts` imports `SkillService` from `services/skill/skill.service.ts`. +- `services/install/install.service.ts` imports the same skill service boundary. +- `services/setup/setup.service.ts`, `commands/init.ts`, and `services/status/status.service.ts` import built-ins from `services/skill/skill-builtins.ts`. +- Registry service continues to depend on `ConfigManager`, `GlobalConfigManager`, and Git helpers. + +## Implementation Summary + +- `services/skill/skill.service.ts` is now a thin facade over installer, registry, and index services. +- `services/skill/registry/registry-skill-discovery.ts` owns registry-directory skill discovery and local registry containment checks. +- `services/skill/installer/skill-installer.service.ts` owns target resolution, installable skill discovery plus add/list/remove installed skill behavior. +- `commands/skill.ts` owns interactive registry skill selection and cancellation handling. +- `services/skill/registry/skill-registry.service.ts` owns registry merge, cache, update, and add/remove source workflows. +- `services/skill/index/skill-index.service.ts` owns search/index lifecycle policy. +- `services/skill/index/skill-index.repository.ts` owns JSON index persistence. + +## Error Handling +**How do we handle failures?** + +- Preserve existing `CliError`, `ValidationError`, `NotFoundError`, stale index fallback, and local registry error messages where practical. +- Command-specific error rendering remains in command layer or existing `withErrorHandler`. + +## Performance Considerations +**How do we keep it fast?** + +- Preserve merged registry promise caching and prepared repository caching. +- Preserve index TTL, seed index, and GitHub fetch concurrency. + +## Security Notes +**What security measures are in place?** + +- Preserve registry id and skill name validation. +- Preserve local registry symlink/path containment checks. +- Preserve guarded cache removal under `~/.ai-devkit/skills`. diff --git a/docs/ai/planning/2026-09-07-feature-skill-module-service-refactor.md b/docs/ai/planning/2026-09-07-feature-skill-module-service-refactor.md new file mode 100644 index 00000000..45b7e494 --- /dev/null +++ b/docs/ai/planning/2026-09-07-feature-skill-module-service-refactor.md @@ -0,0 +1,65 @@ +--- +phase: planning +title: Project Planning & Task Breakdown +description: Break down work into actionable tasks and estimate timeline +--- + +# Project Planning & Task Breakdown + +## Milestones +**What are the major checkpoints?** + +- [x] Milestone 1: Move skill-domain helpers and service classes under `services/skill`. +- [x] Milestone 2: Replace `SkillManager` imports with `SkillService` and update command/service tests. +- [x] Milestone 3: Run focused skill tests, lint, and build. + +## Task Breakdown +**What specific work needs to be done?** + +### Phase 1: Foundation +- [x] Task 1.1: Create `services/skill` folder structure. +- [x] Task 1.2: Move validation, description, built-ins, registry source, and local registry helpers. +- [x] Task 1.3: Move `SkillRegistry`, `SkillIndex`, and installer behavior into service files. + +### Phase 2: Core Features +- [x] Task 2.1: Implement `SkillService` as the public boundary. +- [x] Task 2.2: Update `commands/skill.ts`, setup service, install service, and related tests to import `SkillService`. +- [x] Task 2.3: Remove obsolete `lib/SkillManager.ts`, `lib/SkillRegistry.ts`, `lib/SkillIndex.ts`, `lib/BuiltinSkills.ts`, and skill-specific util files. + +### Phase 3: Integration & Polish +- [x] Task 3.1: Update test file paths and mocks. +- [x] Task 3.2: Fix lint/type errors from moved imports. +- [x] Task 3.3: Run validation and review the diff. + +## Progress Summary + +Implementation moved the skill module into the agreed service-layer shape. The command layer now delegates registry workflows through `SkillService`, while registry cache/config behavior lives in `registry/skill-registry.service.ts`, install/list/remove behavior lives in `installer/skill-installer.service.ts`, and index persistence is split into `index/skill-index.repository.ts`. + +## Dependencies +**What needs to happen in what order?** + +- Move helpers before moving services so imports can be updated in a controlled order. +- Move service classes before command updates. +- Test updates depend on final source paths. + +## Timeline & Estimates +**When will things be done?** + +- Single-pairing-session refactor. +- Highest risk is test mock churn and ESM import path mistakes. + +## Risks & Mitigation +**What could go wrong?** + +- Risk: changing behavior while moving code. + Mitigation: preserve method bodies first, then only make targeted orchestration edits. +- Risk: tests mock removed module paths. + Mitigation: update mocks to new service paths and run focused tests. +- Risk: index repository split changes persistence behavior. + Mitigation: keep read/write behavior equivalent and verify with existing skill tests. + +## Resources Needed +**What do we need to succeed?** + +- Existing Vitest coverage for command, manager, registry, and skill utilities. +- Package scripts: `npm --workspace packages/cli test`, `lint`, and `build`. diff --git a/docs/ai/requirements/2026-09-07-feature-skill-module-service-refactor.md b/docs/ai/requirements/2026-09-07-feature-skill-module-service-refactor.md new file mode 100644 index 00000000..dd6f48f5 --- /dev/null +++ b/docs/ai/requirements/2026-09-07-feature-skill-module-service-refactor.md @@ -0,0 +1,77 @@ +--- +phase: requirements +title: Requirements & Problem Understanding +description: Clarify the problem space, gather requirements, and define success criteria +--- + +# Requirements & Problem Understanding + +## Problem Statement +**What problem are we solving?** + +- The CLI skill module is currently split across `commands/skill.ts`, `lib/SkillManager.ts`, `lib/SkillRegistry.ts`, `lib/SkillIndex.ts`, and several `util/skill*` helpers. +- `SkillManager` has grown into a broad class that owns installation, removal, listing, environment resolution, prompt handling, filesystem operations, and registry/index delegation. +- `commands/skill.ts` contains registry mutation workflow details that should live behind a service boundary. +- Maintainers and contributors are affected because skill behavior is harder to scale, search, and test in focused units. + +## Goals & Objectives +**What do we want to achieve?** + +- Move skill behavior under `packages/cli/src/services/skill`. +- Introduce a clear command-to-service layering model: + - command layer parses CLI args, owns exit codes, and renders command output. + - skill service layer owns application workflows. + - collaborator services own registry, installer, and index behavior. +- Replace the broad `SkillManager` concept with `SkillService`, without keeping a long-term compatibility wrapper. +- Use explicit, searchable names such as `skill-registry.service.ts`, `skill-installer.service.ts`, and `skill-index.repository.ts`. +- Keep behavior and public CLI contracts unchanged. +- Preserve existing local registry security checks, registry conflict handling, install behavior, built-in skill behavior, and index fallback behavior. + +Non-goals: +- Do not change user-facing CLI syntax. +- Do not redesign registry/index algorithms beyond the structural move. +- Do not introduce a package-wide clean architecture pattern. +- Do not push or publish changes; user will review before push. + +## User Stories & Use Cases +**How will users interact with the solution?** + +- As a maintainer, I want `commands/skill.ts` to call a single skill service boundary so command code stays easy to scan. +- As a contributor, I want registry, installer, and index behavior grouped by subdomain so I can find related logic quickly. +- As a test author, I want smaller services and helpers so tests can target one workflow without mocking unrelated behavior. +- Existing CLI users should keep using: + - `ai-devkit skill add` + - `ai-devkit skill add-registry` + - `ai-devkit skill remove-registry` + - `ai-devkit skill list` + - `ai-devkit skill remove` + - `ai-devkit skill update` + - `ai-devkit skill find` + - `ai-devkit skill rebuild-index` + +## Success Criteria +**How will we know when we're done?** + +- `commands/skill.ts` delegates skill workflows to `services/skill/skill.service.ts`. +- Skill files are organized under: + - `services/skill/registry` + - `services/skill/installer` + - `services/skill/index` + - shared root skill-domain files. +- Internal imports no longer depend on `lib/SkillManager.ts`, `lib/SkillRegistry.ts`, `lib/SkillIndex.ts`, `lib/BuiltinSkills.ts`, `util/skill.ts`, `util/skill-registry.ts`, or `util/local-registry.ts`. +- Existing tests are updated to the new paths and still pass. +- `npm --workspace packages/cli test -- skill`, `npm --workspace packages/cli run lint`, and `npm --workspace packages/cli run build` pass. + +## Constraints & Assumptions +**What limitations do we need to work within?** + +- The repo uses ESM-style `.js` import specifiers in TypeScript source. +- The refactor should follow existing `commands -> services -> lower-level helpers` direction used by install/setup commands. +- Some tests mock module paths directly, so tests must be moved or updated with the source move. +- `SkillService` remains internal to `packages/cli`; no external compatibility wrapper is required. +- Existing untracked files in the main checkout are unrelated and must not be modified. + +## Questions & Open Items +**What do we still need to clarify?** + +- None. Naming and structure have been agreed with the user in this thread. diff --git a/docs/ai/testing/2026-09-07-feature-skill-module-service-refactor.md b/docs/ai/testing/2026-09-07-feature-skill-module-service-refactor.md new file mode 100644 index 00000000..cdaa1078 --- /dev/null +++ b/docs/ai/testing/2026-09-07-feature-skill-module-service-refactor.md @@ -0,0 +1,77 @@ +--- +phase: testing +title: Testing Strategy +description: Define testing approach, test cases, and quality assurance +--- + +# Testing Strategy + +## Test Coverage Goals +**What level of testing do we aim for?** + +- Preserve existing behavioral coverage after moving code. +- Keep unit tests focused on helpers and services. +- Run command tests to verify CLI-to-service wiring. +- Run build to verify declaration emit and ESM import paths. + +## Unit Tests +**What individual components need testing?** + +### Skill Validation / Description +- [x] Skill and registry validation accepts/rejects the same inputs as before. +- [x] Description extraction preserves frontmatter and fallback behavior. + +### Registry Source / Registry Discovery +- [x] Local registry source parsing and normalization behavior is preserved. +- [x] Duplicate canonical local folders are still rejected. +- [x] Registry skill discovery, local registry path containment, and metadata limits remain covered by registry tests. + +### Skill Services +- [x] Skill installer preserves add/list/remove behavior. +- [x] Skill installer exposes installable skill discovery without owning command prompts. +- [x] Skill installer target resolution remains covered through add/list/remove behavior. +- [x] Skill registry service preserves add/remove registry workflow. +- [x] Skill index service preserves search, rebuild, refresh, stale fallback, and per-registry update behavior. +- [x] Index/update coverage is split from the broader skill service test file. + +## Integration Tests +**How do we test component interactions?** + +- [x] `commands/skill.ts` calls `SkillService` for skill and registry workflows and owns omitted-skill interactive selection. +- [x] `services/install/install.service.ts` installs configured skills through `SkillService`. +- [x] `services/setup/setup.service.ts` installs built-in skills through the new built-ins/service path. + +## End-to-End Tests +**What user flows need validation?** + +- [x] Existing automated command/service tests cover the critical CLI flows. +- [x] Manual CLI smoke is optional because behavior is structural and existing tests are broad. + +## Test Data +**What data do we use for testing?** + +- Existing test fixtures, mocked config managers, mocked filesystem, mocked Git/GitHub helpers, and mocked terminal UI. + +## Test Reporting & Coverage +**How do we verify and communicate test results?** + +- Required evidence: + - `npm --workspace packages/cli test -- skill`: exit 0, 8 files and 175 tests passed. + - `npm --workspace packages/cli run lint`: exit 0, with one pre-existing warning in `commands/channel.ts`. + - `npm run build`: exit 0, all 6 workspace projects built. + - `npm --workspace packages/cli test`: exit 0, 96 files and 1147 tests passed. + +## Manual Testing +**What requires human validation?** + +- User code review before push. + +## Performance Testing +**How do we validate performance?** + +- No separate performance testing required. Preserve existing index concurrency and cache behavior. + +## Bug Tracking +**How do we manage issues?** + +- Any failing validation becomes an implementation task before final review. diff --git a/packages/cli/src/__tests__/commands/init.test.ts b/packages/cli/src/__tests__/commands/init.test.ts index f378a37c..d21bfa96 100644 --- a/packages/cli/src/__tests__/commands/init.test.ts +++ b/packages/cli/src/__tests__/commands/init.test.ts @@ -5,7 +5,7 @@ const { mockTemplateManager, mockEnvironmentSelector, mockPhaseSelector, - mockSkillManager, + mockSkillService, mockUi, mockConfirm, mockLoadInitTemplate, @@ -38,7 +38,7 @@ const { selectPhases: vi.fn(), displaySelectionSummary: vi.fn(), } as any, - mockSkillManager: { addSkill: vi.fn() } as any, + mockSkillService: { addSkill: vi.fn() } as any, mockUi: { warning: vi.fn(), error: vi.fn(), @@ -87,11 +87,11 @@ vi.mock('../../lib/PhaseSelector.js', () => ({ PhaseSelector: vi.fn(function () { return mockPhaseSelector; }) })); -vi.mock('../../lib/SkillManager.js', () => ({ - SkillManager: vi.fn(function () { return mockSkillManager; }) +vi.mock('../../services/skill/skill.service.js', () => ({ + SkillService: vi.fn(function () { return mockSkillService; }) })); -vi.mock('../../lib/BuiltinSkills.js', () => ({ +vi.mock('../../services/skill/skill-builtins.js', () => ({ BUILTIN_SKILL_REGISTRY: 'codeaholicguy/ai-devkit', getBuiltinSkillNames: (...args: unknown[]) => mockGetBuiltinSkillNames(...args), })); @@ -109,7 +109,7 @@ vi.mock('../../util/terminal.js', () => ({ })); import { initCommand } from '../../commands/init.js'; -import { BUILTIN_SKILL_REGISTRY } from '../../lib/BuiltinSkills.js'; +import { BUILTIN_SKILL_REGISTRY } from '../../services/skill/skill-builtins.js'; function confirmCallsMatching(pattern: RegExp): any[] { return mockConfirm.mock.calls.filter(([config]: any[]) => @@ -146,7 +146,7 @@ describe('init command', () => { mockPhaseSelector.selectPhases.mockResolvedValue(['requirements']); - mockSkillManager.addSkill.mockResolvedValue(undefined); + mockSkillService.addSkill.mockResolvedValue(undefined); mockLoadInitTemplate.mockResolvedValue({}); mockIsInteractiveTerminal.mockReturnValue(true); mockReconcileAndInstall.mockResolvedValue({ @@ -344,7 +344,7 @@ describe('init command', () => { const builtinPromptCalls = confirmCallsMatching(/Install AI DevKit built-in skills/); expect(builtinPromptCalls.length).toBe(1); - expect(mockSkillManager.addSkill).not.toHaveBeenCalled(); + expect(mockSkillService.addSkill).not.toHaveBeenCalled(); expect(mockGetBuiltinSkillNames).not.toHaveBeenCalled(); }); @@ -385,7 +385,7 @@ describe('init command', () => { const builtinPrompts = confirmCallsMatching(/Install AI DevKit built-in skills/); expect(builtinPrompts).toHaveLength(0); - expect(mockSkillManager.addSkill).not.toHaveBeenCalled(); + expect(mockSkillService.addSkill).not.toHaveBeenCalled(); expect(mockGetBuiltinSkillNames).not.toHaveBeenCalled(); expect(mockUi.info).toHaveBeenCalledWith( expect.stringMatching(/non-interactive|--built-in/) @@ -514,7 +514,7 @@ describe('init command', () => { const builtinPrompts = confirmCallsMatching(/Install AI DevKit built-in skills/); expect(builtinPrompts).toHaveLength(0); - expect(mockSkillManager.addSkill).not.toHaveBeenCalled(); + expect(mockSkillService.addSkill).not.toHaveBeenCalled(); }); it('installs built-in skills under --yes when --built-in is also passed', async () => { diff --git a/packages/cli/src/__tests__/commands/skill.test.ts b/packages/cli/src/__tests__/commands/skill.test.ts index 95ed4a2d..1694385f 100644 --- a/packages/cli/src/__tests__/commands/skill.test.ts +++ b/packages/cli/src/__tests__/commands/skill.test.ts @@ -2,59 +2,43 @@ import { Command } from 'commander'; import { registerSkillCommand } from '../../commands/skill.js'; import { ui } from '../../util/terminal-ui.js'; -import { SkillManager } from '../../lib/SkillManager.js'; +import { SkillService } from '../../services/skill/skill.service.js'; -const mockRemoveCache = vi.hoisted(() => vi.fn()); const mockGetBuiltinSkillNames = vi.hoisted(() => vi.fn()); +const mockIsInteractiveTerminal = vi.hoisted(() => vi.fn(() => true)); +const mockCheckbox = vi.hoisted(() => vi.fn()); const mockAddSkill = vi.fn(); +const mockAddSkills = vi.fn(); +const mockAddRegistry = vi.fn(); const mockListGlobalSkills = vi.fn(); +const mockListInstallableSkills = 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(); -const mockGlobalGetSkillRegistries = vi.fn(); -const mockGlobalAddSkillRegistry = vi.fn(); -const mockGlobalRemoveSkillRegistry = vi.fn(); +const mockRemoveRegistry = 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), - }; }), -})); - -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), - }; }), + ConfigManager: vi.fn(function () { return {}; }), })); -vi.mock('../../lib/SkillManager.js', () => ({ - SkillManager: vi.fn(function () { return { +vi.mock('../../services/skill/skill.service.js', () => ({ + SkillService: vi.fn(function () { return { addSkill: (...args: unknown[]) => mockAddSkill(...args), + addSkills: (...args: unknown[]) => mockAddSkills(...args), + addRegistry: (...args: unknown[]) => mockAddRegistry(...args), listGlobalSkills: (...args: unknown[]) => mockListGlobalSkills(...args), + listInstallableSkills: (...args: unknown[]) => mockListInstallableSkills(...args), listSkills: (...args: unknown[]) => mockListSkills(...args), removeSkill: (...args: unknown[]) => mockRemoveSkill(...args), - cacheRegistry: (...args: unknown[]) => mockCacheRegistry(...args), - updateSkillIndexForRegistry: (...args: unknown[]) => mockUpdateSkillIndexForRegistry(...args), - removeSkillIndexForRegistry: (...args: unknown[]) => mockRemoveSkillIndexForRegistry(...args), - removeRegistryCache: (...args: unknown[]) => mockRemoveCache(...args), + removeRegistry: (...args: unknown[]) => mockRemoveRegistry(...args), updateSkills: vi.fn(), findSkills: vi.fn(), rebuildIndex: vi.fn(), }; }), })); -vi.mock('../../lib/BuiltinSkills.js', () => ({ +vi.mock('../../services/skill/skill-builtins.js', () => ({ BUILTIN_SKILL_REGISTRY: 'codeaholicguy/ai-devkit', getBuiltinSkillNames: (...args: unknown[]) => mockGetBuiltinSkillNames(...args), })); @@ -70,79 +54,78 @@ vi.mock('../../util/terminal-ui.js', () => ({ }, })); +vi.mock('../../util/terminal.js', () => ({ + isInteractiveTerminal: (...args: unknown[]) => mockIsInteractiveTerminal(...args), +})); + +vi.mock('@inquirer/prompts', () => ({ + checkbox: (...args: unknown[]) => mockCheckbox(...args), +})); + describe('skill command', () => { beforeEach(() => { vi.clearAllMocks(); mockAddSkill.mockImplementation(async () => undefined); + mockAddSkills.mockImplementation(async () => undefined); + mockAddRegistry.mockResolvedValue('added'); mockListGlobalSkills.mockResolvedValue([]); + mockListInstallableSkills.mockResolvedValue([ + { name: 'frontend-design', description: 'Frontend skill' }, + { name: 'debug' }, + ]); mockListSkills.mockResolvedValue([]); mockRemoveSkill.mockImplementation(async () => undefined); - mockCacheRegistry.mockResolvedValue('/tmp/registry-cache'); - mockUpdateSkillIndexForRegistry.mockImplementation(async () => undefined); - mockRemoveSkillIndexForRegistry.mockImplementation(async () => undefined); - mockRemoveCache.mockResolvedValue(undefined); - mockProjectGetSkillRegistries.mockResolvedValue({}); - mockProjectAddSkillRegistry.mockResolvedValue({}); - mockGlobalGetSkillRegistries.mockResolvedValue({}); - mockGlobalAddSkillRegistry.mockResolvedValue({}); - mockProjectRemoveSkillRegistry.mockResolvedValue({}); - mockGlobalRemoveSkillRegistry.mockResolvedValue({}); + mockRemoveRegistry.mockResolvedValue('project'); mockGetBuiltinSkillNames.mockResolvedValue(['remote-one', 'remote-two']); + mockIsInteractiveTerminal.mockReturnValue(true); + mockCheckbox.mockResolvedValue(['frontend-design']); 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(mockRemoveRegistry).toHaveBeenCalledWith('example/skills', { global: undefined }); 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' }); + mockRemoveRegistry.mockResolvedValue('global'); 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(mockRemoveRegistry).toHaveBeenCalledWith('example/skills', { global: true }); 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' }); + mockRemoveRegistry.mockRejectedValue(new Error('Registry "codeaholicguy/ai-devkit" is built in and cannot be unregistered.')); 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' }); + mockRemoveRegistry.mockRejectedValue(new Error('Registry example/skills is not registered (try --global).')); 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 () => { + mockRemoveRegistry.mockRejectedValue(new Error('Registry x/missing is not registered (try --global).')); 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 () => { + mockRemoveRegistry.mockRejectedValue(new Error('Invalid registry ID format: "invalid". Expected format: "org/repo"')); 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(); + expect(ui.error).toHaveBeenCalledWith(expect.stringContaining('Invalid registry ID format')); }); it('adds an opaque registry URL to project config by default', async () => { @@ -151,34 +134,21 @@ describe('skill command', () => { await program.parseAsync(['node', 'test', 'skill', 'add-registry', 'example/private-skills', 'git@example.com:example/private-skills.git']); - expect(mockProjectAddSkillRegistry).toHaveBeenCalledWith( + expect(mockAddRegistry).toHaveBeenCalledWith( 'example/private-skills', 'git@example.com:example/private-skills.git', { force: undefined }, ); - expect(mockGlobalAddSkillRegistry).not.toHaveBeenCalled(); - expect(mockCacheRegistry).toHaveBeenCalledWith( - 'example/private-skills', - 'git@example.com:example/private-skills.git', - ); - expect(mockUpdateSkillIndexForRegistry).toHaveBeenCalledWith('example/private-skills', '/tmp/registry-cache'); - expect(mockCacheRegistry.mock.invocationCallOrder[0]).toBeLessThan( - mockUpdateSkillIndexForRegistry.mock.invocationCallOrder[0], - ); }); it('reports an identical target-scope registry as already registered', async () => { - mockProjectGetSkillRegistries.mockResolvedValue({ 'anthropics/skills': 'same-url' }); + mockAddRegistry.mockResolvedValue('already-registered'); const program = new Command(); registerSkillCommand(program); await program.parseAsync(['node', 'test', 'skill', 'add-registry', 'anthropics/skills', 'same-url']); - expect(mockProjectAddSkillRegistry).toHaveBeenCalledWith( - 'anthropics/skills', - 'same-url', - { force: undefined }, - ); + expect(mockAddRegistry).toHaveBeenCalledWith('anthropics/skills', 'same-url', { force: undefined }); expect(ui.info).toHaveBeenCalledWith('Registry "anthropics/skills" is already registered.'); expect(ui.success).not.toHaveBeenCalled(); }); @@ -189,23 +159,21 @@ describe('skill command', () => { await program.parseAsync(['node', 'test', 'skill', 'add-registry', 'example/private-skills', 'opaque-url', globalFlag]); - expect(mockGlobalGetSkillRegistries).toHaveBeenCalledOnce(); - expect(mockGlobalAddSkillRegistry).toHaveBeenCalledWith( + expect(mockAddRegistry).toHaveBeenCalledWith( 'example/private-skills', 'opaque-url', - { force: undefined }, + { global: true, force: undefined }, ); - expect(mockProjectAddSkillRegistry).not.toHaveBeenCalled(); }); it.each(['-f', '--force'])('forwards %s and reports a forced update', async forceFlag => { - mockProjectGetSkillRegistries.mockResolvedValue({ 'example/private-skills': 'old-url' }); + mockAddRegistry.mockResolvedValue('updated'); const program = new Command(); registerSkillCommand(program); await program.parseAsync(['node', 'test', 'skill', 'add-registry', 'example/private-skills', 'new-url', forceFlag]); - expect(mockProjectAddSkillRegistry).toHaveBeenCalledWith( + expect(mockAddRegistry).toHaveBeenCalledWith( 'example/private-skills', 'new-url', { force: true }, @@ -223,7 +191,7 @@ describe('skill command', () => { await program.parseAsync(['node', 'test', 'skill', 'add-registry', 'anthropics/skills', url]); - expect(mockProjectAddSkillRegistry).toHaveBeenCalledWith( + expect(mockAddRegistry).toHaveBeenCalledWith( 'anthropics/skills', url, { force: undefined }, @@ -235,18 +203,17 @@ describe('skill command', () => { async id => { const program = new Command(); registerSkillCommand(program); + mockAddRegistry.mockRejectedValue(new Error(`Invalid registry ID format: "${id}". Expected format: "org/repo"`)); await program.parseAsync(['node', 'test', 'skill', 'add-registry', id, 'opaque-url']); expect(ui.error).toHaveBeenCalledWith(expect.stringContaining('Invalid registry ID format')); expect(process.exit).toHaveBeenCalledWith(1); - expect(mockProjectAddSkillRegistry).not.toHaveBeenCalled(); - expect(mockGlobalAddSkillRegistry).not.toHaveBeenCalled(); } ); it('rejects a target-scope conflict without calling the setter', async () => { - mockProjectGetSkillRegistries.mockResolvedValue({ 'example/private-skills': 'old-url' }); + mockAddRegistry.mockRejectedValue(new Error('Registry "example/private-skills" is already registered with a different URL. Use --force to overwrite it.')); const program = new Command(); registerSkillCommand(program); @@ -255,7 +222,6 @@ describe('skill command', () => { expect(ui.error).toHaveBeenCalledWith( 'Failed to add registry: Registry "example/private-skills" is already registered with a different URL. Use --force to overwrite it.' ); - expect(mockProjectAddSkillRegistry).not.toHaveBeenCalled(); }); it('documents add-registry arguments and scope/conflict flags', () => { @@ -276,16 +242,26 @@ describe('skill command', () => { expect(skillCommand?.commands.some(command => command.name() === 'list-registries')).toBe(false); }); - it('parses skill add with registry only and forwards undefined skill name', async () => { + it('prompts for skill add when skill name is omitted', async () => { const program = new Command(); registerSkillCommand(program); await program.parseAsync(['node', 'test', 'skill', 'add', 'anthropics/skills']); - expect(mockAddSkill).toHaveBeenCalledWith('anthropics/skills', undefined, { + expect(mockListInstallableSkills).toHaveBeenCalledWith('anthropics/skills'); + expect(mockCheckbox).toHaveBeenCalledWith({ + message: 'Select skill(s) to install', + choices: [ + { name: 'frontend-design - Frontend skill', value: 'frontend-design' }, + { name: 'debug', value: 'debug' }, + ], + required: true, + }); + expect(mockAddSkills).toHaveBeenCalledWith('anthropics/skills', ['frontend-design'], { global: undefined, environments: undefined, }); + expect(mockAddSkill).not.toHaveBeenCalled(); expect(process.stderr.write).not.toHaveBeenCalled(); }); @@ -299,12 +275,14 @@ describe('skill command', () => { global: undefined, environments: undefined, }); + expect(mockListInstallableSkills).not.toHaveBeenCalled(); + expect(mockAddSkills).not.toHaveBeenCalled(); }); it('shows a warning instead of exiting when skill selection is cancelled', async () => { - mockAddSkill.mockImplementation(async () => { - throw new Error('Skill selection cancelled.'); - }); + const error = new Error('User cancelled'); + error.name = 'ExitPromptError'; + mockCheckbox.mockRejectedValue(error); const program = new Command(); registerSkillCommand(program); @@ -313,6 +291,22 @@ describe('skill command', () => { expect(ui.warning).toHaveBeenCalledWith('Skill selection cancelled.'); expect(ui.error).not.toHaveBeenCalled(); + expect(mockAddSkills).not.toHaveBeenCalled(); + }); + + it('fails before prompting when skill name is omitted in non-interactive mode', async () => { + mockIsInteractiveTerminal.mockReturnValue(false); + + const program = new Command(); + registerSkillCommand(program); + + await program.parseAsync(['node', 'test', 'skill', 'add', 'anthropics/skills']); + + expect(ui.error).toHaveBeenCalledWith('Failed to add skill: Skill name is required in non-interactive mode. Re-run with: ai-devkit skill add '); + expect(process.exit).toHaveBeenCalledWith(1); + expect(mockListInstallableSkills).not.toHaveBeenCalled(); + expect(mockCheckbox).not.toHaveBeenCalled(); + expect(mockAddSkills).not.toHaveBeenCalled(); }); it('installs all built-in skills with skill add --built-in', async () => { @@ -331,7 +325,7 @@ describe('skill command', () => { environments: undefined, }); expect(mockGetBuiltinSkillNames).toHaveBeenCalledOnce(); - expect(SkillManager).toHaveBeenCalledTimes(1); + expect(SkillService).toHaveBeenCalledTimes(1); }); it('exits when skill add has neither registry nor --built-in', async () => { diff --git a/packages/cli/src/__tests__/services/install/install.service.test.ts b/packages/cli/src/__tests__/services/install/install.service.test.ts index 3d9d2c55..5485550e 100644 --- a/packages/cli/src/__tests__/services/install/install.service.test.ts +++ b/packages/cli/src/__tests__/services/install/install.service.test.ts @@ -18,7 +18,7 @@ const mockTemplateManager: any = { copyPhaseTemplate: vi.fn() }; -const mockSkillManager: any = { +const mockSkillService: any = { addSkill: vi.fn() }; @@ -42,12 +42,12 @@ vi.mock('../../../lib/EnvironmentSelector.js', () => ({ EnvironmentSelector: vi.fn() })); -vi.mock('../../../lib/SkillManager.js', () => ({ - SkillManager: vi.fn(function () { return mockSkillManager; }) +vi.mock('../../../services/skill/skill.service.js', () => ({ + SkillService: vi.fn(function () { return mockSkillService; }) })); import { getInstallExitCode, reconcileAndInstall } from '../../../services/install/install.service.js'; -import { SkillManager } from '../../../lib/SkillManager.js'; +import { SkillService } from '../../../services/skill/skill.service.js'; describe('install service', () => { const installConfig = { @@ -78,7 +78,7 @@ describe('install service', () => { mockTemplateManager.setupMultipleEnvironments.mockResolvedValue([]); mockTemplateManager.copyPhaseTemplate.mockResolvedValue('docs/ai/requirements/README.md'); - mockSkillManager.addSkill.mockResolvedValue(undefined); + mockSkillService.addSkill.mockResolvedValue(undefined); mockConfirm.mockResolvedValue(false); mockIsInteractiveTerminal.mockReturnValue(true); }); @@ -116,8 +116,8 @@ describe('install service', () => { const report = await reconcileAndInstall(mixedRegistryConfig, {}); - expect(SkillManager).toHaveBeenCalledTimes(1); - expect(mockSkillManager.addSkill).toHaveBeenCalledTimes(3); + expect(SkillService).toHaveBeenCalledTimes(1); + expect(mockSkillService.addSkill).toHaveBeenCalledTimes(3); expect(report.skills.installed).toBe(3); }); @@ -203,12 +203,12 @@ describe('install service', () => { registries: { team: 'https://example.com/team-skills.git' } })); expect(mockConfigManager.update.mock.invocationCallOrder[0]).toBeLessThan( - mockSkillManager.addSkill.mock.invocationCallOrder[0] + mockSkillService.addSkill.mock.invocationCallOrder[0] ); }); it('reports skill failures as warnings and continues', async () => { - mockSkillManager.addSkill.mockRejectedValue(new Error('network down')); + mockSkillService.addSkill.mockRejectedValue(new Error('network down')); const report = await reconcileAndInstall(installConfig, {}); diff --git a/packages/cli/src/__tests__/services/setup/setup-builtins.test.ts b/packages/cli/src/__tests__/services/setup/setup-builtins.test.ts index 58ab66f6..94a66251 100644 --- a/packages/cli/src/__tests__/services/setup/setup-builtins.test.ts +++ b/packages/cli/src/__tests__/services/setup/setup-builtins.test.ts @@ -8,13 +8,13 @@ const { mockAddSkill, mockGetBuiltinSkillNames } = vi.hoisted(() => ({ mockGetBuiltinSkillNames: vi.fn(), })); -vi.mock('../../../lib/SkillManager.js', () => ({ - SkillManager: vi.fn(function () { +vi.mock('../../../services/skill/skill.service.js', () => ({ + SkillService: vi.fn(function () { return { addSkill: (...args: unknown[]) => mockAddSkill(...args) }; }), })); -vi.mock('../../../lib/BuiltinSkills.js', () => ({ +vi.mock('../../../services/skill/skill-builtins.js', () => ({ BUILTIN_SKILL_REGISTRY: 'codeaholicguy/ai-devkit', getBuiltinSkillNames: (...args: unknown[]) => mockGetBuiltinSkillNames(...args), })); diff --git a/packages/cli/src/__tests__/services/skill/index/skill-index.service.test.ts b/packages/cli/src/__tests__/services/skill/index/skill-index.service.test.ts new file mode 100644 index 00000000..02f2d7fa --- /dev/null +++ b/packages/cli/src/__tests__/services/skill/index/skill-index.service.test.ts @@ -0,0 +1,611 @@ +import type { MockedClass, Mocked } from 'vitest'; +import fs from "fs-extra"; +import * as os from "os"; +import * as path from "path"; +import { SkillService } from "../../../../services/skill/skill.service.js"; +import { ConfigManager } from "../../../../lib/Config.js"; +import { EnvironmentSelector } from "../../../../lib/EnvironmentSelector.js"; +import { GlobalConfigManager } from "../../../../lib/GlobalConfig.js"; +import * as gitUtil from "../../../../util/git.js"; +import * as skillUtil from "../../../../services/skill/skill-validation.js"; + +vi.mock("fs-extra", () => ({ + default: { + pathExists: vi.fn(), + ensureDir: vi.fn(), + symlink: vi.fn(), + copy: vi.fn(), + lstat: vi.fn(), + remove: vi.fn(), + readdir: vi.fn(), + opendir: vi.fn(), + realpath: vi.fn(), + readFile: vi.fn(), + readJson: vi.fn(), + stat: vi.fn(), + writeJson: vi.fn(), + }, +})); +vi.mock("../../../../lib/Config.js", () => ({ + ConfigManager: vi.fn(function () { return { + addSkill: vi.fn(), + create: vi.fn(), + getSkillRegistries: vi.fn(), + read: vi.fn(), + removeSkill: vi.fn(), + update: vi.fn(), + }; }), +})); +vi.mock("../../../../lib/EnvironmentSelector.js", () => ({ + EnvironmentSelector: vi.fn(function () { return { + selectEnvironments: vi.fn(), + selectSkillEnvironments: vi.fn(), + selectGlobalSkillEnvironments: vi.fn(), + confirmOverride: vi.fn(), + displaySelectionSummary: vi.fn(), + }; }), +})); +vi.mock("../../../../lib/GlobalConfig.js", () => ({ + GlobalConfigManager: vi.fn(function () { return { + getSkillRegistries: vi.fn(), + }; }), +})); +vi.mock("../../../../util/git.js", () => ({ + ensureGitInstalled: vi.fn(), + cloneRepository: vi.fn(), + pullRepository: vi.fn(), + isGitRepository: vi.fn(), + fetchGitHead: vi.fn(), + isInsideGitWorkTreeSync: vi.fn(), + localBranchExistsSync: vi.fn(), + getWorktreePathsForBranchSync: vi.fn(), +})); +vi.mock("../../../../services/skill/skill-validation.js", () => ({ + validateRegistryId: vi.fn(), + validateSkillName: vi.fn(), + isValidSkillName: vi.fn(), +})); +vi.mock("../../../../services/skill/skill-description.js", () => ({ + extractSkillDescription: vi.fn(), +})); +vi.mock("../../../../util/terminal.js", () => ({ + isInteractiveTerminal: vi.fn(() => true), +})); + +vi.mock("ora", () => ({ + default: vi.fn(function () { return { + start: vi.fn().mockReturnThis(), + succeed: vi.fn().mockReturnThis(), + fail: vi.fn().mockReturnThis(), + warn: vi.fn().mockReturnThis(), + stop: vi.fn().mockReturnThis(), + text: '', + isSpinning: false, + }; }), +})); + +import * as skillDescription from "../../../../services/skill/skill-description.js"; +const mockedSkillDescription = skillDescription as Mocked; + +const mockedFs = fs as Mocked; +const MockedConfigManager = ConfigManager as MockedClass< + typeof ConfigManager +>; +const MockedEnvironmentSelector = EnvironmentSelector as MockedClass< + typeof EnvironmentSelector +>; +const MockedGlobalConfigManager = GlobalConfigManager as MockedClass< + typeof GlobalConfigManager +>; +const mockedGitUtil = gitUtil as Mocked; +const mockedSkillUtil = skillUtil as Mocked; + +function mockFetch(response: any) { + global.fetch = vi.fn().mockResolvedValue({ + ok: true, + json: () => Promise.resolve(response) + }); +} + + + +describe("SkillService", () => { + let skillManager: SkillService; + let mockConfigManager: Mocked; + let mockEnvironmentSelector: Mocked; + let mockGlobalConfigManager: Mocked; + + beforeEach(() => { + vi.clearAllMocks(); + vi.spyOn(console, "log").mockImplementation(() => { }); + + mockConfigManager = new MockedConfigManager() as Mocked; + mockEnvironmentSelector = + new MockedEnvironmentSelector() as Mocked; + mockGlobalConfigManager = + new MockedGlobalConfigManager() as Mocked; + + mockGlobalConfigManager.getSkillRegistries.mockResolvedValue({}); + mockConfigManager.getSkillRegistries.mockResolvedValue({}); + + skillManager = new SkillService( + mockConfigManager, + mockEnvironmentSelector, + mockGlobalConfigManager, + ); + + mockedSkillUtil.validateRegistryId.mockImplementation(() => { }); + mockedSkillUtil.validateSkillName.mockImplementation(() => { }); + mockedSkillUtil.isValidSkillName.mockImplementation((name: string) => /^[a-z0-9]+(-[a-z0-9]+)*$/.test(name)); + mockedGitUtil.ensureGitInstalled.mockResolvedValue(undefined); + mockConfigManager.addSkill.mockResolvedValue({} as any); + (mockedFs.realpath as any).mockImplementation(async (checkedPath: string) => checkedPath); + (mockedFs.stat as any).mockResolvedValue({ size: 100 }); + (mockedFs.opendir as any).mockResolvedValue({ + async *[Symbol.asyncIterator]() { }, + }); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe("updateSkills", () => { + + beforeEach(() => { + vi.spyOn(console, "log").mockImplementation(() => { }); + mockedGitUtil.ensureGitInstalled.mockResolvedValue(undefined); + }); + + it("does not require git when there is no cached git registry to update", async () => { + (mockedFs.pathExists as any).mockResolvedValue(false); + + await skillManager.updateSkills(); + + expect(mockedGitUtil.ensureGitInstalled).not.toHaveBeenCalled(); + }); + + it("should return empty summary when cache directory does not exist", async () => { + (mockedFs.pathExists as any).mockResolvedValue(false); + + const result = await skillManager.updateSkills(); + + expect(result).toEqual({ + total: 0, + successful: 0, + skipped: 0, + failed: 0, + results: [], + }); + // UI utility outputs symbol and message as separate parameters + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining("⚠"), + expect.stringContaining("No skills cache found"), + ); + }); + + it("should update all registries when no registryId provided", async () => { + (mockedFs.pathExists as any).mockResolvedValue(true); + (mockedFs.readdir as any) + .mockResolvedValueOnce([ + { name: "anthropics", isDirectory: () => true }, + { name: "openai", isDirectory: () => true }, + ]) + .mockResolvedValueOnce([ + { name: "skills", isDirectory: () => true }, + ]) + .mockResolvedValueOnce([ + { name: "tools", isDirectory: () => true }, + ]); + + (mockedGitUtil.isGitRepository as any).mockResolvedValue(true); + (mockedGitUtil.pullRepository as any).mockResolvedValue(undefined); + + const result = await skillManager.updateSkills(); + + expect(result.total).toBe(2); + expect(result.successful).toBe(2); + expect(result.skipped).toBe(0); + expect(result.failed).toBe(0); + expect(mockedGitUtil.pullRepository).toHaveBeenCalledTimes(2); + }); + + it("should update only specific registry when registryId provided", async () => { + (mockedFs.pathExists as any).mockResolvedValue(true); + (mockedFs.readdir as any) + .mockResolvedValueOnce([ + { name: "anthropics", isDirectory: () => true }, + { name: "openai", isDirectory: () => true }, + ]) + .mockResolvedValueOnce([ + { name: "skills", isDirectory: () => true }, + ]) + .mockResolvedValueOnce([ + { name: "tools", isDirectory: () => true }, + ]); + + (mockedGitUtil.isGitRepository as any).mockResolvedValue(true); + (mockedGitUtil.pullRepository as any).mockResolvedValue(undefined); + + const result = await skillManager.updateSkills("anthropics/skills"); + + expect(result.total).toBe(1); + expect(result.successful).toBe(1); + expect(result.results[0].registryId).toBe("anthropics/skills"); + expect(mockedGitUtil.pullRepository).toHaveBeenCalledTimes(1); + }); + + it("should throw error when specific registry not found", async () => { + (mockedFs.pathExists as any).mockResolvedValue(true); + (mockedFs.readdir as any) + .mockResolvedValueOnce([ + { name: "anthropics", isDirectory: () => true }, + ]) + .mockResolvedValueOnce([ + { name: "skills", isDirectory: () => true }, + ]); + + await expect( + skillManager.updateSkills("nonexistent/registry"), + ).rejects.toThrow('Registry "nonexistent/registry" not found in cache'); + }); + + it("should skip non-git directories", async () => { + (mockedFs.pathExists as any).mockResolvedValue(true); + (mockedFs.readdir as any) + .mockResolvedValueOnce([ + { name: "anthropics", isDirectory: () => true }, + ]) + .mockResolvedValueOnce([ + { name: "skills", isDirectory: () => true }, + ]); + + (mockedGitUtil.isGitRepository as any).mockResolvedValue(false); + + const result = await skillManager.updateSkills(); + + expect(result.total).toBe(1); + expect(result.skipped).toBe(1); + expect(result.successful).toBe(0); + expect(result.results[0].status).toBe("skipped"); + expect(result.results[0].message).toBe("Not a git repository"); + expect(mockedGitUtil.pullRepository).not.toHaveBeenCalled(); + }); + + it("should handle git pull errors and continue", async () => { + (mockedFs.pathExists as any).mockResolvedValue(true); + (mockedFs.readdir as any) + .mockResolvedValueOnce([ + { name: "anthropics", isDirectory: () => true }, + { name: "openai", isDirectory: () => true }, + ]) + .mockResolvedValueOnce([ + { name: "skills", isDirectory: () => true }, + ]) + .mockResolvedValueOnce([ + { name: "tools", isDirectory: () => true }, + ]); + + (mockedGitUtil.isGitRepository as any).mockResolvedValue(true); + (mockedGitUtil.pullRepository as any) + .mockRejectedValueOnce(new Error("You have unstaged changes")) + .mockResolvedValueOnce(undefined); + + const result = await skillManager.updateSkills(); + + expect(result.total).toBe(2); + expect(result.successful).toBe(1); + expect(result.failed).toBe(1); + expect(result.results[0].status).toBe("error"); + expect(result.results[0].message).toContain("unstaged changes"); + expect(result.results[1].status).toBe("success"); + }); + + it("should collect and report all errors", async () => { + (mockedFs.pathExists as any).mockResolvedValue(true); + (mockedFs.readdir as any) + .mockResolvedValueOnce([ + { name: "anthropics", isDirectory: () => true }, + ]) + .mockResolvedValueOnce([ + { name: "skills", isDirectory: () => true }, + ]); + + (mockedGitUtil.isGitRepository as any).mockResolvedValue(true); + (mockedGitUtil.pullRepository as any).mockRejectedValue( + new Error("Network error"), + ); + + const result = await skillManager.updateSkills(); + + expect(result.failed).toBe(1); + expect(result.results[0].error).toBeDefined(); + expect(result.results[0].error?.message).toBe("Network error"); + }); + + it("should show progress for each registry", async () => { + (mockedFs.pathExists as any).mockResolvedValue(true); + (mockedFs.readdir as any) + .mockResolvedValueOnce([ + { name: "anthropics", isDirectory: () => true }, + ]) + .mockResolvedValueOnce([ + { name: "skills", isDirectory: () => true }, + ]); + + (mockedGitUtil.isGitRepository as any).mockResolvedValue(true); + (mockedGitUtil.pullRepository as any).mockResolvedValue(undefined); + + await skillManager.updateSkills(); + + // Summary now uses ui.summary() which formats differently + // It outputs "✓ 1 updated" as a single colored string + expect(console.log).toHaveBeenCalled(); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining("updated"), + ); + }); + + it("should display summary after updates", async () => { + (mockedFs.pathExists as any).mockResolvedValue(true); + (mockedFs.readdir as any) + .mockResolvedValueOnce([ + { name: "anthropics", isDirectory: () => true }, + ]) + .mockResolvedValueOnce([ + { name: "skills", isDirectory: () => true }, + ]); + + (mockedGitUtil.isGitRepository as any).mockResolvedValue(true); + (mockedGitUtil.pullRepository as any).mockResolvedValue(undefined); + + await skillManager.updateSkills(); + + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining("Summary:"), + ); + expect(console.log).toHaveBeenCalledWith( + expect.stringContaining("1 updated"), + ); + }); + + it("should handle mixed results (success, skip, error)", async () => { + (mockedFs.pathExists as any).mockResolvedValue(true); + (mockedFs.readdir as any) + .mockResolvedValueOnce([ + { name: "anthropics", isDirectory: () => true }, + { name: "openai", isDirectory: () => true }, + { name: "custom", isDirectory: () => true }, + ]) + .mockResolvedValueOnce([ + { name: "skills", isDirectory: () => true }, + ]) + .mockResolvedValueOnce([ + { name: "tools", isDirectory: () => true }, + ]) + .mockResolvedValueOnce([ + { name: "manual", isDirectory: () => true }, + ]); + + (mockedGitUtil.isGitRepository as any) + .mockResolvedValueOnce(true) // anthropics/skills - git repo + .mockResolvedValueOnce(false) // openai/tools - not git + .mockResolvedValueOnce(true); // custom/manual - git repo + + (mockedGitUtil.pullRepository as any) + .mockResolvedValueOnce(undefined) // anthropics/skills - success + .mockRejectedValueOnce(new Error("Merge conflict")); // custom/manual - error + + const result = await skillManager.updateSkills(); + + expect(result.total).toBe(3); + expect(result.successful).toBe(1); + expect(result.skipped).toBe(1); + expect(result.failed).toBe(1); + }); + }); + + describe("findSkills", () => { + const mockSkillIndex = { + meta: { + version: 1, + createdAt: Date.now() - 1000, + updatedAt: Date.now() - 1000, + registriesHash: "repo1|repo2", + registryHeads: { + "anthropics/skills": "abc123", + "vercel-labs/agent-skills": "def456", + }, + }, + skills: [ + { + name: "typescript-helper", + registry: "anthropics/skills", + path: "skills/typescript-helper", + description: "TypeScript development utilities", + lastIndexed: Date.now(), + }, + { + name: "react-components", + registry: "vercel-labs/agent-skills", + path: "skills/react-components", + description: "Build React components with best practices", + lastIndexed: Date.now(), + }, + { + name: "frontend-design", + registry: "anthropics/skills", + path: "skills/frontend-design", + description: "Frontend design patterns and components", + lastIndexed: Date.now(), + }, + ], + }; + + beforeEach(() => { + mockGlobalConfigManager.getSkillRegistries.mockResolvedValue({}); + + mockedGitUtil.fetchGitHead.mockImplementation(async (url: string) => { + if (url.includes('anthropics')) return 'abc123'; + if (url.includes('vercel')) return 'def456'; + return '000000'; + }); + }); + + 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"); + }); + + it("should load and use fresh index when available", async () => { + (mockedFs.pathExists as any).mockResolvedValue(true); + (mockedFs.readJson as any).mockResolvedValue(mockSkillIndex); + + const results = await skillManager.findSkills("typescript"); + + expect(mockedFs.readJson).toHaveBeenCalledWith( + expect.stringContaining("skills.json") + ); + expect(results).toHaveLength(1); + expect(results[0].name).toBe("typescript-helper"); + }); + + it("should search by skill name", async () => { + (mockedFs.pathExists as any).mockResolvedValue(true); + (mockedFs.readJson as any).mockResolvedValue(mockSkillIndex); + + const results = await skillManager.findSkills("react"); + + expect(results).toHaveLength(1); + expect(results[0].name).toBe("react-components"); + }); + + it("should search by description", async () => { + (mockedFs.pathExists as any).mockResolvedValue(true); + (mockedFs.readJson as any).mockResolvedValue(mockSkillIndex); + + const results = await skillManager.findSkills("design"); + + expect(results).toHaveLength(1); + expect(results[0].name).toBe("frontend-design"); + }); + + it("should be case-insensitive", async () => { + (mockedFs.pathExists as any).mockResolvedValue(true); + (mockedFs.readJson as any).mockResolvedValue(mockSkillIndex); + + const results = await skillManager.findSkills("TYPESCRIPT"); + + expect(results).toHaveLength(1); + expect(results[0].name).toBe("typescript-helper"); + }); + + it("should return multiple matches", async () => { + (mockedFs.pathExists as any).mockResolvedValue(true); + (mockedFs.readJson as any).mockResolvedValue(mockSkillIndex); + + const results = await skillManager.findSkills("component"); + + expect(results).toHaveLength(2); + expect(results.map(r => r.name)).toContain("react-components"); + expect(results.map(r => r.name)).toContain("frontend-design"); + }); + + it("should return empty array when no matches found", async () => { + (mockedFs.pathExists as any).mockResolvedValue(true); + (mockedFs.readJson as any).mockResolvedValue(mockSkillIndex); + + const results = await skillManager.findSkills("nonexistent"); + + expect(results).toEqual([]); + }); + + it("indexes configured non-GitHub registries from matching local cache on refresh", async () => { + mockFetch({ registries: {} }); + mockGlobalConfigManager.getSkillRegistries.mockResolvedValue({ + "example/private-skills": "git@example.com:example/private-skills.git", + }); + mockConfigManager.getSkillRegistries.mockResolvedValue({}); + mockedGitUtil.fetchGitHead.mockResolvedValue("unused"); + mockedSkillDescription.extractSkillDescription.mockReturnValue("ASF experiment workflow"); + + const expectedRegistryPath = path.join( + os.homedir(), + ".ai-devkit", + "skills", + "example", + "private-skills", + ); + const unrelatedRegistryPath = path.join( + os.homedir(), + ".ai-devkit", + "skills", + "unrelated", + "skills", + ); + + (mockedFs.pathExists as any).mockImplementation(async (checkedPath: string) => { + return checkedPath === expectedRegistryPath + || checkedPath === path.join(expectedRegistryPath, "skills") + || checkedPath === path.join(expectedRegistryPath, "skills", "asf", "SKILL.md") + || checkedPath.endsWith("skills.json"); + }); + (mockedFs.readJson as any).mockResolvedValue({ + meta: { + version: 1, + createdAt: Date.now() - 1000, + updatedAt: Date.now() - 1000, + registryHeads: {}, + }, + skills: [ + { + name: "unrelated-skill", + registry: "unrelated/skills", + path: "skills/unrelated-skill", + description: "Should not be indexed", + lastIndexed: Date.now(), + }, + ], + }); + (mockedFs.readdir as any).mockImplementation(async (checkedPath: string) => { + if (checkedPath === path.join(expectedRegistryPath, "skills")) { + return [{ name: "asf", isDirectory: () => true }]; + } + if (checkedPath === path.join(unrelatedRegistryPath, "skills")) { + return [{ name: "unrelated-skill", isDirectory: () => true }]; + } + return []; + }); + (mockedFs.realpath as any).mockImplementation(async (checkedPath: string) => checkedPath); + (mockedFs.opendir as any).mockImplementation(async (checkedPath: string) => ({ + async *[Symbol.asyncIterator]() { + if (checkedPath === path.join(expectedRegistryPath, "skills")) { + yield { name: "asf", isDirectory: () => true, isSymbolicLink: () => false }; + } + if (checkedPath === path.join(unrelatedRegistryPath, "skills")) { + yield { name: "unrelated-skill", isDirectory: () => true, isSymbolicLink: () => false }; + } + }, + })); + (mockedFs.readFile as any).mockResolvedValue("# ASF\n\nASF experiment workflow"); + + const results = await skillManager.findSkills("asf", { refresh: true }); + + expect(results).toEqual([ + expect.objectContaining({ + name: "asf", + registry: "example/private-skills", + path: "skills/asf", + description: "ASF experiment workflow", + }), + ]); + expect(mockedFs.opendir).toHaveBeenCalledWith( + path.join(expectedRegistryPath, "skills"), + ); + expect(mockedFs.opendir).not.toHaveBeenCalledWith( + path.join(unrelatedRegistryPath, "skills"), + ); + }); + }); + +}); diff --git a/packages/cli/src/__tests__/util/local-registry.test.ts b/packages/cli/src/__tests__/services/skill/registry/registry-skill-discovery.test.ts similarity index 97% rename from packages/cli/src/__tests__/util/local-registry.test.ts rename to packages/cli/src/__tests__/services/skill/registry/registry-skill-discovery.test.ts index 246c1a47..c3b3d3e7 100644 --- a/packages/cli/src/__tests__/util/local-registry.test.ts +++ b/packages/cli/src/__tests__/services/skill/registry/registry-skill-discovery.test.ts @@ -6,7 +6,7 @@ import { LOCAL_REGISTRY_MAX_ENTRIES, LOCAL_REGISTRY_MAX_SKILL_MD_BYTES, resolveContainedSkill, -} from '../../util/local-registry.js'; +} from '../../../../services/skill/registry/registry-skill-discovery.js'; describe('local registry filesystem boundary', () => { let temp: string; diff --git a/packages/cli/src/__tests__/util/skill-registry.test.ts b/packages/cli/src/__tests__/services/skill/registry/skill-registry-source.test.ts similarity index 98% rename from packages/cli/src/__tests__/util/skill-registry.test.ts rename to packages/cli/src/__tests__/services/skill/registry/skill-registry-source.test.ts index cca8a485..4d3fadfd 100644 --- a/packages/cli/src/__tests__/util/skill-registry.test.ts +++ b/packages/cli/src/__tests__/services/skill/registry/skill-registry-source.test.ts @@ -8,7 +8,7 @@ import { parseLocalRegistryPath, planSkillRegistryAdd, planSkillRegistryRemove, -} from '../../util/skill-registry.js'; +} from '../../../../services/skill/registry/skill-registry-source.js'; describe('registry sources', () => { it('classifies only file URLs as persisted local sources', () => { diff --git a/packages/cli/src/__tests__/lib/SkillRegistry.test.ts b/packages/cli/src/__tests__/services/skill/registry/skill-registry.service.test.ts similarity index 62% rename from packages/cli/src/__tests__/lib/SkillRegistry.test.ts rename to packages/cli/src/__tests__/services/skill/registry/skill-registry.service.test.ts index 0772843e..33ff25dc 100644 --- a/packages/cli/src/__tests__/lib/SkillRegistry.test.ts +++ b/packages/cli/src/__tests__/services/skill/registry/skill-registry.service.test.ts @@ -1,10 +1,10 @@ import type { Mocked } from 'vitest'; import fs from 'fs-extra'; import * as path from 'path'; -import { SkillRegistry, SKILL_CACHE_DIR } from '../../lib/SkillRegistry.js'; -import { ConfigManager } from '../../lib/Config.js'; -import { GlobalConfigManager } from '../../lib/GlobalConfig.js'; -import * as gitUtil from '../../util/git.js'; +import { SkillRegistryService, SKILL_CACHE_DIR } from '../../../../services/skill/registry/skill-registry.service.js'; +import { ConfigManager } from '../../../../lib/Config.js'; +import { GlobalConfigManager } from '../../../../lib/GlobalConfig.js'; +import * as gitUtil from '../../../../util/git.js'; const mockUi = vi.hoisted(() => ({ info: vi.fn(), @@ -23,26 +23,27 @@ vi.mock('fs-extra', () => ({ readdir: vi.fn(), opendir: vi.fn(), realpath: vi.fn(), + remove: vi.fn(), }, })); -vi.mock('../../util/git.js', () => ({ +vi.mock('../../../../util/git.js', () => ({ ensureGitInstalled: vi.fn(), cloneRepository: vi.fn(), isGitRepository: vi.fn(), pullRepository: vi.fn(), })); -vi.mock('../../util/terminal-ui.js', () => ({ ui: mockUi })); +vi.mock('../../../../util/terminal-ui.js', () => ({ ui: mockUi })); const mockedFs = fs as Mocked; const mockedGit = gitUtil as Mocked; -function createRegistry(): SkillRegistry { - return new SkillRegistry({} as ConfigManager, {} as GlobalConfigManager); +function createRegistry(): SkillRegistryService { + return new SkillRegistryService({} as ConfigManager, {} as GlobalConfigManager); } -describe('SkillRegistry merged catalog', () => { +describe('SkillRegistryService merged catalog', () => { afterEach(() => { vi.unstubAllGlobals(); }); @@ -59,7 +60,7 @@ describe('SkillRegistry merged catalog', () => { const globalConfigManager = { getSkillRegistries: vi.fn().mockResolvedValue({ 'global/skills': 'global-url' }), } as unknown as GlobalConfigManager; - const registry = new SkillRegistry(configManager, globalConfigManager); + const registry = new SkillRegistryService(configManager, globalConfigManager); const [first, second] = await Promise.all([ registry.fetchMergedRegistry(), @@ -75,7 +76,7 @@ describe('SkillRegistry merged catalog', () => { }); }); -describe('SkillRegistry repository preparation', () => { +describe('SkillRegistryService repository preparation', () => { const registryId = 'example/skills'; const secondRegistryId = 'other/skills'; const gitUrl = 'https://github.com/example/skills.git'; @@ -174,6 +175,33 @@ describe('SkillRegistry repository preparation', () => { ); }); + it('prepares the registry repository in the local cache', async () => { + const repoPath = path.join(SKILL_CACHE_DIR, registryId); + mockedFs.pathExists.mockResolvedValue(false); + mockedGit.cloneRepository.mockResolvedValue(repoPath); + + const result = await createRegistry().cacheRegistry(registryId, gitUrl); + + expect(mockedGit.ensureGitInstalled).toHaveBeenCalledOnce(); + expect(mockedGit.cloneRepository).toHaveBeenCalledWith(SKILL_CACHE_DIR, registryId, gitUrl); + expect(result).toBe(repoPath); + }); + + it('removes the contained registry cache directory', async () => { + await createRegistry().removeRegistryCache('example/skills'); + + expect(mockedFs.remove).toHaveBeenCalledWith( + path.join(SKILL_CACHE_DIR, 'example', 'skills'), + ); + }); + + it('refuses paths that escape the cache root', async () => { + await expect( + createRegistry().removeRegistryCache('../escaped'), + ).rejects.toThrow(/outside/); + expect(mockedFs.remove).not.toHaveBeenCalled(); + }); + it('prepares a local registry once without invoking Git or writing', async () => { const localPath = '/tmp/local-skills'; mockedFs.realpath.mockResolvedValue(localPath); @@ -220,7 +248,7 @@ describe('SkillRegistry repository preparation', () => { } as Awaited>); mockedFs.pathExists.mockResolvedValue(true); mockedFs.readdir.mockResolvedValue([]); - const registry = new SkillRegistry( + const registry = new SkillRegistryService( { getSkillRegistries: vi.fn().mockResolvedValue({ [registryId]: 'file:///tmp/local-skills' }) } as unknown as ConfigManager, { getSkillRegistries: vi.fn().mockResolvedValue({}) } as unknown as GlobalConfigManager, ); @@ -234,3 +262,90 @@ describe('SkillRegistry repository preparation', () => { expect(mockedFs.ensureDir).not.toHaveBeenCalled(); }); }); + +describe('SkillRegistryService registry source mutations', () => { + const registryId = 'example/private-skills'; + const gitUrl = 'git@example.com:example/private-skills.git'; + const cachedPath = path.join(SKILL_CACHE_DIR, registryId); + + beforeEach(() => { + vi.clearAllMocks(); + mockedFs.pathExists.mockResolvedValue(false); + mockedFs.ensureDir.mockResolvedValue(undefined); + mockedGit.cloneRepository.mockResolvedValue(cachedPath); + }); + + it('adds a project registry source and prepares its cache', async () => { + const configManager = { + getSkillRegistries: vi.fn().mockResolvedValue({}), + addSkillRegistry: vi.fn().mockResolvedValue({}), + } as unknown as ConfigManager; + const globalConfigManager = { + getSkillRegistries: vi.fn().mockResolvedValue({}), + } as unknown as GlobalConfigManager; + const registry = new SkillRegistryService(configManager, globalConfigManager); + + await expect(registry.addRegistrySource(registryId, gitUrl)).resolves.toEqual({ + status: 'added', + registryPath: cachedPath, + }); + + expect(configManager.addSkillRegistry).toHaveBeenCalledWith(registryId, gitUrl, { force: undefined }); + expect(mockedGit.cloneRepository).toHaveBeenCalledWith(SKILL_CACHE_DIR, registryId, gitUrl); + }); + + it('adds a global registry source through global config', async () => { + const configManager = { + getSkillRegistries: vi.fn().mockResolvedValue({}), + } as unknown as ConfigManager; + const globalConfigManager = { + getSkillRegistries: vi.fn().mockResolvedValue({}), + addSkillRegistry: vi.fn().mockResolvedValue({}), + } as unknown as GlobalConfigManager; + const registry = new SkillRegistryService(configManager, globalConfigManager); + + await expect(registry.addRegistrySource(registryId, gitUrl, { global: true })).resolves.toMatchObject({ + status: 'added', + }); + + expect(globalConfigManager.addSkillRegistry).toHaveBeenCalledWith(registryId, gitUrl, { force: undefined }); + expect(configManager.getSkillRegistries).toHaveBeenCalledOnce(); + }); + + it('does not prepare cache again for an already registered source', async () => { + const configManager = { + getSkillRegistries: vi.fn().mockResolvedValue({ [registryId]: gitUrl }), + addSkillRegistry: vi.fn().mockResolvedValue({}), + } as unknown as ConfigManager; + const globalConfigManager = { + getSkillRegistries: vi.fn().mockResolvedValue({}), + } as unknown as GlobalConfigManager; + const registry = new SkillRegistryService(configManager, globalConfigManager); + + await expect(registry.addRegistrySource(registryId, gitUrl)).resolves.toEqual({ + status: 'already-registered', + registryPath: undefined, + }); + + expect(mockedGit.cloneRepository).not.toHaveBeenCalled(); + }); + + it('removes a project registry source', async () => { + const configManager = { + getSkillRegistries: vi.fn().mockResolvedValue({ [registryId]: gitUrl }), + removeSkillRegistry: vi.fn().mockResolvedValue({}), + } as unknown as ConfigManager; + const registry = new SkillRegistryService(configManager, {} as GlobalConfigManager); + + await expect(registry.removeRegistrySource(registryId)).resolves.toBe('project'); + + expect(configManager.removeSkillRegistry).toHaveBeenCalledWith(registryId); + expect(mockedFs.remove).not.toHaveBeenCalled(); + }); + + it('protects the built-in registry from removal', async () => { + const registry = new SkillRegistryService({} as ConfigManager, {} as GlobalConfigManager); + + await expect(registry.removeRegistrySource('codeaholicguy/ai-devkit')).rejects.toThrow(/built in/); + }); +}); diff --git a/packages/cli/src/__tests__/lib/BuiltinSkills.test.ts b/packages/cli/src/__tests__/services/skill/skill-builtins.test.ts similarity index 85% rename from packages/cli/src/__tests__/lib/BuiltinSkills.test.ts rename to packages/cli/src/__tests__/services/skill/skill-builtins.test.ts index 22b17eb5..e6029271 100644 --- a/packages/cli/src/__tests__/lib/BuiltinSkills.test.ts +++ b/packages/cli/src/__tests__/services/skill/skill-builtins.test.ts @@ -3,7 +3,7 @@ import { readFile } from 'node:fs/promises'; const mockWarning = vi.fn(); -vi.mock('../../util/terminal-ui.js', () => ({ +vi.mock('../../../util/terminal-ui.js', () => ({ ui: { warning: (...args: unknown[]) => mockWarning(...args), }, @@ -23,7 +23,7 @@ describe('getBuiltinSkillNames', () => { }); vi.stubGlobal('fetch', fetchMock); - const { getBuiltinSkillNames } = await import('../../lib/BuiltinSkills.js'); + const { getBuiltinSkillNames } = await import('../../../services/skill/skill-builtins.js'); await expect(getBuiltinSkillNames()).resolves.toEqual(['remote-one', 'remote-two']); await expect(getBuiltinSkillNames()).resolves.toEqual(['remote-one', 'remote-two']); @@ -37,7 +37,7 @@ describe('getBuiltinSkillNames', () => { it('falls back to the bundled list when the manifest cannot be fetched', async () => { vi.stubGlobal('fetch', vi.fn().mockRejectedValue(new Error('network unavailable'))); - const { getBuiltinSkillNames } = await import('../../lib/BuiltinSkills.js'); + const { getBuiltinSkillNames } = await import('../../../services/skill/skill-builtins.js'); const names = await getBuiltinSkillNames(); expect(names).toHaveLength(23); @@ -54,7 +54,7 @@ describe('getBuiltinSkillNames', () => { status: 404, })); - const { getBuiltinSkillNames } = await import('../../lib/BuiltinSkills.js'); + const { getBuiltinSkillNames } = await import('../../../services/skill/skill-builtins.js'); await expect(getBuiltinSkillNames()).resolves.toHaveLength(23); expect(mockWarning).toHaveBeenCalledWith( @@ -70,7 +70,7 @@ describe('getBuiltinSkillNames', () => { }, })); - const { getBuiltinSkillNames } = await import('../../lib/BuiltinSkills.js'); + const { getBuiltinSkillNames } = await import('../../../services/skill/skill-builtins.js'); await expect(getBuiltinSkillNames()).resolves.toHaveLength(23); expect(mockWarning).toHaveBeenCalledWith( @@ -91,7 +91,7 @@ describe('getBuiltinSkillNames', () => { json: async () => manifest, })); - const { getBuiltinSkillNames } = await import('../../lib/BuiltinSkills.js'); + const { getBuiltinSkillNames } = await import('../../../services/skill/skill-builtins.js'); await expect(getBuiltinSkillNames()).resolves.toHaveLength(23); expect(mockWarning).toHaveBeenCalledWith( @@ -102,7 +102,7 @@ describe('getBuiltinSkillNames', () => { describe('built-in skills manifest', () => { it('starts with the current built-in skill names', async () => { - const manifestPath = new URL('../../../../../skills/built-in.json', import.meta.url); + const manifestPath = new URL('../../../../../../skills/built-in.json', import.meta.url); const manifest = JSON.parse(await readFile(manifestPath, 'utf8')); expect(manifest).toHaveLength(23); diff --git a/packages/cli/src/__tests__/util/skill.test.ts b/packages/cli/src/__tests__/services/skill/skill-validation.test.ts similarity index 99% rename from packages/cli/src/__tests__/util/skill.test.ts rename to packages/cli/src/__tests__/services/skill/skill-validation.test.ts index de290ba8..9483e1a6 100644 --- a/packages/cli/src/__tests__/util/skill.test.ts +++ b/packages/cli/src/__tests__/services/skill/skill-validation.test.ts @@ -1,4 +1,4 @@ -import { validateRegistryId, validateSkillName, isValidSkillName } from '../../util/skill.js'; +import { validateRegistryId, validateSkillName, isValidSkillName } from '../../../services/skill/skill-validation.js'; describe('Skill Validation Utilities', () => { describe('validateRegistryId', () => { diff --git a/packages/cli/src/__tests__/lib/SkillManager.test.ts b/packages/cli/src/__tests__/services/skill/skill.service.test.ts similarity index 63% rename from packages/cli/src/__tests__/lib/SkillManager.test.ts rename to packages/cli/src/__tests__/services/skill/skill.service.test.ts index 781ef6de..84c137f7 100644 --- a/packages/cli/src/__tests__/lib/SkillManager.test.ts +++ b/packages/cli/src/__tests__/services/skill/skill.service.test.ts @@ -2,12 +2,12 @@ import type { MockedClass, Mocked, Mock } from 'vitest'; import fs from "fs-extra"; import * as os from "os"; import * as path from "path"; -import { SkillManager } from "../../lib/SkillManager.js"; -import { ConfigManager } from "../../lib/Config.js"; -import { EnvironmentSelector } from "../../lib/EnvironmentSelector.js"; -import { GlobalConfigManager } from "../../lib/GlobalConfig.js"; -import * as gitUtil from "../../util/git.js"; -import * as skillUtil from "../../util/skill.js"; +import { SkillService } from "../../../services/skill/skill.service.js"; +import { ConfigManager } from "../../../lib/Config.js"; +import { EnvironmentSelector } from "../../../lib/EnvironmentSelector.js"; +import { GlobalConfigManager } from "../../../lib/GlobalConfig.js"; +import * as gitUtil from "../../../util/git.js"; +import * as skillUtil from "../../../services/skill/skill-validation.js"; vi.mock("fs-extra", () => ({ default: { @@ -18,13 +18,15 @@ vi.mock("fs-extra", () => ({ lstat: vi.fn(), remove: vi.fn(), readdir: vi.fn(), + opendir: vi.fn(), realpath: vi.fn(), readFile: vi.fn(), readJson: vi.fn(), + stat: vi.fn(), writeJson: vi.fn(), }, })); -vi.mock("../../lib/Config.js", () => ({ +vi.mock("../../../lib/Config.js", () => ({ ConfigManager: vi.fn(function () { return { addSkill: vi.fn(), create: vi.fn(), @@ -34,7 +36,7 @@ vi.mock("../../lib/Config.js", () => ({ update: vi.fn(), }; }), })); -vi.mock("../../lib/EnvironmentSelector.js", () => ({ +vi.mock("../../../lib/EnvironmentSelector.js", () => ({ EnvironmentSelector: vi.fn(function () { return { selectEnvironments: vi.fn(), selectSkillEnvironments: vi.fn(), @@ -43,12 +45,12 @@ vi.mock("../../lib/EnvironmentSelector.js", () => ({ displaySelectionSummary: vi.fn(), }; }), })); -vi.mock("../../lib/GlobalConfig.js", () => ({ +vi.mock("../../../lib/GlobalConfig.js", () => ({ GlobalConfigManager: vi.fn(function () { return { getSkillRegistries: vi.fn(), }; }), })); -vi.mock("../../util/git.js", () => ({ +vi.mock("../../../util/git.js", () => ({ ensureGitInstalled: vi.fn(), cloneRepository: vi.fn(), pullRepository: vi.fn(), @@ -58,18 +60,17 @@ vi.mock("../../util/git.js", () => ({ localBranchExistsSync: vi.fn(), getWorktreePathsForBranchSync: vi.fn(), })); -vi.mock("../../util/skill.js", () => ({ +vi.mock("../../../services/skill/skill-validation.js", () => ({ validateRegistryId: vi.fn(), validateSkillName: vi.fn(), isValidSkillName: vi.fn(), +})); +vi.mock("../../../services/skill/skill-description.js", () => ({ extractSkillDescription: vi.fn(), })); -vi.mock("../../util/terminal.js", () => ({ +vi.mock("../../../util/terminal.js", () => ({ isInteractiveTerminal: vi.fn(() => true), })); -vi.mock("@inquirer/prompts", () => ({ - checkbox: vi.fn(), -})); vi.mock("ora", () => ({ default: vi.fn(function () { return { @@ -83,10 +84,10 @@ vi.mock("ora", () => ({ }; }), })); -import { isInteractiveTerminal } from "../../util/terminal.js"; -import { checkbox } from "@inquirer/prompts"; +import { isInteractiveTerminal } from "../../../util/terminal.js"; +import * as skillDescription from "../../../services/skill/skill-description.js"; const mockIsInteractiveTerminal = isInteractiveTerminal as Mock; -const mockCheckbox = checkbox as unknown as Mock; +const mockedSkillDescription = skillDescription as Mocked; const mockedFs = fs as Mocked; const MockedConfigManager = ConfigManager as MockedClass< @@ -110,8 +111,8 @@ function mockFetch(response: any) { -describe("SkillManager", () => { - let skillManager: SkillManager; +describe("SkillService", () => { + let skillManager: SkillService; let mockConfigManager: Mocked; let mockEnvironmentSelector: Mocked; let mockGlobalConfigManager: Mocked; @@ -129,7 +130,7 @@ describe("SkillManager", () => { mockGlobalConfigManager.getSkillRegistries.mockResolvedValue({}); mockConfigManager.getSkillRegistries.mockResolvedValue({}); - skillManager = new SkillManager( + skillManager = new SkillService( mockConfigManager, mockEnvironmentSelector, mockGlobalConfigManager, @@ -172,7 +173,12 @@ describe("SkillManager", () => { (mockedFs.ensureDir as any).mockResolvedValue(undefined); (mockedFs.symlink as any).mockResolvedValue(undefined); (mockedFs.copy as any).mockResolvedValue(undefined); + (mockedFs.realpath as any).mockImplementation(async (checkedPath: string) => checkedPath); + (mockedFs.stat as any).mockResolvedValue({ size: 100 }); (mockedFs.readdir as any).mockResolvedValue([]); + (mockedFs.opendir as any).mockResolvedValue({ + async *[Symbol.asyncIterator]() { }, + }); (mockedFs.readFile as any)?.mockResolvedValue?.(''); mockConfigManager.read.mockResolvedValue({ @@ -188,6 +194,13 @@ describe("SkillManager", () => { (mockedFs.readdir as any).mockResolvedValue( skillNames.map(name => ({ name, isDirectory: () => true })), ); + (mockedFs.opendir as any).mockResolvedValue({ + async *[Symbol.asyncIterator]() { + for (const name of skillNames) { + yield { name, isDirectory: () => true, isSymbolicLink: () => false }; + } + }, + }); (mockedFs.pathExists as any).mockImplementation((checkPath: string) => { if (checkPath === mockRepoPath) { return Promise.resolve(true); @@ -207,6 +220,12 @@ describe("SkillManager", () => { return Promise.resolve(false); }); + (mockedFs.realpath as any).mockImplementation(async (checkPath: string) => { + if (checkPath.endsWith(`${path.sep}broken-skill${path.sep}SKILL.md`)) { + throw new Error("Missing SKILL.md"); + } + return checkPath; + }); (mockedFs.readFile as any) = vi.fn().mockImplementation((filePath: string) => { const matchedSkill = skillNames.find(skillName => filePath.endsWith(`${skillName}${path.sep}SKILL.md`), @@ -218,7 +237,7 @@ describe("SkillManager", () => { : "description: Debug skill", ); }); - mockedSkillUtil.extractSkillDescription.mockImplementation((content: string) => + mockedSkillDescription.extractSkillDescription.mockImplementation((content: string) => content.replace("description: ", ""), ); }; @@ -425,8 +444,8 @@ describe("SkillManager", () => { it("should read custom registries from global config", async () => { const customGitUrl = "https://github.com/custom/skills.git"; - const { GlobalConfigManager: RealGlobalConfigManager } = await vi.importActual( - "../../lib/GlobalConfig.js", + const { GlobalConfigManager: RealGlobalConfigManager } = await vi.importActual( + "../../../lib/GlobalConfig.js", ); const realGlobalConfigManager = new RealGlobalConfigManager(); @@ -455,7 +474,7 @@ describe("SkillManager", () => { }, }); - const skillManagerWithRealGlobal = new SkillManager( + const skillManagerWithRealGlobal = new SkillService( mockConfigManager, mockEnvironmentSelector, realGlobalConfigManager, @@ -637,75 +656,47 @@ describe("SkillManager", () => { ); }); - it("should prompt for multiple skill selection when skill name is omitted", async () => { + it("should list installable skills when skill name is omitted at the command layer", async () => { configureRegistrySkills(["frontend-design", "debug"]); - mockCheckbox.mockResolvedValue(["debug", "frontend-design"]); - mockIsInteractiveTerminal.mockReturnValue(true); - - await skillManager.addSkill(mockRegistryId, undefined as any); + const skills = await skillManager.listInstallableSkills(mockRegistryId); - expect(mockCheckbox).toHaveBeenCalled(); - expect(mockedSkillUtil.validateSkillName).toHaveBeenCalledWith("debug"); - expect(mockedSkillUtil.validateSkillName).toHaveBeenCalledWith("frontend-design"); - expect(mockConfigManager.addSkill).toHaveBeenNthCalledWith(1, { - registry: mockRegistryId, - name: "debug", - }); - expect(mockConfigManager.addSkill).toHaveBeenNthCalledWith(2, { - registry: mockRegistryId, - name: "frontend-design", - }); - expect(mockConfigManager.addSkill).toHaveBeenCalledTimes(2); + expect(skills).toEqual([ + { name: "debug", description: "Debug skill" }, + { name: "frontend-design", description: "Frontend skill" }, + ]); + expect(mockedSkillUtil.validateRegistryId).toHaveBeenCalledWith(mockRegistryId); + expect(mockConfigManager.addSkill).not.toHaveBeenCalled(); }); - it("should fail when skill name is omitted in non-interactive mode", async () => { - mockIsInteractiveTerminal.mockReturnValue(false); - + it("should fail when skill name is omitted before install", async () => { await expect( skillManager.addSkill(mockRegistryId, undefined as any), - ).rejects.toThrow('Skill name is required in non-interactive mode. Re-run with: ai-devkit skill add '); - - expect(mockCheckbox).not.toHaveBeenCalled(); + ).rejects.toThrow('Skill name is required. Re-run with: ai-devkit skill add '); }); - it("should use cached registry contents for multi-selection when pull fails", async () => { + it("should use cached registry contents when listing installable skills and pull fails", async () => { configureRegistrySkills(["debug", "frontend-design"]); mockedGitUtil.pullRepository.mockRejectedValue(new Error('network down')); - mockCheckbox.mockResolvedValue(["debug", "frontend-design"]); - mockIsInteractiveTerminal.mockReturnValue(true); - - await skillManager.addSkill(mockRegistryId, undefined as any); + const skills = await skillManager.listInstallableSkills(mockRegistryId); - expect(mockCheckbox).toHaveBeenCalled(); - expect(mockConfigManager.addSkill).toHaveBeenCalledTimes(2); + expect(skills.map(skill => skill.name)).toEqual(["debug", "frontend-design"]); expect(console.log).toHaveBeenCalledWith( expect.stringContaining("⚠"), expect.stringContaining("Using cached registry contents"), ); }); - it("should stop without installing when skill selection is cancelled", async () => { - configureRegistrySkills(["debug"]); - const error = new Error('User cancelled'); - error.name = 'ExitPromptError'; - mockCheckbox.mockRejectedValue(error); - - mockIsInteractiveTerminal.mockReturnValue(true); - - await expect( - skillManager.addSkill(mockRegistryId, undefined as any), - ).rejects.toThrow('Skill selection cancelled.'); - - expect(mockConfigManager.addSkill).not.toHaveBeenCalled(); - expect(mockedFs.symlink).not.toHaveBeenCalled(); - }); - it("should throw a clear error when the registry has no valid skills", async () => { (mockedFs.readdir as any).mockResolvedValue([ { name: "broken-skill", isDirectory: () => true }, ]); + (mockedFs.opendir as any).mockResolvedValue({ + async *[Symbol.asyncIterator]() { + yield { name: "broken-skill", isDirectory: () => true, isSymbolicLink: () => false }; + }, + }); (mockedFs.pathExists as any).mockImplementation((checkPath: string) => { if (checkPath === mockRepoPath) { return Promise.resolve(true); @@ -716,18 +707,13 @@ describe("SkillManager", () => { return Promise.resolve(false); }); - mockIsInteractiveTerminal.mockReturnValue(true); - await expect( - skillManager.addSkill(mockRegistryId, undefined as any), + skillManager.listInstallableSkills(mockRegistryId), ).rejects.toThrow(`No valid skills found in ${mockRegistryId}.`); - - expect(mockCheckbox).not.toHaveBeenCalled(); }); - it("should support global installation after interactive multi-selection", async () => { + it("should support global installation of multiple explicit skills", async () => { configureRegistrySkills(["debug", "frontend-design"]); - mockCheckbox.mockResolvedValue(["debug", "frontend-design"]); (mockedFs.pathExists as any).mockImplementation((checkPath: string) => { if (checkPath === path.join(os.homedir(), ".claude", "skills", "debug")) { return Promise.resolve(false); @@ -756,9 +742,7 @@ describe("SkillManager", () => { return Promise.resolve(false); }); - mockIsInteractiveTerminal.mockReturnValue(true); - - await skillManager.addSkill(mockRegistryId, undefined as any, { global: true, environments: ["claude"] }); + await skillManager.addSkills(mockRegistryId, ["debug", "frontend-design"], { global: true, environments: ["claude"] }); expect(mockedFs.symlink).toHaveBeenCalledWith( expect.any(String), @@ -1173,491 +1157,5 @@ describe("SkillManager", () => { }); }); - describe("updateSkills", () => { - - beforeEach(() => { - vi.spyOn(console, "log").mockImplementation(() => { }); - mockedGitUtil.ensureGitInstalled.mockResolvedValue(undefined); - }); - - it("does not require git when there is no cached git registry to update", async () => { - (mockedFs.pathExists as any).mockResolvedValue(false); - - await skillManager.updateSkills(); - - expect(mockedGitUtil.ensureGitInstalled).not.toHaveBeenCalled(); - }); - - it("should return empty summary when cache directory does not exist", async () => { - (mockedFs.pathExists as any).mockResolvedValue(false); - - const result = await skillManager.updateSkills(); - - expect(result).toEqual({ - total: 0, - successful: 0, - skipped: 0, - failed: 0, - results: [], - }); - // UI utility outputs symbol and message as separate parameters - expect(console.log).toHaveBeenCalledWith( - expect.stringContaining("⚠"), - expect.stringContaining("No skills cache found"), - ); - }); - - it("should update all registries when no registryId provided", async () => { - (mockedFs.pathExists as any).mockResolvedValue(true); - (mockedFs.readdir as any) - .mockResolvedValueOnce([ - { name: "anthropics", isDirectory: () => true }, - { name: "openai", isDirectory: () => true }, - ]) - .mockResolvedValueOnce([ - { name: "skills", isDirectory: () => true }, - ]) - .mockResolvedValueOnce([ - { name: "tools", isDirectory: () => true }, - ]); - - (mockedGitUtil.isGitRepository as any).mockResolvedValue(true); - (mockedGitUtil.pullRepository as any).mockResolvedValue(undefined); - - const result = await skillManager.updateSkills(); - - expect(result.total).toBe(2); - expect(result.successful).toBe(2); - expect(result.skipped).toBe(0); - expect(result.failed).toBe(0); - expect(mockedGitUtil.pullRepository).toHaveBeenCalledTimes(2); - }); - - it("should update only specific registry when registryId provided", async () => { - (mockedFs.pathExists as any).mockResolvedValue(true); - (mockedFs.readdir as any) - .mockResolvedValueOnce([ - { name: "anthropics", isDirectory: () => true }, - { name: "openai", isDirectory: () => true }, - ]) - .mockResolvedValueOnce([ - { name: "skills", isDirectory: () => true }, - ]) - .mockResolvedValueOnce([ - { name: "tools", isDirectory: () => true }, - ]); - - (mockedGitUtil.isGitRepository as any).mockResolvedValue(true); - (mockedGitUtil.pullRepository as any).mockResolvedValue(undefined); - - const result = await skillManager.updateSkills("anthropics/skills"); - - expect(result.total).toBe(1); - expect(result.successful).toBe(1); - expect(result.results[0].registryId).toBe("anthropics/skills"); - expect(mockedGitUtil.pullRepository).toHaveBeenCalledTimes(1); - }); - - it("should throw error when specific registry not found", async () => { - (mockedFs.pathExists as any).mockResolvedValue(true); - (mockedFs.readdir as any) - .mockResolvedValueOnce([ - { name: "anthropics", isDirectory: () => true }, - ]) - .mockResolvedValueOnce([ - { name: "skills", isDirectory: () => true }, - ]); - - await expect( - skillManager.updateSkills("nonexistent/registry"), - ).rejects.toThrow('Registry "nonexistent/registry" not found in cache'); - }); - - it("should skip non-git directories", async () => { - (mockedFs.pathExists as any).mockResolvedValue(true); - (mockedFs.readdir as any) - .mockResolvedValueOnce([ - { name: "anthropics", isDirectory: () => true }, - ]) - .mockResolvedValueOnce([ - { name: "skills", isDirectory: () => true }, - ]); - - (mockedGitUtil.isGitRepository as any).mockResolvedValue(false); - - const result = await skillManager.updateSkills(); - - expect(result.total).toBe(1); - expect(result.skipped).toBe(1); - expect(result.successful).toBe(0); - expect(result.results[0].status).toBe("skipped"); - expect(result.results[0].message).toBe("Not a git repository"); - expect(mockedGitUtil.pullRepository).not.toHaveBeenCalled(); - }); - - it("should handle git pull errors and continue", async () => { - (mockedFs.pathExists as any).mockResolvedValue(true); - (mockedFs.readdir as any) - .mockResolvedValueOnce([ - { name: "anthropics", isDirectory: () => true }, - { name: "openai", isDirectory: () => true }, - ]) - .mockResolvedValueOnce([ - { name: "skills", isDirectory: () => true }, - ]) - .mockResolvedValueOnce([ - { name: "tools", isDirectory: () => true }, - ]); - - (mockedGitUtil.isGitRepository as any).mockResolvedValue(true); - (mockedGitUtil.pullRepository as any) - .mockRejectedValueOnce(new Error("You have unstaged changes")) - .mockResolvedValueOnce(undefined); - - const result = await skillManager.updateSkills(); - - expect(result.total).toBe(2); - expect(result.successful).toBe(1); - expect(result.failed).toBe(1); - expect(result.results[0].status).toBe("error"); - expect(result.results[0].message).toContain("unstaged changes"); - expect(result.results[1].status).toBe("success"); - }); - - it("should collect and report all errors", async () => { - (mockedFs.pathExists as any).mockResolvedValue(true); - (mockedFs.readdir as any) - .mockResolvedValueOnce([ - { name: "anthropics", isDirectory: () => true }, - ]) - .mockResolvedValueOnce([ - { name: "skills", isDirectory: () => true }, - ]); - - (mockedGitUtil.isGitRepository as any).mockResolvedValue(true); - (mockedGitUtil.pullRepository as any).mockRejectedValue( - new Error("Network error"), - ); - - const result = await skillManager.updateSkills(); - - expect(result.failed).toBe(1); - expect(result.results[0].error).toBeDefined(); - expect(result.results[0].error?.message).toBe("Network error"); - }); - - it("should show progress for each registry", async () => { - (mockedFs.pathExists as any).mockResolvedValue(true); - (mockedFs.readdir as any) - .mockResolvedValueOnce([ - { name: "anthropics", isDirectory: () => true }, - ]) - .mockResolvedValueOnce([ - { name: "skills", isDirectory: () => true }, - ]); - - (mockedGitUtil.isGitRepository as any).mockResolvedValue(true); - (mockedGitUtil.pullRepository as any).mockResolvedValue(undefined); - - await skillManager.updateSkills(); - - // Summary now uses ui.summary() which formats differently - // It outputs "✓ 1 updated" as a single colored string - expect(console.log).toHaveBeenCalled(); - expect(console.log).toHaveBeenCalledWith( - expect.stringContaining("updated"), - ); - }); - - it("should display summary after updates", async () => { - (mockedFs.pathExists as any).mockResolvedValue(true); - (mockedFs.readdir as any) - .mockResolvedValueOnce([ - { name: "anthropics", isDirectory: () => true }, - ]) - .mockResolvedValueOnce([ - { name: "skills", isDirectory: () => true }, - ]); - - (mockedGitUtil.isGitRepository as any).mockResolvedValue(true); - (mockedGitUtil.pullRepository as any).mockResolvedValue(undefined); - - await skillManager.updateSkills(); - - expect(console.log).toHaveBeenCalledWith( - expect.stringContaining("Summary:"), - ); - expect(console.log).toHaveBeenCalledWith( - expect.stringContaining("1 updated"), - ); - }); - - it("should handle mixed results (success, skip, error)", async () => { - (mockedFs.pathExists as any).mockResolvedValue(true); - (mockedFs.readdir as any) - .mockResolvedValueOnce([ - { name: "anthropics", isDirectory: () => true }, - { name: "openai", isDirectory: () => true }, - { name: "custom", isDirectory: () => true }, - ]) - .mockResolvedValueOnce([ - { name: "skills", isDirectory: () => true }, - ]) - .mockResolvedValueOnce([ - { name: "tools", isDirectory: () => true }, - ]) - .mockResolvedValueOnce([ - { name: "manual", isDirectory: () => true }, - ]); - - (mockedGitUtil.isGitRepository as any) - .mockResolvedValueOnce(true) // anthropics/skills - git repo - .mockResolvedValueOnce(false) // openai/tools - not git - .mockResolvedValueOnce(true); // custom/manual - git repo - - (mockedGitUtil.pullRepository as any) - .mockResolvedValueOnce(undefined) // anthropics/skills - success - .mockRejectedValueOnce(new Error("Merge conflict")); // custom/manual - error - - const result = await skillManager.updateSkills(); - - expect(result.total).toBe(3); - expect(result.successful).toBe(1); - expect(result.skipped).toBe(1); - expect(result.failed).toBe(1); - }); - }); - - describe("findSkills", () => { - const mockSkillIndex = { - meta: { - version: 1, - createdAt: Date.now() - 1000, - updatedAt: Date.now() - 1000, - registriesHash: "repo1|repo2", - registryHeads: { - "anthropics/skills": "abc123", - "vercel-labs/agent-skills": "def456", - }, - }, - skills: [ - { - name: "typescript-helper", - registry: "anthropics/skills", - path: "skills/typescript-helper", - description: "TypeScript development utilities", - lastIndexed: Date.now(), - }, - { - name: "react-components", - registry: "vercel-labs/agent-skills", - path: "skills/react-components", - description: "Build React components with best practices", - lastIndexed: Date.now(), - }, - { - name: "frontend-design", - registry: "anthropics/skills", - path: "skills/frontend-design", - description: "Frontend design patterns and components", - lastIndexed: Date.now(), - }, - ], - }; - - beforeEach(() => { - mockGlobalConfigManager.getSkillRegistries.mockResolvedValue({}); - - mockedGitUtil.fetchGitHead.mockImplementation(async (url: string) => { - if (url.includes('anthropics')) return 'abc123'; - if (url.includes('vercel')) return 'def456'; - return '000000'; - }); - }); - - 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"); - }); - - it("should load and use fresh index when available", async () => { - (mockedFs.pathExists as any).mockResolvedValue(true); - (mockedFs.readJson as any).mockResolvedValue(mockSkillIndex); - - const results = await skillManager.findSkills("typescript"); - expect(mockedFs.readJson).toHaveBeenCalledWith( - expect.stringContaining("skills.json") - ); - expect(results).toHaveLength(1); - expect(results[0].name).toBe("typescript-helper"); - }); - - it("should search by skill name", async () => { - (mockedFs.pathExists as any).mockResolvedValue(true); - (mockedFs.readJson as any).mockResolvedValue(mockSkillIndex); - - const results = await skillManager.findSkills("react"); - - expect(results).toHaveLength(1); - expect(results[0].name).toBe("react-components"); - }); - - it("should search by description", async () => { - (mockedFs.pathExists as any).mockResolvedValue(true); - (mockedFs.readJson as any).mockResolvedValue(mockSkillIndex); - - const results = await skillManager.findSkills("design"); - - expect(results).toHaveLength(1); - expect(results[0].name).toBe("frontend-design"); - }); - - it("should be case-insensitive", async () => { - (mockedFs.pathExists as any).mockResolvedValue(true); - (mockedFs.readJson as any).mockResolvedValue(mockSkillIndex); - - const results = await skillManager.findSkills("TYPESCRIPT"); - - expect(results).toHaveLength(1); - expect(results[0].name).toBe("typescript-helper"); - }); - - it("should return multiple matches", async () => { - (mockedFs.pathExists as any).mockResolvedValue(true); - (mockedFs.readJson as any).mockResolvedValue(mockSkillIndex); - - const results = await skillManager.findSkills("component"); - - expect(results).toHaveLength(2); - expect(results.map(r => r.name)).toContain("react-components"); - expect(results.map(r => r.name)).toContain("frontend-design"); - }); - - it("should return empty array when no matches found", async () => { - (mockedFs.pathExists as any).mockResolvedValue(true); - (mockedFs.readJson as any).mockResolvedValue(mockSkillIndex); - - const results = await skillManager.findSkills("nonexistent"); - - expect(results).toEqual([]); - }); - - it("indexes configured non-GitHub registries from matching local cache on refresh", async () => { - mockFetch({ registries: {} }); - mockGlobalConfigManager.getSkillRegistries.mockResolvedValue({ - "example/private-skills": "git@example.com:example/private-skills.git", - }); - mockConfigManager.getSkillRegistries.mockResolvedValue({}); - mockedGitUtil.fetchGitHead.mockResolvedValue("unused"); - mockedSkillUtil.extractSkillDescription.mockReturnValue("ASF experiment workflow"); - - const expectedRegistryPath = path.join( - os.homedir(), - ".ai-devkit", - "skills", - "example", - "private-skills", - ); - const unrelatedRegistryPath = path.join( - os.homedir(), - ".ai-devkit", - "skills", - "unrelated", - "skills", - ); - - (mockedFs.pathExists as any).mockImplementation(async (checkedPath: string) => { - return checkedPath === expectedRegistryPath - || checkedPath === path.join(expectedRegistryPath, "skills") - || checkedPath === path.join(expectedRegistryPath, "skills", "asf", "SKILL.md") - || checkedPath.endsWith("skills.json"); - }); - (mockedFs.readJson as any).mockResolvedValue({ - meta: { - version: 1, - createdAt: Date.now() - 1000, - updatedAt: Date.now() - 1000, - registryHeads: {}, - }, - skills: [ - { - name: "unrelated-skill", - registry: "unrelated/skills", - path: "skills/unrelated-skill", - description: "Should not be indexed", - lastIndexed: Date.now(), - }, - ], - }); - (mockedFs.readdir as any).mockImplementation(async (checkedPath: string) => { - if (checkedPath === path.join(expectedRegistryPath, "skills")) { - return [{ name: "asf", isDirectory: () => true }]; - } - if (checkedPath === path.join(unrelatedRegistryPath, "skills")) { - return [{ name: "unrelated-skill", isDirectory: () => true }]; - } - return []; - }); - (mockedFs.readFile as any).mockResolvedValue("# ASF\n\nASF experiment workflow"); - - const results = await skillManager.findSkills("asf", { refresh: true }); - - expect(results).toEqual([ - expect.objectContaining({ - name: "asf", - registry: "example/private-skills", - path: "skills/asf", - description: "ASF experiment workflow", - }), - ]); - expect(mockedFs.readdir).toHaveBeenCalledWith( - path.join(expectedRegistryPath, "skills"), - { withFileTypes: true }, - ); - expect(mockedFs.readdir).not.toHaveBeenCalledWith( - path.join(unrelatedRegistryPath, "skills"), - { withFileTypes: true }, - ); - }); - }); - - describe("cacheRegistry", () => { - it("prepares the registry repository in the local cache", async () => { - const registryId = "example/private-skills"; - const gitUrl = "git@example.com:example/private-skills.git"; - const repoPath = path.join(os.homedir(), ".ai-devkit", "skills", registryId); - - (mockedFs.pathExists as any).mockResolvedValue(false); - (mockedFs.ensureDir as any).mockResolvedValue(undefined); - mockedGitUtil.cloneRepository.mockResolvedValue(repoPath); - - const result = await skillManager.cacheRegistry(registryId, gitUrl); - - expect(mockedGitUtil.ensureGitInstalled).toHaveBeenCalledOnce(); - expect(mockedGitUtil.cloneRepository).toHaveBeenCalledWith( - path.join(os.homedir(), ".ai-devkit", "skills"), - registryId, - gitUrl, - ); - 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__/services/status/status.service.test.ts b/packages/cli/src/__tests__/services/status/status.service.test.ts index 29f9c203..6854385e 100644 --- a/packages/cli/src/__tests__/services/status/status.service.test.ts +++ b/packages/cli/src/__tests__/services/status/status.service.test.ts @@ -5,7 +5,7 @@ const mockGetBuiltinSkillNames = vi.hoisted(() => vi.fn(async () => ['remote-one', 'remote-two']) ); -vi.mock('../../../lib/BuiltinSkills.js', () => ({ +vi.mock('../../../services/skill/skill-builtins.js', () => ({ getBuiltinSkillNames: (...args: unknown[]) => mockGetBuiltinSkillNames(...args), })); diff --git a/packages/cli/src/commands/init.ts b/packages/cli/src/commands/init.ts index 6ad49049..9dd0b1c3 100644 --- a/packages/cli/src/commands/init.ts +++ b/packages/cli/src/commands/init.ts @@ -1,5 +1,5 @@ import { execFileSync } from 'child_process'; -import { BUILTIN_SKILL_REGISTRY, getBuiltinSkillNames } from '../lib/BuiltinSkills.js'; +import { BUILTIN_SKILL_REGISTRY, getBuiltinSkillNames } from '../services/skill/skill-builtins.js'; import { ConfigManager } from '../lib/Config.js'; import { TemplateManager } from '../lib/TemplateManager.js'; import { EnvironmentSelector } from '../lib/EnvironmentSelector.js'; diff --git a/packages/cli/src/commands/skill.ts b/packages/cli/src/commands/skill.ts index 7834327a..a009902f 100644 --- a/packages/cli/src/commands/skill.ts +++ b/packages/cli/src/commands/skill.ts @@ -1,14 +1,14 @@ import { Command } from 'commander'; +import { checkbox } from '@inquirer/prompts'; 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_REGISTRY, getBuiltinSkillNames } from '../lib/BuiltinSkills.js'; +import { SkillService } from '../services/skill/skill.service.js'; +import { BUILTIN_SKILL_REGISTRY, getBuiltinSkillNames } from '../services/skill/skill-builtins.js'; import { ui } from '../util/terminal-ui.js'; import { withErrorHandler } from '../util/errors.js'; +import { isInteractiveTerminal } from '../util/terminal.js'; import { truncate, getErrorMessage } from '../util/text.js'; -import { validateRegistryId } from '../util/skill.js'; -import { normalizeRegistrySourceInput, normalizeRegistrySources, planSkillRegistryAdd } from '../util/skill-registry.js'; +import type { RegistrySkillChoice } from '../services/skill/skill.types.js'; export function registerSkillCommand(program: Command): void { const skillCommand = program @@ -24,7 +24,7 @@ export function registerSkillCommand(program: Command): void { .action(async (registryRepo: string | undefined, skillName: string | undefined, options: { builtIn?: boolean; global?: boolean; env?: string[] }) => { try { const configManager = new ConfigManager(); - const skillManager = new SkillManager(configManager); + const skillService = new SkillService(configManager); const installOptions = { global: options.global, environments: options.env, @@ -36,7 +36,7 @@ export function registerSkillCommand(program: Command): void { } for (const builtInSkill of await getBuiltinSkillNames()) { - await skillManager.addSkill(BUILTIN_SKILL_REGISTRY, builtInSkill, installOptions); + await skillService.addSkill(BUILTIN_SKILL_REGISTRY, builtInSkill, installOptions); } return; @@ -48,7 +48,19 @@ export function registerSkillCommand(program: Command): void { return; } - await skillManager.addSkill(registryRepo, skillName, installOptions); + if (skillName) { + await skillService.addSkill(registryRepo, skillName, installOptions); + return; + } + + if (!isInteractiveTerminal()) { + throw new Error('Skill name is required in non-interactive mode. Re-run with: ai-devkit skill add '); + } + + const selectedSkillNames = await promptForSkillSelection( + await skillService.listInstallableSkills(registryRepo), + ); + await skillService.addSkills(registryRepo, selectedSkillNames, installOptions); } catch (error: unknown) { const message = getErrorMessage(error); if (message === 'Skill selection cancelled.') { @@ -70,29 +82,12 @@ export function registerSkillCommand(program: Command): void { source: string, options: { global?: boolean; force?: boolean }, ) => { - validateRegistryId(id); - const configManager = options.global - ? new GlobalConfigManager() - : new ConfigManager(); - - const registries = await configManager.getSkillRegistries(); - const value = await normalizeRegistrySourceInput(source, process.cwd()); - const [projectRegistries, globalRegistries] = await Promise.all([ - options.global ? new ConfigManager().getSkillRegistries() : Promise.resolve(registries), - options.global ? Promise.resolve(registries) : new GlobalConfigManager().getSkillRegistries(), - ]); - await normalizeRegistrySources({ ...globalRegistries, ...projectRegistries, [id]: value }, process.cwd()); - const mutation = planSkillRegistryAdd(registries, id, value, { force: options.force }); - if (mutation.status !== 'already-registered') { - const skillManager = new SkillManager(new ConfigManager()); - const registryPath = await skillManager.cacheRegistry(id, value); - await skillManager.updateSkillIndexForRegistry(id, registryPath); - } - await configManager.addSkillRegistry(id, value, { force: options.force }); + const skillService = new SkillService(new ConfigManager()); + const status = await skillService.addRegistry(id, source, options); - if (mutation.status === 'already-registered') { + if (status === 'already-registered') { ui.info(`Registry "${id}" is already registered.`); - } else if (mutation.status === 'updated') { + } else if (status === 'updated') { ui.success(`Updated skill registry "${id}".`); } else { ui.success(`Registered skill registry "${id}".`); @@ -107,27 +102,8 @@ export function registerSkillCommand(program: Command): void { 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); - const skillManager = new SkillManager(new ConfigManager()); - await skillManager.removeSkillIndexForRegistry(id); - if (options.global) { - await skillManager.removeRegistryCache(id); - } - - const scope = options.global ? 'global' : 'project'; + const skillService = new SkillService(new ConfigManager()); + const scope = await skillService.removeRegistry(id, options); ui.success(`Removed ${scope} skill registry "${id}".`); })); @@ -138,14 +114,14 @@ export function registerSkillCommand(program: Command): void { .option('-e, --env ', 'Limit global listing to environment(s) (requires --global)') .action(withErrorHandler('list skills', async (options: { global?: boolean; env?: string[] }) => { const configManager = new ConfigManager(); - const skillManager = new SkillManager(configManager); + const skillService = new SkillService(configManager); if (options.env && options.env.length > 0 && !options.global) { throw new Error('--env can only be used with --global'); } if (options.global) { - const skills = await skillManager.listGlobalSkills(options.env); + const skills = await skillService.listGlobalSkills(options.env); if (skills.length === 0) { ui.warning('No global skills installed in the selected environments.'); @@ -167,7 +143,7 @@ export function registerSkillCommand(program: Command): void { return; } - const skills = await skillManager.listSkills(); + const skills = await skillService.listSkills(); if (skills.length === 0) { ui.warning('No skills installed in this project.'); @@ -200,9 +176,9 @@ export function registerSkillCommand(program: Command): void { options: { global?: boolean; env?: string[] }, ) => { const configManager = new ConfigManager(); - const skillManager = new SkillManager(configManager); + const skillService = new SkillService(configManager); - await skillManager.removeSkill(skillName, { + await skillService.removeSkill(skillName, { global: options.global, environments: options.env, }); @@ -213,9 +189,9 @@ export function registerSkillCommand(program: Command): void { .description('Update skills from registries (e.g., ai-devkit skill update or ai-devkit skill update anthropic/skills)') .action(withErrorHandler('update skills', async (registryId?: string) => { const configManager = new ConfigManager(); - const skillManager = new SkillManager(configManager); + const skillService = new SkillService(configManager); - await skillManager.updateSkills(registryId); + await skillService.updateSkills(registryId); })); skillCommand @@ -224,9 +200,9 @@ export function registerSkillCommand(program: Command): void { .option('--refresh', 'Force rebuild the skill index') .action(withErrorHandler('search skills', async (keyword: string, options: { refresh?: boolean }) => { const configManager = new ConfigManager(); - const skillManager = new SkillManager(configManager); + const skillService = new SkillService(configManager); - const results = await skillManager.findSkills(keyword, { refresh: options.refresh }); + const results = await skillService.findSkills(keyword, { refresh: options.refresh }); if (results.length === 0) { ui.warning(`No skills found matching "${keyword}"`); @@ -255,8 +231,28 @@ export function registerSkillCommand(program: Command): void { .option('--output ', 'Output path for the index file') .action(withErrorHandler('rebuild index', async (options: { output?: string }) => { const configManager = new ConfigManager(); - const skillManager = new SkillManager(configManager); + const skillService = new SkillService(configManager); - await skillManager.rebuildIndex(options.output); + await skillService.rebuildIndex(options.output); })); } + +async function promptForSkillSelection(skills: RegistrySkillChoice[]): Promise { + try { + return await checkbox({ + message: 'Select skill(s) to install', + choices: skills.map(skill => ({ + name: skill.description ? `${skill.name} - ${skill.description}` : skill.name, + value: skill.name, + })), + required: true, + }); + } catch (error: unknown) { + if (error instanceof Error && + (error.name === 'ExitPromptError' || error.message.toLowerCase().includes('cancel'))) { + throw new Error('Skill selection cancelled.'); + } + + throw error; + } +} diff --git a/packages/cli/src/lib/Config.ts b/packages/cli/src/lib/Config.ts index 01bdaf70..4de02299 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, normalizeRegistrySources, planSkillRegistryAdd, planSkillRegistryRemove } from '../util/skill-registry.js'; +import { AddSkillRegistryOptions, normalizeRegistrySources, planSkillRegistryAdd, planSkillRegistryRemove } from '../services/skill/registry/skill-registry-source.js'; import { GlobalConfigManager } from './GlobalConfig.js'; import packageJson from '../../package.json' with { type: 'json' }; diff --git a/packages/cli/src/lib/GlobalConfig.ts b/packages/cli/src/lib/GlobalConfig.ts index 6b7b5372..1b912fff 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, normalizeRegistrySources, planSkillRegistryAdd, planSkillRegistryRemove } from '../util/skill-registry.js'; +import { AddSkillRegistryOptions, normalizeRegistrySources, planSkillRegistryAdd, planSkillRegistryRemove } from '../services/skill/registry/skill-registry-source.js'; import { ui } from '../util/terminal-ui.js'; export class GlobalConfigManager { diff --git a/packages/cli/src/lib/InitTemplate.ts b/packages/cli/src/lib/InitTemplate.ts index c78d2589..2f9c42b4 100644 --- a/packages/cli/src/lib/InitTemplate.ts +++ b/packages/cli/src/lib/InitTemplate.ts @@ -3,7 +3,7 @@ import * as path from 'path'; import YAML from 'yaml'; import { AVAILABLE_PHASES, EnvironmentCode, MCP_TRANSPORTS, McpServerDefinition, McpTransport, Phase } from '../types.js'; import { isValidEnvironmentCode } from '../util/env.js'; -import { normalizeRegistrySources } from '../util/skill-registry.js'; +import { normalizeRegistrySources } from '../services/skill/registry/skill-registry-source.js'; export interface InitTemplateSkill { registry: string; diff --git a/packages/cli/src/services/install/install.service.ts b/packages/cli/src/services/install/install.service.ts index b11db01a..ae5b26b6 100644 --- a/packages/cli/src/services/install/install.service.ts +++ b/packages/cli/src/services/install/install.service.ts @@ -1,6 +1,6 @@ import { ConfigManager } from '../../lib/Config.js'; import { EnvironmentSelector } from '../../lib/EnvironmentSelector.js'; -import { SkillManager } from '../../lib/SkillManager.js'; +import { SkillService } from '../skill/skill.service.js'; import { TemplateManager } from '../../lib/TemplateManager.js'; import { InstallConfigData } from '../../util/config.js'; import { installMcpServers, McpInstallReport } from './mcp/index.js'; @@ -47,7 +47,7 @@ export async function reconcileAndInstall( const configManager = new ConfigManager(); const docsDir = await configManager.getDocsDir(); const templateManager = new TemplateManager({ docsDir }); - const skillManager = new SkillManager(configManager, new EnvironmentSelector()); + const skillService = new SkillService(configManager, new EnvironmentSelector()); const report: InstallReport = { environments: { installed: 0, skipped: 0, failed: 0 }, @@ -135,7 +135,7 @@ export async function reconcileAndInstall( for (const skill of config.skills) { try { - const status = await skillManager.addSkill(skill.registry, skill.name); + const status = await skillService.addSkill(skill.registry, skill.name); if (status === 'matched') { report.skills.skipped += 1; } else { diff --git a/packages/cli/src/services/setup/setup.service.ts b/packages/cli/src/services/setup/setup.service.ts index 9135104c..424a83f4 100644 --- a/packages/cli/src/services/setup/setup.service.ts +++ b/packages/cli/src/services/setup/setup.service.ts @@ -4,9 +4,9 @@ import { homedir } from 'os'; import { dirname, join, resolve } from 'path'; import { fileURLToPath } from 'url'; import { promisify } from 'util'; -import { BUILTIN_SKILL_REGISTRY, getBuiltinSkillNames } from '../../lib/BuiltinSkills.js'; +import { BUILTIN_SKILL_REGISTRY, getBuiltinSkillNames } from '../skill/skill-builtins.js'; import { ConfigManager } from '../../lib/Config.js'; -import { SkillManager } from '../../lib/SkillManager.js'; +import { SkillService } from '../../services/skill/skill.service.js'; import { getErrorMessage } from '../../util/text.js'; const execFileAsync = promisify(execFile); @@ -309,10 +309,10 @@ async function defaultRunCommand(command: string, args: string[]): Promise } async function defaultInstallBuiltInSkills(agent: SetupAgent): Promise { - const skillManager = new SkillManager(new ConfigManager()); + const skillService = new SkillService(new ConfigManager()); for (const builtInSkill of await getBuiltinSkillNames()) { - await skillManager.addSkill(BUILTIN_SKILL_REGISTRY, builtInSkill, { + await skillService.addSkill(BUILTIN_SKILL_REGISTRY, builtInSkill, { global: true, environments: [agent], }); diff --git a/packages/cli/src/services/skill/index/skill-index.repository.ts b/packages/cli/src/services/skill/index/skill-index.repository.ts new file mode 100644 index 00000000..6060887a --- /dev/null +++ b/packages/cli/src/services/skill/index/skill-index.repository.ts @@ -0,0 +1,35 @@ +import fs from 'fs-extra'; +import * as os from 'os'; +import * as path from 'path'; +import type { SkillIndexData } from './skill-index.service.js'; + +export const SKILL_INDEX_PATH = path.join(os.homedir(), '.ai-devkit', 'skills.json'); + +export class SkillIndexRepository { + readonly defaultPath = SKILL_INDEX_PATH; + + async exists(indexPath = this.defaultPath): Promise { + return fs.pathExists(indexPath); + } + + async read(indexPath = this.defaultPath): Promise { + try { + if (await fs.pathExists(indexPath)) { + return await fs.readJson(indexPath) as SkillIndexData; + } + } catch { + // Treat unreadable/corrupt indexes as absent; the service decides fallback behavior. + } + + return null; + } + + async readRequired(indexPath = this.defaultPath): Promise { + return await fs.readJson(indexPath) as SkillIndexData; + } + + async write(index: SkillIndexData, indexPath = this.defaultPath): Promise { + await fs.ensureDir(path.dirname(indexPath)); + await fs.writeJson(indexPath, index, { spaces: 2 }); + } +} diff --git a/packages/cli/src/lib/SkillIndex.ts b/packages/cli/src/services/skill/index/skill-index.service.ts similarity index 79% rename from packages/cli/src/lib/SkillIndex.ts rename to packages/cli/src/services/skill/index/skill-index.service.ts index 9b2f1ab6..75a179da 100644 --- a/packages/cli/src/lib/SkillIndex.ts +++ b/packages/cli/src/services/skill/index/skill-index.service.ts @@ -1,17 +1,16 @@ import fs from 'fs-extra'; import * as path from 'path'; -import * as os from 'os'; -import { SkillRegistry, SKILL_CACHE_DIR } from './SkillRegistry.js'; -import { extractSkillDescription, isValidSkillName } from '../util/skill.js'; -import { fetchGitHead } from '../util/git.js'; -import { fetchGitHubSkillPaths, fetchRawGitHubFile } from '../util/github.js'; -import { ui } from '../util/terminal-ui.js'; -import { getErrorMessage } from '../util/text.js'; -import { parseLocalRegistryPath } from '../util/skill-registry.js'; -import { discoverRegistrySkills } from '../util/local-registry.js'; +import { SkillRegistryService, SKILL_CACHE_DIR } from '../registry/skill-registry.service.js'; +import { extractSkillDescription } from '../skill-description.js'; +import { fetchGitHead } from '../../../util/git.js'; +import { fetchGitHubSkillPaths, fetchRawGitHubFile } from '../../../util/github.js'; +import { ui } from '../../../util/terminal-ui.js'; +import { getErrorMessage } from '../../../util/text.js'; +import { parseLocalRegistryPath } from '../registry/skill-registry-source.js'; +import { discoverRegistrySkills } from '../registry/registry-skill-discovery.js'; +import { SkillIndexRepository } from './skill-index.repository.js'; const SEED_INDEX_URL = 'https://raw.githubusercontent.com/codeaholicguy/ai-devkit/main/skills/index.json'; -const SKILL_INDEX_PATH = path.join(os.homedir(), '.ai-devkit', 'skills.json'); const INDEX_TTL_MS = 24 * 60 * 60 * 1000; export interface SkillEntry { @@ -34,9 +33,10 @@ export interface SkillIndexData { skills: SkillEntry[]; } -export class SkillIndex { +export class SkillIndexService { constructor( - private registry: SkillRegistry + private registry: SkillRegistryService, + private repository = new SkillIndexRepository(), ) { } async findSkills(keyword: string, options?: { refresh?: boolean }): Promise { @@ -51,15 +51,14 @@ export class SkillIndex { } async rebuildIndex(outputPath?: string): Promise { - const targetPath = outputPath || SKILL_INDEX_PATH; + const targetPath = outputPath || this.repository.defaultPath; const spinner = ui.spinner('Rebuilding skill index from all registries...'); spinner.start(); try { const newIndex = await this.buildSkillIndex(); - await fs.ensureDir(path.dirname(targetPath)); - await fs.writeJson(targetPath, newIndex, { spaces: 2 }); + await this.repository.write(newIndex, targetPath); spinner.succeed(`Skill index rebuilt: ${newIndex.skills.length} skills`); ui.info(`Written to: ${targetPath}`); } catch (error: unknown) { @@ -74,7 +73,7 @@ export class SkillIndex { return; } - const existingIndex = await this.readExistingIndex(); + const existingIndex = await this.repository.read(); const nextIndex: SkillIndexData = { meta: { version: 1, @@ -88,25 +87,24 @@ export class SkillIndex { ], }; - await fs.ensureDir(path.dirname(SKILL_INDEX_PATH)); - await fs.writeJson(SKILL_INDEX_PATH, nextIndex, { spaces: 2 }); + await this.repository.write(nextIndex); } async removeRegistry(registryId: string): Promise { - const existingIndex = await this.readExistingIndex(); + const existingIndex = await this.repository.read(); if (!existingIndex) return; existingIndex.skills = existingIndex.skills.filter(skill => skill.registry !== registryId); delete existingIndex.meta.registryHeads[registryId]; existingIndex.meta.updatedAt = Date.now(); - await fs.writeJson(SKILL_INDEX_PATH, existingIndex, { spaces: 2 }); + await this.repository.write(existingIndex); } private async ensureSkillIndex(forceRefresh = false): Promise { - const indexExists = await fs.pathExists(SKILL_INDEX_PATH); + const indexExists = await this.repository.exists(); if (indexExists && !forceRefresh) { try { - const index: SkillIndexData = await fs.readJson(SKILL_INDEX_PATH); + const index = await this.repository.readRequired(); const age = Date.now() - (index.meta.updatedAt || 0); if (age < INDEX_TTL_MS) { @@ -125,8 +123,7 @@ export class SkillIndex { const response = await fetch(SEED_INDEX_URL); if (response.ok) { const seedIndex = (await response.json()) as SkillIndexData; - await fs.ensureDir(path.dirname(SKILL_INDEX_PATH)); - await fs.writeJson(SKILL_INDEX_PATH, seedIndex, { spaces: 2 }); + await this.repository.write(seedIndex); spinner.succeed('Seed index fetched successfully'); return this.refreshLocalRegistryEntries(seedIndex); } @@ -140,16 +137,15 @@ export class SkillIndex { try { const newIndex = await this.buildSkillIndex(); - await fs.ensureDir(path.dirname(SKILL_INDEX_PATH)); - await fs.writeJson(SKILL_INDEX_PATH, newIndex, { spaces: 2 }); + await this.repository.write(newIndex); spinner.succeed('Skill index updated'); return newIndex; } catch (error: unknown) { spinner.fail('Failed to build index'); - if (!forceRefresh && await fs.pathExists(SKILL_INDEX_PATH)) { + if (!forceRefresh && await this.repository.exists()) { ui.warning('Using stale index due to error'); - return await fs.readJson(SKILL_INDEX_PATH); + return await this.repository.readRequired(); } throw new Error(`Failed to build skill index: ${getErrorMessage(error)}`); @@ -160,7 +156,7 @@ export class SkillIndex { const registry = await this.registry.fetchMergedRegistry(); const registryIds = Object.keys(registry.registries); - const existingIndex = await this.readExistingIndex(); + const existingIndex = await this.repository.read(); const localSkills = await this.readConfiguredLocalRegistrySkills(registry.registries); ui.info(`Building skill index from ${registryIds.length} registries...`); @@ -271,16 +267,6 @@ export class SkillIndex { }); } - private async readExistingIndex(): Promise { - try { - if (await fs.pathExists(SKILL_INDEX_PATH)) { - return await fs.readJson(SKILL_INDEX_PATH); - } - } catch { /* ignore */ } - - return null; - } - private async refreshLocalRegistryEntries(index: SkillIndexData): Promise { const registry = await this.registry.fetchMergedRegistry(); const localIds = Object.entries(registry.registries) @@ -293,7 +279,7 @@ export class SkillIndex { meta: { ...index.meta, updatedAt: Date.now() }, skills: [...index.skills.filter(skill => !localIds.includes(skill.registry)), ...localSkills], }; - await fs.writeJson(SKILL_INDEX_PATH, next, { spaces: 2 }); + await this.repository.write(next); return next; } @@ -333,30 +319,14 @@ export class SkillIndex { return null; } - const entries = await fs.readdir(skillsPath, { withFileTypes: true }); - const skills: SkillEntry[] = []; - - for (const entry of entries) { - if (!entry.isDirectory() || !isValidSkillName(entry.name)) { - continue; - } - - const skillMdPath = path.join(skillsPath, entry.name, 'SKILL.md'); - if (!await fs.pathExists(skillMdPath)) { - continue; - } - - const content = await fs.readFile(skillMdPath, 'utf-8'); - skills.push({ - name: entry.name, - registry: registryId, - path: path.join('skills', entry.name).split(path.sep).join('/'), - description: extractSkillDescription(content), - lastIndexed: Date.now(), - }); - } - - return skills; + const discovered = await discoverRegistrySkills(registryId, registryPath); + return discovered.map(skill => ({ + name: skill.name, + registry: registryId, + path: path.join('skills', skill.name).split(path.sep).join('/'), + description: skill.description, + lastIndexed: Date.now(), + })); } private mergeSkills(remoteSkills: SkillEntry[], localSkills: SkillEntry[]): SkillEntry[] { diff --git a/packages/cli/src/lib/SkillManager.ts b/packages/cli/src/services/skill/installer/skill-installer.service.ts similarity index 68% rename from packages/cli/src/lib/SkillManager.ts rename to packages/cli/src/services/skill/installer/skill-installer.service.ts index 02227565..d1ea6a5f 100644 --- a/packages/cli/src/lib/SkillManager.ts +++ b/packages/cli/src/services/skill/installer/skill-installer.service.ts @@ -1,81 +1,99 @@ import fs from 'fs-extra'; import * as path from 'path'; import * as os from 'os'; -import { ConfigManager } from './Config.js'; -import { GlobalConfigManager } from './GlobalConfig.js'; -import { EnvironmentSelector } from './EnvironmentSelector.js'; -import { SkillRegistry, SKILL_CACHE_DIR } from './SkillRegistry.js'; -import { SkillIndex } from './SkillIndex.js'; -import { getAllEnvironments, getGlobalSkillPath, getSkillCapableEnvironments, getSkillPath, validateEnvironmentCodes } from '../util/env.js'; -import { validateRegistryId, validateSkillName, extractSkillDescription, isValidSkillName } from '../util/skill.js'; -import { parseLocalRegistryPath } from '../util/skill-registry.js'; -import { discoverRegistrySkills, resolveContainedSkill } from '../util/local-registry.js'; -import { isInteractiveTerminal } from '../util/terminal.js'; -import { ui } from '../util/terminal-ui.js'; -import { ConfigNotFoundError, NotFoundError, ValidationError } from '../util/errors.js'; -import { checkbox } from '@inquirer/prompts'; -import type { EnvironmentCode } from '../types.js'; - -import type { UpdateSummary } from './SkillRegistry.js'; -import type { SkillEntry } from './SkillIndex.js'; - -interface InstalledSkill { - name: string; - registry: string; - environments: string[]; -} - -interface GlobalInstalledSkill { - name: string; - environments: string[]; - path: string; -} - -interface AddSkillOptions { - global?: boolean; - environments?: string[]; -} - -interface RemoveSkillOptions { - global?: boolean; - environments?: string[]; -} - -interface RegistrySkillChoice { - name: string; - description?: string; +import { ConfigManager } from '../../../lib/Config.js'; +import { EnvironmentSelector } from '../../../lib/EnvironmentSelector.js'; +import { SkillRegistryService, SKILL_CACHE_DIR } from '../registry/skill-registry.service.js'; +import { getAllEnvironments, getGlobalSkillPath, getSkillCapableEnvironments, getSkillPath, validateEnvironmentCodes } from '../../../util/env.js'; +import { validateRegistryId, validateSkillName, isValidSkillName } from '../skill-validation.js'; +import { parseLocalRegistryPath } from '../registry/skill-registry-source.js'; +import { discoverRegistrySkills, resolveContainedSkill } from '../registry/registry-skill-discovery.js'; +import { isInteractiveTerminal } from '../../../util/terminal.js'; +import { ui } from '../../../util/terminal-ui.js'; +import { ConfigNotFoundError, NotFoundError, ValidationError } from '../../../util/errors.js'; +import type { EnvironmentCode } from '../../../types.js'; +import type { AddSkillOptions, GlobalInstalledSkill, InstalledSkill, RegistrySkillChoice, RemoveSkillOptions } from '../skill.types.js'; + +interface ResolvedInstallTargets { + targets: string[]; + capableEnvironments: string[]; } -interface ResolvedInstallContext { +interface ResolvedInstallContext extends ResolvedInstallTargets { baseDir: string; - capableEnvironments: string[]; installMode: 'global' | 'project'; - targets: string[]; } -export class SkillManager { - private registry: SkillRegistry; - private index: SkillIndex; - +export class SkillInstallerService { constructor( private configManager: ConfigManager, + private registry: SkillRegistryService, private environmentSelector: EnvironmentSelector = new EnvironmentSelector(), - private globalConfigManager: GlobalConfigManager = new GlobalConfigManager() - ) { - this.registry = new SkillRegistry(configManager, globalConfigManager); - this.index = new SkillIndex(this.registry); - } + ) { } /** * Add a skill to the project */ async addSkill( registryId: string, - skillName?: string, + skillName: string, + options: AddSkillOptions = {} + ): Promise<'installed' | 'matched'> { + if (!skillName) { + throw new ValidationError('Skill name is required. Re-run with: ai-devkit skill add '); + } + + return this.addSkills(registryId, [skillName], options); + } + + async addSkills( + registryId: string, + skillNames: string[], options: AddSkillOptions = {} ): Promise<'installed' | 'matched'> { + if (skillNames.length === 0) { + throw new ValidationError('At least one skill name is required.'); + } + ui.info(`Validating registry: ${registryId}`); validateRegistryId(registryId); + const { repoPath, isLocal } = await this.prepareInstallableRegistry(registryId); + const selectedEnvironments = await this.resolveInstallEnvironments(options); + const installContext = this.buildInstallContext(selectedEnvironments, options); + + let status: 'installed' | 'matched' = 'matched'; + for (const resolvedSkillName of skillNames) { + const itemStatus = await this.installResolvedSkill( + registryId, repoPath, resolvedSkillName, options, installContext, isLocal + ); + if (itemStatus === 'installed') { + status = 'installed'; + } + } + return status; + } + + async listInstallableSkills(registryId: string): Promise { + validateRegistryId(registryId); + const { repoPath } = await this.prepareInstallableRegistry(registryId); + + const skillsDir = path.join(repoPath, 'skills'); + if (!await fs.pathExists(skillsDir)) { + throw new NotFoundError(`No valid skills found in ${registryId}.`, { registryId }); + } + + const skills = await discoverRegistrySkills(registryId, repoPath); + if (skills.length === 0) { + throw new NotFoundError(`No valid skills found in ${registryId}.`, { registryId }); + } + + return skills.map(skill => ({ + name: skill.name, + description: skill.description, + })); + } + + private async prepareInstallableRegistry(registryId: string): Promise<{ repoPath: string; isLocal: boolean }> { const spinner = ui.spinner('Fetching registries...'); spinner.start(); const registry = await this.registry.fetchMergedRegistry(); @@ -92,22 +110,7 @@ export class SkillManager { const repoPath = await this.registry.prepareRegistryRepository(registryId, gitUrl); const isLocal = Boolean(gitUrl && parseLocalRegistryPath(gitUrl) !== null); - const resolvedSkillNames = skillName - ? [skillName] - : await this.resolveSkillNamesFromRegistry(registryId, repoPath, isLocal); - const selectedEnvironments = await this.resolveInstallEnvironments(options); - const installContext = this.buildInstallContext(selectedEnvironments, options); - - let status: 'installed' | 'matched' = 'matched'; - for (const resolvedSkillName of resolvedSkillNames) { - const itemStatus = await this.installResolvedSkill( - registryId, repoPath, resolvedSkillName, options, installContext, isLocal - ); - if (itemStatus === 'installed') { - status = 'installed'; - } - } - return status; + return { repoPath, isLocal }; } /** @@ -123,7 +126,7 @@ export class SkillManager { return []; } - const { targets, capableEnvironments } = this.resolveInstallationTargets(config.environments); + const { targets, capableEnvironments } = resolveInstallationTargets(config.environments); for (const targetDir of targets) { const fullPath = path.join(process.cwd(), targetDir); @@ -255,7 +258,7 @@ export class SkillManager { throw new ConfigNotFoundError('No .ai-devkit.json found. Run: ai-devkit init'); } - const { targets } = this.resolveInstallationTargets(config.environments); + const { targets } = resolveInstallationTargets(config.environments); let removedCount = 0; for (const targetDir of targets) { @@ -354,53 +357,6 @@ export class SkillManager { /** * Update skills from registries */ - async updateSkills(registryId?: string): Promise { - return this.registry.updateSkills(registryId); - } - - async cacheRegistry(registryId: string, source: string): Promise { - return this.registry.prepareRegistryRepository(registryId, source); - } - - /** - * Find skills by keyword across all registries - */ - async findSkills(keyword: string, options?: { refresh?: boolean }): Promise { - return this.index.findSkills(keyword, options); - } - - /** - * Rebuild skill index - */ - async rebuildIndex(outputPath?: string): Promise { - return this.index.rebuildIndex(outputPath); - } - - async updateSkillIndexForRegistry(registryId: string, registryPath: string): Promise { - return this.index.updateRegistryFromCache(registryId, registryPath); - } - - async removeSkillIndexForRegistry(registryId: string): Promise { - return this.index.removeRegistry(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(); @@ -449,32 +405,6 @@ export class SkillManager { return await this.resolveProjectEnvironments(); } - private resolveInstallationTargets( - environments: string[], - isGlobal = false - ): { targets: string[]; capableEnvironments: string[] } { - const targets: string[] = []; - const capableEnvironments: string[] = []; - - for (const env of environments) { - const skillPath = isGlobal ? getGlobalSkillPath(env as EnvironmentCode) : getSkillPath(env as EnvironmentCode); - if (skillPath) { - targets.push(skillPath); - capableEnvironments.push(env); - } - } - - if (targets.length === 0) { - if (isGlobal) { - throw new ValidationError('No global-skill-capable environments configured.'); - } - const supported = getSkillCapableEnvironments().map(env => env.code).join(', '); - throw new ValidationError(`No skill-capable environments configured. Supported: ${supported}`); - } - - return { targets, capableEnvironments }; - } - private async installResolvedSkill( registryId: string, repoPath: string, @@ -529,7 +459,7 @@ export class SkillManager { selectedEnvironments: string[], options: AddSkillOptions ): ResolvedInstallContext { - const { targets, capableEnvironments } = this.resolveInstallationTargets(selectedEnvironments, options.global); + const { targets, capableEnvironments } = resolveInstallationTargets(selectedEnvironments, options.global); return { baseDir: options.global ? os.homedir() : process.cwd(), @@ -563,78 +493,30 @@ export class SkillManager { return skillPath; } - private async resolveSkillNamesFromRegistry(registryId: string, repoPath: string, isLocal: boolean): Promise { - if (!isInteractiveTerminal()) { - throw new ValidationError('Skill name is required in non-interactive mode. Re-run with: ai-devkit skill add '); - } - - const skills = isLocal - ? (await discoverRegistrySkills(registryId, repoPath)).map(skill => ({ - name: skill.name, - description: skill.description, - })) - : await this.listRegistrySkills(registryId, repoPath); - return this.promptForSkillSelection(skills); - } - - private async listRegistrySkills(registryId: string, repoPath: string): Promise { - const skillsDir = path.join(repoPath, 'skills'); - if (!await fs.pathExists(skillsDir)) { - throw new NotFoundError(`No valid skills found in ${registryId}.`, { registryId }); - } - - const entries = await fs.readdir(skillsDir, { withFileTypes: true }); - const skills: RegistrySkillChoice[] = []; - - for (const entry of entries) { - if (!entry.isDirectory() || !isValidSkillName(entry.name)) { - continue; - } - - const skillMdPath = path.join(skillsDir, entry.name, 'SKILL.md'); - if (!await fs.pathExists(skillMdPath)) { - continue; - } +} - let description: string | undefined; - try { - const content = await fs.readFile(skillMdPath, 'utf8'); - description = extractSkillDescription(content); - } catch { - description = undefined; - } +function resolveInstallationTargets( + environments: string[], + isGlobal = false, +): ResolvedInstallTargets { + const targets: string[] = []; + const capableEnvironments: string[] = []; - skills.push({ - name: entry.name, - description, - }); - } - - if (skills.length === 0) { - throw new NotFoundError(`No valid skills found in ${registryId}.`, { registryId }); + for (const env of environments) { + const skillPath = isGlobal ? getGlobalSkillPath(env as EnvironmentCode) : getSkillPath(env as EnvironmentCode); + if (skillPath) { + targets.push(skillPath); + capableEnvironments.push(env); } - - skills.sort((a, b) => a.name.localeCompare(b.name)); - return skills; } - private async promptForSkillSelection(skills: RegistrySkillChoice[]): Promise { - try { - return await checkbox({ - message: 'Select skill(s) to install', - choices: skills.map(skill => ({ - name: skill.description ? `${skill.name} - ${skill.description}` : skill.name, - value: skill.name, - })), - required: true, - }); - } catch (error: unknown) { - if (error instanceof Error && - (error.name === 'ExitPromptError' || error.message.toLowerCase().includes('cancel'))) { - throw new Error('Skill selection cancelled.'); - } - - throw error; + if (targets.length === 0) { + if (isGlobal) { + throw new ValidationError('No global-skill-capable environments configured.'); } + const supported = getSkillCapableEnvironments().map(env => env.code).join(', '); + throw new ValidationError(`No skill-capable environments configured. Supported: ${supported}`); } + + return { targets, capableEnvironments }; } diff --git a/packages/cli/src/util/local-registry.ts b/packages/cli/src/services/skill/registry/registry-skill-discovery.ts similarity index 85% rename from packages/cli/src/util/local-registry.ts rename to packages/cli/src/services/skill/registry/registry-skill-discovery.ts index e2323d6b..0b693a3d 100644 --- a/packages/cli/src/util/local-registry.ts +++ b/packages/cli/src/services/skill/registry/registry-skill-discovery.ts @@ -1,12 +1,13 @@ import fs from 'fs-extra'; import path from 'node:path'; -import { CliError, NotFoundError } from './errors.js'; -import { extractSkillDescription, isValidSkillName } from './skill.js'; +import { CliError, NotFoundError } from '../../../util/errors.js'; +import { isValidSkillName } from '../skill-validation.js'; +import { extractSkillDescription } from '../skill-description.js'; export const LOCAL_REGISTRY_MAX_ENTRIES = 10_000; export const LOCAL_REGISTRY_MAX_SKILL_MD_BYTES = 1024 * 1024; -interface DiscoveredRegistrySkill { +export interface DiscoveredRegistrySkill { name: string; description: string; } @@ -82,13 +83,14 @@ export async function discoverRegistrySkills( ); } if ((!entry.isDirectory() && !entry.isSymbolicLink()) || !isValidSkillName(entry.name)) continue; + const metadataPath = path.join(skillsRoot, entry.name, 'SKILL.md'); + if (!await fs.pathExists(metadataPath)) continue; const skillPath = await resolveContainedSkill(registryId, canonicalRoot, entry.name); - const metadataPath = path.join(skillPath, 'SKILL.md'); - const content = await fs.readFile(metadataPath, 'utf8'); + const content = await fs.readFile(path.join(skillPath, 'SKILL.md'), 'utf8'); skills.push({ name: entry.name, description: extractSkillDescription(content), }); } - return skills; + return skills.sort((left, right) => left.name.localeCompare(right.name)); } diff --git a/packages/cli/src/util/skill-registry.ts b/packages/cli/src/services/skill/registry/skill-registry-source.ts similarity index 98% rename from packages/cli/src/util/skill-registry.ts rename to packages/cli/src/services/skill/registry/skill-registry-source.ts index b53ad381..0cd384ad 100644 --- a/packages/cli/src/util/skill-registry.ts +++ b/packages/cli/src/services/skill/registry/skill-registry-source.ts @@ -1,4 +1,4 @@ -import { CliError } from './errors.js'; +import { CliError } from '../../../util/errors.js'; import fs from 'fs-extra'; import path from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; diff --git a/packages/cli/src/lib/SkillRegistry.ts b/packages/cli/src/services/skill/registry/skill-registry.service.ts similarity index 75% rename from packages/cli/src/lib/SkillRegistry.ts rename to packages/cli/src/services/skill/registry/skill-registry.service.ts index b8e8c784..1dcb0ad3 100644 --- a/packages/cli/src/lib/SkillRegistry.ts +++ b/packages/cli/src/services/skill/registry/skill-registry.service.ts @@ -1,15 +1,18 @@ import fs from 'fs-extra'; import * as path from 'path'; import * as os from 'os'; -import { ConfigManager } from './Config.js'; -import { GlobalConfigManager } from './GlobalConfig.js'; -import { ensureGitInstalled, cloneRepository, isGitRepository, pullRepository } from '../util/git.js'; -import { ui } from '../util/terminal-ui.js'; -import { getErrorMessage } from '../util/text.js'; -import { CliError, NotFoundError } from '../util/errors.js'; -import { parseLocalRegistryPath } from '../util/skill-registry.js'; -import { isValidSkillName } from '../util/skill.js'; -import { LOCAL_REGISTRY_MAX_ENTRIES } from '../util/local-registry.js'; +import { ConfigManager } from '../../../lib/Config.js'; +import { GlobalConfigManager } from '../../../lib/GlobalConfig.js'; +import { ensureGitInstalled, cloneRepository, isGitRepository, pullRepository } from '../../../util/git.js'; +import { ui } from '../../../util/terminal-ui.js'; +import { getErrorMessage } from '../../../util/text.js'; +import { CliError, NotFoundError } from '../../../util/errors.js'; +import { normalizeRegistrySourceInput, normalizeRegistrySources, parseLocalRegistryPath, planSkillRegistryAdd } from './skill-registry-source.js'; +import { isValidSkillName, validateRegistryId } from '../skill-validation.js'; +import { BUILTIN_SKILL_REGISTRY } from '../skill-builtins.js'; +import { LOCAL_REGISTRY_MAX_ENTRIES } from './registry-skill-discovery.js'; +import type { AddSkillRegistryCommandOptions, RemoveSkillRegistryCommandOptions } from '../skill.types.js'; +import type { SkillRegistryAddStatus } from './skill-registry-source.js'; export const REGISTRY_URL = 'https://raw.githubusercontent.com/codeaholicguy/ai-devkit/main/skills/registry.json'; export const SKILL_CACHE_DIR = path.join(os.homedir(), '.ai-devkit', 'skills'); @@ -33,7 +36,12 @@ export interface UpdateSummary { results: UpdateResult[]; } -export class SkillRegistry { +export interface AddRegistryResult { + status: SkillRegistryAddStatus; + registryPath?: string; +} + +export class SkillRegistryService { private mergedRegistry?: Promise; private readonly preparedRepositories = new Map>(); @@ -123,6 +131,79 @@ export class SkillRegistry { return preparation; } + async cacheRegistry(registryId: string, source: string): Promise { + return this.prepareRegistryRepository(registryId, source); + } + + async addRegistrySource( + id: string, + source: string, + options: AddSkillRegistryCommandOptions = {}, + ): Promise { + validateRegistryId(id); + const configManager = options.global + ? this.globalConfigManager + : this.configManager; + + const registries = await configManager.getSkillRegistries(); + const value = await normalizeRegistrySourceInput(source, process.cwd()); + const [projectRegistries, globalRegistries] = await Promise.all([ + options.global ? this.configManager.getSkillRegistries() : Promise.resolve(registries), + options.global ? Promise.resolve(registries) : this.globalConfigManager.getSkillRegistries(), + ]); + await normalizeRegistrySources({ ...globalRegistries, ...projectRegistries, [id]: value }, process.cwd()); + const mutation = planSkillRegistryAdd(registries, id, value, { force: options.force }); + + const registryPath = mutation.status !== 'already-registered' + ? await this.cacheRegistry(id, value) + : undefined; + + await configManager.addSkillRegistry(id, value, { force: options.force }); + return { status: mutation.status, registryPath }; + } + + async removeRegistrySource( + id: string, + options: RemoveSkillRegistryCommandOptions = {}, + ): Promise<'project' | 'global'> { + validateRegistryId(id); + if (id === BUILTIN_SKILL_REGISTRY) { + throw new Error(`Registry "${id}" is built in and cannot be unregistered.`); + } + + const configManager = options.global + ? this.globalConfigManager + : this.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 this.removeRegistryCache(id); + } + + return options.global ? 'global' : 'project'; + } + + /** + * 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 prepareGitRegistry(registryId: string, gitUrl?: string): Promise { await ensureGitInstalled(); return this.refreshOrUseStaleCache(registryId, gitUrl); diff --git a/packages/cli/src/lib/BuiltinSkills.ts b/packages/cli/src/services/skill/skill-builtins.ts similarity index 91% rename from packages/cli/src/lib/BuiltinSkills.ts rename to packages/cli/src/services/skill/skill-builtins.ts index 5fbec4cb..f1661fa7 100644 --- a/packages/cli/src/lib/BuiltinSkills.ts +++ b/packages/cli/src/services/skill/skill-builtins.ts @@ -1,6 +1,6 @@ -import { isValidSkillName } from '../util/skill.js'; -import { getErrorMessage } from '../util/text.js'; -import { ui } from '../util/terminal-ui.js'; +import { isValidSkillName } from './skill-validation.js'; +import { getErrorMessage } from '../../util/text.js'; +import { ui } from '../../util/terminal-ui.js'; const BUILTIN_SKILLS_URL = 'https://raw.githubusercontent.com/codeaholicguy/ai-devkit/main/skills/built-in.json'; diff --git a/packages/cli/src/services/skill/skill-description.ts b/packages/cli/src/services/skill/skill-description.ts new file mode 100644 index 00000000..9ca6f6fb --- /dev/null +++ b/packages/cli/src/services/skill/skill-description.ts @@ -0,0 +1,27 @@ +import matter from 'gray-matter'; + +/** + * Extract skill description from SKILL.md frontmatter + * @param content - Content of SKILL.md file + * @returns Description from frontmatter or first non-empty paragraph + */ +export function extractSkillDescription(content: string): string { + try { + const parsed = matter(content); + + // Try to get description from frontmatter + if (parsed.data && parsed.data.description) { + return String(parsed.data.description).trim(); + } + + // Fallback: use first non-empty paragraph from content + const lines = parsed.content + .split('\n') + .filter((l: string) => l.trim() && !l.startsWith('#')); + + return lines[0]?.trim() || 'No description available'; + } catch (ignoreError) { + // If parsing fails, return fallback + return 'No description available'; + } +} diff --git a/packages/cli/src/util/skill.ts b/packages/cli/src/services/skill/skill-validation.ts similarity index 63% rename from packages/cli/src/util/skill.ts rename to packages/cli/src/services/skill/skill-validation.ts index a56eb321..44258ae2 100644 --- a/packages/cli/src/util/skill.ts +++ b/packages/cli/src/services/skill/skill-validation.ts @@ -1,5 +1,4 @@ -import matter from 'gray-matter'; -import { ValidationError } from './errors.js'; +import { ValidationError } from '../../util/errors.js'; /** * Validates registry ID format @@ -44,29 +43,3 @@ export function validateSkillName(skillName: string): void { export function isValidSkillName(name: string): boolean { return /^[a-z0-9]+(-[a-z0-9]+)*$/.test(name); } - -/** - * Extract skill description from SKILL.md frontmatter - * @param content - Content of SKILL.md file - * @returns Description from frontmatter or first non-empty paragraph - */ -export function extractSkillDescription(content: string): string { - try { - const parsed = matter(content); - - // Try to get description from frontmatter - if (parsed.data && parsed.data.description) { - return String(parsed.data.description).trim(); - } - - // Fallback: use first non-empty paragraph from content - const lines = parsed.content - .split('\n') - .filter((l: string) => l.trim() && !l.startsWith('#')); - - return lines[0]?.trim() || 'No description available'; - } catch (error) { - // If parsing fails, return fallback - return 'No description available'; - } -} \ No newline at end of file diff --git a/packages/cli/src/services/skill/skill.service.ts b/packages/cli/src/services/skill/skill.service.ts new file mode 100644 index 00000000..0c0f84ed --- /dev/null +++ b/packages/cli/src/services/skill/skill.service.ts @@ -0,0 +1,99 @@ +import { ConfigManager } from '../../lib/Config.js'; +import { GlobalConfigManager } from '../../lib/GlobalConfig.js'; +import { EnvironmentSelector } from '../../lib/EnvironmentSelector.js'; +import { SkillInstallerService } from './installer/skill-installer.service.js'; +import { SkillIndexService } from './index/skill-index.service.js'; +import { SkillRegistryService } from './registry/skill-registry.service.js'; +import type { + AddSkillOptions, + AddSkillRegistryCommandOptions, + GlobalInstalledSkill, + InstalledSkill, + RegistrySkillChoice, + RemoveSkillOptions, + RemoveSkillRegistryCommandOptions, +} from './skill.types.js'; +import type { SkillEntry } from './index/skill-index.service.js'; +import type { SkillRegistryAddStatus } from './registry/skill-registry-source.js'; +import type { UpdateSummary } from './registry/skill-registry.service.js'; + +export class SkillService { + private readonly installer: SkillInstallerService; + private readonly registry: SkillRegistryService; + private readonly index: SkillIndexService; + + constructor( + configManager: ConfigManager, + environmentSelector: EnvironmentSelector = new EnvironmentSelector(), + globalConfigManager: GlobalConfigManager = new GlobalConfigManager(), + ) { + this.registry = new SkillRegistryService(configManager, globalConfigManager); + this.index = new SkillIndexService(this.registry); + this.installer = new SkillInstallerService(configManager, this.registry, environmentSelector); + } + + addSkill( + registryId: string, + skillName: string, + options: AddSkillOptions = {}, + ): Promise<'installed' | 'matched'> { + return this.installer.addSkill(registryId, skillName, options); + } + + addSkills( + registryId: string, + skillNames: string[], + options: AddSkillOptions = {}, + ): Promise<'installed' | 'matched'> { + return this.installer.addSkills(registryId, skillNames, options); + } + + listInstallableSkills(registryId: string): Promise { + return this.installer.listInstallableSkills(registryId); + } + + listSkills(): Promise { + return this.installer.listSkills(); + } + + listGlobalSkills(envCodes?: string[]): Promise { + return this.installer.listGlobalSkills(envCodes); + } + + removeSkill(skillName: string, options: RemoveSkillOptions = {}): Promise { + return this.installer.removeSkill(skillName, options); + } + + async addRegistry( + id: string, + source: string, + options: AddSkillRegistryCommandOptions = {}, + ): Promise { + const result = await this.registry.addRegistrySource(id, source, options); + if (result.registryPath) { + await this.index.updateRegistryFromCache(id, result.registryPath); + } + return result.status; + } + + async removeRegistry( + id: string, + options: RemoveSkillRegistryCommandOptions = {}, + ): Promise<'project' | 'global'> { + const scope = await this.registry.removeRegistrySource(id, options); + await this.index.removeRegistry(id); + return scope; + } + + updateSkills(registryId?: string): Promise { + return this.registry.updateSkills(registryId); + } + + findSkills(keyword: string, options?: { refresh?: boolean }): Promise { + return this.index.findSkills(keyword, options); + } + + rebuildIndex(outputPath?: string): Promise { + return this.index.rebuildIndex(outputPath); + } +} diff --git a/packages/cli/src/services/skill/skill.types.ts b/packages/cli/src/services/skill/skill.types.ts new file mode 100644 index 00000000..9f60a83d --- /dev/null +++ b/packages/cli/src/services/skill/skill.types.ts @@ -0,0 +1,35 @@ +export interface InstalledSkill { + name: string; + registry: string; + environments: string[]; +} + +export interface GlobalInstalledSkill { + name: string; + environments: string[]; + path: string; +} + +export interface RegistrySkillChoice { + name: string; + description?: string; +} + +export interface AddSkillOptions { + global?: boolean; + environments?: string[]; +} + +export interface RemoveSkillOptions { + global?: boolean; + environments?: string[]; +} + +export interface AddSkillRegistryCommandOptions { + global?: boolean; + force?: boolean; +} + +export interface RemoveSkillRegistryCommandOptions { + global?: boolean; +} diff --git a/packages/cli/src/services/status/status.service.ts b/packages/cli/src/services/status/status.service.ts index 2c25e59c..323fd9a9 100644 --- a/packages/cli/src/services/status/status.service.ts +++ b/packages/cli/src/services/status/status.service.ts @@ -13,7 +13,7 @@ import { type ReadinessAgentType, type ReadinessStatus, } from '@ai-devkit/agent-manager'; -import { getBuiltinSkillNames } from '../../lib/BuiltinSkills.js'; +import { getBuiltinSkillNames } from '../skill/skill-builtins.js'; import { filterStringRecord } from '../../util/config.js'; import { getGlobalSkillPath, isValidEnvironmentCode } from '../../util/env.js'; import { inspectTmux } from '../../util/tmux.js';