diff --git a/packages/cli/src/ui/commands/hooksCommand.test.ts b/packages/cli/src/ui/commands/hooksCommand.test.ts index 76a204780a..05009513a9 100644 --- a/packages/cli/src/ui/commands/hooksCommand.test.ts +++ b/packages/cli/src/ui/commands/hooksCommand.test.ts @@ -11,6 +11,7 @@ import { MessageType } from '../types.js'; import type { HookRegistryEntry } from '@google/gemini-cli-core'; import { HookType, HookEventName, ConfigSource } from '@google/gemini-cli-core'; import type { CommandContext } from './types.js'; +import { SettingScope } from '../../config/settings.js'; describe('hooksCommand', () => { let mockContext: CommandContext; @@ -34,6 +35,11 @@ describe('hooksCommand', () => { }; }; setValue: ReturnType; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + workspace: { path: string; settings: any }; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + user: { path: string; settings: any }; + forScope: ReturnType; }; beforeEach(() => { @@ -56,6 +62,17 @@ describe('hooksCommand', () => { }; // Create mock settings + const mockUser = { + path: '/mock/user.json', + settings: { hooksConfig: { disabled: [] } }, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any; + const mockWorkspace = { + path: '/mock/workspace.json', + settings: { hooksConfig: { disabled: [] } }, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any; + mockSettings = { merged: { hooksConfig: { @@ -63,7 +80,15 @@ describe('hooksCommand', () => { }, }, setValue: vi.fn(), - }; + workspace: mockWorkspace, + user: mockUser, + forScope: vi.fn((scope) => { + if (scope === SettingScope.User) return mockUser; + if (scope === SettingScope.Workspace) return mockWorkspace; + return mockUser; + }), + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any; // Create mock context with config and settings mockContext = createMockCommandContext({ @@ -272,11 +297,12 @@ describe('hooksCommand', () => { }); it('should enable a hook and update settings', async () => { - // Update the context's settings with disabled hooks - mockContext.services.settings.merged.hooksConfig.disabled = [ + // Update the user settings with disabled hooks + mockSettings.user.settings.hooksConfig.disabled = [ 'test-hook', 'other-hook', ]; + mockSettings.workspace.settings.hooksConfig.disabled = []; const enableCmd = hooksCommand.subCommands!.find( (cmd) => cmd.name === 'enable', @@ -288,7 +314,7 @@ describe('hooksCommand', () => { const result = await enableCmd.action(mockContext, 'test-hook'); expect(mockContext.services.settings.setValue).toHaveBeenCalledWith( - expect.any(String), + SettingScope.User, 'hooksConfig.disabled', ['other-hook'], ); @@ -299,28 +325,8 @@ describe('hooksCommand', () => { expect(result).toEqual({ type: 'message', messageType: 'info', - content: 'Hook "test-hook" enabled successfully.', - }); - }); - - it('should handle error when enabling hook fails', async () => { - mockSettings.setValue.mockImplementationOnce(() => { - throw new Error('Failed to save settings'); - }); - - const enableCmd = hooksCommand.subCommands!.find( - (cmd) => cmd.name === 'enable', - ); - if (!enableCmd?.action) { - throw new Error('enable command must have an action'); - } - - const result = await enableCmd.action(mockContext, 'test-hook'); - - expect(result).toEqual({ - type: 'message', - messageType: 'error', - content: 'Failed to enable hook: Failed to save settings', + content: + 'Hook "test-hook" enabled by removing it from the disabled list in user (/mock/user.json) and workspace (/mock/workspace.json) settings.', }); }); @@ -332,7 +338,7 @@ describe('hooksCommand', () => { const hookEntry = createMockHook( './hooks/test.sh', HookEventName.BeforeTool, - true, + false, // Must be disabled for enable completion ); hookEntry.config.name = 'friendly-name'; @@ -404,7 +410,9 @@ describe('hooksCommand', () => { }); it('should disable a hook and update settings', async () => { - mockContext.services.settings.merged.hooksConfig.disabled = []; + // Ensure not disabled anywhere + mockSettings.workspace.settings.hooksConfig.disabled = []; + mockSettings.user.settings.hooksConfig.disabled = []; const disableCmd = hooksCommand.subCommands!.find( (cmd) => cmd.name === 'disable', @@ -415,8 +423,9 @@ describe('hooksCommand', () => { const result = await disableCmd.action(mockContext, 'test-hook'); + // Should default to workspace if present expect(mockContext.services.settings.setValue).toHaveBeenCalledWith( - expect.any(String), + SettingScope.Workspace, 'hooksConfig.disabled', ['test-hook'], ); @@ -427,13 +436,14 @@ describe('hooksCommand', () => { expect(result).toEqual({ type: 'message', messageType: 'info', - content: 'Hook "test-hook" disabled successfully.', + content: + 'Hook "test-hook" disabled by adding it to the disabled list in workspace (/mock/workspace.json) settings.', }); }); - it('should synchronize with hook system even if hook is already in disabled list', async () => { - // Update the context's settings with the hook already disabled - mockContext.services.settings.merged.hooksConfig.disabled = ['test-hook']; + it('should return info when hook is already disabled', async () => { + // Update the context's settings with the hook already disabled in Workspace + mockSettings.workspace.settings.hooksConfig.disabled = ['test-hook']; const disableCmd = hooksCommand.subCommands!.find( (cmd) => cmd.name === 'disable', @@ -445,38 +455,29 @@ describe('hooksCommand', () => { const result = await disableCmd.action(mockContext, 'test-hook'); expect(mockContext.services.settings.setValue).not.toHaveBeenCalled(); - expect(mockHookSystem.setHookEnabled).toHaveBeenCalledWith( - 'test-hook', - false, - ); - expect(mockConfig.updateDisabledHooks).toHaveBeenCalled(); expect(result).toEqual({ type: 'message', messageType: 'info', - content: 'Hook "test-hook" disabled successfully.', + content: 'Hook "test-hook" is already disabled.', }); }); - it('should handle error when disabling hook fails', async () => { - mockContext.services.settings.merged.hooksConfig.disabled = []; - mockSettings.setValue.mockImplementationOnce(() => { - throw new Error('Failed to save settings'); - }); - + it('should complete hook names using friendly names', () => { const disableCmd = hooksCommand.subCommands!.find( (cmd) => cmd.name === 'disable', + )!; + + const hookEntry = createMockHook( + './hooks/test.sh', + HookEventName.BeforeTool, + true, // Must be enabled for disable completion ); - if (!disableCmd?.action) { - throw new Error('disable command must have an action'); - } + hookEntry.config.name = 'friendly-name'; - const result = await disableCmd.action(mockContext, 'test-hook'); + mockHookSystem.getAllHooks.mockReturnValue([hookEntry]); - expect(result).toEqual({ - type: 'message', - messageType: 'error', - content: 'Failed to disable hook: Failed to save settings', - }); + const completions = disableCmd.completion!(mockContext, 'frie'); + expect(completions).toContain('friendly-name'); }); }); @@ -513,50 +514,52 @@ describe('hooksCommand', () => { expect(result).toEqual([]); }); - it('should return matching hook names', () => { + it('should return matching hook names based on status', () => { const mockHooks: HookRegistryEntry[] = [ - createMockHook('test-hook-1', HookEventName.BeforeTool, true), - createMockHook('test-hook-2', HookEventName.AfterTool, true), - createMockHook('other-hook', HookEventName.AfterAgent, false), + createMockHook('test-hook-enabled', HookEventName.BeforeTool, true), + createMockHook('test-hook-disabled', HookEventName.AfterTool, false), ]; mockHookSystem.getAllHooks.mockReturnValue(mockHooks); const enableCmd = hooksCommand.subCommands!.find( (cmd) => cmd.name === 'enable', - ); - if (!enableCmd?.completion) { - throw new Error('enable command must have completion'); - } + )!; + const disableCmd = hooksCommand.subCommands!.find( + (cmd) => cmd.name === 'disable', + )!; - const result = enableCmd.completion(mockContext, 'test'); - expect(result).toEqual(['test-hook-1', 'test-hook-2']); + const enableResult = enableCmd.completion!(mockContext, 'test'); + expect(enableResult).toEqual(['test-hook-disabled']); + + const disableResult = disableCmd.completion!(mockContext, 'test'); + expect(disableResult).toEqual(['test-hook-enabled']); }); - it('should return all hook names when partial is empty', () => { + it('should return all relevant hook names when partial is empty', () => { const mockHooks: HookRegistryEntry[] = [ - createMockHook('hook-1', HookEventName.BeforeTool, true), - createMockHook('hook-2', HookEventName.AfterTool, true), + createMockHook('hook-enabled', HookEventName.BeforeTool, true), + createMockHook('hook-disabled', HookEventName.AfterTool, false), ]; mockHookSystem.getAllHooks.mockReturnValue(mockHooks); const enableCmd = hooksCommand.subCommands!.find( (cmd) => cmd.name === 'enable', - ); - if (!enableCmd?.completion) { - throw new Error('enable command must have completion'); - } + )!; + const disableCmd = hooksCommand.subCommands!.find( + (cmd) => cmd.name === 'disable', + )!; - const result = enableCmd.completion(mockContext, ''); - expect(result).toEqual(['hook-1', 'hook-2']); + expect(enableCmd.completion!(mockContext, '')).toEqual(['hook-disabled']); + expect(disableCmd.completion!(mockContext, '')).toEqual(['hook-enabled']); }); it('should handle hooks without command name gracefully', () => { const mockHooks: HookRegistryEntry[] = [ - createMockHook('test-hook', HookEventName.BeforeTool, true), + createMockHook('test-hook', HookEventName.BeforeTool, false), { - ...createMockHook('', HookEventName.AfterTool, true), + ...createMockHook('', HookEventName.AfterTool, false), config: { command: '', type: HookType.Command, timeout: 30 }, }, ]; @@ -636,7 +639,7 @@ describe('hooksCommand', () => { const result = await enableAllCmd.action(mockContext, ''); expect(mockContext.services.settings.setValue).toHaveBeenCalledWith( - expect.any(String), + expect.any(String), // enableAll uses legacy logic so it might return 'Workspace' or 'User' depending on ternary 'hooksConfig.disabled', [], ); diff --git a/packages/cli/src/ui/commands/hooksCommand.ts b/packages/cli/src/ui/commands/hooksCommand.ts index 7e4221ebfa..92fa72b235 100644 --- a/packages/cli/src/ui/commands/hooksCommand.ts +++ b/packages/cli/src/ui/commands/hooksCommand.ts @@ -12,7 +12,9 @@ import type { MessageActionReturn, } from '@google/gemini-cli-core'; import { getErrorMessage } from '@google/gemini-cli-core'; -import { SettingScope } from '../../config/settings.js'; +import { SettingScope, isLoadableSettingScope } from '../../config/settings.js'; +import { enableHook, disableHook } from '../../utils/hookSettings.js'; +import { renderHookActionFeedback } from '../../utils/hookUtils.js'; /** * Display a formatted list of hooks with their status @@ -74,39 +76,23 @@ async function enableAction( }; } - // Get current disabled hooks from settings const settings = context.services.settings; - const disabledHooks = settings.merged.hooksConfig.disabled; - // Remove from disabled list if present - const newDisabledHooks = disabledHooks.filter( - (name: string) => name !== hookName, + const result = enableHook(settings, hookName); + + if (result.status === 'success') { + hookSystem.setHookEnabled(hookName, true); + } + + const feedback = renderHookActionFeedback( + result, + (label, path) => `${label} (${path})`, ); - // Update settings (setValue automatically saves) - try { - const scope = settings.workspace - ? SettingScope.Workspace - : SettingScope.User; - settings.setValue(scope, 'hooksConfig.disabled', newDisabledHooks); - - // Update core config so re-initialization (e.g. extension reload) respects the change - config.updateDisabledHooks(settings.merged.hooksConfig.disabled); - - // Enable in hook system - hookSystem.setHookEnabled(hookName, true); - - return { - type: 'message', - messageType: 'info', - content: `Hook "${hookName}" enabled successfully.`, - }; - } catch (error) { - return { - type: 'message', - messageType: 'error', - content: `Failed to enable hook: ${getErrorMessage(error)}`, - }; - } + return { + type: 'message', + messageType: result.status === 'error' ? 'error' : 'info', + content: feedback, + }; } /** @@ -143,44 +129,31 @@ async function disableAction( }; } - // Get current disabled hooks from settings const settings = context.services.settings; - const disabledHooks = settings.merged.hooksConfig.disabled; - // Add to disabled list if not already present - try { - if (!disabledHooks.includes(hookName)) { - const newDisabledHooks = [...disabledHooks, hookName]; + const scope = settings.workspace ? SettingScope.Workspace : SettingScope.User; - const scope = settings.workspace - ? SettingScope.Workspace - : SettingScope.User; - settings.setValue(scope, 'hooksConfig.disabled', newDisabledHooks); - } + const result = disableHook(settings, hookName, scope); - // Update core config so re-initialization (e.g. extension reload) respects the change - config.updateDisabledHooks(settings.merged.hooksConfig.disabled); - - // Always disable in hook system to ensure in-memory state matches settings + if (result.status === 'success') { hookSystem.setHookEnabled(hookName, false); - - return { - type: 'message', - messageType: 'info', - content: `Hook "${hookName}" disabled successfully.`, - }; - } catch (error) { - return { - type: 'message', - messageType: 'error', - content: `Failed to disable hook: ${getErrorMessage(error)}`, - }; } + + const feedback = renderHookActionFeedback( + result, + (label, path) => `${label} (${path})`, + ); + + return { + type: 'message', + messageType: result.status === 'error' ? 'error' : 'info', + content: feedback, + }; } /** - * Completion function for hook names + * Completion function for enabled hook names (to be disabled) */ -function completeHookNames( +function completeEnabledHookNames( context: CommandContext, partialArg: string, ): string[] { @@ -191,8 +164,30 @@ function completeHookNames( if (!hookSystem) return []; const allHooks = hookSystem.getAllHooks(); - const hookNames = allHooks.map((hook) => getHookDisplayName(hook)); - return hookNames.filter((name) => name.startsWith(partialArg)); + return allHooks + .filter((hook) => hook.enabled) + .map((hook) => getHookDisplayName(hook)) + .filter((name) => name.startsWith(partialArg)); +} + +/** + * Completion function for disabled hook names (to be enabled) + */ +function completeDisabledHookNames( + context: CommandContext, + partialArg: string, +): string[] { + const { config } = context.services; + if (!config) return []; + + const hookSystem = config.getHookSystem(); + if (!hookSystem) return []; + + const allHooks = hookSystem.getAllHooks(); + return allHooks + .filter((hook) => !hook.enabled) + .map((hook) => getHookDisplayName(hook)) + .filter((name) => name.startsWith(partialArg)); } /** @@ -247,13 +242,12 @@ async function enableAllAction( } try { - const scope = settings.workspace - ? SettingScope.Workspace - : SettingScope.User; - settings.setValue(scope, 'hooksConfig.disabled', []); - - // Update core config so re-initialization (e.g. extension reload) respects the change - config.updateDisabledHooks(settings.merged.hooksConfig.disabled); + const scopes = [SettingScope.Workspace, SettingScope.User]; + for (const scope of scopes) { + if (isLoadableSettingScope(scope)) { + settings.setValue(scope, 'hooksConfig.disabled', []); + } + } for (const hook of disabledHooks) { const hookName = getHookDisplayName(hook); @@ -325,9 +319,6 @@ async function disableAllAction( : SettingScope.User; settings.setValue(scope, 'hooksConfig.disabled', allHookNames); - // Update core config so re-initialization (e.g. extension reload) respects the change - config.updateDisabledHooks(settings.merged.hooksConfig.disabled); - for (const hook of enabledHooks) { const hookName = getHookDisplayName(hook); hookSystem.setHookEnabled(hookName, false); @@ -361,7 +352,7 @@ const enableCommand: SlashCommand = { kind: CommandKind.BUILT_IN, autoExecute: true, action: enableAction, - completion: completeHookNames, + completion: completeDisabledHookNames, }; const disableCommand: SlashCommand = { @@ -370,7 +361,7 @@ const disableCommand: SlashCommand = { kind: CommandKind.BUILT_IN, autoExecute: true, action: disableAction, - completion: completeHookNames, + completion: completeEnabledHookNames, }; const enableAllCommand: SlashCommand = { diff --git a/packages/cli/src/utils/hookSettings.test.ts b/packages/cli/src/utils/hookSettings.test.ts new file mode 100644 index 0000000000..9de8adb051 --- /dev/null +++ b/packages/cli/src/utils/hookSettings.test.ts @@ -0,0 +1,182 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { enableHook, disableHook } from './hookSettings.js'; +import { SettingScope, type LoadedSettings } from '../config/settings.js'; + +describe('hookSettings', () => { + let mockSettings: LoadedSettings; + let mockUser: { + path: string; + settings: { hooksConfig: { disabled: string[] } }; + }; + let mockWorkspace: { + path: string; + settings: { hooksConfig: { disabled: string[] } }; + }; + let mockSetValue: ReturnType; + + beforeEach(() => { + mockUser = { + path: '/mock/user.json', + settings: { hooksConfig: { disabled: [] } }, + }; + mockWorkspace = { + path: '/mock/workspace.json', + settings: { hooksConfig: { disabled: [] } }, + }; + mockSetValue = vi.fn(); + + mockSettings = { + forScope: (scope: SettingScope) => { + if (scope === SettingScope.User) return mockUser; + if (scope === SettingScope.Workspace) return mockWorkspace; + return mockUser; // Default/Fallback + }, + setValue: mockSetValue, + } as unknown as LoadedSettings; + }); + + describe('enableHook', () => { + it('should return no-op if hook is not disabled in any scope', () => { + const result = enableHook(mockSettings, 'test-hook'); + + expect(result.status).toBe('no-op'); + expect(result.action).toBe('enable'); + expect(result.modifiedScopes).toHaveLength(0); + expect(result.alreadyInStateScopes).toHaveLength(2); // User + Workspace + expect(mockSetValue).not.toHaveBeenCalled(); + }); + + it('should enable hook in User scope if disabled there', () => { + mockUser.settings.hooksConfig.disabled = ['test-hook']; + + const result = enableHook(mockSettings, 'test-hook'); + + expect(result.status).toBe('success'); + expect(result.modifiedScopes).toEqual([ + { scope: SettingScope.User, path: '/mock/user.json' }, + ]); + expect(mockSetValue).toHaveBeenCalledWith( + SettingScope.User, + 'hooksConfig.disabled', + [], + ); + }); + + it('should enable hook in Workspace scope if disabled there', () => { + mockWorkspace.settings.hooksConfig.disabled = ['test-hook']; + + const result = enableHook(mockSettings, 'test-hook'); + + expect(result.status).toBe('success'); + expect(result.modifiedScopes).toEqual([ + { scope: SettingScope.Workspace, path: '/mock/workspace.json' }, + ]); + expect(mockSetValue).toHaveBeenCalledWith( + SettingScope.Workspace, + 'hooksConfig.disabled', + [], + ); + }); + + it('should enable hook in BOTH scopes if disabled in both', () => { + mockUser.settings.hooksConfig.disabled = ['test-hook', 'other']; + mockWorkspace.settings.hooksConfig.disabled = ['test-hook']; + + const result = enableHook(mockSettings, 'test-hook'); + + expect(result.status).toBe('success'); + expect(result.modifiedScopes).toHaveLength(2); + expect(result.modifiedScopes).toContainEqual({ + scope: SettingScope.User, + path: '/mock/user.json', + }); + expect(result.modifiedScopes).toContainEqual({ + scope: SettingScope.Workspace, + path: '/mock/workspace.json', + }); + + expect(mockSetValue).toHaveBeenCalledWith( + SettingScope.Workspace, + 'hooksConfig.disabled', + [], + ); + expect(mockSetValue).toHaveBeenCalledWith( + SettingScope.User, + 'hooksConfig.disabled', + ['other'], + ); + }); + }); + + describe('disableHook', () => { + it('should disable hook in the requested scope', () => { + const result = disableHook( + mockSettings, + 'test-hook', + SettingScope.Workspace, + ); + + expect(result.status).toBe('success'); + expect(result.modifiedScopes).toEqual([ + { scope: SettingScope.Workspace, path: '/mock/workspace.json' }, + ]); + expect(mockSetValue).toHaveBeenCalledWith( + SettingScope.Workspace, + 'hooksConfig.disabled', + ['test-hook'], + ); + }); + + it('should return no-op if already disabled in requested scope', () => { + mockWorkspace.settings.hooksConfig.disabled = ['test-hook']; + + const result = disableHook( + mockSettings, + 'test-hook', + SettingScope.Workspace, + ); + + expect(result.status).toBe('no-op'); + expect(mockSetValue).not.toHaveBeenCalled(); + }); + + it('should disable in requested scope and report if already disabled in other scope', () => { + // User has it disabled + mockUser.settings.hooksConfig.disabled = ['test-hook']; + + // We request disable in Workspace + const result = disableHook( + mockSettings, + 'test-hook', + SettingScope.Workspace, + ); + + expect(result.status).toBe('success'); + expect(result.modifiedScopes).toEqual([ + { scope: SettingScope.Workspace, path: '/mock/workspace.json' }, + ]); + expect(result.alreadyInStateScopes).toEqual([ + { scope: SettingScope.User, path: '/mock/user.json' }, + ]); + expect(mockSetValue).toHaveBeenCalledWith( + SettingScope.Workspace, + 'hooksConfig.disabled', + ['test-hook'], + ); + }); + + it('should return error if invalid scope provided', () => { + // @ts-expect-error - Testing runtime check + const result = disableHook(mockSettings, 'test-hook', 'InvalidScope'); + + expect(result.status).toBe('error'); + expect(result.error).toContain('Invalid settings scope'); + }); + }); +}); diff --git a/packages/cli/src/utils/hookSettings.ts b/packages/cli/src/utils/hookSettings.ts new file mode 100644 index 0000000000..ca1f71a905 --- /dev/null +++ b/packages/cli/src/utils/hookSettings.ts @@ -0,0 +1,160 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { + SettingScope, + isLoadableSettingScope, + type LoadedSettings, +} from '../config/settings.js'; +import { getErrorMessage } from '@google/gemini-cli-core'; +import type { ModifiedScope } from './skillSettings.js'; + +export type HookActionStatus = 'success' | 'no-op' | 'error'; + +/** + * Metadata representing the result of a hook settings operation. + */ +export interface HookActionResult { + status: HookActionStatus; + hookName: string; + action: 'enable' | 'disable'; + /** Scopes where the hook's state was actually changed. */ + modifiedScopes: ModifiedScope[]; + /** Scopes where the hook was already in the desired state. */ + alreadyInStateScopes: ModifiedScope[]; + /** Error message if status is 'error'. */ + error?: string; +} + +/** + * Enables a hook by removing it from all writable disabled lists (User and Workspace). + */ +export function enableHook( + settings: LoadedSettings, + hookName: string, +): HookActionResult { + 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.hooksConfig?.disabled; + if (scopeDisabled?.includes(hookName)) { + foundInDisabledScopes.push({ scope, path: scopePath }); + } else { + alreadyEnabledScopes.push({ scope, path: scopePath }); + } + } + } + + if (foundInDisabledScopes.length === 0) { + return { + status: 'no-op', + hookName, + action: 'enable', + modifiedScopes: [], + alreadyInStateScopes: alreadyEnabledScopes, + }; + } + + const modifiedScopes: ModifiedScope[] = []; + try { + for (const { scope, path } of foundInDisabledScopes) { + if (isLoadableSettingScope(scope)) { + const currentScopeDisabled = + settings.forScope(scope).settings.hooksConfig?.disabled ?? []; + const newDisabled = currentScopeDisabled.filter( + (name) => name !== hookName, + ); + settings.setValue(scope, 'hooksConfig.disabled', newDisabled); + modifiedScopes.push({ scope, path }); + } + } + } catch (error) { + return { + status: 'error', + hookName, + action: 'enable', + modifiedScopes, + alreadyInStateScopes: alreadyEnabledScopes, + error: `Failed to enable hook: ${getErrorMessage(error)}`, + }; + } + + return { + status: 'success', + hookName, + action: 'enable', + modifiedScopes, + alreadyInStateScopes: alreadyEnabledScopes, + }; +} + +/** + * Disables a hook by adding it to the disabled list in the specified scope. + */ +export function disableHook( + settings: LoadedSettings, + hookName: string, + scope: SettingScope, +): HookActionResult { + if (!isLoadableSettingScope(scope)) { + return { + status: 'error', + hookName, + action: 'disable', + modifiedScopes: [], + alreadyInStateScopes: [], + error: `Invalid settings scope: ${scope}`, + }; + } + + const scopePath = settings.forScope(scope).path; + const currentScopeDisabled = + settings.forScope(scope).settings.hooksConfig?.disabled ?? []; + + if (currentScopeDisabled.includes(hookName)) { + return { + status: 'no-op', + hookName, + action: 'disable', + modifiedScopes: [], + alreadyInStateScopes: [{ scope, path: scopePath }], + }; + } + + // 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.hooksConfig?.disabled; + if (otherScopeDisabled?.includes(hookName)) { + alreadyDisabledInOther.push({ + scope: otherScope, + path: settings.forScope(otherScope).path, + }); + } + } + + const newDisabled = [...currentScopeDisabled, hookName]; + settings.setValue(scope, 'hooksConfig.disabled', newDisabled); + + return { + status: 'success', + hookName, + action: 'disable', + modifiedScopes: [{ scope, path: scopePath }], + alreadyInStateScopes: alreadyDisabledInOther, + }; +} diff --git a/packages/cli/src/utils/hookUtils.test.ts b/packages/cli/src/utils/hookUtils.test.ts new file mode 100644 index 0000000000..0a79cd1fe2 --- /dev/null +++ b/packages/cli/src/utils/hookUtils.test.ts @@ -0,0 +1,142 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import { renderHookActionFeedback } from './hookUtils.js'; +import type { HookActionResult } from './hookSettings.js'; +import { SettingScope } from '../config/settings.js'; + +describe('hookUtils', () => { + describe('renderHookActionFeedback', () => { + const mockFormatScope = (label: string, path: string) => + `${label} (${path})`; + + it('should render error message', () => { + const result: HookActionResult = { + status: 'error', + hookName: 'test-hook', + action: 'enable', + modifiedScopes: [], + alreadyInStateScopes: [], + error: 'Something went wrong', + }; + + const message = renderHookActionFeedback(result, mockFormatScope); + expect(message).toBe('Something went wrong'); + }); + + it('should render default error message if error string is missing', () => { + const result: HookActionResult = { + status: 'error', + hookName: 'test-hook', + action: 'enable', + modifiedScopes: [], + alreadyInStateScopes: [], + }; + + const message = renderHookActionFeedback(result, mockFormatScope); + expect(message).toBe( + 'An error occurred while attempting to enable hook "test-hook".', + ); + }); + + it('should render no-op message for enable', () => { + const result: HookActionResult = { + status: 'no-op', + hookName: 'test-hook', + action: 'enable', + modifiedScopes: [], + alreadyInStateScopes: [], + }; + + const message = renderHookActionFeedback(result, mockFormatScope); + expect(message).toBe('Hook "test-hook" is already enabled.'); + }); + + it('should render no-op message for disable', () => { + const result: HookActionResult = { + status: 'no-op', + hookName: 'test-hook', + action: 'disable', + modifiedScopes: [], + alreadyInStateScopes: [], + }; + + const message = renderHookActionFeedback(result, mockFormatScope); + expect(message).toBe('Hook "test-hook" is already disabled.'); + }); + + it('should render success message for enable (single scope)', () => { + const result: HookActionResult = { + status: 'success', + hookName: 'test-hook', + action: 'enable', + modifiedScopes: [{ scope: SettingScope.User, path: '/path/user.json' }], + alreadyInStateScopes: [ + { scope: SettingScope.Workspace, path: '/path/workspace.json' }, + ], + }; + + const message = renderHookActionFeedback(result, mockFormatScope); + expect(message).toBe( + 'Hook "test-hook" enabled by removing it from the disabled list in user (/path/user.json) and workspace (/path/workspace.json) settings.', + ); + }); + + it('should render success message for enable (single scope only affected)', () => { + // E.g. Workspace doesn't exist or isn't loadable, so only User is affected. + const result: HookActionResult = { + status: 'success', + hookName: 'test-hook', + action: 'enable', + modifiedScopes: [{ scope: SettingScope.User, path: '/path/user.json' }], + alreadyInStateScopes: [], + }; + + const message = renderHookActionFeedback(result, mockFormatScope); + expect(message).toBe( + 'Hook "test-hook" enabled by removing it from the disabled list in user (/path/user.json) settings.', + ); + }); + + it('should render success message for disable (single scope)', () => { + const result: HookActionResult = { + status: 'success', + hookName: 'test-hook', + action: 'disable', + modifiedScopes: [ + { scope: SettingScope.Workspace, path: '/path/workspace.json' }, + ], + alreadyInStateScopes: [], + }; + + const message = renderHookActionFeedback(result, mockFormatScope); + expect(message).toBe( + 'Hook "test-hook" disabled by adding it to the disabled list in workspace (/path/workspace.json) settings.', + ); + }); + + it('should render success message for disable (two scopes)', () => { + // E.g. Disabled in Workspace, but ALREADY disabled in User. + const result: HookActionResult = { + status: 'success', + hookName: 'test-hook', + action: 'disable', + modifiedScopes: [ + { scope: SettingScope.Workspace, path: '/path/workspace.json' }, + ], + alreadyInStateScopes: [ + { scope: SettingScope.User, path: '/path/user.json' }, + ], + }; + + const message = renderHookActionFeedback(result, mockFormatScope); + expect(message).toBe( + 'Hook "test-hook" is now disabled in both workspace (/path/workspace.json) and user (/path/user.json) settings.', + ); + }); + }); +}); diff --git a/packages/cli/src/utils/hookUtils.ts b/packages/cli/src/utils/hookUtils.ts new file mode 100644 index 0000000000..c6583975ff --- /dev/null +++ b/packages/cli/src/utils/hookUtils.ts @@ -0,0 +1,67 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { SettingScope } from '../config/settings.js'; +import type { HookActionResult } from './hookSettings.js'; + +/** + * Shared logic for building the core hook action message while allowing the + * caller to control how each scope and its path are rendered (e.g., bolding or + * dimming). + */ +export function renderHookActionFeedback( + result: HookActionResult, + formatScope: (label: string, path: string) => string, +): string { + const { hookName, action, status, error } = result; + + if (status === 'error') { + return ( + error || + `An error occurred while attempting to ${action} hook "${hookName}".` + ); + } + + if (status === 'no-op') { + return `Hook "${hookName}" 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 ? 'workspace' : s.scope.toLowerCase(); + return formatScope(label, s.path); + }; + + const totalAffectedScopes = [ + ...result.modifiedScopes, + ...result.alreadyInStateScopes, + ]; + + if (totalAffectedScopes.length === 0) { + // This case should ideally not happen, but as a safeguard, return a generic message. + return `Hook "${hookName}" ${actionVerb}.`; + } + + if (totalAffectedScopes.length === 2) { + const s1 = formatScopeItem(totalAffectedScopes[0]); + const s2 = formatScopeItem(totalAffectedScopes[1]); + + if (isEnable) { + return `Hook "${hookName}" ${actionVerb} ${preposition} ${s1} and ${s2} settings.`; + } else { + return `Hook "${hookName}" is now disabled in both ${s1} and ${s2} settings.`; + } + } + + const s = formatScopeItem(totalAffectedScopes[0]); + return `Hook "${hookName}" ${actionVerb} ${preposition} ${s} settings.`; +}