feat(cli): align hooks enable/disable with skills and improve completion (#16822)

2026-04-14 23:31:13 -07:00 · 2026-01-27 20:56:06 -05:00
parent 130b596c80
commit 2c09785748
6 changed files with 695 additions and 150 deletions
--- 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<typeof vi.fn>;
+    // 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<typeof vi.fn>;
  };

  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',
        [],
      );
--- 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 = {