From 2d683bb6f8a1fd9348fb0c72230981a46ed16d07 Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Tue, 6 Jan 2026 20:24:29 -0800 Subject: [PATCH] [Skills] Multi-scope skill enablement and shadowing fix (#15953) --- .../cli/src/commands/skills/enable.test.ts | 63 +++++++++++++--- packages/cli/src/commands/skills/enable.ts | 32 ++------ .../cli/src/ui/commands/skillsCommand.test.ts | 43 ++++++++++- packages/cli/src/ui/commands/skillsCommand.ts | 6 +- packages/cli/src/utils/skillSettings.ts | 74 +++++++++++++------ 5 files changed, 154 insertions(+), 64 deletions(-) diff --git a/packages/cli/src/commands/skills/enable.test.ts b/packages/cli/src/commands/skills/enable.test.ts index 25ef9def67..35c7d5b0f5 100644 --- a/packages/cli/src/commands/skills/enable.test.ts +++ b/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( diff --git a/packages/cli/src/commands/skills/enable.ts b/packages/cli/src/commands/skills/enable.ts index b958a8352f..8bfcaa7c2b 100644 --- a/packages/cli/src/commands/skills/enable.ts +++ b/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 ', 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(); }, diff --git a/packages/cli/src/ui/commands/skillsCommand.test.ts b/packages/cli/src/ui/commands/skillsCommand.test.ts index 32f3f58053..511effd0b6 100644 --- a/packages/cli/src/ui/commands/skillsCommand.test.ts +++ b/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), ); diff --git a/packages/cli/src/ui/commands/skillsCommand.ts b/packages/cli/src/ui/commands/skillsCommand.ts index c861a404a5..35a41f461b 100644 --- a/packages/cli/src/ui/commands/skillsCommand.ts +++ b/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, diff --git a/packages/cli/src/utils/skillSettings.ts b/packages/cli/src/utils/skillSettings.ts index 9a2c3cb5a4..78921b7219 100644 --- a/packages/cli/src/utils/skillSettings.ts +++ b/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, }; }