From 1998a713e2010830901f9ea7781723f5000d83d2 Mon Sep 17 00:00:00 2001 From: Abhi <43648792+abhipatel12@users.noreply.github.com> Date: Fri, 16 Jan 2026 19:28:36 -0500 Subject: [PATCH] fix(hooks): enable /hooks disable to reliably stop single hooks (#16804) --- .../cli/src/ui/commands/hooksCommand.test.ts | 12 +++- packages/cli/src/ui/commands/hooksCommand.ts | 47 +++++++------ packages/core/src/config/config.test.ts | 69 +++++++++++-------- packages/core/src/config/config.ts | 28 ++++++-- 4 files changed, 97 insertions(+), 59 deletions(-) diff --git a/packages/cli/src/ui/commands/hooksCommand.test.ts b/packages/cli/src/ui/commands/hooksCommand.test.ts index 2d5588dee8..5bc9908e14 100644 --- a/packages/cli/src/ui/commands/hooksCommand.test.ts +++ b/packages/cli/src/ui/commands/hooksCommand.test.ts @@ -22,6 +22,7 @@ describe('hooksCommand', () => { let mockConfig: { getHookSystem: ReturnType; getEnableHooks: ReturnType; + updateDisabledHooks: ReturnType; }; 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.', }); }); diff --git a/packages/cli/src/ui/commands/hooksCommand.ts b/packages/cli/src/ui/commands/hooksCommand.ts index e8afca5613..0204e1c6e6 100644 --- a/packages/cli/src/ui/commands/hooksCommand.ts +++ b/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); diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index f9cc9bcdfc..c7d5f66a15 100644 --- a/packages/core/src/config/config.test.ts +++ b/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 diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 12158fb5d4..ff9dc47ea2 100644 --- a/packages/core/src/config/config.ts +++ b/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 | 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 */