From 126c32aca4972deba80a875f749fcee1367c4486 Mon Sep 17 00:00:00 2001 From: Tommaso Sciortino Date: Fri, 12 Dec 2025 16:43:46 -0800 Subject: [PATCH] Refresh hooks when refreshing extensions. (#14918) --- .../.gitignore | 1 - integration-tests/test-helper.ts | 9 ++---- packages/cli/src/config/extension-manager.ts | 6 ++-- packages/core/src/hooks/hookRegistry.test.ts | 14 +-------- packages/core/src/hooks/hookRegistry.ts | 24 --------------- packages/core/src/hooks/hookSystem.test.ts | 29 +------------------ packages/core/src/hooks/hookSystem.ts | 24 --------------- .../core/src/utils/extensionLoader.test.ts | 11 +++++++ packages/core/src/utils/extensionLoader.ts | 15 +++++----- 9 files changed, 26 insertions(+), 107 deletions(-) delete mode 100644 .integration-tests/1764635184618/should-be-able-to-list-a-directory/.gitignore diff --git a/.integration-tests/1764635184618/should-be-able-to-list-a-directory/.gitignore b/.integration-tests/1764635184618/should-be-able-to-list-a-directory/.gitignore deleted file mode 100644 index 397b4a7624..0000000000 --- a/.integration-tests/1764635184618/should-be-able-to-list-a-directory/.gitignore +++ /dev/null @@ -1 +0,0 @@ -*.log diff --git a/integration-tests/test-helper.ts b/integration-tests/test-helper.ts index c1a709493a..f0c4ad4c14 100644 --- a/integration-tests/test-helper.ts +++ b/integration-tests/test-helper.ts @@ -195,17 +195,12 @@ export class InteractiveRun { if (!timeout) { timeout = getDefaultTimeout(); } - const found = await poll( + await poll( () => stripAnsi(this.output).toLowerCase().includes(text.toLowerCase()), timeout, 200, ); - expect( - found, - `Did not find expected text: "${text}". Output was:\n${stripAnsi( - this.output, - )}`, - ).toBe(true); + expect(stripAnsi(this.output).toLowerCase()).toContain(text.toLowerCase()); } // This types slowly to make sure command is correct, but only work for short diff --git a/packages/cli/src/config/extension-manager.ts b/packages/cli/src/config/extension-manager.ts index 009fe3ed73..8671c976ac 100644 --- a/packages/cli/src/config/extension-manager.ts +++ b/packages/cli/src/config/extension-manager.ts @@ -346,8 +346,10 @@ export class ExtensionManager extends ExtensionLoader { 'success', ), ); - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.enableExtension(newExtensionConfig.name, SettingScope.User); + await this.enableExtension( + newExtensionConfig.name, + SettingScope.User, + ); } } finally { if (tempDir) { diff --git a/packages/core/src/hooks/hookRegistry.test.ts b/packages/core/src/hooks/hookRegistry.test.ts index 093821ee19..1d1d906993 100644 --- a/packages/core/src/hooks/hookRegistry.test.ts +++ b/packages/core/src/hooks/hookRegistry.test.ts @@ -6,11 +6,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import * as fs from 'node:fs'; -import { - HookRegistry, - ConfigSource, - HookRegistryNotInitializedError, -} from './hookRegistry.js'; +import { HookRegistry, ConfigSource } from './hookRegistry.js'; import type { Storage } from '../config/storage.js'; import { HookEventName, HookType } from './types.js'; import type { Config } from '../config/config.js'; @@ -258,14 +254,6 @@ describe('HookRegistry', () => { ); expect(notificationHooks).toHaveLength(0); }); - - it('should throw error if not initialized', () => { - const uninitializedRegistry = new HookRegistry(mockConfig); - - expect(() => { - uninitializedRegistry.getHooksForEvent(HookEventName.BeforeTool); - }).toThrow(HookRegistryNotInitializedError); - }); }); describe('setHookEnabled', () => { diff --git a/packages/core/src/hooks/hookRegistry.ts b/packages/core/src/hooks/hookRegistry.ts index 3d14e5162d..d170349ade 100644 --- a/packages/core/src/hooks/hookRegistry.ts +++ b/packages/core/src/hooks/hookRegistry.ts @@ -9,16 +9,6 @@ import type { HookDefinition, HookConfig } from './types.js'; import { HookEventName } from './types.js'; import { debugLogger } from '../utils/debugLogger.js'; -/** - * Error thrown when attempting to use HookRegistry before initialization - */ -export class HookRegistryNotInitializedError extends Error { - constructor(message = 'Hook registry not initialized') { - super(message); - this.name = 'HookRegistryNotInitializedError'; - } -} - /** * Configuration source levels in precedence order (highest to lowest) */ @@ -47,7 +37,6 @@ export interface HookRegistryEntry { export class HookRegistry { private readonly config: Config; private entries: HookRegistryEntry[] = []; - private initialized = false; constructor(config: Config) { this.config = config; @@ -57,13 +46,8 @@ export class HookRegistry { * Initialize the registry by processing hooks from config */ async initialize(): Promise { - if (this.initialized) { - return; - } - this.entries = []; this.processHooksFromConfig(); - this.initialized = true; debugLogger.log( `Hook registry initialized with ${this.entries.length} hook entries`, @@ -74,10 +58,6 @@ export class HookRegistry { * Get all hook entries for a specific event */ getHooksForEvent(eventName: HookEventName): HookRegistryEntry[] { - if (!this.initialized) { - throw new HookRegistryNotInitializedError(); - } - return this.entries .filter((entry) => entry.eventName === eventName && entry.enabled) .sort( @@ -90,10 +70,6 @@ export class HookRegistry { * Get all registered hooks */ getAllHooks(): HookRegistryEntry[] { - if (!this.initialized) { - throw new HookRegistryNotInitializedError(); - } - return [...this.entries]; } diff --git a/packages/core/src/hooks/hookSystem.test.ts b/packages/core/src/hooks/hookSystem.test.ts index 94a350ec41..0969a204f0 100644 --- a/packages/core/src/hooks/hookSystem.test.ts +++ b/packages/core/src/hooks/hookSystem.test.ts @@ -127,10 +127,7 @@ describe('HookSystem Integration', () => { 'Hook system initialized successfully', ); - // Verify system is initialized - const status = hookSystem.getStatus(); - expect(status.initialized).toBe(true); - // Note: totalHooks might be 0 if hook validation rejects the test hooks + expect(hookSystem.getAllHooks().length).toBe(1); }); it('should not initialize twice', async () => { @@ -204,12 +201,6 @@ describe('HookSystem Integration', () => { }); expect(result.success).toBe(true); }); - - it('should throw error when not initialized', () => { - expect(() => hookSystem.getEventHandler()).toThrow( - 'Hook system not initialized', - ); - }); }); describe('hook execution', () => { @@ -261,24 +252,6 @@ describe('HookSystem Integration', () => { }); }); - describe('system management', () => { - it('should return correct status when initialized', async () => { - await hookSystem.initialize(); - - const status = hookSystem.getStatus(); - - expect(status.initialized).toBe(true); - // Note: totalHooks might be 0 if hook validation rejects the test hooks - expect(typeof status.totalHooks).toBe('number'); - }); - - it('should return uninitialized status', () => { - const status = hookSystem.getStatus(); - - expect(status.initialized).toBe(false); - }); - }); - describe('hook disabling via settings', () => { it('should not execute disabled hooks from settings', async () => { // Create config with two hooks, one enabled and one disabled via settings diff --git a/packages/core/src/hooks/hookSystem.ts b/packages/core/src/hooks/hookSystem.ts index 596b1ecef4..059340e27c 100644 --- a/packages/core/src/hooks/hookSystem.ts +++ b/packages/core/src/hooks/hookSystem.ts @@ -24,7 +24,6 @@ export class HookSystem { private readonly hookAggregator: HookAggregator; private readonly hookPlanner: HookPlanner; private readonly hookEventHandler: HookEventHandler; - private initialized = false; constructor(config: Config) { const logger: Logger = logs.getLogger(SERVICE_NAME); @@ -49,12 +48,7 @@ export class HookSystem { * Initialize the hook system */ async initialize(): Promise { - if (this.initialized) { - return; - } - await this.hookRegistry.initialize(); - this.initialized = true; debugLogger.debug('Hook system initialized successfully'); } @@ -62,9 +56,6 @@ export class HookSystem { * Get the hook event bus for firing events */ getEventHandler(): HookEventHandler { - if (!this.initialized) { - throw new Error('Hook system not initialized'); - } return this.hookEventHandler; } @@ -88,19 +79,4 @@ export class HookSystem { getAllHooks(): HookRegistryEntry[] { return this.hookRegistry.getAllHooks(); } - - /** - * Get hook system status for debugging - */ - getStatus(): { - initialized: boolean; - totalHooks: number; - } { - const allHooks = this.initialized ? this.hookRegistry.getAllHooks() : []; - - return { - initialized: this.initialized, - totalHooks: allHooks.length, - }; - } } diff --git a/packages/core/src/utils/extensionLoader.test.ts b/packages/core/src/utils/extensionLoader.test.ts index a9328ccd70..4ec6f3641b 100644 --- a/packages/core/src/utils/extensionLoader.test.ts +++ b/packages/core/src/utils/extensionLoader.test.ts @@ -35,6 +35,7 @@ describe('SimpleExtensionLoader', () => { let mockGeminiClientSetTools: MockInstance< typeof GeminiClient.prototype.setTools >; + let mockHookSystemInit: MockInstance; const activeExtension: GeminiCLIExtension = { name: 'test-extension', @@ -61,6 +62,7 @@ describe('SimpleExtensionLoader', () => { } as unknown as McpClientManager; extensionReloadingEnabled = false; mockGeminiClientSetTools = vi.fn(); + mockHookSystemInit = vi.fn(); mockConfig = { getMcpClientManager: () => mockMcpClientManager, getEnableExtensionReloading: () => extensionReloadingEnabled, @@ -68,6 +70,9 @@ describe('SimpleExtensionLoader', () => { isInitialized: () => true, setTools: mockGeminiClientSetTools, })), + getHookSystem: () => ({ + initialize: mockHookSystemInit, + }), } as unknown as Config; }); @@ -125,13 +130,16 @@ describe('SimpleExtensionLoader', () => { mockMcpClientManager.startExtension, ).toHaveBeenCalledExactlyOnceWith(activeExtension); expect(mockRefreshServerHierarchicalMemory).toHaveBeenCalledOnce(); + expect(mockHookSystemInit).toHaveBeenCalledOnce(); expect(mockGeminiClientSetTools).toHaveBeenCalledOnce(); } else { expect(mockMcpClientManager.startExtension).not.toHaveBeenCalled(); expect(mockRefreshServerHierarchicalMemory).not.toHaveBeenCalled(); + expect(mockHookSystemInit).not.toHaveBeenCalled(); expect(mockGeminiClientSetTools).not.toHaveBeenCalledOnce(); } mockRefreshServerHierarchicalMemory.mockClear(); + mockHookSystemInit.mockClear(); mockGeminiClientSetTools.mockClear(); await loader.unloadExtension(activeExtension); @@ -140,10 +148,12 @@ describe('SimpleExtensionLoader', () => { mockMcpClientManager.stopExtension, ).toHaveBeenCalledExactlyOnceWith(activeExtension); expect(mockRefreshServerHierarchicalMemory).toHaveBeenCalledOnce(); + expect(mockHookSystemInit).toHaveBeenCalledOnce(); expect(mockGeminiClientSetTools).toHaveBeenCalledOnce(); } else { expect(mockMcpClientManager.stopExtension).not.toHaveBeenCalled(); expect(mockRefreshServerHierarchicalMemory).not.toHaveBeenCalled(); + expect(mockHookSystemInit).not.toHaveBeenCalled(); expect(mockGeminiClientSetTools).not.toHaveBeenCalledOnce(); } }); @@ -164,6 +174,7 @@ describe('SimpleExtensionLoader', () => { loader.loadExtension(anotherExtension), ]); expect(mockRefreshServerHierarchicalMemory).toHaveBeenCalledOnce(); + expect(mockHookSystemInit).toHaveBeenCalledOnce(); }, ); }, diff --git a/packages/core/src/utils/extensionLoader.ts b/packages/core/src/utils/extensionLoader.ts index 707f30cb4a..45ad37bfcc 100644 --- a/packages/core/src/utils/extensionLoader.ts +++ b/packages/core/src/utils/extensionLoader.ts @@ -111,6 +111,7 @@ export abstract class ExtensionLoader { // reload memory, this is somewhat expensive and also busts the context // cache, we want to only do it once. await refreshServerHierarchicalMemory(this.config); + await this.config.getHookSystem()?.initialize(); } } @@ -134,13 +135,12 @@ export abstract class ExtensionLoader { * then calls `startExtension` to include all extension features into the * program. */ - protected maybeStartExtension( + protected async maybeStartExtension( extension: GeminiCLIExtension, - ): Promise | undefined { + ): Promise { if (this.config && this.config.getEnableExtensionReloading()) { - return this.startExtension(extension); + await this.startExtension(extension); } - return; } /** @@ -192,13 +192,12 @@ export abstract class ExtensionLoader { * then this also performs all necessary steps to remove all extension * features from the rest of the system. */ - protected maybeStopExtension( + protected async maybeStopExtension( extension: GeminiCLIExtension, - ): Promise | undefined { + ): Promise { if (this.config && this.config.getEnableExtensionReloading()) { - return this.stopExtension(extension); + await this.stopExtension(extension); } - return; } async restartExtension(extension: GeminiCLIExtension): Promise {