From ecfaac2dc7e1e9f32338a018a0f87fc1b0615c88 Mon Sep 17 00:00:00 2001 From: Aryan Singh <146713101+dimssu@users.noreply.github.com> Date: Mon, 11 May 2026 22:14:04 +0530 Subject: [PATCH] fix(cli): prevent duplicate SessionStart systemMessage render (#25827) Co-authored-by: Jacob Richman --- packages/cli/src/ui/AppContainer.test.tsx | 36 +++++++ packages/cli/src/ui/AppContainer.tsx | 16 --- .../core/src/hooks/hookEventHandler.test.ts | 97 +++++++++++++++++++ packages/core/src/hooks/hookEventHandler.ts | 5 +- 4 files changed, 136 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/ui/AppContainer.test.tsx b/packages/cli/src/ui/AppContainer.test.tsx index ea9e0629d1..6b1fc93d94 100644 --- a/packages/cli/src/ui/AppContainer.test.tsx +++ b/packages/cli/src/ui/AppContainer.test.tsx @@ -1267,6 +1267,42 @@ describe('AppContainer State Management', () => { }); }); + describe('SessionStart Hook Rendering', () => { + it('does not render systemMessage directly (avoids duplicate with HookSystemMessage event)', async () => { + const mockAddItem = vi.fn(); + mockedUseHistory.mockReturnValue({ + history: [], + addItem: mockAddItem, + updateItem: vi.fn(), + clearItems: vi.fn(), + loadHistory: vi.fn(), + }); + + const fireSessionStartEvent = vi.fn().mockResolvedValue({ + systemMessage: 'Hello from SessionStart hook', + getAdditionalContext: vi.fn(() => undefined), + }); + vi.spyOn(mockConfig, 'getHookSystem').mockReturnValue({ + fireSessionEndEvent: vi.fn().mockResolvedValue(undefined), + fireSessionStartEvent, + } as unknown as ReturnType); + + const { unmount } = await act(async () => renderAppContainer()); + await waitFor(() => expect(fireSessionStartEvent).toHaveBeenCalled()); + + // The direct-render path (the bug) would call addItem with the + // systemMessage text and no `source` field. The HookSystemMessage + // event-listener path (the correct one) always sets `source`. + const directRenderCall = mockAddItem.mock.calls.find( + ([item]) => + item?.text === 'Hello from SessionStart hook' && !item?.source, + ); + expect(directRenderCall).toBeUndefined(); + + unmount(); + }); + }); + describe('Token Counting from Session Stats', () => { it('tracks token counts from session messages', async () => { // Session stats are provided through the SessionStatsProvider context diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index 5c7d4176bc..d77c9adfb6 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -497,16 +497,6 @@ export const AppContainer = (props: AppContainerProps) => { ?.fireSessionStartEvent(sessionStartSource); if (result) { - if (result.systemMessage) { - historyManager.addItem( - { - type: MessageType.INFO, - text: result.systemMessage, - }, - Date.now(), - ); - } - const additionalContext = result.getAdditionalContext(); const geminiClient = config.getGeminiClient(); if (additionalContext && geminiClient) { @@ -549,12 +539,6 @@ export const AppContainer = (props: AppContainerProps) => { debugLogger.error('Error during cleanup:', e), ); }; - // Disable the dependencies check here. historyManager gets flagged - // but we don't want to react to changes to it because each new history - // item, including the ones from the start session hook will cause a - // re-render and an error when we try to reload config. - // - // eslint-disable-next-line react-hooks/exhaustive-deps }, [config, resumedSessionData]); useEffect( diff --git a/packages/core/src/hooks/hookEventHandler.test.ts b/packages/core/src/hooks/hookEventHandler.test.ts index 9e93850101..a47583b316 100644 --- a/packages/core/src/hooks/hookEventHandler.test.ts +++ b/packages/core/src/hooks/hookEventHandler.test.ts @@ -36,6 +36,7 @@ const mockCoreEvents = vi.hoisted(() => ({ emitFeedback: vi.fn(), emitHookStart: vi.fn(), emitHookEnd: vi.fn(), + emitHookSystemMessage: vi.fn(), })); vi.mock('../utils/debugLogger.js', () => ({ @@ -891,4 +892,100 @@ describe('HookEventHandler', () => { ); }); }); + + describe('systemMessage event emission', () => { + const buildMocks = ( + outputFormat: 'json' | 'text', + systemMessage: string, + ) => { + const hookConfig: HookConfig = { + type: HookType.Command, + command: './hook.sh', + timeout: 30000, + }; + const results: HookExecutionResult[] = [ + { + success: true, + duration: 10, + hookConfig, + eventName: HookEventName.SessionStart, + output: { systemMessage }, + outputFormat, + }, + ]; + vi.mocked(mockHookPlanner.createExecutionPlan).mockReturnValue({ + eventName: HookEventName.SessionStart, + hookConfigs: [hookConfig], + sequential: false, + }); + vi.mocked(mockHookRunner.executeHooksParallel).mockResolvedValue(results); + vi.mocked(mockHookAggregator.aggregateResults).mockReturnValue({ + success: true, + allOutputs: [], + errors: [], + totalDuration: 10, + }); + }; + + it('emits HookSystemMessage for json-format hook output', async () => { + buildMocks('json', 'json banner'); + + await hookEventHandler.fireSessionStartEvent(SessionStartSource.Startup); + + expect(mockCoreEvents.emitHookSystemMessage).toHaveBeenCalledTimes(1); + expect(mockCoreEvents.emitHookSystemMessage).toHaveBeenCalledWith( + expect.objectContaining({ + eventName: HookEventName.SessionStart, + message: 'json banner', + }), + ); + }); + + it('emits HookSystemMessage for text-format hook output', async () => { + buildMocks('text', 'plain-text banner'); + + await hookEventHandler.fireSessionStartEvent(SessionStartSource.Startup); + + expect(mockCoreEvents.emitHookSystemMessage).toHaveBeenCalledTimes(1); + expect(mockCoreEvents.emitHookSystemMessage).toHaveBeenCalledWith( + expect.objectContaining({ + eventName: HookEventName.SessionStart, + message: 'plain-text banner', + }), + ); + }); + + it('does not emit when systemMessage is absent', async () => { + const hookConfig: HookConfig = { + type: HookType.Command, + command: './hook.sh', + timeout: 30000, + }; + vi.mocked(mockHookPlanner.createExecutionPlan).mockReturnValue({ + eventName: HookEventName.SessionStart, + hookConfigs: [hookConfig], + sequential: false, + }); + vi.mocked(mockHookRunner.executeHooksParallel).mockResolvedValue([ + { + success: true, + duration: 10, + hookConfig, + eventName: HookEventName.SessionStart, + output: {}, + outputFormat: 'json', + }, + ]); + vi.mocked(mockHookAggregator.aggregateResults).mockReturnValue({ + success: true, + allOutputs: [], + errors: [], + totalDuration: 10, + }); + + await hookEventHandler.fireSessionStartEvent(SessionStartSource.Startup); + + expect(mockCoreEvents.emitHookSystemMessage).not.toHaveBeenCalled(); + }); + }); }); diff --git a/packages/core/src/hooks/hookEventHandler.ts b/packages/core/src/hooks/hookEventHandler.ts index 24dd77d76e..32bd7f8ee0 100644 --- a/packages/core/src/hooks/hookEventHandler.ts +++ b/packages/core/src/hooks/hookEventHandler.ts @@ -459,8 +459,9 @@ export class HookEventHandler { logHookCall(this.context.config, hookCallEvent); - // Emit structured system message event for UI display - if (result.output?.systemMessage && result.outputFormat === 'json') { + // Emit structured system message event for UI display. Covers both + // 'json' and 'text' output formats so plain-text hook stdout also surfaces. + if (result.output?.systemMessage) { coreEvents.emitHookSystemMessage({ hookName, eventName,