Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

This file was deleted.

9 changes: 2 additions & 7 deletions integration-tests/test-helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 4 additions & 2 deletions packages/cli/src/config/extension-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
14 changes: 1 addition & 13 deletions packages/core/src/hooks/hookRegistry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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', () => {
Expand Down
24 changes: 0 additions & 24 deletions packages/core/src/hooks/hookRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
*/
Expand Down Expand Up @@ -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;
Expand All @@ -57,13 +46,8 @@ export class HookRegistry {
* Initialize the registry by processing hooks from config
*/
async initialize(): Promise<void> {
if (this.initialized) {
return;
}

this.entries = [];
this.processHooksFromConfig();
this.initialized = true;

debugLogger.log(
`Hook registry initialized with ${this.entries.length} hook entries`,
Expand All @@ -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(
Expand All @@ -90,10 +70,6 @@ export class HookRegistry {
* Get all registered hooks
*/
getAllHooks(): HookRegistryEntry[] {
if (!this.initialized) {
throw new HookRegistryNotInitializedError();
}

return [...this.entries];
}

Expand Down
29 changes: 1 addition & 28 deletions packages/core/src/hooks/hookSystem.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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
Expand Down
24 changes: 0 additions & 24 deletions packages/core/src/hooks/hookSystem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -49,22 +48,14 @@ export class HookSystem {
* Initialize the hook system
*/
async initialize(): Promise<void> {
if (this.initialized) {
return;
}

await this.hookRegistry.initialize();
this.initialized = true;
debugLogger.debug('Hook system initialized successfully');
}

/**
* Get the hook event bus for firing events
*/
getEventHandler(): HookEventHandler {
if (!this.initialized) {
throw new Error('Hook system not initialized');
}
return this.hookEventHandler;
}

Expand All @@ -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,
};
}
}
11 changes: 11 additions & 0 deletions packages/core/src/utils/extensionLoader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ describe('SimpleExtensionLoader', () => {
let mockGeminiClientSetTools: MockInstance<
typeof GeminiClient.prototype.setTools
>;
let mockHookSystemInit: MockInstance;

const activeExtension: GeminiCLIExtension = {
name: 'test-extension',
Expand All @@ -61,13 +62,17 @@ describe('SimpleExtensionLoader', () => {
} as unknown as McpClientManager;
extensionReloadingEnabled = false;
mockGeminiClientSetTools = vi.fn();
mockHookSystemInit = vi.fn();
mockConfig = {
getMcpClientManager: () => mockMcpClientManager,
getEnableExtensionReloading: () => extensionReloadingEnabled,
getGeminiClient: vi.fn(() => ({
isInitialized: () => true,
setTools: mockGeminiClientSetTools,
})),
getHookSystem: () => ({
initialize: mockHookSystemInit,
}),
} as unknown as Config;
});

Expand Down Expand Up @@ -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);
Expand All @@ -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();
}
});
Expand All @@ -164,6 +174,7 @@ describe('SimpleExtensionLoader', () => {
loader.loadExtension(anotherExtension),
]);
expect(mockRefreshServerHierarchicalMemory).toHaveBeenCalledOnce();
expect(mockHookSystemInit).toHaveBeenCalledOnce();
},
);
},
Expand Down
15 changes: 7 additions & 8 deletions packages/core/src/utils/extensionLoader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}

Expand All @@ -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<void> | undefined {
): Promise<void> {
if (this.config && this.config.getEnableExtensionReloading()) {
return this.startExtension(extension);
await this.startExtension(extension);
}
return;
}

/**
Expand Down Expand Up @@ -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<void> | undefined {
): Promise<void> {
if (this.config && this.config.getEnableExtensionReloading()) {
return this.stopExtension(extension);
await this.stopExtension(extension);
}
return;
}

async restartExtension(extension: GeminiCLIExtension): Promise<void> {
Expand Down