Refactor skill command UI boundary

packages/cli/src/__tests__/commands/skill.test.ts

@@ -1,12 +1,17 @@
 import { Command } from 'commander';
 
-import { registerSkillCommand } from '../../commands/skill.js';
+import { registerSkillCommand } from '../../commands/skill/index.js';
 import { ui } from '../../util/terminal-ui.js';
 import { SkillService } from '../../services/skill/skill.service.js';
 
 const mockGetBuiltinSkillNames = vi.hoisted(() => vi.fn());
 const mockIsInteractiveTerminal = vi.hoisted(() => vi.fn(() => true));
 const mockCheckbox = vi.hoisted(() => vi.fn());
+const mockConfigRead = vi.hoisted(() => vi.fn());
+const mockConfigCreate = vi.hoisted(() => vi.fn());
+const mockConfigUpdate = vi.hoisted(() => vi.fn());
+const mockSelectSkillEnvironments = vi.hoisted(() => vi.fn());
+const mockSelectGlobalSkillEnvironments = vi.hoisted(() => vi.fn());
 
 
 const mockAddSkill = vi.fn();
@@ -19,7 +24,18 @@ const mockRemoveSkill = vi.fn();
 const mockRemoveRegistry = vi.fn();
 
 vi.mock('../../lib/Config.js', () => ({
-  ConfigManager: vi.fn(function () { return {}; }),
+  ConfigManager: vi.fn(function () { return {
+    read: (...args: unknown[]) => mockConfigRead(...args),
+    create: (...args: unknown[]) => mockConfigCreate(...args),
+    update: (...args: unknown[]) => mockConfigUpdate(...args),
+  }; }),
+}));
+
+vi.mock('../../lib/EnvironmentSelector.js', () => ({
+  EnvironmentSelector: vi.fn(function () { return {
+    selectSkillEnvironments: (...args: unknown[]) => mockSelectSkillEnvironments(...args),
+    selectGlobalSkillEnvironments: (...args: unknown[]) => mockSelectGlobalSkillEnvironments(...args),
+  }; }),
 }));
 
 vi.mock('../../services/skill/skill.service.js', () => ({
@@ -65,19 +81,41 @@ vi.mock('@inquirer/prompts', () => ({
 describe('skill command', () => {
   beforeEach(() => {
     vi.clearAllMocks();
-    mockAddSkill.mockImplementation(async () => undefined);
-    mockAddSkills.mockImplementation(async () => undefined);
+    mockAddSkill.mockImplementation(async (registryId: string, skillName: string, options: { global?: boolean; environments?: string[] }) => ({
+      status: 'installed',
+      registryId,
+      installMode: options.global ? 'global' : 'project',
+      environments: options.environments || [],
+      items: [{ skillName, target: `.claude/skills/${skillName}`, action: 'symlinked' }],
+    }));
+    mockAddSkills.mockImplementation(async (registryId: string, skillNames: string[], options: { global?: boolean; environments?: string[] }) => ({
+      status: 'installed',
+      registryId,
+      installMode: options.global ? 'global' : 'project',
+      environments: options.environments || [],
+      items: skillNames.map(skillName => ({ skillName, target: `.claude/skills/${skillName}`, action: 'symlinked' })),
+    }));
     mockAddRegistry.mockResolvedValue('added');
     mockListGlobalSkills.mockResolvedValue([]);
     mockListInstallableSkills.mockResolvedValue([
       { name: 'frontend-design', description: 'Frontend skill' },
       { name: 'debug' },
     ]);
     mockListSkills.mockResolvedValue([]);
-    mockRemoveSkill.mockImplementation(async () => undefined);
+    mockRemoveSkill.mockImplementation(async (skillName: string, options: { global?: boolean }) => ({
+      skillName,
+      scope: options.global ? 'global' : 'project',
+      removedTargets: [],
+      failures: [],
+    }));
     mockRemoveRegistry.mockResolvedValue('project');
     mockGetBuiltinSkillNames.mockResolvedValue(['remote-one', 'remote-two']);
     mockIsInteractiveTerminal.mockReturnValue(true);
+    mockConfigRead.mockResolvedValue({ environments: ['claude'] });
+    mockConfigCreate.mockResolvedValue({ environments: [] });
+    mockConfigUpdate.mockResolvedValue({});
+    mockSelectSkillEnvironments.mockResolvedValue(['claude']);
+    mockSelectGlobalSkillEnvironments.mockResolvedValue(['claude']);
     mockCheckbox.mockResolvedValue(['frontend-design']);
     vi.spyOn(process, 'exit').mockImplementation((() => undefined) as any);
     vi.spyOn(process.stderr, 'write').mockImplementation((() => true) as any);
@@ -259,7 +297,7 @@ describe('skill command', () => {
     });
     expect(mockAddSkills).toHaveBeenCalledWith('anthropics/skills', ['frontend-design'], {
       global: undefined,
-      environments: undefined,
+      environments: ['claude'],
     });
     expect(mockAddSkill).not.toHaveBeenCalled();
     expect(process.stderr.write).not.toHaveBeenCalled();
@@ -273,7 +311,7 @@ describe('skill command', () => {
 
     expect(mockAddSkill).toHaveBeenCalledWith('anthropics/skills', 'frontend-design', {
       global: undefined,
-      environments: undefined,
+      environments: ['claude'],
     });
     expect(mockListInstallableSkills).not.toHaveBeenCalled();
     expect(mockAddSkills).not.toHaveBeenCalled();
@@ -318,11 +356,11 @@ describe('skill command', () => {
     expect(mockAddSkill).toHaveBeenCalledTimes(2);
     expect(mockAddSkill).toHaveBeenCalledWith('codeaholicguy/ai-devkit', 'remote-one', {
       global: undefined,
-      environments: undefined,
+      environments: ['claude'],
     });
     expect(mockAddSkill).toHaveBeenCalledWith('codeaholicguy/ai-devkit', 'remote-two', {
       global: undefined,
-      environments: undefined,
+      environments: ['claude'],
     });
     expect(mockGetBuiltinSkillNames).toHaveBeenCalledOnce();
     expect(SkillService).toHaveBeenCalledTimes(1);

packages/cli/src/__tests__/services/install/install.service.test.ts

@@ -78,7 +78,13 @@ describe('install service', () => {
     mockTemplateManager.setupMultipleEnvironments.mockResolvedValue([]);
     mockTemplateManager.copyPhaseTemplate.mockResolvedValue('docs/ai/requirements/README.md');
 
-    mockSkillService.addSkill.mockResolvedValue(undefined);
+    mockSkillService.addSkill.mockImplementation(async (registryId: string, skillName: string) => ({
+      status: 'installed',
+      registryId,
+      installMode: 'project',
+      environments: ['codex'],
+      items: [{ skillName, target: `.codex/skills/${skillName}`, action: 'symlinked' }],
+    }));
     mockConfirm.mockResolvedValue(false);
     mockIsInteractiveTerminal.mockReturnValue(true);
   });

packages/cli/src/__tests__/services/skill/index/skill-index.service.test.ts

@@ -4,7 +4,6 @@ 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";
@@ -36,15 +35,6 @@ vi.mock("../../../../lib/Config.js", () => ({
     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(),
@@ -91,9 +81,6 @@ const mockedFs = fs as Mocked<typeof fs>;
 const MockedConfigManager = ConfigManager as MockedClass<
   typeof ConfigManager
 >;
-const MockedEnvironmentSelector = EnvironmentSelector as MockedClass<
-  typeof EnvironmentSelector
->;
 const MockedGlobalConfigManager = GlobalConfigManager as MockedClass<
   typeof GlobalConfigManager
 >;
@@ -112,16 +99,13 @@ function mockFetch(response: any) {
 describe("SkillService", () => {
   let skillManager: SkillService;
   let mockConfigManager: Mocked<ConfigManager>;
-  let mockEnvironmentSelector: Mocked<EnvironmentSelector>;
   let mockGlobalConfigManager: Mocked<GlobalConfigManager>;
 
   beforeEach(() => {
     vi.clearAllMocks();
     vi.spyOn(console, "log").mockImplementation(() => { });
 
     mockConfigManager = new MockedConfigManager() as Mocked<ConfigManager>;
-    mockEnvironmentSelector =
-      new MockedEnvironmentSelector() as Mocked<EnvironmentSelector>;
     mockGlobalConfigManager =
       new MockedGlobalConfigManager() as Mocked<GlobalConfigManager>;
 
@@ -130,7 +114,6 @@ describe("SkillService", () => {
 
     skillManager = new SkillService(
       mockConfigManager,
-      mockEnvironmentSelector,
       mockGlobalConfigManager,
     );
 
@@ -177,11 +160,7 @@ describe("SkillService", () => {
         failed: 0,
         results: [],
       });
-      // UI utility outputs symbol and message as separate parameters
-      expect(console.log).toHaveBeenCalledWith(
-        expect.stringContaining("⚠"),
-        expect.stringContaining("No skills cache found"),
-      );
+      expect(console.log).not.toHaveBeenCalled();
     });
 
     it("should update all registries when no registryId provided", async () => {
@@ -336,14 +315,10 @@ describe("SkillService", () => {
       (mockedGitUtil.isGitRepository as any).mockResolvedValue(true);
       (mockedGitUtil.pullRepository as any).mockResolvedValue(undefined);
 
-      await skillManager.updateSkills();
+      const result = 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"),
-      );
+      expect(result.successful).toBe(1);
+      expect(console.log).not.toHaveBeenCalled();
     });
 
     it("should display summary after updates", async () => {
@@ -359,14 +334,14 @@ describe("SkillService", () => {
       (mockedGitUtil.isGitRepository as any).mockResolvedValue(true);
       (mockedGitUtil.pullRepository as any).mockResolvedValue(undefined);
 
-      await skillManager.updateSkills();
+      const result = await skillManager.updateSkills();
 
-      expect(console.log).toHaveBeenCalledWith(
-        expect.stringContaining("Summary:"),
-      );
-      expect(console.log).toHaveBeenCalledWith(
-        expect.stringContaining("1 updated"),
-      );
+      expect(result).toMatchObject({
+        successful: 1,
+        skipped: 0,
+        failed: 0,
+      });
+      expect(console.log).not.toHaveBeenCalled();
     });
 
     it("should handle mixed results (success, skip, error)", async () => {

packages/cli/src/__tests__/services/skill/registry/skill-registry.service.test.ts

@@ -97,8 +97,6 @@ describe('SkillRegistryService repository preparation', () => {
     await registry.prepareRegistryRepository(registryId, gitUrl);
 
     expect(mockedGit.pullRepository).toHaveBeenCalledTimes(1);
-    expect(mockUi.info).toHaveBeenCalledWith(`Refreshing registry ${registryId}...`);
-    expect(mockUi.success).toHaveBeenCalledWith(`Registry ${registryId} refreshed.`);
   });
 
   it('shares one in-flight refresh between concurrent preparations', async () => {
@@ -136,10 +134,6 @@ describe('SkillRegistryService repository preparation', () => {
     await expect(registry.prepareRegistryRepository(registryId, gitUrl)).resolves.toBe(cachedPath);
 
     expect(mockedGit.pullRepository).toHaveBeenCalledTimes(1);
-    expect(mockUi.warning).toHaveBeenCalledTimes(1);
-    expect(mockUi.warning).toHaveBeenCalledWith(
-      `Could not refresh registry ${registryId}: network down. Using cached registry contents for this run.`,
-    );
   });
 
   it('reuses a failed no-cache preparation without retrying', async () => {
@@ -169,10 +163,6 @@ describe('SkillRegistryService repository preparation', () => {
 
     expect(mockedGit.isGitRepository).toHaveBeenCalledTimes(1);
     expect(mockedGit.pullRepository).not.toHaveBeenCalled();
-    expect(mockUi.warning).toHaveBeenCalledTimes(1);
-    expect(mockUi.warning).toHaveBeenCalledWith(
-      `Cached registry ${registryId} is not a git repository, using as-is.`,
-    );
   });
 
   it('prepares the registry repository in the local cache', async () => {

packages/cli/src/__tests__/services/skill/skill-builtins.test.ts

@@ -1,19 +1,10 @@
 import { beforeEach, describe, expect, it, vi } from 'vitest';
 import { readFile } from 'node:fs/promises';
 
-const mockWarning = vi.fn();
-
-vi.mock('../../../util/terminal-ui.js', () => ({
-  ui: {
-    warning: (...args: unknown[]) => mockWarning(...args),
-  },
-}));
-
 describe('getBuiltinSkillNames', () => {
   beforeEach(() => {
     vi.resetModules();
     vi.unstubAllGlobals();
-    mockWarning.mockReset();
   });
 
   it('returns the live bare-array manifest and fetches it once per process', async () => {
@@ -31,7 +22,6 @@ describe('getBuiltinSkillNames', () => {
     expect(fetchMock).toHaveBeenCalledWith(
       'https://raw.githubusercontent.com/codeaholicguy/ai-devkit/main/skills/built-in.json'
     );
-    expect(mockWarning).not.toHaveBeenCalled();
   });
 
   it('falls back to the bundled list when the manifest cannot be fetched', async () => {
@@ -43,9 +33,6 @@ describe('getBuiltinSkillNames', () => {
     expect(names).toHaveLength(23);
     expect(names).toContain('agent-communication');
     expect(names).toContain('tdd');
-    expect(mockWarning).toHaveBeenCalledWith(
-      'Failed to load built-in skills manifest: network unavailable. Using bundled fallback.'
-    );
   });
 
   it('falls back to the bundled list for an unsuccessful response', async () => {
@@ -57,9 +44,6 @@ describe('getBuiltinSkillNames', () => {
     const { getBuiltinSkillNames } = await import('../../../services/skill/skill-builtins.js');
 
     await expect(getBuiltinSkillNames()).resolves.toHaveLength(23);
-    expect(mockWarning).toHaveBeenCalledWith(
-      'Failed to load built-in skills manifest: HTTP 404. Using bundled fallback.'
-    );
   });
 
   it('falls back to the bundled list when response JSON cannot be parsed', async () => {
@@ -73,9 +57,6 @@ describe('getBuiltinSkillNames', () => {
     const { getBuiltinSkillNames } = await import('../../../services/skill/skill-builtins.js');
 
     await expect(getBuiltinSkillNames()).resolves.toHaveLength(23);
-    expect(mockWarning).toHaveBeenCalledWith(
-      'Failed to load built-in skills manifest: Unexpected token. Using bundled fallback.'
-    );
   });
 
   it.each([
@@ -94,9 +75,6 @@ describe('getBuiltinSkillNames', () => {
     const { getBuiltinSkillNames } = await import('../../../services/skill/skill-builtins.js');
 
     await expect(getBuiltinSkillNames()).resolves.toHaveLength(23);
-    expect(mockWarning).toHaveBeenCalledWith(
-      expect.stringMatching(/^Failed to load built-in skills manifest: .+ Using bundled fallback\.$/)
-    );
   });
 });