From a26463b056dbf66d5a2744109ee7319c434ab4a0 Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Tue, 6 Jan 2026 20:11:19 -0800 Subject: [PATCH] [Skills] Foundation: Centralize management logic and feedback rendering (#15952) --- .../cli/src/commands/skills/disable.test.ts | 13 +- packages/cli/src/commands/skills/disable.ts | 15 +-- .../cli/src/commands/skills/enable.test.ts | 13 +- packages/cli/src/commands/skills/enable.ts | 15 +-- .../cli/src/ui/commands/skillsCommand.test.ts | 44 ++++++- packages/cli/src/ui/commands/skillsCommand.ts | 56 ++++---- packages/cli/src/utils/skillSettings.ts | 123 ++++++++++++++++++ packages/cli/src/utils/skillUtils.ts | 66 ++++++++++ 8 files changed, 275 insertions(+), 70 deletions(-) create mode 100644 packages/cli/src/utils/skillSettings.ts create mode 100644 packages/cli/src/utils/skillUtils.ts diff --git a/packages/cli/src/commands/skills/disable.test.ts b/packages/cli/src/commands/skills/disable.test.ts index 20850c6ecc..325a93499d 100644 --- a/packages/cli/src/commands/skills/disable.test.ts +++ b/packages/cli/src/commands/skills/disable.test.ts @@ -36,6 +36,7 @@ vi.mock('../../config/settings.js', async (importOriginal) => { return { ...actual, loadSettings: vi.fn(), + isLoadableSettingScope: vi.fn((s) => s === 'User' || s === 'Workspace'), }; }); @@ -78,7 +79,7 @@ describe('skills disable command', () => { ); expect(emitConsoleLog).toHaveBeenCalledWith( 'log', - 'Skill "skill1" successfully disabled in scope "User".', + 'Skill "skill1" disabled by adding it to the disabled list in user settings.', ); }); @@ -86,22 +87,20 @@ describe('skills disable command', () => { const mockSettings = { forScope: vi.fn().mockReturnValue({ settings: { skills: { disabled: ['skill1'] } }, + path: '/user/settings.json', }), setValue: vi.fn(), }; - mockLoadSettings.mockReturnValue( + vi.mocked(loadSettings).mockReturnValue( mockSettings as unknown as LoadedSettings, ); - await handleDisable({ - name: 'skill1', - scope: SettingScope.User as LoadableSettingScope, - }); + await handleDisable({ name: 'skill1', scope: SettingScope.User }); expect(mockSettings.setValue).not.toHaveBeenCalled(); expect(emitConsoleLog).toHaveBeenCalledWith( 'log', - 'Skill "skill1" is already disabled in scope "User".', + 'Skill "skill1" is already disabled.', ); }); }); diff --git a/packages/cli/src/commands/skills/disable.ts b/packages/cli/src/commands/skills/disable.ts index 1923d0e989..ef100c74f2 100644 --- a/packages/cli/src/commands/skills/disable.ts +++ b/packages/cli/src/commands/skills/disable.ts @@ -12,6 +12,8 @@ import { } from '../../config/settings.js'; import { debugLogger } from '@google/gemini-cli-core'; import { exitCli } from '../utils.js'; +import { disableSkill } from '../../utils/skillSettings.js'; +import { renderSkillActionFeedback } from '../../utils/skillUtils.js'; interface DisableArgs { name: string; @@ -23,17 +25,8 @@ export async function handleDisable(args: DisableArgs) { const workspaceDir = process.cwd(); const settings = loadSettings(workspaceDir); - const currentDisabled = - settings.forScope(scope).settings.skills?.disabled || []; - - if (currentDisabled.includes(name)) { - debugLogger.log(`Skill "${name}" is already disabled in scope "${scope}".`); - return; - } - - const newDisabled = [...currentDisabled, name]; - settings.setValue(scope, 'skills.disabled', newDisabled); - debugLogger.log(`Skill "${name}" successfully disabled in scope "${scope}".`); + const result = disableSkill(settings, name, scope); + debugLogger.log(renderSkillActionFeedback(result, (label, _path) => label)); } export const disableCommand: CommandModule = { diff --git a/packages/cli/src/commands/skills/enable.test.ts b/packages/cli/src/commands/skills/enable.test.ts index a720bb0ca9..25ef9def67 100644 --- a/packages/cli/src/commands/skills/enable.test.ts +++ b/packages/cli/src/commands/skills/enable.test.ts @@ -36,6 +36,7 @@ vi.mock('../../config/settings.js', async (importOriginal) => { return { ...actual, loadSettings: vi.fn(), + isLoadableSettingScope: vi.fn((s) => s === 'User' || s === 'Workspace'), }; }); @@ -78,7 +79,7 @@ describe('skills enable command', () => { ); expect(emitConsoleLog).toHaveBeenCalledWith( 'log', - 'Skill "skill1" successfully enabled in scope "User".', + 'Skill "skill1" enabled by removing it from the disabled list in user settings.', ); }); @@ -86,22 +87,20 @@ describe('skills enable command', () => { const mockSettings = { forScope: vi.fn().mockReturnValue({ settings: { skills: { disabled: [] } }, + path: '/user/settings.json', }), setValue: vi.fn(), }; - mockLoadSettings.mockReturnValue( + vi.mocked(loadSettings).mockReturnValue( mockSettings as unknown as LoadedSettings, ); - await handleEnable({ - name: 'skill1', - scope: SettingScope.User as LoadableSettingScope, - }); + await handleEnable({ name: 'skill1', scope: SettingScope.User }); expect(mockSettings.setValue).not.toHaveBeenCalled(); expect(emitConsoleLog).toHaveBeenCalledWith( 'log', - 'Skill "skill1" is already enabled in scope "User".', + 'Skill "skill1" is already enabled.', ); }); }); diff --git a/packages/cli/src/commands/skills/enable.ts b/packages/cli/src/commands/skills/enable.ts index 7b5e19d88f..b958a8352f 100644 --- a/packages/cli/src/commands/skills/enable.ts +++ b/packages/cli/src/commands/skills/enable.ts @@ -12,6 +12,8 @@ import { } from '../../config/settings.js'; import { debugLogger } from '@google/gemini-cli-core'; import { exitCli } from '../utils.js'; +import { enableSkill } from '../../utils/skillSettings.js'; +import { renderSkillActionFeedback } from '../../utils/skillUtils.js'; interface EnableArgs { name: string; @@ -23,17 +25,8 @@ export async function handleEnable(args: EnableArgs) { const workspaceDir = process.cwd(); const settings = loadSettings(workspaceDir); - const currentDisabled = - settings.forScope(scope).settings.skills?.disabled || []; - const newDisabled = currentDisabled.filter((d) => d !== name); - - if (currentDisabled.length === newDisabled.length) { - debugLogger.log(`Skill "${name}" is already enabled in scope "${scope}".`); - return; - } - - settings.setValue(scope, 'skills.disabled', newDisabled); - debugLogger.log(`Skill "${name}" successfully enabled in scope "${scope}".`); + const result = enableSkill(settings, name, scope); + debugLogger.log(renderSkillActionFeedback(result, (label, _path) => label)); } export const enableCommand: CommandModule = { diff --git a/packages/cli/src/ui/commands/skillsCommand.test.ts b/packages/cli/src/ui/commands/skillsCommand.test.ts index cba9c9ff4e..32f3f58053 100644 --- a/packages/cli/src/ui/commands/skillsCommand.test.ts +++ b/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(); + 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), ); diff --git a/packages/cli/src/ui/commands/skillsCommand.ts b/packages/cli/src/ui/commands/skillsCommand.ts index 156516e9ea..c861a404a5 100644 --- a/packages/cli/src/ui/commands/skillsCommand.ts +++ b/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(), ); diff --git a/packages/cli/src/utils/skillSettings.ts b/packages/cli/src/utils/skillSettings.ts new file mode 100644 index 0000000000..9a2c3cb5a4 --- /dev/null +++ b/packages/cli/src/utils/skillSettings.ts @@ -0,0 +1,123 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { + isLoadableSettingScope, + type SettingScope, + type LoadedSettings, +} from '../config/settings.js'; + +export interface ModifiedScope { + scope: SettingScope; + path: string; +} + +export type SkillActionStatus = 'success' | 'no-op' | 'error'; + +/** + * Metadata representing the result of a skill settings operation. + */ +export interface SkillActionResult { + status: SkillActionStatus; + skillName: string; + action: 'enable' | 'disable'; + /** Scopes where the skill's state was actually changed. */ + modifiedScopes: ModifiedScope[]; + /** Scopes where the skill was already in the desired state. */ + alreadyInStateScopes: ModifiedScope[]; + /** Error message if status is 'error'. */ + error?: string; +} + +/** + * Enables a skill by removing it from the specified disabled list. + */ +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 scopePath = settings.forScope(scope).path; + const currentScopeDisabled = + settings.forScope(scope).settings.skills?.disabled ?? []; + + if (!currentScopeDisabled.includes(skillName)) { + return { + status: 'no-op', + skillName, + action: 'enable', + modifiedScopes: [], + alreadyInStateScopes: [{ scope, path: scopePath }], + }; + } + + const newDisabled = currentScopeDisabled.filter((name) => name !== skillName); + settings.setValue(scope, 'skills.disabled', newDisabled); + + return { + status: 'success', + skillName, + action: 'enable', + modifiedScopes: [{ scope, path: scopePath }], + alreadyInStateScopes: [], + }; +} + +/** + * Disables a skill by adding it to the disabled list in the specified scope. + */ +export function disableSkill( + settings: LoadedSettings, + skillName: string, + scope: SettingScope, +): SkillActionResult { + if (!isLoadableSettingScope(scope)) { + return { + status: 'error', + skillName, + action: 'disable', + modifiedScopes: [], + alreadyInStateScopes: [], + error: `Invalid settings scope: ${scope}`, + }; + } + + const scopePath = settings.forScope(scope).path; + const currentScopeDisabled = + settings.forScope(scope).settings.skills?.disabled ?? []; + + if (currentScopeDisabled.includes(skillName)) { + return { + status: 'no-op', + skillName, + action: 'disable', + modifiedScopes: [], + alreadyInStateScopes: [{ scope, path: scopePath }], + }; + } + + const newDisabled = [...currentScopeDisabled, skillName]; + settings.setValue(scope, 'skills.disabled', newDisabled); + + return { + status: 'success', + skillName, + action: 'disable', + modifiedScopes: [{ scope, path: scopePath }], + alreadyInStateScopes: [], + }; +} diff --git a/packages/cli/src/utils/skillUtils.ts b/packages/cli/src/utils/skillUtils.ts new file mode 100644 index 0000000000..1a86e04127 --- /dev/null +++ b/packages/cli/src/utils/skillUtils.ts @@ -0,0 +1,66 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { SettingScope } from '../config/settings.js'; +import type { SkillActionResult } from './skillSettings.js'; + +/** + * Shared logic for building the core skill action message while allowing the + * caller to control how each scope and its path are rendered (e.g., bolding or + * dimming). + * + * This function ONLY returns the description of what happened. It is up to the + * caller to append any interface-specific guidance (like "Use /skills reload" + * or "Restart required"). + */ +export function renderSkillActionFeedback( + result: SkillActionResult, + formatScope: (label: string, path: string) => string, +): string { + const { skillName, action, status, error } = result; + + if (status === 'error') { + return ( + error || + `An error occurred while attempting to ${action} skill "${skillName}".` + ); + } + + if (status === 'no-op') { + return `Skill "${skillName}" is already ${action === 'enable' ? 'enabled' : 'disabled'}.`; + } + + const isEnable = action === 'enable'; + const actionVerb = isEnable ? 'enabled' : 'disabled'; + const preposition = isEnable + ? 'by removing it from the disabled list in' + : 'by adding it to the disabled list in'; + + const formatScopeItem = (s: { scope: SettingScope; path: string }) => { + const label = + s.scope === SettingScope.Workspace ? 'project' : s.scope.toLowerCase(); + return formatScope(label, s.path); + }; + + const totalAffectedScopes = [ + ...result.modifiedScopes, + ...result.alreadyInStateScopes, + ]; + + if (totalAffectedScopes.length === 2) { + const s1 = formatScopeItem(totalAffectedScopes[0]); + const s2 = formatScopeItem(totalAffectedScopes[1]); + + if (isEnable) { + return `Skill "${skillName}" ${actionVerb} ${preposition} ${s1} and ${s2} settings.`; + } else { + return `Skill "${skillName}" is now disabled in both ${s1} and ${s2} settings.`; + } + } + + const s = formatScopeItem(totalAffectedScopes[0]); + return `Skill "${skillName}" ${actionVerb} ${preposition} ${s} settings.`; +}