From 9722ec9b6a4c4c6da9682a2bf4efb6b7d854efe7 Mon Sep 17 00:00:00 2001 From: Sehoon Shon Date: Fri, 16 Jan 2026 14:39:07 -0500 Subject: [PATCH] fix(core): surface warnings for invalid hook event names in configuration (#16788) (#16873) --- packages/core/src/hooks/hookRegistry.test.ts | 52 +++++++++++++++++++- packages/core/src/hooks/hookRegistry.ts | 11 ++++- packages/core/src/hooks/types.ts | 5 ++ 3 files changed, 65 insertions(+), 3 deletions(-) diff --git a/packages/core/src/hooks/hookRegistry.test.ts b/packages/core/src/hooks/hookRegistry.test.ts index f98f475054..5d6d3ccced 100644 --- a/packages/core/src/hooks/hookRegistry.test.ts +++ b/packages/core/src/hooks/hookRegistry.test.ts @@ -8,7 +8,12 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import * as fs from 'node:fs'; import { HookRegistry } from './hookRegistry.js'; import type { Storage } from '../config/storage.js'; -import { ConfigSource, HookEventName, HookType } from './types.js'; +import { + ConfigSource, + HookEventName, + HookType, + HOOKS_CONFIG_FIELDS, +} from './types.js'; import type { Config } from '../config/config.js'; import type { HookDefinition } from './types.js'; @@ -645,6 +650,51 @@ describe('HookRegistry', () => { expect.objectContaining({ type: 'invalid-type' }), ); }); + + it('should skip known config fields and warn on invalid event names', async () => { + const configWithExtras: Record = { + InvalidEvent: [], + BeforeTool: [ + { + hooks: [ + { + type: 'command', + command: './test.sh', + }, + ], + }, + ], + }; + + // Add all known config fields dynamically + for (const field of HOOKS_CONFIG_FIELDS) { + configWithExtras[field] = field === 'disabled' ? [] : true; + } + + vi.mocked(mockConfig.getHooks).mockReturnValue( + configWithExtras as unknown as { + [K in HookEventName]?: HookDefinition[]; + }, + ); + + await hookRegistry.initialize(); + + // Should only load the valid hook + expect(hookRegistry.getAllHooks()).toHaveLength(1); + + // Should skip all known config fields without warnings + for (const field of HOOKS_CONFIG_FIELDS) { + expect(mockDebugLogger.warn).not.toHaveBeenCalledWith( + expect.stringContaining(`Invalid hook event name: ${field}`), + ); + } + + // Should warn on truly invalid event name + expect(mockCoreEvents.emitFeedback).toHaveBeenCalledWith( + 'warning', + expect.stringContaining('Invalid hook event name: "InvalidEvent"'), + ); + }); }); describe('project hook warnings', () => { diff --git a/packages/core/src/hooks/hookRegistry.ts b/packages/core/src/hooks/hookRegistry.ts index f3961a8cce..072f049f0a 100644 --- a/packages/core/src/hooks/hookRegistry.ts +++ b/packages/core/src/hooks/hookRegistry.ts @@ -6,7 +6,7 @@ import type { Config } from '../config/config.js'; import type { HookDefinition, HookConfig } from './types.js'; -import { HookEventName, ConfigSource } from './types.js'; +import { HookEventName, ConfigSource, HOOKS_CONFIG_FIELDS } from './types.js'; import { debugLogger } from '../utils/debugLogger.js'; import { TrustedHooksManager } from './trustedHooks.js'; import { coreEvents } from '../utils/events.js'; @@ -169,8 +169,15 @@ please review the project settings (.gemini/settings.json) and remove them.`; source: ConfigSource, ): void { for (const [eventName, definitions] of Object.entries(hooksConfig)) { + if (HOOKS_CONFIG_FIELDS.includes(eventName)) { + continue; + } + if (!this.isValidEventName(eventName)) { - debugLogger.warn(`Invalid hook event name: ${eventName}`); + coreEvents.emitFeedback( + 'warning', + `Invalid hook event name: "${eventName}" from ${source} config. Skipping.`, + ); continue; } diff --git a/packages/core/src/hooks/types.ts b/packages/core/src/hooks/types.ts index 8d6e203778..7457db83ea 100644 --- a/packages/core/src/hooks/types.ts +++ b/packages/core/src/hooks/types.ts @@ -44,6 +44,11 @@ export enum HookEventName { BeforeToolSelection = 'BeforeToolSelection', } +/** + * Fields in the hooks configuration that are not hook event names + */ +export const HOOKS_CONFIG_FIELDS = ['enabled', 'disabled', 'notifications']; + /** * Hook configuration entry */