[Skills] Multi-scope skill enablement and shadowing fix (#15953)

packages/cli/src/commands/skills/enable.test.ts

@@ -11,7 +11,6 @@ import {
   loadSettings,
   SettingScope,
   type LoadedSettings,
-  type LoadableSettingScope,
 } from '../../config/settings.js';
 
 const emitConsoleLog = vi.hoisted(() => vi.fn());
@@ -58,8 +57,14 @@ describe('skills enable command', () => {
   describe('handleEnable', () => {
     it('should enable a disabled skill in user scope', async () => {
       const mockSettings = {
-        forScope: vi.fn().mockReturnValue({
-          settings: { skills: { disabled: ['skill1'] } },
+        forScope: vi.fn().mockImplementation((scope) => {
+          if (scope === SettingScope.User) {
+            return {
+              settings: { skills: { disabled: ['skill1'] } },
+              path: '/user/settings.json',
+            };
+          }
+          return { settings: {}, path: '/project/settings.json' };
         }),
         setValue: vi.fn(),
       };
@@ -67,10 +72,7 @@ describe('skills enable command', () => {
         mockSettings as unknown as LoadedSettings,
       );
 
-      await handleEnable({
-        name: 'skill1',
-        scope: SettingScope.User as LoadableSettingScope,
-      });
+      await handleEnable({ name: 'skill1' });
 
       expect(mockSettings.setValue).toHaveBeenCalledWith(
         SettingScope.User,
@@ -79,7 +81,48 @@ describe('skills enable command', () => {
       );
       expect(emitConsoleLog).toHaveBeenCalledWith(
         'log',
-        'Skill "skill1" enabled by removing it from the disabled list in user settings.',
+        'Skill "skill1" enabled by removing it from the disabled list in user and project settings.',
+      );
+    });
+
+    it('should enable a skill across multiple scopes', async () => {
+      const mockSettings = {
+        forScope: vi.fn().mockImplementation((scope) => {
+          if (scope === SettingScope.User) {
+            return {
+              settings: { skills: { disabled: ['skill1'] } },
+              path: '/user/settings.json',
+            };
+          }
+          if (scope === SettingScope.Workspace) {
+            return {
+              settings: { skills: { disabled: ['skill1'] } },
+              path: '/workspace/settings.json',
+            };
+          }
+          return { settings: {}, path: '' };
+        }),
+        setValue: vi.fn(),
+      };
+      mockLoadSettings.mockReturnValue(
+        mockSettings as unknown as LoadedSettings,
+      );
+
+      await handleEnable({ name: 'skill1' });
+
+      expect(mockSettings.setValue).toHaveBeenCalledWith(
+        SettingScope.User,
+        'skills.disabled',
+        [],
+      );
+      expect(mockSettings.setValue).toHaveBeenCalledWith(
+        SettingScope.Workspace,
+        'skills.disabled',
+        [],
+      );
+      expect(emitConsoleLog).toHaveBeenCalledWith(
+        'log',
+        'Skill "skill1" enabled by removing it from the disabled list in project and user settings.',
       );
     });
 
@@ -91,11 +134,11 @@ describe('skills enable command', () => {
         }),
         setValue: vi.fn(),
       };
-      vi.mocked(loadSettings).mockReturnValue(
+      mockLoadSettings.mockReturnValue(
         mockSettings as unknown as LoadedSettings,
       );
 
-      await handleEnable({ name: 'skill1', scope: SettingScope.User });
+      await handleEnable({ name: 'skill1' });
 
       expect(mockSettings.setValue).not.toHaveBeenCalled();
       expect(emitConsoleLog).toHaveBeenCalledWith(

packages/cli/src/commands/skills/enable.ts

@@ -5,11 +5,7 @@
  */
 
 import type { CommandModule } from 'yargs';
-import {
-  loadSettings,
-  SettingScope,
-  type LoadableSettingScope,
-} from '../../config/settings.js';
+import { loadSettings } from '../../config/settings.js';
 import { debugLogger } from '@google/gemini-cli-core';
 import { exitCli } from '../utils.js';
 import { enableSkill } from '../../utils/skillSettings.js';
@@ -17,15 +13,14 @@ import { renderSkillActionFeedback } from '../../utils/skillUtils.js';
 
 interface EnableArgs {
   name: string;
-  scope: LoadableSettingScope;
 }
 
 export async function handleEnable(args: EnableArgs) {
-  const { name, scope } = args;
+  const { name } = args;
   const workspaceDir = process.cwd();
   const settings = loadSettings(workspaceDir);
 
-  const result = enableSkill(settings, name, scope);
+  const result = enableSkill(settings, name);
   debugLogger.log(renderSkillActionFeedback(result, (label, _path) => label));
 }
 
@@ -33,25 +28,14 @@ export const enableCommand: CommandModule = {
   command: 'enable <name>',
   describe: 'Enables an agent skill.',
   builder: (yargs) =>
-    yargs
-      .positional('name', {
-        describe: 'The name of the skill to enable.',
-        type: 'string',
-        demandOption: true,
-      })
-      .option('scope', {
-        alias: 's',
-        describe: 'The scope to enable the skill in (user or project).',
-        type: 'string',
-        default: 'user',
-        choices: ['user', 'project'],
-      }),
+    yargs.positional('name', {
+      describe: 'The name of the skill to enable.',
+      type: 'string',
+      demandOption: true,
+    }),
   handler: async (argv) => {
-    const scope: LoadableSettingScope =
-      argv['scope'] === 'project' ? SettingScope.Workspace : SettingScope.User;
     await handleEnable({
       name: argv['name'] as string,
-      scope,
     });
     await exitCli();
   },

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

@@ -193,7 +193,6 @@ 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[] } } };
@@ -212,7 +211,47 @@ describe('skillsCommand', () => {
       expect(context.ui.addItem).toHaveBeenCalledWith(
         expect.objectContaining({
           type: MessageType.INFO,
-          text: 'Skill "skill1" enabled by removing it from the disabled list in project settings. Use "/skills reload" for it to take effect.',
+          text: 'Skill "skill1" enabled by removing it from the disabled list in project and user settings. Use "/skills reload" for it to take effect.',
+        }),
+        expect.any(Number),
+      );
+    });
+
+    it('should enable a skill across multiple scopes', async () => {
+      const enableCmd = skillsCommand.subCommands!.find(
+        (s) => s.name === 'enable',
+      )!;
+      (
+        context.services.settings as unknown as {
+          user: { settings: { skills: { disabled: string[] } } };
+        }
+      ).user.settings = {
+        skills: { disabled: ['skill1'] },
+      };
+      (
+        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(
+        SettingScope.User,
+        'skills.disabled',
+        [],
+      );
+      expect(context.services.settings.setValue).toHaveBeenCalledWith(
+        SettingScope.Workspace,
+        'skills.disabled',
+        [],
+      );
+      expect(context.ui.addItem).toHaveBeenCalledWith(
+        expect.objectContaining({
+          type: MessageType.INFO,
+          text: 'Skill "skill1" enabled by removing it from the disabled list in project and user settings. Use "/skills reload" for it to take effect.',
         }),
         expect.any(Number),
       );

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

@@ -127,11 +127,7 @@ async function enableAction(
     return;
   }
 
-  const scope = context.services.settings.workspace.path
-    ? SettingScope.Workspace
-    : SettingScope.User;
-
-  const result = enableSkill(context.services.settings, skillName, scope);
+  const result = enableSkill(context.services.settings, skillName);
 
   let feedback = renderSkillActionFeedback(
     result,

packages/cli/src/utils/skillSettings.ts

@@ -5,8 +5,8 @@
  */
 
 import {
+  SettingScope,
   isLoadableSettingScope,
-  type SettingScope,
   type LoadedSettings,
 } from '../config/settings.js';
 
@@ -33,47 +33,57 @@ export interface SkillActionResult {
 }
 
 /**
- * Enables a skill by removing it from the specified disabled list.
+ * Enables a skill by removing it from all writable disabled lists (User and Workspace).
  */
 export function enableSkill(
   settings: LoadedSettings,
   skillName: string,
-  scope: SettingScope,
 ): SkillActionResult {
-  if (!isLoadableSettingScope(scope)) {
-    return {
-      status: 'error',
-      skillName,
-      action: 'enable',
-      modifiedScopes: [],
-      alreadyInStateScopes: [],
-      error: `Invalid settings scope: ${scope}`,
-    };
+  const writableScopes = [SettingScope.Workspace, SettingScope.User];
+  const foundInDisabledScopes: ModifiedScope[] = [];
+  const alreadyEnabledScopes: ModifiedScope[] = [];
+
+  for (const scope of writableScopes) {
+    if (isLoadableSettingScope(scope)) {
+      const scopePath = settings.forScope(scope).path;
+      const scopeDisabled = settings.forScope(scope).settings.skills?.disabled;
+      if (scopeDisabled?.includes(skillName)) {
+        foundInDisabledScopes.push({ scope, path: scopePath });
+      } else {
+        alreadyEnabledScopes.push({ scope, path: scopePath });
+      }
+    }
   }
 
-  const scopePath = settings.forScope(scope).path;
-  const currentScopeDisabled =
-    settings.forScope(scope).settings.skills?.disabled ?? [];
-
-  if (!currentScopeDisabled.includes(skillName)) {
+  if (foundInDisabledScopes.length === 0) {
     return {
       status: 'no-op',
       skillName,
       action: 'enable',
       modifiedScopes: [],
-      alreadyInStateScopes: [{ scope, path: scopePath }],
+      alreadyInStateScopes: alreadyEnabledScopes,
     };
   }
 
-  const newDisabled = currentScopeDisabled.filter((name) => name !== skillName);
-  settings.setValue(scope, 'skills.disabled', newDisabled);
+  const modifiedScopes: ModifiedScope[] = [];
+  for (const { scope, path } of foundInDisabledScopes) {
+    if (isLoadableSettingScope(scope)) {
+      const currentScopeDisabled =
+        settings.forScope(scope).settings.skills?.disabled ?? [];
+      const newDisabled = currentScopeDisabled.filter(
+        (name) => name !== skillName,
+      );
+      settings.setValue(scope, 'skills.disabled', newDisabled);
+      modifiedScopes.push({ scope, path });
+    }
+  }
 
   return {
     status: 'success',
     skillName,
     action: 'enable',
-    modifiedScopes: [{ scope, path: scopePath }],
-    alreadyInStateScopes: [],
+    modifiedScopes,
+    alreadyInStateScopes: alreadyEnabledScopes,
   };
 }
 
@@ -110,6 +120,24 @@ export function disableSkill(
     };
   }
 
+  // Check if it's already disabled in the other writable scope
+  const otherScope =
+    scope === SettingScope.Workspace
+      ? SettingScope.User
+      : SettingScope.Workspace;
+  const alreadyDisabledInOther: ModifiedScope[] = [];
+
+  if (isLoadableSettingScope(otherScope)) {
+    const otherScopeDisabled =
+      settings.forScope(otherScope).settings.skills?.disabled;
+    if (otherScopeDisabled?.includes(skillName)) {
+      alreadyDisabledInOther.push({
+        scope: otherScope,
+        path: settings.forScope(otherScope).path,
+      });
+    }
+  }
+
   const newDisabled = [...currentScopeDisabled, skillName];
   settings.setValue(scope, 'skills.disabled', newDisabled);
 
@@ -118,6 +146,6 @@ export function disableSkill(
     skillName,
     action: 'disable',
     modifiedScopes: [{ scope, path: scopePath }],
-    alreadyInStateScopes: [],
+    alreadyInStateScopes: alreadyDisabledInOther,
   };
 }