From 4f220e945ab3c7b6a119a4c306a683a835e0bea7 Mon Sep 17 00:00:00 2001 From: shishu314 Date: Wed, 22 Oct 2025 17:39:27 -0400 Subject: [PATCH] feat(infra) - Add logging for when user tries to exit multiple times (#11218) Co-authored-by: gemini-cli-robot --- packages/cli/src/ui/AppContainer.test.tsx | 255 ++++++++++---------- packages/cli/src/ui/AppContainer.tsx | 99 ++++---- packages/core/src/telemetry/index.ts | 1 + packages/core/src/telemetry/metrics.test.ts | 24 ++ packages/core/src/telemetry/metrics.ts | 16 ++ 5 files changed, 219 insertions(+), 176 deletions(-) diff --git a/packages/cli/src/ui/AppContainer.test.tsx b/packages/cli/src/ui/AppContainer.test.tsx index abd4b47420..d12dc918fc 100644 --- a/packages/cli/src/ui/AppContainer.test.tsx +++ b/packages/cli/src/ui/AppContainer.test.tsx @@ -105,6 +105,7 @@ import { useSessionStats } from './contexts/SessionContext.js'; import { useTextBuffer } from './components/shared/text-buffer.js'; import { useLogger } from './hooks/useLogger.js'; import { useLoadingIndicator } from './hooks/useLoadingIndicator.js'; +import { useKeypress, type Key } from './hooks/useKeypress.js'; import { measureElement } from 'ink'; import { useTerminalSize } from './hooks/useTerminalSize.js'; import { ShellExecutionService } from '@google/gemini-cli-core'; @@ -136,6 +137,7 @@ describe('AppContainer State Management', () => { const mockedUseTextBuffer = useTextBuffer as Mock; const mockedUseLogger = useLogger as Mock; const mockedUseLoadingIndicator = useLoadingIndicator as Mock; + const mockedUseKeypress = useKeypress as Mock; beforeEach(() => { vi.clearAllMocks(); @@ -1089,16 +1091,55 @@ describe('AppContainer State Management', () => { }); }); - describe('Keyboard Input Handling', () => { - it('should block quit command during authentication', () => { - mockedUseAuthCommand.mockReturnValue({ - authState: 'unauthenticated', - setAuthState: vi.fn(), - authError: null, - onAuthError: vi.fn(), + describe('Keyboard Input Handling (CTRL+C / CTRL+D)', () => { + let handleGlobalKeypress: (key: Key) => void; + let mockHandleSlashCommand: Mock; + let mockCancelOngoingRequest: Mock; + let rerender: () => void; + + // Helper function to reduce boilerplate in tests + const setupKeypressTest = () => { + const { rerender: inkRerender } = render( + , + ); + + rerender = () => + inkRerender( + , + ); + }; + + const pressKey = (key: Partial, times = 1) => { + for (let i = 0; i < times; i++) { + handleGlobalKeypress({ + name: 'c', + ctrl: false, + meta: false, + shift: false, + ...key, + } as Key); + rerender(); + } + }; + + beforeEach(() => { + // Capture the keypress handler from the AppContainer + mockedUseKeypress.mockImplementation((callback: (key: Key) => void) => { + handleGlobalKeypress = callback; }); - const mockHandleSlashCommand = vi.fn(); + // Mock slash command handler + mockHandleSlashCommand = vi.fn(); mockedUseSlashCommandProcessor.mockReturnValue({ handleSlashCommand: mockHandleSlashCommand, slashCommands: [], @@ -1108,86 +1149,10 @@ describe('AppContainer State Management', () => { confirmationRequest: null, }); - render( - , - ); - - expect(mockHandleSlashCommand).not.toHaveBeenCalledWith('/quit'); - }); - - it('should prevent exit command when text buffer has content', () => { - mockedUseTextBuffer.mockReturnValue({ - text: 'some user input', - setText: vi.fn(), - }); - - const mockHandleSlashCommand = vi.fn(); - mockedUseSlashCommandProcessor.mockReturnValue({ - handleSlashCommand: mockHandleSlashCommand, - slashCommands: [], - pendingHistoryItems: [], - commandContext: {}, - shellConfirmationRequest: null, - confirmationRequest: null, - }); - - render( - , - ); - - expect(mockHandleSlashCommand).not.toHaveBeenCalledWith('/quit'); - }); - - it('should require double Ctrl+C to exit when dialogs are open', () => { - vi.useFakeTimers(); - - mockedUseThemeCommand.mockReturnValue({ - isThemeDialogOpen: true, - openThemeDialog: vi.fn(), - handleThemeSelect: vi.fn(), - handleThemeHighlight: vi.fn(), - }); - - const mockHandleSlashCommand = vi.fn(); - mockedUseSlashCommandProcessor.mockReturnValue({ - handleSlashCommand: mockHandleSlashCommand, - slashCommands: [], - pendingHistoryItems: [], - commandContext: {}, - shellConfirmationRequest: null, - confirmationRequest: null, - }); - - render( - , - ); - - expect(mockHandleSlashCommand).not.toHaveBeenCalledWith('/quit'); - - expect(mockHandleSlashCommand).not.toHaveBeenCalledWith('/quit'); - - vi.useRealTimers(); - }); - - it('should cancel ongoing request on first Ctrl+C', () => { - const mockCancelOngoingRequest = vi.fn(); + // Mock request cancellation + mockCancelOngoingRequest = vi.fn(); mockedUseGeminiStream.mockReturnValue({ - streamingState: 'responding', + streamingState: 'idle', submitQuery: vi.fn(), initError: null, pendingHistoryItems: [], @@ -1195,57 +1160,93 @@ describe('AppContainer State Management', () => { cancelOngoingRequest: mockCancelOngoingRequest, }); - const mockHandleSlashCommand = vi.fn(); - mockedUseSlashCommandProcessor.mockReturnValue({ - handleSlashCommand: mockHandleSlashCommand, - slashCommands: [], - pendingHistoryItems: [], - commandContext: {}, - shellConfirmationRequest: null, - confirmationRequest: null, + // Default empty text buffer + mockedUseTextBuffer.mockReturnValue({ + text: '', + setText: vi.fn(), }); - render( - , - ); - - expect(mockHandleSlashCommand).not.toHaveBeenCalledWith('/quit'); + vi.useFakeTimers(); }); - it('should reset Ctrl+C state after timeout', () => { - vi.useFakeTimers(); + afterEach(() => { + vi.useRealTimers(); + }); - const mockHandleSlashCommand = vi.fn(); - mockedUseSlashCommandProcessor.mockReturnValue({ - handleSlashCommand: mockHandleSlashCommand, - slashCommands: [], - pendingHistoryItems: [], - commandContext: {}, - shellConfirmationRequest: null, - confirmationRequest: null, + describe('CTRL+C', () => { + it('should cancel ongoing request on first press', () => { + mockedUseGeminiStream.mockReturnValue({ + streamingState: 'responding', + submitQuery: vi.fn(), + initError: null, + pendingHistoryItems: [], + thought: null, + cancelOngoingRequest: mockCancelOngoingRequest, + }); + setupKeypressTest(); + + pressKey({ name: 'c', ctrl: true }); + + expect(mockCancelOngoingRequest).toHaveBeenCalledTimes(1); + expect(mockHandleSlashCommand).not.toHaveBeenCalled(); }); - render( - , - ); + it('should quit on second press', () => { + setupKeypressTest(); - expect(mockHandleSlashCommand).not.toHaveBeenCalledWith('/quit'); + pressKey({ name: 'c', ctrl: true }, 2); - vi.advanceTimersByTime(1001); + expect(mockCancelOngoingRequest).toHaveBeenCalledTimes(2); + expect(mockHandleSlashCommand).toHaveBeenCalledWith('/quit'); + }); - expect(mockHandleSlashCommand).not.toHaveBeenCalledWith('/quit'); + it('should reset press count after a timeout', () => { + setupKeypressTest(); - vi.useRealTimers(); + pressKey({ name: 'c', ctrl: true }); + expect(mockHandleSlashCommand).not.toHaveBeenCalled(); + + // Advance timer past the reset threshold + vi.advanceTimersByTime(1001); + + pressKey({ name: 'c', ctrl: true }); + expect(mockHandleSlashCommand).not.toHaveBeenCalled(); + }); + }); + + describe('CTRL+D', () => { + it('should do nothing if text buffer is not empty', () => { + mockedUseTextBuffer.mockReturnValue({ + text: 'some text', + setText: vi.fn(), + }); + setupKeypressTest(); + + pressKey({ name: 'd', ctrl: true }, 2); + + expect(mockHandleSlashCommand).not.toHaveBeenCalled(); + }); + + it('should quit on second press if buffer is empty', () => { + setupKeypressTest(); + + pressKey({ name: 'd', ctrl: true }, 2); + + expect(mockHandleSlashCommand).toHaveBeenCalledWith('/quit'); + }); + + it('should reset press count after a timeout', () => { + setupKeypressTest(); + + pressKey({ name: 'd', ctrl: true }); + expect(mockHandleSlashCommand).not.toHaveBeenCalled(); + + // Advance timer past the reset threshold + vi.advanceTimersByTime(1001); + + pressKey({ name: 'd', ctrl: true }); + expect(mockHandleSlashCommand).not.toHaveBeenCalled(); + }); }); }); diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index c8a878129d..135516db6f 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -41,6 +41,7 @@ import { getAllGeminiMdFilenames, AuthType, clearCachedCredentialFile, + recordExitFail, ShellExecutionService, debugLogger, } from '@google/gemini-cli-core'; @@ -798,9 +799,9 @@ Logging in with Google... Please restart Gemini CLI to continue. const [showFullTodos, setShowFullTodos] = useState(false); const [renderMarkdown, setRenderMarkdown] = useState(true); - const [ctrlCPressedOnce, setCtrlCPressedOnce] = useState(false); + const [ctrlCPressCount, setCtrlCPressCount] = useState(0); const ctrlCTimerRef = useRef(null); - const [ctrlDPressedOnce, setCtrlDPressedOnce] = useState(false); + const [ctrlDPressCount, setCtrlDPressCount] = useState(0); const ctrlDTimerRef = useRef(null); const [constrainHeight, setConstrainHeight] = useState(true); const [ideContextState, setIdeContextState] = useState< @@ -878,6 +879,42 @@ Logging in with Google... Please restart Gemini CLI to continue. }; }, [handleNewMessage]); + useEffect(() => { + if (ctrlCTimerRef.current) { + clearTimeout(ctrlCTimerRef.current); + ctrlCTimerRef.current = null; + } + if (ctrlCPressCount > 2) { + recordExitFail(config); + } + if (ctrlCPressCount > 1) { + handleSlashCommand('/quit'); + } else { + ctrlCTimerRef.current = setTimeout(() => { + setCtrlCPressCount(0); + ctrlCTimerRef.current = null; + }, CTRL_EXIT_PROMPT_DURATION_MS); + } + }, [ctrlCPressCount, config, setCtrlCPressCount, handleSlashCommand]); + + useEffect(() => { + if (ctrlDTimerRef.current) { + clearTimeout(ctrlDTimerRef.current); + ctrlCTimerRef.current = null; + } + if (ctrlDPressCount > 2) { + recordExitFail(config); + } + if (ctrlDPressCount > 1) { + handleSlashCommand('/quit'); + } else { + ctrlDTimerRef.current = setTimeout(() => { + setCtrlDPressCount(0); + ctrlDTimerRef.current = null; + }, CTRL_EXIT_PROMPT_DURATION_MS); + } + }, [ctrlDPressCount, config, setCtrlDPressCount, handleSlashCommand]); + const handleEscapePromptChange = useCallback((showPrompt: boolean) => { setShowEscapePrompt(showPrompt); }, []); @@ -908,28 +945,6 @@ Logging in with Google... Please restart Gemini CLI to continue. settings.merged.ui?.customWittyPhrases, ); - const handleExit = useCallback( - ( - pressedOnce: boolean, - setPressedOnce: (value: boolean) => void, - timerRef: React.MutableRefObject, - ) => { - if (pressedOnce) { - if (timerRef.current) { - clearTimeout(timerRef.current); - } - handleSlashCommand('/quit'); - } else { - setPressedOnce(true); - timerRef.current = setTimeout(() => { - setPressedOnce(false); - timerRef.current = null; - }, CTRL_EXIT_PROMPT_DURATION_MS); - } - }, - [handleSlashCommand], - ); - const handleGlobalKeypress = useCallback( (key: Key) => { // Debug log keystrokes if enabled @@ -938,26 +953,17 @@ Logging in with Google... Please restart Gemini CLI to continue. } if (keyMatchers[Command.QUIT](key)) { - if (!ctrlCPressedOnce) { - cancelOngoingRequest?.(); - } + // If the user presses Ctrl+C, we want to cancel any ongoing requests. + // This should happen regardless of the count. + cancelOngoingRequest?.(); - if (!ctrlCPressedOnce) { - setCtrlCPressedOnce(true); - ctrlCTimerRef.current = setTimeout(() => { - setCtrlCPressedOnce(false); - ctrlCTimerRef.current = null; - }, CTRL_EXIT_PROMPT_DURATION_MS); - return; - } - - handleExit(ctrlCPressedOnce, setCtrlCPressedOnce, ctrlCTimerRef); + setCtrlCPressCount((prev) => prev + 1); return; } else if (keyMatchers[Command.EXIT](key)) { if (buffer.text.length > 0) { return; } - handleExit(ctrlDPressedOnce, setCtrlDPressedOnce, ctrlDTimerRef); + setCtrlDPressCount((prev) => prev + 1); return; } @@ -1001,14 +1007,9 @@ Logging in with Google... Please restart Gemini CLI to continue. setShowErrorDetails, config, ideContextState, - handleExit, - ctrlCPressedOnce, - setCtrlCPressedOnce, - ctrlCTimerRef, + setCtrlCPressCount, buffer.text.length, - ctrlDPressedOnce, - setCtrlDPressedOnce, - ctrlDTimerRef, + setCtrlDPressCount, handleSlashCommand, cancelOngoingRequest, activePtyId, @@ -1143,8 +1144,8 @@ Logging in with Google... Please restart Gemini CLI to continue. filteredConsoleMessages, ideContextState, renderMarkdown, - ctrlCPressedOnce, - ctrlDPressedOnce, + ctrlCPressedOnce: ctrlCPressCount >= 1, + ctrlDPressedOnce: ctrlDPressCount >= 1, showEscapePrompt, isFocused, elapsedTime, @@ -1224,8 +1225,8 @@ Logging in with Google... Please restart Gemini CLI to continue. filteredConsoleMessages, ideContextState, renderMarkdown, - ctrlCPressedOnce, - ctrlDPressedOnce, + ctrlCPressCount, + ctrlDPressCount, showEscapePrompt, isFocused, elapsedTime, diff --git a/packages/core/src/telemetry/index.ts b/packages/core/src/telemetry/index.ts index 77af50fe06..dfcf860107 100644 --- a/packages/core/src/telemetry/index.ts +++ b/packages/core/src/telemetry/index.ts @@ -108,6 +108,7 @@ export { // Custom metrics for token usage and API responses recordCustomTokenUsageMetrics, recordCustomApiResponseMetrics, + recordExitFail, // OpenTelemetry GenAI semantic convention for token usage and operation duration recordGenAiClientTokenUsage, recordGenAiClientOperationDuration, diff --git a/packages/core/src/telemetry/metrics.test.ts b/packages/core/src/telemetry/metrics.test.ts index ce0034847d..ee97a8771c 100644 --- a/packages/core/src/telemetry/metrics.test.ts +++ b/packages/core/src/telemetry/metrics.test.ts @@ -92,6 +92,7 @@ describe('Telemetry Metrics', () => { let recordGenAiClientTokenUsageModule: typeof import('./metrics.js').recordGenAiClientTokenUsage; let recordGenAiClientOperationDurationModule: typeof import('./metrics.js').recordGenAiClientOperationDuration; let recordFlickerFrameModule: typeof import('./metrics.js').recordFlickerFrame; + let recordExitFailModule: typeof import('./metrics.js').recordExitFail; let recordAgentRunMetricsModule: typeof import('./metrics.js').recordAgentRunMetrics; beforeEach(async () => { @@ -133,6 +134,7 @@ describe('Telemetry Metrics', () => { recordGenAiClientOperationDurationModule = metricsJsModule.recordGenAiClientOperationDuration; recordFlickerFrameModule = metricsJsModule.recordFlickerFrame; + recordExitFailModule = metricsJsModule.recordExitFail; recordAgentRunMetricsModule = metricsJsModule.recordAgentRunMetrics; const otelApiModule = await import('@opentelemetry/api'); @@ -170,6 +172,28 @@ describe('Telemetry Metrics', () => { }); }); + describe('recordExitFail', () => { + it('does not record metrics if not initialized', () => { + const config = makeFakeConfig({}); + recordExitFailModule(config); + expect(mockCounterAddFn).not.toHaveBeenCalled(); + }); + + it('records a exit fail event when initialized', () => { + const config = makeFakeConfig({}); + initializeMetricsModule(config); + recordExitFailModule(config); + + // Called for session, then for exit fail + expect(mockCounterAddFn).toHaveBeenCalledTimes(2); + expect(mockCounterAddFn).toHaveBeenNthCalledWith(2, 1, { + 'session.id': 'test-session-id', + 'installation.id': 'test-installation-id', + 'user.email': 'test@example.com', + }); + }); + }); + describe('initializeMetrics', () => { const mockConfig = { getSessionId: () => 'test-session-id', diff --git a/packages/core/src/telemetry/metrics.ts b/packages/core/src/telemetry/metrics.ts index 2e9be58155..4123ed5325 100644 --- a/packages/core/src/telemetry/metrics.ts +++ b/packages/core/src/telemetry/metrics.ts @@ -56,6 +56,7 @@ const REGRESSION_PERCENTAGE_CHANGE = 'gemini_cli.performance.regression.percentage_change'; const BASELINE_COMPARISON = 'gemini_cli.performance.baseline.comparison'; const FLICKER_FRAME_COUNT = 'gemini_cli.ui.flicker.count'; +const EXIT_FAIL_COUNT = 'gemini_cli.exit.fail.count'; const baseMetricDefinition = { getCommonAttributes, @@ -175,6 +176,12 @@ const COUNTER_DEFINITIONS = { assign: (c: Counter) => (flickerFrameCounter = c), attributes: {} as Record, }, + [EXIT_FAIL_COUNT]: { + description: 'Counts CLI exit failures.', + valueType: ValueType.INT, + assign: (c: Counter) => (exitFailCounter = c), + attributes: {} as Record, + }, } as const; const HISTOGRAM_DEFINITIONS = { @@ -458,6 +465,7 @@ let agentRunCounter: Counter | undefined; let agentDurationHistogram: Histogram | undefined; let agentTurnsHistogram: Histogram | undefined; let flickerFrameCounter: Counter | undefined; +let exitFailCounter: Counter | undefined; // OpenTelemetry GenAI Semantic Convention Metrics let genAiClientTokenUsageHistogram: Histogram | undefined; @@ -623,6 +631,14 @@ export function recordFlickerFrame(config: Config): void { flickerFrameCounter.add(1, baseMetricDefinition.getCommonAttributes(config)); } +/** + * Records a metric for when user failed to exit + */ +export function recordExitFail(config: Config): void { + if (!exitFailCounter || !isMetricsInitialized) return; + exitFailCounter.add(1, baseMetricDefinition.getCommonAttributes(config)); +} + /** * Records a metric for when an invalid chunk is received from a stream. */