diff --git a/docs/hooks/index.md b/docs/hooks/index.md index 9837a50b2a..a601529aa6 100644 --- a/docs/hooks/index.md +++ b/docs/hooks/index.md @@ -414,13 +414,22 @@ precedence rules. ### Configuration layers -Hook configurations are applied in the following order of precedence (higher -numbers override lower numbers): +Hook configurations are applied in the following order of execution (lower +numbers run first): -1. **System defaults:** Built-in default settings (lowest precedence) -2. **User settings:** `~/.gemini/settings.json` -3. **Project settings:** `.gemini/settings.json` in your project directory -4. **System settings:** `/etc/gemini-cli/settings.json` (highest precedence) +1. **Project settings:** `.gemini/settings.json` in your project directory + (highest priority) +2. **User settings:** `~/.gemini/settings.json` +3. **System settings:** `/etc/gemini-cli/settings.json` +4. **Extensions:** Internal hooks defined by installed extensions (lowest + priority) + +#### Deduplication and shadowing + +If multiple hooks with the identical **name** and **command** are discovered +across different configuration layers, Gemini CLI deduplicates them. The hook +from the higher-priority layer (e.g., Project) will be kept, and others will be +ignored. Within each level, hooks run in the order they are declared in the configuration. @@ -450,8 +459,9 @@ configuration. **Configuration properties:** -- **`name`** (string, required): Unique identifier for the hook used in - `/hooks enable/disable` commands +- **`name`** (string, recommended): Unique identifier for the hook used in + `/hooks enable/disable` commands. If omitted, the `command` path is used as + the identifier. - **`type`** (string, required): Hook type - currently only `"command"` is supported - **`command`** (string, required): Path to the script or command to execute @@ -498,6 +508,8 @@ You can temporarily enable or disable individual hooks using commands: These commands allow you to control hook execution without editing configuration files. The hook name should match the `name` field in your hook configuration. +Changes made via these commands are persisted to your global User settings +(`~/.gemini/settings.json`). ### Disabled hooks configuration diff --git a/packages/cli/src/config/settingsSchema.test.ts b/packages/cli/src/config/settingsSchema.test.ts index ad003ee853..81f57feefe 100644 --- a/packages/cli/src/config/settingsSchema.test.ts +++ b/packages/cli/src/config/settingsSchema.test.ts @@ -356,6 +356,18 @@ describe('SettingsSchema', () => { 'Enable local and remote subagents. Warning: Experimental feature, uses YOLO mode for subagents', ); }); + + it('should have name and description in hook definitions', () => { + const hookDef = SETTINGS_SCHEMA_DEFINITIONS['HookDefinitionArray']; + expect(hookDef).toBeDefined(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const hookItemProperties = (hookDef as any).items.properties.hooks.items + .properties; + expect(hookItemProperties.name).toBeDefined(); + expect(hookItemProperties.name.type).toBe('string'); + expect(hookItemProperties.description).toBeDefined(); + expect(hookItemProperties.description.type).toBe('string'); + }); }); it('has JSON schema definitions for every referenced ref', () => { diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index f8fdae9ca8..7508c19731 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -1891,6 +1891,10 @@ export const SETTINGS_SCHEMA_DEFINITIONS: Record< type: 'object', description: 'Individual hook configuration.', properties: { + name: { + type: 'string', + description: 'Unique identifier for the hook.', + }, type: { type: 'string', description: @@ -1901,6 +1905,10 @@ export const SETTINGS_SCHEMA_DEFINITIONS: Record< description: 'Shell command to execute. Receives JSON input via stdin and returns JSON output via stdout.', }, + description: { + type: 'string', + description: 'A description of the hook.', + }, timeout: { type: 'number', description: 'Timeout in milliseconds for hook execution.', diff --git a/packages/cli/src/ui/commands/hooksCommand.test.ts b/packages/cli/src/ui/commands/hooksCommand.test.ts index 47b1282bc8..990228809c 100644 --- a/packages/cli/src/ui/commands/hooksCommand.test.ts +++ b/packages/cli/src/ui/commands/hooksCommand.test.ts @@ -309,6 +309,24 @@ describe('hooksCommand', () => { content: 'Failed to enable hook: Failed to save settings', }); }); + + it('should complete hook names using friendly names', () => { + const enableCmd = hooksCommand.subCommands!.find( + (cmd) => cmd.name === 'enable', + )!; + + const hookEntry = createMockHook( + './hooks/test.sh', + HookEventName.BeforeTool, + true, + ); + hookEntry.config.name = 'friendly-name'; + + mockHookSystem.getAllHooks.mockReturnValue([hookEntry]); + + const completions = enableCmd.completion!(mockContext, 'frie'); + expect(completions).toContain('friendly-name'); + }); }); describe('disable subcommand', () => { diff --git a/packages/cli/src/ui/commands/hooksCommand.ts b/packages/cli/src/ui/commands/hooksCommand.ts index cc2cfb4fcc..6bbfbb83e7 100644 --- a/packages/cli/src/ui/commands/hooksCommand.ts +++ b/packages/cli/src/ui/commands/hooksCommand.ts @@ -213,7 +213,7 @@ function completeHookNames( * Get a display name for a hook */ function getHookDisplayName(hook: HookRegistryEntry): string { - return hook.config.command || 'unknown-hook'; + return hook.config.name || hook.config.command || 'unknown-hook'; } const panelCommand: SlashCommand = { diff --git a/packages/cli/src/ui/components/views/HooksList.tsx b/packages/cli/src/ui/components/views/HooksList.tsx index b733381555..1fe3ca71d5 100644 --- a/packages/cli/src/ui/components/views/HooksList.tsx +++ b/packages/cli/src/ui/components/views/HooksList.tsx @@ -9,7 +9,13 @@ import { Box, Text } from 'ink'; interface HooksListProps { hooks: ReadonlyArray<{ - config: { command?: string; type: string; timeout?: number }; + config: { + command?: string; + type: string; + name?: string; + description?: string; + timeout?: number; + }; source: string; eventName: string; matcher?: string; @@ -50,7 +56,8 @@ export const HooksList: React.FC = ({ hooks }) => { {eventHooks.map((hook, index) => { - const hookName = hook.config.command || 'unknown'; + const hookName = + hook.config.name || hook.config.command || 'unknown'; const statusColor = hook.enabled ? 'green' : 'gray'; const statusText = hook.enabled ? 'enabled' : 'disabled'; @@ -63,8 +70,14 @@ export const HooksList: React.FC = ({ hooks }) => { + {hook.config.description && ( + {hook.config.description} + )} Source: {hook.source} + {hook.config.name && + hook.config.command && + ` | Command: ${hook.config.command}`} {hook.matcher && ` | Matcher: ${hook.matcher}`} {hook.sequential && ` | Sequential`} {hook.config.timeout && diff --git a/packages/core/src/hooks/hookPlanner.test.ts b/packages/core/src/hooks/hookPlanner.test.ts index c408266ecb..245385f6af 100644 --- a/packages/core/src/hooks/hookPlanner.test.ts +++ b/packages/core/src/hooks/hookPlanner.test.ts @@ -282,6 +282,71 @@ describe('HookPlanner', () => { ); }); + it('should deduplicate based on both name and command', () => { + const mockEntries: HookRegistryEntry[] = [ + { + config: { + name: 'hook1', + type: HookType.Command, + command: './same.sh', + }, + source: ConfigSource.Project, + eventName: HookEventName.BeforeTool, + enabled: true, + }, + { + config: { + name: 'hook1', + type: HookType.Command, + command: './same.sh', + }, + source: ConfigSource.User, + eventName: HookEventName.BeforeTool, + enabled: true, + }, // Same name, same command -> deduplicate + { + config: { + name: 'hook2', + type: HookType.Command, + command: './same.sh', + }, + source: ConfigSource.Project, + eventName: HookEventName.BeforeTool, + enabled: true, + }, // Different name, same command -> distinct + { + config: { + name: 'hook1', + type: HookType.Command, + command: './different.sh', + }, + source: ConfigSource.Project, + eventName: HookEventName.BeforeTool, + enabled: true, + }, // Same name, different command -> distinct + { + config: { type: HookType.Command, command: './no-name.sh' }, + source: ConfigSource.Project, + eventName: HookEventName.BeforeTool, + enabled: true, + }, + { + config: { type: HookType.Command, command: './no-name.sh' }, + source: ConfigSource.User, + eventName: HookEventName.BeforeTool, + enabled: true, + }, // No name, same command -> deduplicate + ]; + + vi.mocked(mockHookRegistry.getHooksForEvent).mockReturnValue(mockEntries); + + const plan = hookPlanner.createExecutionPlan(HookEventName.BeforeTool); + + expect(plan).not.toBeNull(); + // hook1:same.sh (deduped), hook2:same.sh, hook1:different.sh, :no-name.sh (deduped) + expect(plan!.hookConfigs).toHaveLength(4); + }); + it('should match trigger for session events', () => { const mockEntries: HookRegistryEntry[] = [ { diff --git a/packages/core/src/hooks/hookPlanner.ts b/packages/core/src/hooks/hookPlanner.ts index 4a0f00a8e1..9c40fbbd25 100644 --- a/packages/core/src/hooks/hookPlanner.ts +++ b/packages/core/src/hooks/hookPlanner.ts @@ -141,7 +141,9 @@ export class HookPlanner { * Generate a unique key for a hook entry */ private getHookKey(entry: HookRegistryEntry): string { - return `command:${entry.config.command}`; + const name = entry.config.name || ''; + const command = entry.config.command || ''; + return `${name}:${command}`; } } diff --git a/packages/core/src/hooks/hookRegistry.test.ts b/packages/core/src/hooks/hookRegistry.test.ts index 1d1d906993..b3537c2581 100644 --- a/packages/core/src/hooks/hookRegistry.test.ts +++ b/packages/core/src/hooks/hookRegistry.test.ts @@ -190,6 +190,39 @@ describe('HookRegistry', () => { expect(hookRegistry.getAllHooks()).toHaveLength(0); expect(mockDebugLogger.warn).toHaveBeenCalled(); // At least some warnings should be logged }); + + it('should respect disabled hooks using friendly name', async () => { + const mockHooksConfig = { + BeforeTool: [ + { + hooks: [ + { + name: 'disabled-hook', + type: 'command', + command: './hooks/test.sh', + }, + ], + }, + ], + }; + + // Update mock to return the hooks configuration + vi.mocked(mockConfig.getHooks).mockReturnValue( + mockHooksConfig as unknown as { + [K in HookEventName]?: HookDefinition[]; + }, + ); + vi.mocked(mockConfig.getDisabledHooks).mockReturnValue(['disabled-hook']); + + await hookRegistry.initialize(); + + const hooks = hookRegistry.getAllHooks(); + expect(hooks).toHaveLength(1); + expect(hooks[0].enabled).toBe(false); + expect( + hookRegistry.getHooksForEvent(HookEventName.BeforeTool), + ).toHaveLength(0); + }); }); describe('getHooksForEvent', () => { @@ -305,6 +338,77 @@ describe('HookRegistry', () => { 'No hooks found matching "non-existent-hook"', ); }); + + it('should prefer hook name over command for identification', async () => { + const mockHooksConfig = { + BeforeTool: [ + { + hooks: [ + { + name: 'friendly-name', + type: 'command', + command: './hooks/test.sh', + }, + ], + }, + ], + }; + + vi.mocked(mockConfig.getHooks).mockReturnValue( + mockHooksConfig as unknown as { + [K in HookEventName]?: HookDefinition[]; + }, + ); + + await hookRegistry.initialize(); + + // Should be enabled initially + let hooks = hookRegistry.getHooksForEvent(HookEventName.BeforeTool); + expect(hooks).toHaveLength(1); + + // Disable using friendly name + hookRegistry.setHookEnabled('friendly-name', false); + hooks = hookRegistry.getHooksForEvent(HookEventName.BeforeTool); + expect(hooks).toHaveLength(0); + + // Identification by command should NOT work when name is present + hookRegistry.setHookEnabled('./hooks/test.sh', true); + expect(mockDebugLogger.warn).toHaveBeenCalledWith( + 'No hooks found matching "./hooks/test.sh"', + ); + }); + + it('should use command as identifier when name is missing', async () => { + const mockHooksConfig = { + BeforeTool: [ + { + hooks: [ + { + type: 'command', + command: './hooks/no-name.sh', + }, + ], + }, + ], + }; + + vi.mocked(mockConfig.getHooks).mockReturnValue( + mockHooksConfig as unknown as { + [K in HookEventName]?: HookDefinition[]; + }, + ); + + await hookRegistry.initialize(); + + // Should be enabled initially + let hooks = hookRegistry.getHooksForEvent(HookEventName.BeforeTool); + expect(hooks).toHaveLength(1); + + // Disable using command + hookRegistry.setHookEnabled('./hooks/no-name.sh', false); + hooks = hookRegistry.getHooksForEvent(HookEventName.BeforeTool); + expect(hooks).toHaveLength(0); + }); }); describe('malformed configuration handling', () => { diff --git a/packages/core/src/hooks/hookRegistry.ts b/packages/core/src/hooks/hookRegistry.ts index a074160808..53fc42bde4 100644 --- a/packages/core/src/hooks/hookRegistry.ts +++ b/packages/core/src/hooks/hookRegistry.ts @@ -96,10 +96,12 @@ export class HookRegistry { } /** - * Get hook name for display purposes + * Get hook name for identification and display purposes */ - private getHookName(entry: HookRegistryEntry): string { - return entry.config.command || 'unknown-command'; + private getHookName( + entry: HookRegistryEntry | { config: HookConfig }, + ): string { + return entry.config.name || entry.config.command || 'unknown-command'; } /** diff --git a/packages/core/src/hooks/hookRunner.test.ts b/packages/core/src/hooks/hookRunner.test.ts index 8892c42218..1635e2516e 100644 --- a/packages/core/src/hooks/hookRunner.test.ts +++ b/packages/core/src/hooks/hookRunner.test.ts @@ -28,6 +28,18 @@ vi.mock('node:child_process', async (importOriginal) => { }; }); +// Mock debugLogger using vi.hoisted +const mockDebugLogger = vi.hoisted(() => ({ + log: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), +})); + +vi.mock('../utils/debugLogger.js', () => ({ + debugLogger: mockDebugLogger, +})); + // Mock console methods const mockConsole = { log: vi.fn(), @@ -162,6 +174,31 @@ describe('HookRunner', () => { expect(result.stderr).toBe(errorMessage); }); + it('should use hook name in error messages if available', async () => { + const namedConfig: HookConfig = { + name: 'my-friendly-hook', + type: HookType.Command, + command: './hooks/fail.sh', + }; + + // Mock error during spawn + vi.mocked(spawn).mockImplementationOnce(() => { + throw new Error('Spawn error'); + }); + + await hookRunner.executeHook( + namedConfig, + HookEventName.BeforeTool, + mockInput, + ); + + expect(mockDebugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining( + '(hook: my-friendly-hook): Error: Spawn error', + ), + ); + }); + it('should handle command hook timeout', async () => { const shortTimeoutConfig: HookConfig = { type: HookType.Command, diff --git a/packages/core/src/hooks/hookRunner.ts b/packages/core/src/hooks/hookRunner.ts index 5c38041c4a..ff5dce98fd 100644 --- a/packages/core/src/hooks/hookRunner.ts +++ b/packages/core/src/hooks/hookRunner.ts @@ -55,8 +55,8 @@ export class HookRunner { ); } catch (error) { const duration = Date.now() - startTime; - const hookSource = hookConfig.command || 'unknown'; - const errorMessage = `Hook execution failed for event '${eventName}' (source: ${hookSource}): ${error}`; + const hookId = hookConfig.name || hookConfig.command || 'unknown'; + const errorMessage = `Hook execution failed for event '${eventName}' (hook: ${hookId}): ${error}`; debugLogger.warn(`Hook execution error (non-fatal): ${errorMessage}`); return { diff --git a/packages/core/src/hooks/types.ts b/packages/core/src/hooks/types.ts index c35be64484..f2d2887513 100644 --- a/packages/core/src/hooks/types.ts +++ b/packages/core/src/hooks/types.ts @@ -40,6 +40,8 @@ export enum HookEventName { export interface CommandHookConfig { type: HookType.Command; command: string; + name?: string; + description?: string; timeout?: number; } diff --git a/schemas/settings.schema.json b/schemas/settings.schema.json index bcec7eaaf1..1f369d149b 100644 --- a/schemas/settings.schema.json +++ b/schemas/settings.schema.json @@ -1867,6 +1867,10 @@ "type": "object", "description": "Individual hook configuration.", "properties": { + "name": { + "type": "string", + "description": "Unique identifier for the hook." + }, "type": { "type": "string", "description": "Type of hook (currently only \"command\" supported)." @@ -1875,6 +1879,10 @@ "type": "string", "description": "Shell command to execute. Receives JSON input via stdin and returns JSON output via stdout." }, + "description": { + "type": "string", + "description": "A description of the hook." + }, "timeout": { "type": "number", "description": "Timeout in milliseconds for hook execution."