[Skills] Foundation: Centralize management logic and feedback rendering (#15952)

packages/cli/src/ui/commands/skillsCommand.test.ts

@@ -12,6 +12,15 @@ import type { CommandContext } from './types.js';
 import type { Config, SkillDefinition } from '@google/gemini-cli-core';
 import { SettingScope, type LoadedSettings } from '../../config/settings.js';
 
+vi.mock('../../config/settings.js', async (importOriginal) => {
+  const actual =
+    await importOriginal<typeof import('../../config/settings.js')>();
+  return {
+    ...actual,
+    isLoadableSettingScope: vi.fn((s) => s === 'User' || s === 'Workspace'),
+  };
+});
+
 describe('skillsCommand', () => {
   let context: CommandContext;
 
@@ -135,6 +144,28 @@ describe('skillsCommand', () => {
       ).workspace = {
         path: '/workspace',
       };
+
+      interface MockSettings {
+        user: { settings: unknown; path: string };
+        workspace: { settings: unknown; path: string };
+        forScope: unknown;
+      }
+
+      const settings = context.services.settings as unknown as MockSettings;
+
+      settings.forScope = vi.fn((scope) => {
+        if (scope === SettingScope.User) return settings.user;
+        if (scope === SettingScope.Workspace) return settings.workspace;
+        return { settings: {}, path: '' };
+      });
+      settings.user = {
+        settings: {},
+        path: '/user/settings.json',
+      };
+      settings.workspace = {
+        settings: {},
+        path: '/workspace',
+      };
     });
 
     it('should disable a skill', async () => {
@@ -151,7 +182,7 @@ describe('skillsCommand', () => {
       expect(context.ui.addItem).toHaveBeenCalledWith(
         expect.objectContaining({
           type: MessageType.INFO,
-          text: expect.stringContaining('Skill "skill1" disabled'),
+          text: 'Skill "skill1" disabled by adding it to the disabled list in project settings. Use "/skills reload" for it to take effect.',
         }),
         expect.any(Number),
       );
@@ -162,6 +193,15 @@ describe('skillsCommand', () => {
         (s) => s.name === 'enable',
       )!;
       context.services.settings.merged.skills = { disabled: ['skill1'] };
+      // Also need to mock the scope-specific disabled list
+      (
+        context.services.settings as unknown as {
+          workspace: { settings: { skills: { disabled: string[] } } };
+        }
+      ).workspace.settings = {
+        skills: { disabled: ['skill1'] },
+      };
+
       await enableCmd.action!(context, 'skill1');
 
       expect(context.services.settings.setValue).toHaveBeenCalledWith(
@@ -172,7 +212,7 @@ describe('skillsCommand', () => {
       expect(context.ui.addItem).toHaveBeenCalledWith(
         expect.objectContaining({
           type: MessageType.INFO,
-          text: expect.stringContaining('Skill "skill1" enabled'),
+          text: 'Skill "skill1" enabled by removing it from the disabled list in project settings. Use "/skills reload" for it to take effect.',
         }),
         expect.any(Number),
       );

packages/cli/src/ui/commands/skillsCommand.ts

@@ -16,6 +16,8 @@ import {
   type HistoryItemInfo,
 } from '../types.js';
 import { SettingScope } from '../../config/settings.js';
+import { enableSkill, disableSkill } from '../../utils/skillSettings.js';
+import { renderSkillActionFeedback } from '../../utils/skillUtils.js';
 
 async function listAction(
   context: CommandContext,
@@ -86,29 +88,24 @@ async function disableAction(
     return;
   }
 
-  const currentDisabled =
-    context.services.settings.merged.skills?.disabled ?? [];
-  if (currentDisabled.includes(skillName)) {
-    context.ui.addItem(
-      {
-        type: MessageType.INFO,
-        text: `Skill "${skillName}" is already disabled.`,
-      },
-      Date.now(),
-    );
-    return;
-  }
-
-  const newDisabled = [...currentDisabled, skillName];
   const scope = context.services.settings.workspace.path
     ? SettingScope.Workspace
     : SettingScope.User;
 
-  context.services.settings.setValue(scope, 'skills.disabled', newDisabled);
+  const result = disableSkill(context.services.settings, skillName, scope);
+
+  let feedback = renderSkillActionFeedback(
+    result,
+    (label, _path) => `${label}`,
+  );
+  if (result.status === 'success') {
+    feedback += ' Use "/skills reload" for it to take effect.';
+  }
+
   context.ui.addItem(
     {
       type: MessageType.INFO,
-      text: `Skill "${skillName}" disabled in ${scope} settings. Use "/skills reload" for it to take effect.`,
+      text: feedback,
     },
     Date.now(),
   );
@@ -130,29 +127,24 @@ async function enableAction(
     return;
   }
 
-  const currentDisabled =
-    context.services.settings.merged.skills?.disabled ?? [];
-  if (!currentDisabled.includes(skillName)) {
-    context.ui.addItem(
-      {
-        type: MessageType.INFO,
-        text: `Skill "${skillName}" is not disabled.`,
-      },
-      Date.now(),
-    );
-    return;
-  }
-
-  const newDisabled = currentDisabled.filter((name) => name !== skillName);
   const scope = context.services.settings.workspace.path
     ? SettingScope.Workspace
     : SettingScope.User;
 
-  context.services.settings.setValue(scope, 'skills.disabled', newDisabled);
+  const result = enableSkill(context.services.settings, skillName, scope);
+
+  let feedback = renderSkillActionFeedback(
+    result,
+    (label, _path) => `${label}`,
+  );
+  if (result.status === 'success') {
+    feedback += ' Use "/skills reload" for it to take effect.';
+  }
+
   context.ui.addItem(
     {
       type: MessageType.INFO,
-      text: `Skill "${skillName}" enabled in ${scope} settings. Use "/skills reload" for it to take effect.`,
+      text: feedback,
     },
     Date.now(),
   );