Skip to content

Commit 1847de6

Browse files
committed
feat(cli): prioritize project skill registries over global and default
1 parent 3659dd9 commit 1847de6

File tree

9 files changed

+354
-2
lines changed

9 files changed

+354
-2
lines changed
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
---
2+
phase: design
3+
title: System Design & Architecture
4+
description: Merge registry sources with project-first precedence for skill installation
5+
---
6+
7+
# System Design & Architecture
8+
9+
## Architecture Overview
10+
```mermaid
11+
graph TD
12+
SkillAdd[ai-devkit skill add] --> SkillManager
13+
SkillManager --> DefaultRegistry[Remote default registry.json]
14+
SkillManager --> GlobalConfig[~/.ai-devkit/.ai-devkit.json]
15+
SkillManager --> ProjectConfig[./.ai-devkit.json]
16+
DefaultRegistry --> Merge[Registry merge]
17+
GlobalConfig --> Merge
18+
ProjectConfig --> Merge
19+
Merge --> Resolved[Resolved registry map]
20+
```
21+
22+
- `SkillManager.fetchMergedRegistry` remains the single merge point.
23+
- `ConfigManager` adds project registry extraction.
24+
- Merge order is implemented as object spread with source ordering.
25+
26+
## Data Models
27+
- Registry map shape: `Record<string, string>`.
28+
- Project registry extraction supports:
29+
- `registries` at root.
30+
- `skills.registries` when `skills` is an object.
31+
32+
## API Design
33+
- `ConfigManager.getSkillRegistries(): Promise<Record<string, string>>`.
34+
- `SkillManager.fetchMergedRegistry()` now merges three sources.
35+
36+
## Component Breakdown
37+
- `packages/cli/src/lib/Config.ts`: parse project registry mappings.
38+
- `packages/cli/src/lib/SkillManager.ts`: apply precedence order.
39+
- Tests:
40+
- `packages/cli/src/__tests__/lib/Config.test.ts`
41+
- `packages/cli/src/__tests__/lib/SkillManager.test.ts`
42+
43+
## Design Decisions
44+
- Keep merge logic centralized in `SkillManager` to avoid drift.
45+
- Keep parser tolerant to allow gradual config evolution.
46+
- Favor project determinism by applying project map last.
47+
48+
## Non-Functional Requirements
49+
- No additional network calls.
50+
- No change to failure mode when default registry fetch fails (still supports fallback sources).
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
---
2+
phase: implementation
3+
title: Implementation Guide
4+
description: Implementation notes for project-level registry override precedence
5+
---
6+
7+
# Implementation Guide
8+
9+
## Development Setup
10+
- Work in feature branch/worktree: `feature-project-skill-registry-priority`.
11+
- Install deps via `npm ci`.
12+
13+
## Code Structure
14+
- `SkillManager` owns merged registry resolution.
15+
- `ConfigManager` owns project config parsing helpers.
16+
17+
## Implementation Notes
18+
### Core Features
19+
- Added `ConfigManager.getSkillRegistries()` to read project registry map from:
20+
- `registries` (root), or
21+
- `skills.registries` (legacy-compatible fallback when `skills` is object).
22+
- Updated `SkillManager.fetchMergedRegistry()` to merge in this order:
23+
- default registry,
24+
- global registries,
25+
- project registries.
26+
27+
### Patterns & Best Practices
28+
- Ignore malformed/non-string registry values.
29+
- Keep merge deterministic and centralized.
30+
31+
## Error Handling
32+
- If project config has no valid registry map, return `{}` and continue.
33+
- Existing default-registry fetch warning behavior remains unchanged.
34+
35+
## Performance Considerations
36+
- No new network requests.
37+
- Constant-time map merge relative to source map sizes.
38+
39+
## Check Implementation (Phase 6)
40+
- Requirements-to-code mapping:
41+
- Project registry source support implemented in `packages/cli/src/lib/Config.ts` via `getSkillRegistries()`.
42+
- Precedence contract (`project > global > default`) implemented in `packages/cli/src/lib/SkillManager.ts` within `fetchMergedRegistry()`.
43+
- Compatibility preserved:
44+
- Existing global override behavior still works.
45+
- Default registry fetch fallback behavior unchanged.
46+
47+
## Code Review (Phase 8)
48+
- Reviewed changed production files for regressions in install flow and config parsing.
49+
- No blocking issues found.
50+
- Residual risk: only unit-level validation was run; end-to-end CLI fixture validation is still optional follow-up.
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
---
2+
phase: planning
3+
title: Project Planning & Task Breakdown
4+
description: Implement and validate project/global/default registry precedence
5+
---
6+
7+
# Project Planning & Task Breakdown
8+
9+
## Milestones
10+
- [x] Milestone 1: Define requirements and precedence contract.
11+
- [x] Milestone 2: Implement registry source parsing and merge order.
12+
- [x] Milestone 3: Add automated tests and validate feature docs.
13+
14+
## Task Breakdown
15+
### Phase 1: Requirements & Design
16+
- [x] Task 1.1: Confirm desired precedence (`project > global > default`).
17+
- [x] Task 1.2: Define where project registry mappings are read from.
18+
19+
### Phase 2: Implementation
20+
- [x] Task 2.1: Add `ConfigManager.getSkillRegistries()`.
21+
- [x] Task 2.2: Update `SkillManager.fetchMergedRegistry()` merge order.
22+
23+
### Phase 3: Validation
24+
- [x] Task 3.1: Add/adjust tests for project registry parsing.
25+
- [x] Task 3.2: Add/adjust tests for precedence conflicts.
26+
- [x] Task 3.3: Run focused CLI tests and feature lint.
27+
28+
## Dependencies
29+
- Existing `ConfigManager` and `GlobalConfigManager` APIs.
30+
- Existing `SkillManager` registry merge flow.
31+
32+
## Timeline & Estimates
33+
- Implementation and tests: same working session.
34+
- Validation: focused unit suite execution.
35+
36+
## Risks & Mitigation
37+
- Risk: project config schema ambiguity.
38+
- Mitigation: support both root and nested registry map formats and ignore invalid entries.
39+
40+
## Execution Log
41+
- 2026-02-27: Ran focused tests for `ConfigManager` and `SkillManager` (73 passing).
42+
- 2026-02-27: Ran `npx ai-devkit@latest lint --feature project-skill-registry-priority` (pass).
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
---
2+
phase: requirements
3+
title: Requirements & Problem Understanding
4+
description: Add project-level registry source with deterministic precedence for skill installation
5+
---
6+
7+
# Requirements & Problem Understanding
8+
9+
## Problem Statement
10+
- Skill installation currently resolves registries from default remote registry plus global config (`~/.ai-devkit/.ai-devkit.json`).
11+
- Teams need project-specific overrides in repository config so installs are reproducible across contributors and CI.
12+
- Without project-level override, users must edit global state and cannot keep registry decisions version-controlled.
13+
14+
## Goals & Objectives
15+
- Add project `.ai-devkit.json` as an additional registry source for skill installation.
16+
- Enforce deterministic conflict precedence: `project > global > default`.
17+
- Preserve backward compatibility for existing projects and global-only users.
18+
19+
## Non-Goals
20+
- Redesigning the entire `.ai-devkit.json` schema.
21+
- Changing non-install commands that do not rely on registry resolution.
22+
- Introducing remote registry auth or secret management.
23+
24+
## User Stories & Use Cases
25+
- As a project maintainer, I can define custom registry mappings in project config so all contributors use the same registry source.
26+
- As a developer with personal global overrides, project overrides still win inside that repository.
27+
- As an existing user with only global config, behavior remains unchanged.
28+
29+
## Success Criteria
30+
- `ai-devkit skill add` reads registry maps from project config, global config, and default registry.
31+
- On key collision, selected URL follows `project > global > default`.
32+
- Unit tests cover precedence and parsing behavior.
33+
34+
## Constraints & Assumptions
35+
- Current runtime already reads `.ai-devkit.json` via `ConfigManager`.
36+
- Project configs may contain either root `registries` or nested `skills.registries`; both should be accepted for resilience.
37+
- Invalid non-string entries are ignored.
38+
39+
## Questions & Open Items
40+
- None blocking for implementation.
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
---
2+
phase: testing
3+
title: Testing Strategy
4+
description: Test precedence and parsing for project/global/default skill registries
5+
---
6+
7+
# Testing Strategy
8+
9+
## Test Coverage Goals
10+
- Unit coverage for new/changed behavior in `ConfigManager` and `SkillManager`.
11+
- Validate precedence conflict resolution and parser resilience.
12+
13+
## Unit Tests
14+
### ConfigManager
15+
- [x] Reads registry map from root `registries`.
16+
- [x] Falls back to nested `skills.registries` when root map is absent.
17+
- [x] Returns empty map when no valid registry map exists.
18+
19+
### SkillManager
20+
- [x] Uses custom global registry over default (existing behavior retained).
21+
- [x] Uses project registry over global and default on ID collision.
22+
23+
## Integration Tests
24+
- [ ] Optional follow-up: CLI-level `skill add` using fixture `.ai-devkit.json` with project overrides.
25+
26+
## Test Reporting & Coverage
27+
- Focused command executed:
28+
- `npm run test --workspace=packages/cli -- --runInBand src/__tests__/lib/Config.test.ts src/__tests__/lib/SkillManager.test.ts`
29+
- Result: 2 suites passed, 73 tests passed, 0 failed.
30+
- Feature documentation lint:
31+
- `npx ai-devkit@latest lint --feature project-skill-registry-priority`
32+
- Result: pass.
33+
34+
## Coverage Gaps
35+
- No known unit-test gaps for changed paths.
36+
- Optional integration follow-up remains open for full CLI fixture validation.

packages/cli/src/__tests__/lib/Config.test.ts

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -404,4 +404,66 @@ describe('ConfigManager', () => {
404404
expect(mockFs.writeJson).not.toHaveBeenCalled();
405405
});
406406
});
407+
408+
describe('getSkillRegistries', () => {
409+
it('returns registries from root-level "registries"', async () => {
410+
(mockFs.pathExists as any).mockResolvedValue(true);
411+
(mockFs.readJson as any).mockResolvedValue({
412+
version: '1.0.0',
413+
environments: ['cursor'],
414+
phases: [],
415+
registries: {
416+
'project/skills': 'https://github.com/project/skills.git',
417+
'invalid/entry': 123
418+
},
419+
createdAt: '2024-01-01T00:00:00.000Z',
420+
updatedAt: '2024-01-01T00:00:00.000Z'
421+
});
422+
423+
const registries = await configManager.getSkillRegistries();
424+
425+
expect(registries).toEqual({
426+
'project/skills': 'https://github.com/project/skills.git'
427+
});
428+
});
429+
430+
it('falls back to nested "skills.registries" when root registries are missing', async () => {
431+
(mockFs.pathExists as any).mockResolvedValue(true);
432+
(mockFs.readJson as any).mockResolvedValue({
433+
version: '1.0.0',
434+
environments: ['cursor'],
435+
phases: [],
436+
skills: {
437+
registries: {
438+
'nested/skills': 'https://github.com/nested/skills.git',
439+
'invalid/value': false
440+
}
441+
},
442+
createdAt: '2024-01-01T00:00:00.000Z',
443+
updatedAt: '2024-01-01T00:00:00.000Z'
444+
});
445+
446+
const registries = await configManager.getSkillRegistries();
447+
448+
expect(registries).toEqual({
449+
'nested/skills': 'https://github.com/nested/skills.git'
450+
});
451+
});
452+
453+
it('returns empty object when no registry map exists', async () => {
454+
(mockFs.pathExists as any).mockResolvedValue(true);
455+
(mockFs.readJson as any).mockResolvedValue({
456+
version: '1.0.0',
457+
environments: ['cursor'],
458+
phases: [],
459+
skills: [{ registry: 'codeaholicguy/ai-devkit', name: 'debug' }],
460+
createdAt: '2024-01-01T00:00:00.000Z',
461+
updatedAt: '2024-01-01T00:00:00.000Z'
462+
});
463+
464+
const registries = await configManager.getSkillRegistries();
465+
466+
expect(registries).toEqual({});
467+
});
468+
});
407469
});

packages/cli/src/__tests__/lib/SkillManager.test.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ describe("SkillManager", () => {
7575
new MockedGlobalConfigManager() as jest.Mocked<GlobalConfigManager>;
7676

7777
mockGlobalConfigManager.getSkillRegistries.mockResolvedValue({});
78+
mockConfigManager.getSkillRegistries.mockResolvedValue({});
7879

7980
skillManager = new SkillManager(
8081
mockConfigManager,
@@ -216,6 +217,56 @@ describe("SkillManager", () => {
216217
);
217218
});
218219

220+
it("should prefer project registry URL over global and default", async () => {
221+
const defaultGitUrl = "https://github.com/default/skills.git";
222+
const globalGitUrl = "https://github.com/global/skills.git";
223+
const projectGitUrl = "https://github.com/project/skills.git";
224+
225+
mockFetch({
226+
registries: {
227+
[mockRegistryId]: defaultGitUrl,
228+
},
229+
});
230+
231+
mockGlobalConfigManager.getSkillRegistries.mockResolvedValue({
232+
[mockRegistryId]: globalGitUrl,
233+
});
234+
mockConfigManager.getSkillRegistries.mockResolvedValue({
235+
[mockRegistryId]: projectGitUrl,
236+
});
237+
238+
const repoPath = path.join(
239+
os.homedir(),
240+
".ai-devkit",
241+
"skills",
242+
mockRegistryId,
243+
);
244+
245+
(mockedFs.pathExists as any).mockImplementation((checkPath: string) => {
246+
if (checkPath === repoPath) {
247+
return Promise.resolve(false);
248+
}
249+
250+
if (checkPath.includes(`${path.sep}skills${path.sep}${mockSkillName}`)) {
251+
return Promise.resolve(true);
252+
}
253+
254+
if (checkPath.endsWith(`${path.sep}SKILL.md`)) {
255+
return Promise.resolve(true);
256+
}
257+
258+
return Promise.resolve(true);
259+
});
260+
261+
await skillManager.addSkill(mockRegistryId, mockSkillName);
262+
263+
expect(mockedGitUtil.cloneRepository).toHaveBeenCalledWith(
264+
path.join(os.homedir(), ".ai-devkit", "skills"),
265+
mockRegistryId,
266+
projectGitUrl,
267+
);
268+
});
269+
219270
it("should read custom registries from global config", async () => {
220271
const customGitUrl = "https://github.com/custom/skills.git";
221272
const { GlobalConfigManager: RealGlobalConfigManager } = jest.requireActual(

packages/cli/src/lib/Config.ts

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,4 +112,23 @@ export class ConfigManager {
112112
skills.push(skill);
113113
return this.update({ skills });
114114
}
115+
116+
async getSkillRegistries(): Promise<Record<string, string>> {
117+
const config = await this.read() as any;
118+
const rootRegistries = config?.registries;
119+
const nestedRegistries =
120+
config?.skills && !Array.isArray(config.skills)
121+
? config.skills.registries
122+
: undefined;
123+
124+
const registries = rootRegistries ?? nestedRegistries;
125+
126+
if (!registries || typeof registries !== 'object' || Array.isArray(registries)) {
127+
return {};
128+
}
129+
130+
return Object.fromEntries(
131+
Object.entries(registries).filter(([, value]) => typeof value === 'string')
132+
) as Record<string, string>;
133+
}
115134
}

packages/cli/src/lib/SkillManager.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -379,12 +379,14 @@ export class SkillManager {
379379
defaultRegistries = {};
380380
}
381381

382-
const customRegistries = await this.globalConfigManager.getSkillRegistries();
382+
const globalRegistries = await this.globalConfigManager.getSkillRegistries();
383+
const projectRegistries = await this.configManager.getSkillRegistries();
383384

384385
return {
385386
registries: {
386387
...defaultRegistries,
387-
...customRegistries
388+
...globalRegistries,
389+
...projectRegistries
388390
}
389391
};
390392
}

0 commit comments

Comments
 (0)