fix(hooks): enable /hooks disable to reliably stop single hooks (#16804)

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

@@ -22,6 +22,7 @@ describe('hooksCommand', () => {
   let mockConfig: {
     getHookSystem: ReturnType<typeof vi.fn>;
     getEnableHooks: ReturnType<typeof vi.fn>;
+    updateDisabledHooks: ReturnType<typeof vi.fn>;
   };
   let mockSettings: {
     merged: {
@@ -51,6 +52,7 @@ describe('hooksCommand', () => {
     mockConfig = {
       getHookSystem: vi.fn().mockReturnValue(mockHookSystem),
       getEnableHooks: vi.fn().mockReturnValue(true),
+      updateDisabledHooks: vi.fn(),
     };
 
     // Create mock settings
@@ -429,7 +431,7 @@ describe('hooksCommand', () => {
       });
     });
 
-    it('should return info when hook is already disabled', async () => {
+    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.hooks.disabled = ['test-hook'];
 
@@ -443,11 +445,15 @@ describe('hooksCommand', () => {
       const result = await disableCmd.action(mockContext, 'test-hook');
 
       expect(mockContext.services.settings.setValue).not.toHaveBeenCalled();
-      expect(mockHookSystem.setHookEnabled).not.toHaveBeenCalled();
+      expect(mockHookSystem.setHookEnabled).toHaveBeenCalledWith(
+        'test-hook',
+        false,
+      );
+      expect(mockConfig.updateDisabledHooks).toHaveBeenCalled();
       expect(result).toEqual({
         type: 'message',
         messageType: 'info',
-        content: 'Hook "test-hook" is already disabled.',
+        content: 'Hook "test-hook" disabled successfully.',
       });
     });
 

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

@@ -89,6 +89,9 @@ async function enableAction(
       : SettingScope.User;
     settings.setValue(scope, 'hooks.disabled', newDisabledHooks);
 
+    // Update core config so re-initialization (e.g. extension reload) respects the change
+    config.updateDisabledHooks(settings.merged.hooks.disabled);
+
     // Enable in hook system
     hookSystem.setHookEnabled(hookName, true);
 
@@ -144,36 +147,32 @@ async function disableAction(
   const settings = context.services.settings;
   const disabledHooks = settings.merged.hooks.disabled;
   // Add to disabled list if not already present
-  if (!disabledHooks.includes(hookName)) {
-    const newDisabledHooks = [...disabledHooks, hookName];
+  try {
+    if (!disabledHooks.includes(hookName)) {
+      const newDisabledHooks = [...disabledHooks, hookName];
 
-    // Update settings (setValue automatically saves)
-    try {
       const scope = settings.workspace
         ? SettingScope.Workspace
         : SettingScope.User;
       settings.setValue(scope, 'hooks.disabled', newDisabledHooks);
-
-      // Disable in hook system
-      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)}`,
-      };
     }
-  } else {
+
+    // Update core config so re-initialization (e.g. extension reload) respects the change
+    config.updateDisabledHooks(settings.merged.hooks.disabled);
+
+    // Always disable in hook system to ensure in-memory state matches settings
+    hookSystem.setHookEnabled(hookName, false);
+
     return {
       type: 'message',
       messageType: 'info',
-      content: `Hook "${hookName}" is already disabled.`,
+      content: `Hook "${hookName}" disabled successfully.`,
+    };
+  } catch (error) {
+    return {
+      type: 'message',
+      messageType: 'error',
+      content: `Failed to disable hook: ${getErrorMessage(error)}`,
     };
   }
 }
@@ -253,6 +252,9 @@ async function enableAllAction(
       : SettingScope.User;
     settings.setValue(scope, 'hooks.disabled', []);
 
+    // Update core config so re-initialization (e.g. extension reload) respects the change
+    config.updateDisabledHooks(settings.merged.hooks.disabled);
+
     for (const hook of disabledHooks) {
       const hookName = getHookDisplayName(hook);
       hookSystem.setHookEnabled(hookName, true);
@@ -323,6 +325,9 @@ async function disableAllAction(
       : SettingScope.User;
     settings.setValue(scope, 'hooks.disabled', allHookNames);
 
+    // Update core config so re-initialization (e.g. extension reload) respects the change
+    config.updateDisabledHooks(settings.merged.hooks.disabled);
+
     for (const hook of enabledHooks) {
       const hookName = getHookDisplayName(hook);
       hookSystem.setHookEnabled(hookName, false);

packages/core/src/config/config.test.ts

@@ -1532,40 +1532,16 @@ describe('Config getHooks', () => {
   });
 
   it('should return the hooks configuration when provided', () => {
-    const mockHooks: { [K in HookEventName]?: HookDefinition[] } = {
-      [HookEventName.BeforeTool]: [
+    const mockHooks = {
+      BeforeTool: [
         {
-          matcher: 'write_file',
-          hooks: [
-            {
-              type: HookType.Command,
-              command: 'echo "test hook"',
-              timeout: 5000,
-            },
-          ],
-        },
-      ],
-      [HookEventName.AfterTool]: [
-        {
-          hooks: [
-            {
-              type: HookType.Command,
-              command: './hooks/after-tool.sh',
-              timeout: 10000,
-            },
-          ],
+          hooks: [{ type: HookType.Command, command: 'echo 1' }],
         },
       ],
     };
-
-    const config = new Config({
-      ...baseParams,
-      hooks: mockHooks,
-    });
-
+    const config = new Config({ ...baseParams, hooks: mockHooks });
     const retrievedHooks = config.getHooks();
     expect(retrievedHooks).toEqual(mockHooks);
-    expect(retrievedHooks).toBe(mockHooks); // Should return the same reference
   });
 
   it('should return hooks with all supported event types', () => {
@@ -1842,6 +1818,43 @@ describe('Availability Service Integration', () => {
   });
 });
 
+describe('Hooks configuration', () => {
+  const baseParams: ConfigParameters = {
+    sessionId: 'test',
+    targetDir: '.',
+    debugMode: false,
+    model: 'test-model',
+    cwd: '.',
+    hooks: { disabled: ['initial-hook'] },
+  };
+
+  it('updateDisabledHooks should update the disabled list', () => {
+    const config = new Config(baseParams);
+    expect(config.getDisabledHooks()).toEqual(['initial-hook']);
+
+    const newDisabled = ['new-hook-1', 'new-hook-2'];
+    config.updateDisabledHooks(newDisabled);
+
+    expect(config.getDisabledHooks()).toEqual(['new-hook-1', 'new-hook-2']);
+  });
+
+  it('updateDisabledHooks should only update disabled list and not definitions', () => {
+    const initialHooks = {
+      BeforeAgent: [
+        {
+          hooks: [{ type: HookType.Command, command: 'initial' }],
+        },
+      ],
+    };
+    const config = new Config({ ...baseParams, hooks: initialHooks });
+
+    config.updateDisabledHooks(['some-hook']);
+
+    expect(config.getDisabledHooks()).toEqual(['some-hook']);
+    expect(config.getHooks()).toEqual(initialHooks);
+  });
+});
+
 describe('Config Quota & Preview Model Access', () => {
   let config: Config;
   // eslint-disable-next-line @typescript-eslint/no-explicit-any

packages/core/src/config/config.ts

@@ -511,13 +511,11 @@ export class Config {
   private pendingIncludeDirectories: string[];
   private readonly enableHooks: boolean;
   private readonly enableHooksUI: boolean;
-  private readonly hooks:
-    | { [K in HookEventName]?: HookDefinition[] }
-    | undefined;
-  private readonly projectHooks:
+  private hooks: { [K in HookEventName]?: HookDefinition[] } | undefined;
+  private projectHooks:
     | ({ [K in HookEventName]?: HookDefinition[] } & { disabled?: string[] })
     | undefined;
-  private readonly disabledHooks: string[];
+  private disabledHooks: string[];
   private experiments: Experiments | undefined;
   private experimentsPromise: Promise<void> | undefined;
   private hookSystem?: HookSystem;
@@ -710,8 +708,15 @@ export class Config {
     };
     this.retryFetchErrors = params.retryFetchErrors ?? false;
     this.disableYoloMode = params.disableYoloMode ?? false;
-    this.hooks = params.hooks;
-    this.projectHooks = params.projectHooks;
+
+    if (params.hooks) {
+      const { disabled: _, ...restOfHooks } = params.hooks;
+      this.hooks = restOfHooks;
+    }
+    if (params.projectHooks) {
+      this.projectHooks = params.projectHooks;
+    }
+
     this.experiments = params.experiments;
     this.onModelChange = params.onModelChange;
     this.onReload = params.onReload;
@@ -1930,6 +1935,15 @@ export class Config {
     return this.projectHooks;
   }
 
+  /**
+   * Update the list of disabled hooks dynamically.
+   * This is used to keep the running system in sync with settings changes
+   * without risk of loading new hook definitions into memory.
+   */
+  updateDisabledHooks(disabledHooks: string[]): void {
+    this.disabledHooks = disabledHooks;
+  }
+
   /**
    * Get disabled hooks list
    */