diff --git a/packages/cli/src/ui/auth/useAuth.ts b/packages/cli/src/ui/auth/useAuth.ts index 87a553ec6..6e41ec658 100644 --- a/packages/cli/src/ui/auth/useAuth.ts +++ b/packages/cli/src/ui/auth/useAuth.ts @@ -15,8 +15,6 @@ import { AuthType, getErrorMessage, logAuth, - fireNotificationHook, - NotificationType, } from '@qwen-code/qwen-code-core'; import { useCallback, useEffect, useState } from 'react'; import type { LoadedSettings } from '../../config/settings.js'; @@ -170,20 +168,8 @@ export const useAuthCommand = ( const authEvent = new AuthEvent(authType, 'manual', 'success'); logAuth(config, authEvent); - // Fire auth_success notification hook - const messageBus = config.getMessageBus(); - const hooksEnabled = config.getEnableHooks(); - if (hooksEnabled && messageBus) { - fireNotificationHook( - messageBus, - `Successfully authenticated with ${authType}`, - NotificationType.AuthSuccess, - 'Authentication successful', - ).catch(() => { - // Silently ignore errors - fireNotificationHook has internal error handling - // and notification hooks should not block the auth flow - }); - } + // Note: auth_success notification hook is now fired inside config.refreshAuth() + // to ensure consistent behavior across interactive and non-interactive modes }, [settings, handleAuthFailure, config, addItem, onAuthChange], ); diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 828ef9c3e..eedf5a8ec 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -36,6 +36,8 @@ import { RipGrepTool } from '../tools/ripGrep.js'; import { logRipgrepFallback } from '../telemetry/loggers.js'; import { RipgrepFallbackEvent } from '../telemetry/types.js'; import { ToolRegistry } from '../tools/tool-registry.js'; +import { fireNotificationHook } from '../core/toolHookTriggers.js'; +import type { MessageBus } from '../confirmation-bus/message-bus.js'; function createToolMock(toolName: string) { const ToolMock = vi.fn(); @@ -195,6 +197,10 @@ vi.mock('../ide/ide-client.js', () => ({ import { BaseLlmClient } from '../core/baseLlmClient.js'; vi.mock('../core/baseLlmClient.js'); +// Mock fireNotificationHook from toolHookTriggers +vi.mock('../core/toolHookTriggers.js', () => ({ + fireNotificationHook: vi.fn().mockResolvedValue({}), +})); describe('Server Config (config.ts)', () => { const MODEL = 'qwen3-coder-plus'; @@ -317,6 +323,64 @@ describe('Server Config (config.ts)', () => { expect(GeminiClient).toHaveBeenCalledWith(config); }); + it('should fire auth_success notification hook when hooks are enabled', async () => { + const mockMessageBus = { request: vi.fn() }; + const config = new Config({ + ...baseParams, + enableHooks: true, + }); + // Set messageBus using the setter + config.setMessageBus(mockMessageBus as unknown as MessageBus); + + const authType = AuthType.USE_GEMINI; + const mockContentConfig = { + apiKey: 'test-key', + model: 'qwen3-coder-plus', + authType, + }; + + vi.mocked(resolveContentGeneratorConfigWithSources).mockReturnValue({ + config: mockContentConfig as ContentGeneratorConfig, + sources: {}, + }); + + await config.refreshAuth(authType); + + // Verify that fireNotificationHook was called with correct parameters + expect(fireNotificationHook).toHaveBeenCalledWith( + mockMessageBus, + `Successfully authenticated with ${authType}`, + 'auth_success', + 'Authentication successful', + ); + }); + + it('should not fire notification hook when hooks are disabled', async () => { + const config = new Config({ + ...baseParams, + enableHooks: false, + }); + const authType = AuthType.USE_GEMINI; + const mockContentConfig = { + apiKey: 'test-key', + model: 'qwen3-coder-plus', + authType, + }; + + vi.mocked(resolveContentGeneratorConfigWithSources).mockReturnValue({ + config: mockContentConfig as ContentGeneratorConfig, + sources: {}, + }); + + // Clear any previous calls + vi.mocked(fireNotificationHook).mockClear(); + + await config.refreshAuth(authType); + + // Verify that fireNotificationHook was not called + expect(fireNotificationHook).not.toHaveBeenCalled(); + }); + it('should not strip thoughts when switching from Vertex to GenAI', async () => { const config = new Config(baseParams); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index bfacde2a0..6a5ea0de1 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -94,9 +94,10 @@ import { } from '../confirmation-bus/types.js'; import { PermissionMode, - type NotificationType, + NotificationType, type PermissionSuggestion, } from '../hooks/types.js'; +import { fireNotificationHook } from '../core/toolHookTriggers.js'; // Utils import { shouldAttemptBrowserLaunch } from '../utils/browser.js'; @@ -982,6 +983,21 @@ export class Config { // Initialize BaseLlmClient now that the ContentGenerator is available this.baseLlmClient = new BaseLlmClient(this.contentGenerator, this); + + // Fire auth_success notification hook (supports both interactive & non-interactive) + const messageBus = this.getMessageBus(); + const hooksEnabled = this.getEnableHooks(); + if (hooksEnabled && messageBus) { + fireNotificationHook( + messageBus, + `Successfully authenticated with ${authMethod}`, + NotificationType.AuthSuccess, + 'Authentication successful', + ).catch(() => { + // Silently ignore errors - fireNotificationHook has internal error handling + // and notification hooks should not block the auth flow + }); + } } /** diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index 0f16f367a..ea14948a9 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -42,6 +42,7 @@ import * as path from 'node:path'; import { MessageBusType } from '../confirmation-bus/types.js'; import type { HookExecutionResponse } from '../confirmation-bus/types.js'; import { type NotificationType } from '../hooks/types.js'; +import type { MessageBus } from '../confirmation-bus/message-bus.js'; vi.mock('fs/promises', () => ({ writeFile: vi.fn(), @@ -2858,7 +2859,7 @@ describe('CoreToolScheduler plan mode with ask_user_question', () => { // Integration tests for the fire* functions describe('Fire hook functions integration', () => { - let mockMessageBus: { request: Mock }; + let mockMessageBus: { request: ReturnType }; beforeEach(() => { mockMessageBus = { @@ -2882,7 +2883,7 @@ describe('Fire hook functions integration', () => { mockMessageBus.request.mockResolvedValue(mockResponse); const result = await firePreToolUseHook( - mockMessageBus, + mockMessageBus as unknown as MessageBus, 'testTool', { param: 'value' }, 'toolu_test', @@ -2921,7 +2922,7 @@ describe('Fire hook functions integration', () => { mockMessageBus.request.mockResolvedValue(mockResponse); const result = await firePreToolUseHook( - mockMessageBus, + mockMessageBus as unknown as MessageBus, 'testTool', { param: 'value' }, 'toolu_test', @@ -2952,7 +2953,7 @@ describe('Fire hook functions integration', () => { mockMessageBus.request.mockRejectedValue(new Error('Network error')); const result = await firePreToolUseHook( - mockMessageBus, + mockMessageBus as unknown as MessageBus, 'testTool', { param: 'value' }, 'toolu_test', @@ -2968,6 +2969,8 @@ describe('Fire hook functions integration', () => { const { firePostToolUseHook } = await import('./toolHookTriggers.js'); const mockResponse: HookExecutionResponse = { + type: MessageBusType.HOOK_EXECUTION_RESPONSE, + correlationId: 'test-correlation-id', success: true, output: { permission_decision: 'proceed', @@ -2977,7 +2980,7 @@ describe('Fire hook functions integration', () => { mockMessageBus.request.mockResolvedValue(mockResponse); const result = await firePostToolUseHook( - mockMessageBus, + mockMessageBus as unknown as MessageBus, 'testTool', { param: 'value' }, { response: 'result' }, @@ -3005,7 +3008,7 @@ describe('Fire hook functions integration', () => { mockMessageBus.request.mockResolvedValue(mockResponse); const result = await firePostToolUseHook( - mockMessageBus, + mockMessageBus as unknown as MessageBus, 'testTool', { param: 'value' }, { response: 'result' }, @@ -3053,7 +3056,7 @@ describe('Fire hook functions integration', () => { mockMessageBus.request.mockResolvedValue(mockResponse); const result = await firePostToolUseFailureHook( - mockMessageBus, + mockMessageBus as unknown as MessageBus, 'toolu_test', 'testTool', { param: 'value' }, @@ -3102,7 +3105,7 @@ describe('Fire hook functions integration', () => { mockMessageBus.request.mockResolvedValue(mockResponse); const result = await fireNotificationHook( - mockMessageBus, + mockMessageBus as unknown as MessageBus, 'Test message', 'info' as NotificationType, 'Test Title', @@ -3155,7 +3158,7 @@ describe('Fire hook functions integration', () => { mockMessageBus.request.mockResolvedValue(mockResponse); const result = await firePermissionRequestHook( - mockMessageBus, + mockMessageBus as unknown as MessageBus, 'testTool', { param: 'value' }, 'full', @@ -3186,7 +3189,7 @@ describe('Fire hook functions integration', () => { mockMessageBus.request.mockResolvedValue(mockResponse); const result = await firePermissionRequestHook( - mockMessageBus, + mockMessageBus as unknown as MessageBus, 'testTool', { param: 'value' }, 'full', @@ -3220,7 +3223,7 @@ describe('Fire hook functions integration', () => { mockMessageBus.request.mockResolvedValue(mockResponse); const result = await firePermissionRequestHook( - mockMessageBus, + mockMessageBus as unknown as MessageBus, 'testTool', { param: 'value' }, 'full',