From 10ae84869a394a6214f26e812539f0285af6e72f Mon Sep 17 00:00:00 2001 From: Adib234 <30782825+Adib234@users.noreply.github.com> Date: Mon, 29 Dec 2025 15:46:10 -0500 Subject: [PATCH] Migrate console to coreEvents.emitFeedback or debugLogger (#15219) --- GEMINI.md | 9 + eslint.config.js | 1 + .../src/commands/command-registry.ts | 3 +- .../a2a-server/src/config/settings.test.ts | 3 +- packages/a2a-server/src/http/app.ts | 4 +- packages/cli/src/gemini.test.tsx | 10 +- packages/cli/src/gemini.tsx | 30 +++- packages/cli/src/nonInteractiveCli.test.ts | 17 +- .../cli/src/services/FileCommandLoader.ts | 17 +- packages/cli/src/ui/AppContainer.tsx | 10 +- .../cli/src/ui/IdeIntegrationNudge.test.tsx | 28 ++- .../cli/src/ui/auth/AuthInProgress.test.tsx | 25 ++- .../components/EditorSettingsDialog.test.tsx | 5 +- .../ui/components/EditorSettingsDialog.tsx | 6 +- .../components/IdeTrustChangeDialog.test.tsx | 7 +- .../ui/components/IdeTrustChangeDialog.tsx | 3 +- .../src/ui/components/InputPrompt.test.tsx | 10 +- .../cli/src/ui/components/InputPrompt.tsx | 4 +- .../cli/src/ui/components/Notifications.tsx | 4 +- .../cli/src/ui/components/SettingsDialog.tsx | 8 +- .../src/ui/components/shared/MaxSizedBox.tsx | 10 +- .../src/ui/components/shared/text-buffer.ts | 17 +- .../cli/src/ui/hooks/atCommandProcessor.ts | 2 +- .../ui/hooks/slashCommandProcessor.test.tsx | 2 - .../cli/src/ui/hooks/slashCommandProcessor.ts | 6 - .../cli/src/ui/hooks/useIncludeDirsTrust.tsx | 7 +- packages/cli/src/ui/hooks/useSelectionList.ts | 3 +- .../src/ui/hooks/useSessionBrowser.test.ts | 22 +-- .../cli/src/ui/hooks/useSessionBrowser.ts | 6 +- packages/cli/src/ui/hooks/useShellHistory.ts | 6 +- packages/cli/src/ui/utils/ConsolePatcher.ts | 2 + .../src/ui/utils/InlineMarkdownRenderer.tsx | 3 +- packages/cli/src/utils/errors.test.ts | 166 ++++++++++++++---- packages/cli/src/utils/errors.ts | 19 +- packages/cli/src/utils/relaunch.test.ts | 28 ++- packages/cli/src/utils/relaunch.ts | 7 +- packages/cli/src/utils/sandbox.ts | 22 ++- .../utils/sessionCleanup.integration.test.ts | 8 +- packages/cli/src/utils/sessionCleanup.test.ts | 127 ++------------ packages/cli/src/utils/sessionCleanup.ts | 8 +- packages/cli/src/utils/sessions.test.ts | 82 ++++----- packages/cli/src/utils/sessions.ts | 22 ++- .../src/validateNonInterActiveAuth.test.ts | 17 +- .../core/src/confirmation-bus/message-bus.ts | 3 +- packages/core/src/ide/ide-client.ts | 2 +- packages/core/src/mcp/oauth-utils.ts | 5 +- .../token-storage/keychain-token-storage.ts | 6 +- packages/core/src/policy/config.ts | 3 +- .../strategies/compositeStrategy.test.ts | 24 +-- .../routing/strategies/compositeStrategy.ts | 7 +- .../clearcut-logger/clearcut-logger.ts | 10 +- packages/core/src/telemetry/sdk.ts | 7 +- packages/core/src/tools/edit.test.ts | 2 +- packages/core/src/tools/memoryTool.ts | 7 - packages/core/src/tools/smart-edit.test.ts | 2 +- packages/core/src/tools/tool-registry.ts | 2 +- packages/core/src/tools/web-search.ts | 3 +- packages/core/src/tools/write-file.ts | 3 +- packages/core/src/utils/debugLogger.ts | 1 + packages/core/src/utils/editCorrector.ts | 9 +- .../core/src/utils/errorReporting.test.ts | 39 ++-- packages/core/src/utils/errorReporting.ts | 38 ++-- packages/core/src/utils/getFolderStructure.ts | 5 +- packages/core/src/utils/memoryDiscovery.ts | 2 +- packages/core/src/utils/retry.ts | 8 +- packages/core/src/utils/summarizer.test.ts | 5 +- 66 files changed, 564 insertions(+), 425 deletions(-) diff --git a/GEMINI.md b/GEMINI.md index d37661d7eb..3da20efb75 100644 --- a/GEMINI.md +++ b/GEMINI.md @@ -391,6 +391,15 @@ When working in the `/docs` directory, follow the guidelines in this section: Only write high-value comments if at all. Avoid talking to the user through comments. +## Logging and Error Handling + +- **Avoid Console Statements:** Do not use `console.log`, `console.error`, or + similar methods directly. +- **Non-User-Facing Logs:** For developer-facing debug messages, use + `debugLogger` (from `@google/gemini-cli-core`). +- **User-Facing Feedback:** To surface errors or warnings to the user, use + `coreEvents.emitFeedback` (from `@google/gemini-cli-core`). + ## General requirements - If there is something you do not understand or is ambiguous, seek confirmation diff --git a/eslint.config.js b/eslint.config.js index 996d1fc0f2..79186794bc 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -164,6 +164,7 @@ export default tseslint.config( 'prefer-arrow-callback': 'error', 'prefer-const': ['error', { destructuring: 'all' }], radix: 'error', + 'no-console': 'error', 'default-case': 'error', '@typescript-eslint/await-thenable': ['error'], '@typescript-eslint/no-floating-promises': ['error'], diff --git a/packages/a2a-server/src/commands/command-registry.ts b/packages/a2a-server/src/commands/command-registry.ts index 0643eeefce..47e2800d9d 100644 --- a/packages/a2a-server/src/commands/command-registry.ts +++ b/packages/a2a-server/src/commands/command-registry.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { debugLogger } from '@google/gemini-cli-core'; import { ExtensionsCommand } from './extensions.js'; import { InitCommand } from './init.js'; import { RestoreCommand } from './restore.js'; @@ -20,7 +21,7 @@ class CommandRegistry { register(command: Command) { if (this.commands.has(command.name)) { - console.warn(`Command ${command.name} already registered. Skipping.`); + debugLogger.warn(`Command ${command.name} already registered. Skipping.`); return; } diff --git a/packages/a2a-server/src/config/settings.test.ts b/packages/a2a-server/src/config/settings.test.ts index f9bc876c5a..0aebbb2a94 100644 --- a/packages/a2a-server/src/config/settings.test.ts +++ b/packages/a2a-server/src/config/settings.test.ts @@ -9,6 +9,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import * as os from 'node:os'; import { loadSettings, USER_SETTINGS_PATH } from './settings.js'; +import { debugLogger } from '@google/gemini-cli-core'; const mocks = vi.hoisted(() => { const suffix = Math.random().toString(36).slice(2); @@ -75,7 +76,7 @@ describe('loadSettings', () => { fs.rmSync(mockWorkspaceDir, { recursive: true, force: true }); } } catch (e) { - console.error('Failed to cleanup temp dirs', e); + debugLogger.error('Failed to cleanup temp dirs', e); } vi.restoreAllMocks(); }); diff --git a/packages/a2a-server/src/http/app.ts b/packages/a2a-server/src/http/app.ts index 2439f09b07..8d7be4f7a1 100644 --- a/packages/a2a-server/src/http/app.ts +++ b/packages/a2a-server/src/http/app.ts @@ -25,7 +25,7 @@ import { loadConfig, loadEnvironment, setTargetDir } from '../config/config.js'; import { loadSettings } from '../config/settings.js'; import { loadExtensions } from '../config/extension.js'; import { commandRegistry } from '../commands/command-registry.js'; -import { SimpleExtensionLoader } from '@google/gemini-cli-core'; +import { debugLogger, SimpleExtensionLoader } from '@google/gemini-cli-core'; import type { Command, CommandArgument } from '../commands/types.js'; import { GitService } from '@google/gemini-cli-core'; @@ -239,7 +239,7 @@ export async function createApp() { ): CommandResponse | undefined => { const commandName = command.name; if (visited.includes(commandName)) { - console.warn( + debugLogger.warn( `Command ${commandName} already inserted in the response, skipping`, ); return undefined; diff --git a/packages/cli/src/gemini.test.tsx b/packages/cli/src/gemini.test.tsx index 8b9e91d181..632b15b9a8 100644 --- a/packages/cli/src/gemini.test.tsx +++ b/packages/cli/src/gemini.test.tsx @@ -29,6 +29,7 @@ import { type Config, type ResumedSessionData, debugLogger, + coreEvents, } from '@google/gemini-cli-core'; import { act } from 'react'; import { type InitializationResult } from './core/initializer.js'; @@ -819,9 +820,7 @@ describe('gemini.tsx main function kitty protocol', () => { .mockImplementation((code) => { throw new MockProcessExitError(code); }); - const consoleErrorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); + const emitFeedbackSpy = vi.spyOn(coreEvents, 'emitFeedback'); vi.mocked(loadSettings).mockReturnValue({ merged: { advanced: {}, security: { auth: {} }, ui: { theme: 'test' } }, @@ -875,12 +874,13 @@ describe('gemini.tsx main function kitty protocol', () => { if (!(e instanceof MockProcessExitError)) throw e; } - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(emitFeedbackSpy).toHaveBeenCalledWith( + 'error', expect.stringContaining('Error resuming session: Session not found'), ); expect(processExitSpy).toHaveBeenCalledWith(42); processExitSpy.mockRestore(); - consoleErrorSpy.mockRestore(); + emitFeedbackSpy.mockRestore(); }); it.skip('should log error when cleanupExpiredSessions fails', async () => { diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index f648bc0a4e..3279155d1a 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -41,6 +41,7 @@ import { type ResumedSessionData, type OutputPayload, type ConsoleLogPayload, + type UserFeedbackPayload, sessionId, logUserPrompt, AuthType, @@ -597,7 +598,8 @@ export async function main() { // Use the existing session ID to continue recording to the same session config.setSessionId(resumedSessionData.conversation.sessionId); } catch (error) { - console.error( + coreEvents.emitFeedback( + 'error', `Error resuming session: ${error instanceof Error ? error.message : 'Unknown error'}`, ); await runExitCleanup(); @@ -719,13 +721,25 @@ export function initializeOutputListenersAndFlush() { } }); - coreEvents.on(CoreEvent.ConsoleLog, (payload: ConsoleLogPayload) => { - if (payload.type === 'error' || payload.type === 'warn') { - writeToStderr(payload.content); - } else { - writeToStdout(payload.content); - } - }); + if (coreEvents.listenerCount(CoreEvent.ConsoleLog) === 0) { + coreEvents.on(CoreEvent.ConsoleLog, (payload: ConsoleLogPayload) => { + if (payload.type === 'error' || payload.type === 'warn') { + writeToStderr(payload.content); + } else { + writeToStdout(payload.content); + } + }); + } + + if (coreEvents.listenerCount(CoreEvent.UserFeedback) === 0) { + coreEvents.on(CoreEvent.UserFeedback, (payload: UserFeedbackPayload) => { + if (payload.severity === 'error' || payload.severity === 'warning') { + writeToStderr(payload.message); + } else { + writeToStdout(payload.message); + } + }); + } } coreEvents.drainBacklogs(); } diff --git a/packages/cli/src/nonInteractiveCli.test.ts b/packages/cli/src/nonInteractiveCli.test.ts index 388f3e6454..7c2b61bb55 100644 --- a/packages/cli/src/nonInteractiveCli.test.ts +++ b/packages/cli/src/nonInteractiveCli.test.ts @@ -44,6 +44,7 @@ const mockCoreEvents = vi.hoisted(() => ({ off: vi.fn(), drainBacklogs: vi.fn(), emit: vi.fn(), + emitFeedback: vi.fn(), })); vi.mock('@google/gemini-cli-core', async (importOriginal) => { @@ -785,11 +786,6 @@ describe('runNonInteractive', () => { throw testError; }); - // Mock console.error to capture JSON error output - const consoleErrorJsonSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); - let thrownError: Error | null = null; try { await runNonInteractive({ @@ -807,7 +803,8 @@ describe('runNonInteractive', () => { // Should throw because of mocked process.exit expect(thrownError?.message).toBe('process.exit(1) called'); - expect(consoleErrorJsonSpy).toHaveBeenCalledWith( + expect(mockCoreEvents.emitFeedback).toHaveBeenCalledWith( + 'error', JSON.stringify( { session_id: 'test-session-id', @@ -831,11 +828,6 @@ describe('runNonInteractive', () => { throw fatalError; }); - // Mock console.error to capture JSON error output - const consoleErrorJsonSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); - let thrownError: Error | null = null; try { await runNonInteractive({ @@ -853,7 +845,8 @@ describe('runNonInteractive', () => { // Should throw because of mocked process.exit with custom exit code expect(thrownError?.message).toBe('process.exit(42) called'); - expect(consoleErrorJsonSpy).toHaveBeenCalledWith( + expect(mockCoreEvents.emitFeedback).toHaveBeenCalledWith( + 'error', JSON.stringify( { session_id: 'test-session-id', diff --git a/packages/cli/src/services/FileCommandLoader.ts b/packages/cli/src/services/FileCommandLoader.ts index a8da797e36..688fb0ce0e 100644 --- a/packages/cli/src/services/FileCommandLoader.ts +++ b/packages/cli/src/services/FileCommandLoader.ts @@ -10,7 +10,7 @@ import toml from '@iarna/toml'; import { glob } from 'glob'; import { z } from 'zod'; import type { Config } from '@google/gemini-cli-core'; -import { Storage } from '@google/gemini-cli-core'; +import { Storage, coreEvents } from '@google/gemini-cli-core'; import type { ICommandLoader } from './types.js'; import type { CommandContext, @@ -126,7 +126,8 @@ export class FileCommandLoader implements ICommandLoader { !signal.aborted && (error as { code?: string })?.code !== 'ENOENT' ) { - console.error( + coreEvents.emitFeedback( + 'error', `[FileCommandLoader] Error loading commands from ${dirInfo.path}:`, error, ); @@ -189,7 +190,8 @@ export class FileCommandLoader implements ICommandLoader { try { fileContent = await fs.readFile(filePath, 'utf-8'); } catch (error: unknown) { - console.error( + coreEvents.emitFeedback( + 'error', `[FileCommandLoader] Failed to read file ${filePath}:`, error instanceof Error ? error.message : String(error), ); @@ -200,7 +202,8 @@ export class FileCommandLoader implements ICommandLoader { try { parsed = toml.parse(fileContent); } catch (error: unknown) { - console.error( + coreEvents.emitFeedback( + 'error', `[FileCommandLoader] Failed to parse TOML file ${filePath}:`, error instanceof Error ? error.message : String(error), ); @@ -210,7 +213,8 @@ export class FileCommandLoader implements ICommandLoader { const validationResult = TomlCommandDefSchema.safeParse(parsed); if (!validationResult.success) { - console.error( + coreEvents.emitFeedback( + 'error', `[FileCommandLoader] Skipping invalid command file: ${filePath}. Validation errors:`, validationResult.error.flatten(), ); @@ -278,7 +282,8 @@ export class FileCommandLoader implements ICommandLoader { _args: string, ): Promise => { if (!context.invocation) { - console.error( + coreEvents.emitFeedback( + 'error', `[FileCommandLoader] Critical error: Command '${baseCommandName}' was executed without invocation context.`, ); return { diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index 4986a22617..bba0f1fd4e 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -842,16 +842,8 @@ Logging in with Google... Restarting Gemini CLI to continue. const handleClearScreen = useCallback(() => { historyManager.clearItems(); clearConsoleMessagesState(); - if (!isAlternateBuffer) { - console.clear(); - } refreshStatic(); - }, [ - historyManager, - clearConsoleMessagesState, - refreshStatic, - isAlternateBuffer, - ]); + }, [historyManager, clearConsoleMessagesState, refreshStatic]); const { handleInput: vimHandleInput } = useVim(buffer, handleFinalSubmit); diff --git a/packages/cli/src/ui/IdeIntegrationNudge.test.tsx b/packages/cli/src/ui/IdeIntegrationNudge.test.tsx index a91f2718be..1d39828b14 100644 --- a/packages/cli/src/ui/IdeIntegrationNudge.test.tsx +++ b/packages/cli/src/ui/IdeIntegrationNudge.test.tsx @@ -4,14 +4,30 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi, afterEach } from 'vitest'; -import { render } from 'ink-testing-library'; +import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest'; +import { render } from '../test-utils/render.js'; import { act } from 'react'; import { IdeIntegrationNudge } from './IdeIntegrationNudge.js'; import { KeypressProvider } from './contexts/KeypressContext.js'; +import { debugLogger } from '@google/gemini-cli-core'; const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); +// Mock debugLogger +vi.mock('@google/gemini-cli-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + debugLogger: { + log: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + }, + }; +}); + describe('IdeIntegrationNudge', () => { const defaultProps = { ide: { @@ -21,24 +37,20 @@ describe('IdeIntegrationNudge', () => { onComplete: vi.fn(), }; - const originalError = console.error; - afterEach(() => { - console.error = originalError; vi.restoreAllMocks(); vi.unstubAllEnvs(); }); beforeEach(() => { - console.error = (...args) => { + vi.mocked(debugLogger.warn).mockImplementation((...args) => { if ( typeof args[0] === 'string' && /was not wrapped in act/.test(args[0]) ) { return; } - originalError.call(console, ...args); - }; + }); vi.stubEnv('GEMINI_CLI_IDE_SERVER_PORT', ''); vi.stubEnv('GEMINI_CLI_IDE_WORKSPACE_PATH', ''); }); diff --git a/packages/cli/src/ui/auth/AuthInProgress.test.tsx b/packages/cli/src/ui/auth/AuthInProgress.test.tsx index 958cc9f82d..4c5f0490c2 100644 --- a/packages/cli/src/ui/auth/AuthInProgress.test.tsx +++ b/packages/cli/src/ui/auth/AuthInProgress.test.tsx @@ -5,12 +5,27 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { render } from 'ink-testing-library'; +import { render } from '../../test-utils/render.js'; import { act } from 'react'; import { AuthInProgress } from './AuthInProgress.js'; import { useKeypress, type Key } from '../hooks/useKeypress.js'; +import { debugLogger } from '@google/gemini-cli-core'; // Mock dependencies +vi.mock('@google/gemini-cli-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + debugLogger: { + log: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + }, + }; +}); + vi.mock('../hooks/useKeypress.js', () => ({ useKeypress: vi.fn(), })); @@ -22,24 +37,20 @@ vi.mock('../components/CliSpinner.js', () => ({ describe('AuthInProgress', () => { const onTimeout = vi.fn(); - const originalError = console.error; - beforeEach(() => { vi.clearAllMocks(); vi.useFakeTimers(); - console.error = (...args) => { + vi.mocked(debugLogger.error).mockImplementation((...args) => { if ( typeof args[0] === 'string' && args[0].includes('was not wrapped in act') ) { return; } - originalError.call(console, ...args); - }; + }); }); afterEach(() => { - console.error = originalError; vi.useRealTimers(); }); diff --git a/packages/cli/src/ui/components/EditorSettingsDialog.test.tsx b/packages/cli/src/ui/components/EditorSettingsDialog.test.tsx index 9aeff70d93..ac5a16580b 100644 --- a/packages/cli/src/ui/components/EditorSettingsDialog.test.tsx +++ b/packages/cli/src/ui/components/EditorSettingsDialog.test.tsx @@ -12,6 +12,7 @@ import type { LoadedSettings } from '../../config/settings.js'; import { KeypressProvider } from '../contexts/KeypressContext.js'; import { act } from 'react'; import { waitFor } from '../../test-utils/async.js'; +import { debugLogger } from '@google/gemini-cli-core'; vi.mock('@google/gemini-cli-core', async (importOriginal) => { const actual = @@ -101,7 +102,7 @@ describe('EditorSettingsDialog', () => { await waitFor(() => { const frame = lastFrame() || ''; if (!frame.includes('> Apply To')) { - console.log( + debugLogger.debug( 'Waiting for scope focus. Current frame:', JSON.stringify(frame), ); @@ -166,7 +167,7 @@ describe('EditorSettingsDialog', () => { const frame = lastFrame() || ''; if (!frame.includes('(Also modified')) { - console.log( + debugLogger.debug( 'Modified message test failure. Frame:', JSON.stringify(frame), ); diff --git a/packages/cli/src/ui/components/EditorSettingsDialog.tsx b/packages/cli/src/ui/components/EditorSettingsDialog.tsx index ed15376de1..6886d43319 100644 --- a/packages/cli/src/ui/components/EditorSettingsDialog.tsx +++ b/packages/cli/src/ui/components/EditorSettingsDialog.tsx @@ -24,6 +24,7 @@ import { EDITOR_DISPLAY_NAMES, } from '@google/gemini-cli-core'; import { useKeypress } from '../hooks/useKeypress.js'; +import { coreEvents } from '@google/gemini-cli-core'; interface EditorDialogProps { onSelect: ( @@ -68,7 +69,10 @@ export function EditorSettingsDialog({ ) : 0; if (editorIndex === -1) { - console.error(`Editor is not supported: ${currentPreference}`); + coreEvents.emitFeedback( + 'error', + `Editor is not supported: ${currentPreference}`, + ); editorIndex = 0; } diff --git a/packages/cli/src/ui/components/IdeTrustChangeDialog.test.tsx b/packages/cli/src/ui/components/IdeTrustChangeDialog.test.tsx index 4f63095512..18479128d6 100644 --- a/packages/cli/src/ui/components/IdeTrustChangeDialog.test.tsx +++ b/packages/cli/src/ui/components/IdeTrustChangeDialog.test.tsx @@ -8,6 +8,7 @@ import { vi, describe, it, expect, beforeEach } from 'vitest'; import * as processUtils from '../../utils/processUtils.js'; import { renderWithProviders } from '../../test-utils/render.js'; import { IdeTrustChangeDialog } from './IdeTrustChangeDialog.js'; +import { debugLogger } from '@google/gemini-cli-core'; describe('IdeTrustChangeDialog', () => { beforeEach(() => { @@ -39,8 +40,8 @@ describe('IdeTrustChangeDialog', () => { }); it('renders a generic message and logs an error for NONE reason', () => { - const consoleErrorSpy = vi - .spyOn(console, 'error') + const debugLoggerWarnSpy = vi + .spyOn(debugLogger, 'warn') .mockImplementation(() => {}); const { lastFrame } = renderWithProviders( , @@ -48,7 +49,7 @@ describe('IdeTrustChangeDialog', () => { const frameText = lastFrame(); expect(frameText).toContain('Workspace trust has changed.'); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerWarnSpy).toHaveBeenCalledWith( 'IdeTrustChangeDialog rendered with unexpected reason "NONE"', ); }); diff --git a/packages/cli/src/ui/components/IdeTrustChangeDialog.tsx b/packages/cli/src/ui/components/IdeTrustChangeDialog.tsx index 2412840ec8..5ef6e76f2a 100644 --- a/packages/cli/src/ui/components/IdeTrustChangeDialog.tsx +++ b/packages/cli/src/ui/components/IdeTrustChangeDialog.tsx @@ -9,6 +9,7 @@ import { theme } from '../semantic-colors.js'; import { useKeypress } from '../hooks/useKeypress.js'; import { relaunchApp } from '../../utils/processUtils.js'; import { type RestartReason } from '../hooks/useIdeTrustListener.js'; +import { debugLogger } from '@google/gemini-cli-core'; interface IdeTrustChangeDialogProps { reason: RestartReason; @@ -28,7 +29,7 @@ export const IdeTrustChangeDialog = ({ reason }: IdeTrustChangeDialogProps) => { let message = 'Workspace trust has changed.'; if (reason === 'NONE') { // This should not happen, but provides a fallback and a debug log. - console.error( + debugLogger.warn( 'IdeTrustChangeDialog rendered with unexpected reason "NONE"', ); } else if (reason === 'CONNECTION_CHANGE') { diff --git a/packages/cli/src/ui/components/InputPrompt.test.tsx b/packages/cli/src/ui/components/InputPrompt.test.tsx index 0b48366d08..35ac7d4a71 100644 --- a/packages/cli/src/ui/components/InputPrompt.test.tsx +++ b/packages/cli/src/ui/components/InputPrompt.test.tsx @@ -15,7 +15,7 @@ import { calculateTransformedLine, } from './shared/text-buffer.js'; import type { Config } from '@google/gemini-cli-core'; -import { ApprovalMode } from '@google/gemini-cli-core'; +import { ApprovalMode, debugLogger } from '@google/gemini-cli-core'; import * as path from 'node:path'; import type { CommandContext, SlashCommand } from '../commands/types.js'; import { CommandKind } from '../commands/types.js'; @@ -577,8 +577,8 @@ describe('InputPrompt', () => { }); it('should handle errors during clipboard operations', async () => { - const consoleErrorSpy = vi - .spyOn(console, 'error') + const debugLoggerErrorSpy = vi + .spyOn(debugLogger, 'error') .mockImplementation(() => {}); vi.mocked(clipboardUtils.clipboardHasImage).mockRejectedValue( new Error('Clipboard error'), @@ -592,14 +592,14 @@ describe('InputPrompt', () => { stdin.write('\x16'); // Ctrl+V }); await waitFor(() => { - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerErrorSpy).toHaveBeenCalledWith( 'Error handling clipboard image:', expect.any(Error), ); }); expect(mockBuffer.setText).not.toHaveBeenCalled(); - consoleErrorSpy.mockRestore(); + debugLoggerErrorSpy.mockRestore(); unmount(); }); }); diff --git a/packages/cli/src/ui/components/InputPrompt.tsx b/packages/cli/src/ui/components/InputPrompt.tsx index 5aeb0078ab..8aac3d0fd4 100644 --- a/packages/cli/src/ui/components/InputPrompt.tsx +++ b/packages/cli/src/ui/components/InputPrompt.tsx @@ -24,7 +24,7 @@ import { useKeypress } from '../hooks/useKeypress.js'; import { keyMatchers, Command } from '../keyMatchers.js'; import type { CommandContext, SlashCommand } from '../commands/types.js'; import type { Config } from '@google/gemini-cli-core'; -import { ApprovalMode } from '@google/gemini-cli-core'; +import { ApprovalMode, debugLogger } from '@google/gemini-cli-core'; import { parseInputForHighlighting, parseSegmentsFromTokens, @@ -354,7 +354,7 @@ export const InputPrompt: React.FC = ({ const offset = buffer.getOffset(); buffer.replaceRangeByOffset(offset, offset, textToInsert); } catch (error) { - console.error('Error handling clipboard image:', error); + debugLogger.error('Error handling clipboard image:', error); } }, [buffer, config]); diff --git a/packages/cli/src/ui/components/Notifications.tsx b/packages/cli/src/ui/components/Notifications.tsx index 3517d383f2..f36e9708fc 100644 --- a/packages/cli/src/ui/components/Notifications.tsx +++ b/packages/cli/src/ui/components/Notifications.tsx @@ -12,7 +12,7 @@ import { theme } from '../semantic-colors.js'; import { StreamingState } from '../types.js'; import { UpdateNotification } from './UpdateNotification.js'; -import { GEMINI_DIR, Storage } from '@google/gemini-cli-core'; +import { GEMINI_DIR, Storage, debugLogger } from '@google/gemini-cli-core'; import * as fs from 'node:fs/promises'; import os from 'node:os'; @@ -66,7 +66,7 @@ export const Notifications = () => { }); await fs.writeFile(screenReaderNudgeFilePath, 'true'); } catch (error) { - console.error('Error storing screen reader nudge', error); + debugLogger.error('Error storing screen reader nudge', error); } } }; diff --git a/packages/cli/src/ui/components/SettingsDialog.tsx b/packages/cli/src/ui/components/SettingsDialog.tsx index edcd5ee215..1b1235edde 100644 --- a/packages/cli/src/ui/components/SettingsDialog.tsx +++ b/packages/cli/src/ui/components/SettingsDialog.tsx @@ -42,7 +42,7 @@ import { type SettingsValue, TOGGLE_TYPES, } from '../../config/settingsSchema.js'; -import { debugLogger } from '@google/gemini-cli-core'; +import { coreEvents, debugLogger } from '@google/gemini-cli-core'; import { keyMatchers, Command } from '../keyMatchers.js'; import type { Config } from '@google/gemini-cli-core'; import { useUIState } from '../contexts/UIStateContext.js'; @@ -254,7 +254,11 @@ export function SettingsDialog({ if (key === 'general.vimMode' && newValue !== vimEnabled) { // Call toggleVimEnabled to sync the VimModeContext local state toggleVimEnabled().catch((error) => { - console.error('Failed to toggle vim mode:', error); + coreEvents.emitFeedback( + 'error', + 'Failed to toggle vim mode:', + error, + ); }); } diff --git a/packages/cli/src/ui/components/shared/MaxSizedBox.tsx b/packages/cli/src/ui/components/shared/MaxSizedBox.tsx index 86b36cd28d..d8b15d104d 100644 --- a/packages/cli/src/ui/components/shared/MaxSizedBox.tsx +++ b/packages/cli/src/ui/components/shared/MaxSizedBox.tsx @@ -10,6 +10,7 @@ import stringWidth from 'string-width'; import { theme } from '../../semantic-colors.js'; import { toCodePoints } from '../../utils/textUtils.js'; import { useOverflowActions } from '../../contexts/OverflowContext.js'; +import { debugLogger } from '@google/gemini-cli-core'; let enableDebugLog = false; @@ -28,7 +29,7 @@ function debugReportError(message: string, element: React.ReactNode) { if (!enableDebugLog) return; if (!React.isValidElement(element)) { - console.error( + debugLogger.warn( message, `Invalid element: '${String(element)}' typeof=${typeof element}`, ); @@ -44,10 +45,13 @@ function debugReportError(message: string, element: React.ReactNode) { const lineNumber = elementWithSource._source?.lineNumber; sourceMessage = fileName ? `${fileName}:${lineNumber}` : ''; } catch (error) { - console.error('Error while trying to get file name:', error); + debugLogger.warn('Error while trying to get file name:', error); } - console.error(message, `${String(element.type)}. Source: ${sourceMessage}`); + debugLogger.warn( + message, + `${String(element.type)}. Source: ${sourceMessage}`, + ); } interface MaxSizedBoxProps { children?: React.ReactNode; diff --git a/packages/cli/src/ui/components/shared/text-buffer.ts b/packages/cli/src/ui/components/shared/text-buffer.ts index 637a684899..abcdbdc420 100644 --- a/packages/cli/src/ui/components/shared/text-buffer.ts +++ b/packages/cli/src/ui/components/shared/text-buffer.ts @@ -10,7 +10,12 @@ import os from 'node:os'; import pathMod from 'node:path'; import * as path from 'node:path'; import { useState, useCallback, useEffect, useMemo, useReducer } from 'react'; -import { coreEvents, CoreEvent, unescapePath } from '@google/gemini-cli-core'; +import { + coreEvents, + CoreEvent, + debugLogger, + unescapePath, +} from '@google/gemini-cli-core'; import { toCodePoints, cpLen, @@ -1411,7 +1416,7 @@ function textBufferReducerLogic( break; default: { const exhaustiveCheck: never = dir; - console.error( + debugLogger.error( `Unknown visual movement direction: ${exhaustiveCheck}`, ); return state; @@ -1751,7 +1756,7 @@ function textBufferReducerLogic( default: { const exhaustiveCheck: never = action; - console.error(`Unknown action encountered: ${exhaustiveCheck}`); + debugLogger.error(`Unknown action encountered: ${exhaustiveCheck}`); return state; } } @@ -2173,7 +2178,11 @@ export function useTextBuffer({ newText = newText.replace(/\r\n?/g, '\n'); dispatch({ type: 'set_text', payload: newText, pushToUndo: false }); } catch (err) { - console.error('[useTextBuffer] external editor error', err); + coreEvents.emitFeedback( + 'error', + '[useTextBuffer] external editor error', + err, + ); } finally { coreEvents.emit(CoreEvent.ExternalEditorClosed); if (wasRaw) setRawMode?.(true); diff --git a/packages/cli/src/ui/hooks/atCommandProcessor.ts b/packages/cli/src/ui/hooks/atCommandProcessor.ts index 99a9cfa95d..40fb42ea44 100644 --- a/packages/cli/src/ui/hooks/atCommandProcessor.ts +++ b/packages/cli/src/ui/hooks/atCommandProcessor.ts @@ -502,7 +502,7 @@ export async function handleAtCommand({ const errorMessages = resourceReadDisplays .filter((d) => d.status === ToolCallStatus.Error) .map((d) => d.resultDisplay); - console.error(errorMessages); + debugLogger.error(errorMessages); const errorMsg = `Exiting due to an error processing the @ command: ${firstError.resultDisplay}`; return { processedQuery: null, error: errorMsg }; } diff --git a/packages/cli/src/ui/hooks/slashCommandProcessor.test.tsx b/packages/cli/src/ui/hooks/slashCommandProcessor.test.tsx index 154fe5cfc0..476a414717 100644 --- a/packages/cli/src/ui/hooks/slashCommandProcessor.test.tsx +++ b/packages/cli/src/ui/hooks/slashCommandProcessor.test.tsx @@ -244,7 +244,6 @@ describe('useSlashCommandProcessor', () => { }); expect(mockClearItems).toHaveBeenCalled(); - expect(console.clear).not.toHaveBeenCalled(); }); it('should call console.clear if alternate buffer is not active', async () => { @@ -262,7 +261,6 @@ describe('useSlashCommandProcessor', () => { }); expect(mockClearItems).toHaveBeenCalled(); - expect(console.clear).toHaveBeenCalled(); }); }); diff --git a/packages/cli/src/ui/hooks/slashCommandProcessor.ts b/packages/cli/src/ui/hooks/slashCommandProcessor.ts index f85ec21c6b..c3ead63f87 100644 --- a/packages/cli/src/ui/hooks/slashCommandProcessor.ts +++ b/packages/cli/src/ui/hooks/slashCommandProcessor.ts @@ -50,7 +50,6 @@ import { type ExtensionUpdateStatus, } from '../state/extensions.js'; import { appEvents } from '../../utils/events.js'; -import { useAlternateBuffer } from './useAlternateBuffer.js'; import { LogoutConfirmationDialog, LogoutChoice, @@ -96,7 +95,6 @@ export const useSlashCommandProcessor = ( const [commands, setCommands] = useState( undefined, ); - const alternateBuffer = useAlternateBuffer(); const [reloadTrigger, setReloadTrigger] = useState(0); const reloadCommands = useCallback(() => { @@ -212,9 +210,6 @@ export const useSlashCommandProcessor = ( addItem, clear: () => { clearItems(); - if (!alternateBuffer) { - console.clear(); - } refreshStatic(); setBannerVisible(false); }, @@ -238,7 +233,6 @@ export const useSlashCommandProcessor = ( }, }), [ - alternateBuffer, config, settings, gitService, diff --git a/packages/cli/src/ui/hooks/useIncludeDirsTrust.tsx b/packages/cli/src/ui/hooks/useIncludeDirsTrust.tsx index daad658ae0..53f613898b 100644 --- a/packages/cli/src/ui/hooks/useIncludeDirsTrust.tsx +++ b/packages/cli/src/ui/hooks/useIncludeDirsTrust.tsx @@ -8,7 +8,10 @@ import { useEffect } from 'react'; import type { Config } from '@google/gemini-cli-core'; import { loadTrustedFolders } from '../../config/trustedFolders.js'; import { expandHomeDir } from '../utils/directoryUtils.js'; -import { refreshServerHierarchicalMemory } from '@google/gemini-cli-core'; +import { + debugLogger, + refreshServerHierarchicalMemory, +} from '@google/gemini-cli-core'; import { MultiFolderTrustDialog } from '../components/MultiFolderTrustDialog.js'; import type { UseHistoryManagerReturn } from './useHistoryManager.js'; import { MessageType, type HistoryItem } from '../types.js'; @@ -133,7 +136,7 @@ export function useIncludeDirsTrust( } if (undefinedTrustDirs.length > 0) { - console.log( + debugLogger.log( 'Creating custom dialog with undecidedDirs:', undefinedTrustDirs, ); diff --git a/packages/cli/src/ui/hooks/useSelectionList.ts b/packages/cli/src/ui/hooks/useSelectionList.ts index 85fe5d6f88..11ce449f11 100644 --- a/packages/cli/src/ui/hooks/useSelectionList.ts +++ b/packages/cli/src/ui/hooks/useSelectionList.ts @@ -7,6 +7,7 @@ import { useReducer, useRef, useEffect, useCallback } from 'react'; import { useKeypress, type Key } from './useKeypress.js'; import { keyMatchers, Command } from '../keyMatchers.js'; +import { debugLogger } from '@google/gemini-cli-core'; export interface SelectionListItem { key: string; @@ -198,7 +199,7 @@ function selectionListReducer( default: { const exhaustiveCheck: never = action; - console.error(`Unknown selection list action: ${exhaustiveCheck}`); + debugLogger.warn(`Unknown selection list action: ${exhaustiveCheck}`); return state; } } diff --git a/packages/cli/src/ui/hooks/useSessionBrowser.test.ts b/packages/cli/src/ui/hooks/useSessionBrowser.test.ts index 80df6f3290..a376d525c9 100644 --- a/packages/cli/src/ui/hooks/useSessionBrowser.test.ts +++ b/packages/cli/src/ui/hooks/useSessionBrowser.test.ts @@ -19,6 +19,7 @@ import type { ConversationRecord, MessageRecord, } from '@google/gemini-cli-core'; +import { coreEvents } from '@google/gemini-cli-core'; // Mock modules vi.mock('fs/promises'); @@ -52,6 +53,7 @@ describe('useSessionBrowser', () => { beforeEach(() => { vi.resetAllMocks(); + vi.spyOn(coreEvents, 'emitFeedback').mockImplementation(() => {}); mockedPath.join.mockImplementation((...args) => args.join('/')); vi.mocked(mockConfig.storage.getProjectTempDir).mockReturnValue( MOCKED_PROJECT_TEMP_DIR, @@ -100,9 +102,6 @@ describe('useSessionBrowser', () => { fileName: MOCKED_FILENAME, } as SessionInfo; mockedFs.readFile.mockRejectedValue(new Error('File not found')); - const consoleErrorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); const { result } = renderHook(() => useSessionBrowser(mockConfig, mockOnLoadHistory), @@ -112,9 +111,12 @@ describe('useSessionBrowser', () => { await result.current.handleResumeSession(mockSession); }); - expect(consoleErrorSpy).toHaveBeenCalled(); + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'error', + 'Error resuming session:', + expect.any(Error), + ); expect(result.current.isSessionBrowserOpen).toBe(false); - consoleErrorSpy.mockRestore(); }); it('should handle JSON parse error', async () => { @@ -124,9 +126,6 @@ describe('useSessionBrowser', () => { fileName: MOCKED_FILENAME, } as SessionInfo; mockedFs.readFile.mockResolvedValue('invalid json'); - const consoleErrorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); const { result } = renderHook(() => useSessionBrowser(mockConfig, mockOnLoadHistory), @@ -136,9 +135,12 @@ describe('useSessionBrowser', () => { await result.current.handleResumeSession(mockSession); }); - expect(consoleErrorSpy).toHaveBeenCalled(); + expect(coreEvents.emitFeedback).toHaveBeenCalledWith( + 'error', + 'Error resuming session:', + expect.any(Error), + ); expect(result.current.isSessionBrowserOpen).toBe(false); - consoleErrorSpy.mockRestore(); }); }); diff --git a/packages/cli/src/ui/hooks/useSessionBrowser.ts b/packages/cli/src/ui/hooks/useSessionBrowser.ts index 8d048b3efe..1dbced887d 100644 --- a/packages/cli/src/ui/hooks/useSessionBrowser.ts +++ b/packages/cli/src/ui/hooks/useSessionBrowser.ts @@ -14,7 +14,7 @@ import type { ResumedSessionData, } from '@google/gemini-cli-core'; import type { Part } from '@google/genai'; -import { partListUnionToString } from '@google/gemini-cli-core'; +import { partListUnionToString, coreEvents } from '@google/gemini-cli-core'; import type { SessionInfo } from '../../utils/sessionUtils.js'; import { MessageType, ToolCallStatus } from '../types.js'; @@ -79,7 +79,7 @@ export const useSessionBrowser = ( resumedSessionData, ); } catch (error) { - console.error('Error resuming session:', error); + coreEvents.emitFeedback('error', 'Error resuming session:', error); setIsSessionBrowserOpen(false); } }, @@ -103,7 +103,7 @@ export const useSessionBrowser = ( chatRecordingService.deleteSession(session.file); } } catch (error) { - console.error('Error deleting session:', error); + coreEvents.emitFeedback('error', 'Error deleting session:', error); throw error; } }, diff --git a/packages/cli/src/ui/hooks/useShellHistory.ts b/packages/cli/src/ui/hooks/useShellHistory.ts index 113ff5c9d2..a341606c4f 100644 --- a/packages/cli/src/ui/hooks/useShellHistory.ts +++ b/packages/cli/src/ui/hooks/useShellHistory.ts @@ -7,7 +7,7 @@ import { useState, useEffect, useCallback } from 'react'; import * as fs from 'node:fs/promises'; import * as path from 'node:path'; -import { isNodeError, Storage } from '@google/gemini-cli-core'; +import { debugLogger, isNodeError, Storage } from '@google/gemini-cli-core'; const MAX_HISTORY_LENGTH = 100; @@ -52,7 +52,7 @@ async function readHistoryFile(filePath: string): Promise { return result; } catch (err) { if (isNodeError(err) && err.code === 'ENOENT') return []; - console.error('Error reading history:', err); + debugLogger.error('Error reading history:', err); return []; } } @@ -65,7 +65,7 @@ async function writeHistoryFile( await fs.mkdir(path.dirname(filePath), { recursive: true }); await fs.writeFile(filePath, history.join('\n')); } catch (error) { - console.error('Error writing shell history:', error); + debugLogger.error('Error writing shell history:', error); } } diff --git a/packages/cli/src/ui/utils/ConsolePatcher.ts b/packages/cli/src/ui/utils/ConsolePatcher.ts index 7b6a09875e..3674c5614e 100644 --- a/packages/cli/src/ui/utils/ConsolePatcher.ts +++ b/packages/cli/src/ui/utils/ConsolePatcher.ts @@ -4,6 +4,8 @@ * SPDX-License-Identifier: Apache-2.0 */ +/* eslint-disable no-console */ + import util from 'node:util'; import type { ConsoleMessageItem } from '../types.js'; diff --git a/packages/cli/src/ui/utils/InlineMarkdownRenderer.tsx b/packages/cli/src/ui/utils/InlineMarkdownRenderer.tsx index 370b03d6f5..8d4c6a7da6 100644 --- a/packages/cli/src/ui/utils/InlineMarkdownRenderer.tsx +++ b/packages/cli/src/ui/utils/InlineMarkdownRenderer.tsx @@ -8,6 +8,7 @@ import React from 'react'; import { Text } from 'ink'; import { theme } from '../semantic-colors.js'; import stringWidth from 'string-width'; +import { debugLogger } from '@google/gemini-cli-core'; // Constants for Markdown parsing const BOLD_MARKER_LENGTH = 2; // For "**" @@ -144,7 +145,7 @@ const RenderInlineInternal: React.FC = ({ ); } } catch (e) { - console.error('Error parsing inline markdown part:', fullMatch, e); + debugLogger.warn('Error parsing inline markdown part:', fullMatch, e); renderedNode = null; } diff --git a/packages/cli/src/utils/errors.test.ts b/packages/cli/src/utils/errors.test.ts index 8c65efe1b3..c5b7a7e7fe 100644 --- a/packages/cli/src/utils/errors.test.ts +++ b/packages/cli/src/utils/errors.test.ts @@ -4,9 +4,22 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { vi, type MockInstance } from 'vitest'; +import { + vi, + type MockInstance, + describe, + it, + expect, + beforeEach, + afterEach, +} from 'vitest'; import type { Config } from '@google/gemini-cli-core'; -import { OutputFormat, FatalInputError } from '@google/gemini-cli-core'; +import { + OutputFormat, + FatalInputError, + debugLogger, + coreEvents, +} from '@google/gemini-cli-core'; import { getErrorMessage, handleError, @@ -14,6 +27,12 @@ import { handleCancellationError, handleMaxTurnsExceededError, } from './errors.js'; +import { runSyncCleanup } from './cleanup.js'; + +// Mock the cleanup module +vi.mock('./cleanup.js', () => ({ + runSyncCleanup: vi.fn(), +})); // Mock the core modules vi.mock('@google/gemini-cli-core', async (importOriginal) => { @@ -63,6 +82,9 @@ vi.mock('@google/gemini-cli-core', async (importOriginal) => { JsonStreamEventType: { RESULT: 'result', }, + coreEvents: { + emitFeedback: vi.fn(), + }, FatalToolExecutionError: class extends Error { constructor(message: string) { super(message); @@ -85,7 +107,10 @@ vi.mock('@google/gemini-cli-core', async (importOriginal) => { describe('errors', () => { let mockConfig: Config; let processExitSpy: MockInstance; - let consoleErrorSpy: MockInstance; + let debugLoggerErrorSpy: MockInstance; + let debugLoggerWarnSpy: MockInstance; + let coreEventsEmitFeedbackSpy: MockInstance; + let runSyncCleanupSpy: MockInstance; const TEST_SESSION_ID = 'test-session-123'; @@ -93,8 +118,19 @@ describe('errors', () => { // Reset mocks vi.clearAllMocks(); - // Mock console.error - consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + // Mock debugLogger + debugLoggerErrorSpy = vi + .spyOn(debugLogger, 'error') + .mockImplementation(() => {}); + debugLoggerWarnSpy = vi + .spyOn(debugLogger, 'warn') + .mockImplementation(() => {}); + + // Mock coreEvents + coreEventsEmitFeedbackSpy = vi.mocked(coreEvents.emitFeedback); + + // Mock runSyncCleanup + runSyncCleanupSpy = vi.mocked(runSyncCleanup); // Mock process.exit to throw instead of actually exiting processExitSpy = vi.spyOn(process, 'exit').mockImplementation((code) => { @@ -110,7 +146,8 @@ describe('errors', () => { }); afterEach(() => { - consoleErrorSpy.mockRestore(); + debugLoggerErrorSpy.mockRestore(); + debugLoggerWarnSpy.mockRestore(); processExitSpy.mockRestore(); }); @@ -141,14 +178,14 @@ describe('errors', () => { ).mockReturnValue(OutputFormat.TEXT); }); - it('should log error message and re-throw', () => { + it('should re-throw without logging to debugLogger', () => { const testError = new Error('Test error'); expect(() => { handleError(testError, mockConfig); }).toThrow(testError); - expect(consoleErrorSpy).toHaveBeenCalledWith('API Error: Test error'); + expect(debugLoggerErrorSpy).not.toHaveBeenCalled(); }); it('should handle non-Error objects', () => { @@ -157,8 +194,6 @@ describe('errors', () => { expect(() => { handleError(testError, mockConfig); }).toThrow(testError); - - expect(consoleErrorSpy).toHaveBeenCalledWith('API Error: String error'); }); }); @@ -169,14 +204,16 @@ describe('errors', () => { ).mockReturnValue(OutputFormat.JSON); }); - it('should format error as JSON and exit with default code', () => { + it('should format error as JSON, emit feedback exactly once, and exit with default code', () => { const testError = new Error('Test error'); expect(() => { handleError(testError, mockConfig); }).toThrow('process.exit called with code: 1'); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledTimes(1); + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledWith( + 'error', JSON.stringify( { session_id: TEST_SESSION_ID, @@ -190,16 +227,20 @@ describe('errors', () => { 2, ), ); + expect(debugLoggerErrorSpy).not.toHaveBeenCalled(); + expect(runSyncCleanupSpy).toHaveBeenCalled(); }); - it('should use custom error code when provided', () => { + it('should use custom error code when provided and only surface once', () => { const testError = new Error('Test error'); expect(() => { handleError(testError, mockConfig, 42); }).toThrow('process.exit called with code: 42'); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledTimes(1); + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledWith( + 'error', JSON.stringify( { session_id: TEST_SESSION_ID, @@ -213,16 +254,19 @@ describe('errors', () => { 2, ), ); + expect(debugLoggerErrorSpy).not.toHaveBeenCalled(); }); - it('should extract exitCode from FatalError instances', () => { + it('should extract exitCode from FatalError instances and only surface once', () => { const fatalError = new FatalInputError('Fatal error'); expect(() => { handleError(fatalError, mockConfig); }).toThrow('process.exit called with code: 42'); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledTimes(1); + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledWith( + 'error', JSON.stringify( { session_id: TEST_SESSION_ID, @@ -236,6 +280,7 @@ describe('errors', () => { 2, ), ); + expect(debugLoggerErrorSpy).not.toHaveBeenCalled(); }); it('should handle error with code property', () => { @@ -259,7 +304,8 @@ describe('errors', () => { handleError(errorWithStatus, mockConfig); }).toThrow('process.exit called with code: 1'); // string codes become 1 - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledWith( + 'error', JSON.stringify( { session_id: TEST_SESSION_ID, @@ -283,12 +329,14 @@ describe('errors', () => { ).mockReturnValue(OutputFormat.STREAM_JSON); }); - it('should emit result event and exit', () => { + it('should emit result event, run cleanup, and exit', () => { const testError = new Error('Test error'); expect(() => { handleError(testError, mockConfig); }).toThrow('process.exit called with code: 1'); + + expect(runSyncCleanupSpy).toHaveBeenCalled(); }); it('should extract exitCode from FatalError instances', () => { @@ -312,10 +360,10 @@ describe('errors', () => { ).mockReturnValue(OutputFormat.TEXT); }); - it('should log error message to stderr', () => { + it('should log error message to stderr (via debugLogger) for non-fatal', () => { handleToolError(toolName, toolError, mockConfig); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerWarnSpy).toHaveBeenCalledWith( 'Error executing tool test-tool: Tool failed', ); }); @@ -329,10 +377,24 @@ describe('errors', () => { 'Custom display message', ); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerWarnSpy).toHaveBeenCalledWith( 'Error executing tool test-tool: Custom display message', ); }); + + it('should emit feedback exactly once for fatal errors and not use debugLogger', () => { + expect(() => { + handleToolError(toolName, toolError, mockConfig, 'no_space_left'); + }).toThrow('process.exit called with code: 54'); + + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledTimes(1); + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledWith( + 'error', + 'Error executing tool test-tool: Tool failed', + ); + expect(debugLoggerErrorSpy).not.toHaveBeenCalled(); + expect(runSyncCleanupSpy).toHaveBeenCalled(); + }); }); describe('in JSON mode', () => { @@ -351,29 +413,32 @@ describe('errors', () => { 'invalid_tool_params', ); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerWarnSpy).toHaveBeenCalledWith( 'Error executing tool test-tool: Tool failed', ); // Should not exit for non-fatal errors expect(processExitSpy).not.toHaveBeenCalled(); + expect(coreEventsEmitFeedbackSpy).not.toHaveBeenCalled(); }); it('should not exit for file not found errors', () => { handleToolError(toolName, toolError, mockConfig, 'file_not_found'); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerWarnSpy).toHaveBeenCalledWith( 'Error executing tool test-tool: Tool failed', ); expect(processExitSpy).not.toHaveBeenCalled(); + expect(coreEventsEmitFeedbackSpy).not.toHaveBeenCalled(); }); it('should not exit for permission denied errors', () => { handleToolError(toolName, toolError, mockConfig, 'permission_denied'); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerWarnSpy).toHaveBeenCalledWith( 'Error executing tool test-tool: Tool failed', ); expect(processExitSpy).not.toHaveBeenCalled(); + expect(coreEventsEmitFeedbackSpy).not.toHaveBeenCalled(); }); it('should not exit for path not in workspace errors', () => { @@ -384,10 +449,11 @@ describe('errors', () => { 'path_not_in_workspace', ); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerWarnSpy).toHaveBeenCalledWith( 'Error executing tool test-tool: Tool failed', ); expect(processExitSpy).not.toHaveBeenCalled(); + expect(coreEventsEmitFeedbackSpy).not.toHaveBeenCalled(); }); it('should prefer resultDisplay over error message', () => { @@ -399,7 +465,7 @@ describe('errors', () => { 'Display message', ); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerWarnSpy).toHaveBeenCalledWith( 'Error executing tool test-tool: Display message', ); expect(processExitSpy).not.toHaveBeenCalled(); @@ -407,12 +473,14 @@ describe('errors', () => { }); describe('fatal errors', () => { - it('should exit immediately for NO_SPACE_LEFT errors', () => { + it('should exit immediately for NO_SPACE_LEFT errors and only surface once', () => { expect(() => { handleToolError(toolName, toolError, mockConfig, 'no_space_left'); }).toThrow('process.exit called with code: 54'); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledTimes(1); + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledWith( + 'error', JSON.stringify( { session_id: TEST_SESSION_ID, @@ -426,6 +494,8 @@ describe('errors', () => { 2, ), ); + expect(debugLoggerErrorSpy).not.toHaveBeenCalled(); + expect(runSyncCleanupSpy).toHaveBeenCalled(); }); }); }); @@ -437,15 +507,17 @@ describe('errors', () => { ).mockReturnValue(OutputFormat.STREAM_JSON); }); - it('should emit result event and exit for fatal errors', () => { + it('should emit result event, run cleanup, and exit for fatal errors', () => { expect(() => { handleToolError(toolName, toolError, mockConfig, 'no_space_left'); }).toThrow('process.exit called with code: 54'); + expect(runSyncCleanupSpy).toHaveBeenCalled(); + expect(coreEventsEmitFeedbackSpy).not.toHaveBeenCalled(); // Stream mode uses emitEvent }); it('should log to stderr and not exit for non-fatal errors', () => { handleToolError(toolName, toolError, mockConfig, 'invalid_tool_params'); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerWarnSpy).toHaveBeenCalledWith( 'Error executing tool test-tool: Tool failed', ); expect(processExitSpy).not.toHaveBeenCalled(); @@ -461,12 +533,18 @@ describe('errors', () => { ).mockReturnValue(OutputFormat.TEXT); }); - it('should log cancellation message and exit with 130', () => { + it('should emit feedback exactly once, run cleanup, and exit with 130', () => { expect(() => { handleCancellationError(mockConfig); }).toThrow('process.exit called with code: 130'); - expect(consoleErrorSpy).toHaveBeenCalledWith('Operation cancelled.'); + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledTimes(1); + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledWith( + 'error', + 'Operation cancelled.', + ); + expect(debugLoggerErrorSpy).not.toHaveBeenCalled(); + expect(runSyncCleanupSpy).toHaveBeenCalled(); }); }); @@ -477,12 +555,14 @@ describe('errors', () => { ).mockReturnValue(OutputFormat.JSON); }); - it('should format cancellation as JSON and exit with 130', () => { + it('should format cancellation as JSON, emit feedback once, and exit with 130', () => { expect(() => { handleCancellationError(mockConfig); }).toThrow('process.exit called with code: 130'); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledTimes(1); + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledWith( + 'error', JSON.stringify( { session_id: TEST_SESSION_ID, @@ -496,6 +576,7 @@ describe('errors', () => { 2, ), ); + expect(debugLoggerErrorSpy).not.toHaveBeenCalled(); }); }); @@ -510,6 +591,7 @@ describe('errors', () => { expect(() => { handleCancellationError(mockConfig); }).toThrow('process.exit called with code: 130'); + expect(coreEventsEmitFeedbackSpy).not.toHaveBeenCalled(); }); }); }); @@ -522,14 +604,18 @@ describe('errors', () => { ).mockReturnValue(OutputFormat.TEXT); }); - it('should log max turns message and exit with 53', () => { + it('should emit feedback exactly once, run cleanup, and exit with 53', () => { expect(() => { handleMaxTurnsExceededError(mockConfig); }).toThrow('process.exit called with code: 53'); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledTimes(1); + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledWith( + 'error', 'Reached max session turns for this session. Increase the number of turns by specifying maxSessionTurns in settings.json.', ); + expect(debugLoggerErrorSpy).not.toHaveBeenCalled(); + expect(runSyncCleanupSpy).toHaveBeenCalled(); }); }); @@ -540,12 +626,14 @@ describe('errors', () => { ).mockReturnValue(OutputFormat.JSON); }); - it('should format max turns error as JSON and exit with 53', () => { + it('should format max turns error as JSON, emit feedback once, and exit with 53', () => { expect(() => { handleMaxTurnsExceededError(mockConfig); }).toThrow('process.exit called with code: 53'); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledTimes(1); + expect(coreEventsEmitFeedbackSpy).toHaveBeenCalledWith( + 'error', JSON.stringify( { session_id: TEST_SESSION_ID, @@ -560,6 +648,7 @@ describe('errors', () => { 2, ), ); + expect(debugLoggerErrorSpy).not.toHaveBeenCalled(); }); }); @@ -574,6 +663,7 @@ describe('errors', () => { expect(() => { handleMaxTurnsExceededError(mockConfig); }).toThrow('process.exit called with code: 53'); + expect(coreEventsEmitFeedbackSpy).not.toHaveBeenCalled(); }); }); }); diff --git a/packages/cli/src/utils/errors.ts b/packages/cli/src/utils/errors.ts index e46db098f1..b70ccfa3d1 100644 --- a/packages/cli/src/utils/errors.ts +++ b/packages/cli/src/utils/errors.ts @@ -16,6 +16,8 @@ import { FatalCancellationError, FatalToolExecutionError, isFatalToolError, + debugLogger, + coreEvents, } from '@google/gemini-cli-core'; import { runSyncCleanup } from './cleanup.js'; @@ -103,11 +105,10 @@ export function handleError( config.getSessionId(), ); - console.error(formattedError); + coreEvents.emitFeedback('error', formattedError); runSyncCleanup(); process.exit(getNumericExitCode(errorCode)); } else { - console.error(errorMessage); throw error; } } @@ -155,16 +156,16 @@ export function handleToolError( errorType ?? toolExecutionError.exitCode, config.getSessionId(), ); - console.error(formattedError); + coreEvents.emitFeedback('error', formattedError); } else { - console.error(errorMessage); + coreEvents.emitFeedback('error', errorMessage); } runSyncCleanup(); process.exit(toolExecutionError.exitCode); } // Non-fatal: log and continue - console.error(errorMessage); + debugLogger.warn(errorMessage); } /** @@ -196,11 +197,11 @@ export function handleCancellationError(config: Config): never { config.getSessionId(), ); - console.error(formattedError); + coreEvents.emitFeedback('error', formattedError); runSyncCleanup(); process.exit(cancellationError.exitCode); } else { - console.error(cancellationError.message); + coreEvents.emitFeedback('error', cancellationError.message); runSyncCleanup(); process.exit(cancellationError.exitCode); } @@ -237,11 +238,11 @@ export function handleMaxTurnsExceededError(config: Config): never { config.getSessionId(), ); - console.error(formattedError); + coreEvents.emitFeedback('error', formattedError); runSyncCleanup(); process.exit(maxTurnsError.exitCode); } else { - console.error(maxTurnsError.message); + coreEvents.emitFeedback('error', maxTurnsError.message); runSyncCleanup(); process.exit(maxTurnsError.exitCode); } diff --git a/packages/cli/src/utils/relaunch.test.ts b/packages/cli/src/utils/relaunch.test.ts index e627a07a43..2ad5e06a73 100644 --- a/packages/cli/src/utils/relaunch.test.ts +++ b/packages/cli/src/utils/relaunch.test.ts @@ -18,6 +18,19 @@ import { RELAUNCH_EXIT_CODE } from './processUtils.js'; import type { ChildProcess } from 'node:child_process'; import { spawn } from 'node:child_process'; +const mocks = vi.hoisted(() => ({ + writeToStderr: vi.fn(), +})); + +vi.mock('@google/gemini-cli-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + writeToStderr: mocks.writeToStderr, + }; +}); + vi.mock('node:child_process', async (importOriginal) => { const actual = await importOriginal(); return { @@ -33,23 +46,21 @@ import { relaunchAppInChildProcess, relaunchOnExitCode } from './relaunch.js'; describe('relaunchOnExitCode', () => { let processExitSpy: MockInstance; - let consoleErrorSpy: MockInstance; let stdinResumeSpy: MockInstance; beforeEach(() => { processExitSpy = vi.spyOn(process, 'exit').mockImplementation(() => { throw new Error('PROCESS_EXIT_CALLED'); }); - consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); stdinResumeSpy = vi .spyOn(process.stdin, 'resume') .mockImplementation(() => process.stdin); vi.clearAllMocks(); + mocks.writeToStderr.mockClear(); }); afterEach(() => { processExitSpy.mockRestore(); - consoleErrorSpy.mockRestore(); stdinResumeSpy.mockRestore(); }); @@ -90,9 +101,10 @@ describe('relaunchOnExitCode', () => { ); expect(runner).toHaveBeenCalledTimes(1); - expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Fatal error: Failed to relaunch the CLI process.', - error, + expect(mocks.writeToStderr).toHaveBeenCalledWith( + expect.stringContaining( + 'Fatal error: Failed to relaunch the CLI process.', + ), ); expect(stdinResumeSpy).toHaveBeenCalled(); expect(processExitSpy).toHaveBeenCalledWith(1); @@ -101,7 +113,6 @@ describe('relaunchOnExitCode', () => { describe('relaunchAppInChildProcess', () => { let processExitSpy: MockInstance; - let consoleErrorSpy: MockInstance; let stdinPauseSpy: MockInstance; let stdinResumeSpy: MockInstance; @@ -113,6 +124,7 @@ describe('relaunchAppInChildProcess', () => { beforeEach(() => { vi.clearAllMocks(); + mocks.writeToStderr.mockClear(); process.env = { ...originalEnv }; delete process.env['GEMINI_CLI_NO_RELAUNCH']; @@ -124,7 +136,6 @@ describe('relaunchAppInChildProcess', () => { processExitSpy = vi.spyOn(process, 'exit').mockImplementation(() => { throw new Error('PROCESS_EXIT_CALLED'); }); - consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); stdinPauseSpy = vi .spyOn(process.stdin, 'pause') .mockImplementation(() => process.stdin); @@ -140,7 +151,6 @@ describe('relaunchAppInChildProcess', () => { process.execPath = originalExecPath; processExitSpy.mockRestore(); - consoleErrorSpy.mockRestore(); stdinPauseSpy.mockRestore(); stdinResumeSpy.mockRestore(); }); diff --git a/packages/cli/src/utils/relaunch.ts b/packages/cli/src/utils/relaunch.ts index 1142efc717..d0405c32f1 100644 --- a/packages/cli/src/utils/relaunch.ts +++ b/packages/cli/src/utils/relaunch.ts @@ -6,6 +6,7 @@ import { spawn } from 'node:child_process'; import { RELAUNCH_EXIT_CODE } from './processUtils.js'; +import { writeToStderr } from '@google/gemini-cli-core'; export async function relaunchOnExitCode(runner: () => Promise) { while (true) { @@ -17,7 +18,11 @@ export async function relaunchOnExitCode(runner: () => Promise) { } } catch (error) { process.stdin.resume(); - console.error('Fatal error: Failed to relaunch the CLI process.', error); + const errorMessage = + error instanceof Error ? (error.stack ?? error.message) : String(error); + writeToStderr( + `Fatal error: Failed to relaunch the CLI process.\n${errorMessage}\n`, + ); process.exit(1); } } diff --git a/packages/cli/src/utils/sandbox.ts b/packages/cli/src/utils/sandbox.ts index 8e5b7e0de2..c05f2cf3a4 100644 --- a/packages/cli/src/utils/sandbox.ts +++ b/packages/cli/src/utils/sandbox.ts @@ -251,7 +251,9 @@ export async function start_sandbox( } // stop if image is missing - if (!(await ensureSandboxImageIsPresent(config.command, image))) { + if ( + !(await ensureSandboxImageIsPresent(config.command, image, cliConfig)) + ) { const remedy = image === LOCAL_DEV_SANDBOX_IMAGE_NAME ? 'Try running `npm run build:all` or `npm run build:sandbox` under the gemini-cli repo to build it locally, or check the image name and your network connection.' @@ -718,8 +720,12 @@ async function imageExists(sandbox: string, image: string): Promise { }); } -async function pullImage(sandbox: string, image: string): Promise { - console.info(`Attempting to pull image ${image} using ${sandbox}...`); +async function pullImage( + sandbox: string, + image: string, + cliConfig?: Config, +): Promise { + debugLogger.debug(`Attempting to pull image ${image} using ${sandbox}...`); return new Promise((resolve) => { const args = ['pull', image]; const pullProcess = spawn(sandbox, args, { stdio: 'pipe' }); @@ -727,11 +733,14 @@ async function pullImage(sandbox: string, image: string): Promise { let stderrData = ''; const onStdoutData = (data: Buffer) => { - console.info(data.toString().trim()); // Show pull progress + if (cliConfig?.getDebugMode() || process.env['DEBUG']) { + debugLogger.log(data.toString().trim()); // Show pull progress + } }; const onStderrData = (data: Buffer) => { stderrData += data.toString(); + // eslint-disable-next-line no-console console.error(data.toString().trim()); // Show pull errors/info from the command itself }; @@ -745,7 +754,7 @@ async function pullImage(sandbox: string, image: string): Promise { const onClose = (code: number | null) => { if (code === 0) { - console.info(`Successfully pulled image ${image}.`); + debugLogger.log(`Successfully pulled image ${image}.`); cleanup(); resolve(true); } else { @@ -788,6 +797,7 @@ async function pullImage(sandbox: string, image: string): Promise { async function ensureSandboxImageIsPresent( sandbox: string, image: string, + cliConfig?: Config, ): Promise { debugLogger.log(`Checking for sandbox image: ${image}`); if (await imageExists(sandbox, image)) { @@ -801,7 +811,7 @@ async function ensureSandboxImageIsPresent( return false; } - if (await pullImage(sandbox, image)) { + if (await pullImage(sandbox, image, cliConfig)) { // After attempting to pull, check again to be certain if (await imageExists(sandbox, image)) { debugLogger.log(`Sandbox image ${image} is now available after pulling.`); diff --git a/packages/cli/src/utils/sessionCleanup.integration.test.ts b/packages/cli/src/utils/sessionCleanup.integration.test.ts index a0dbcf157f..eec9a12592 100644 --- a/packages/cli/src/utils/sessionCleanup.integration.test.ts +++ b/packages/cli/src/utils/sessionCleanup.integration.test.ts @@ -7,7 +7,11 @@ import { describe, it, expect, vi } from 'vitest'; import { cleanupExpiredSessions } from './sessionCleanup.js'; import type { Settings } from '../config/settings.js'; -import { SESSION_FILE_PREFIX, type Config } from '@google/gemini-cli-core'; +import { + SESSION_FILE_PREFIX, + type Config, + debugLogger, +} from '@google/gemini-cli-core'; // Create a mock config for integration testing function createTestConfig(): Config { @@ -112,7 +116,7 @@ describe('Session Cleanup Integration', () => { }); it('should validate configuration and fail gracefully', async () => { - const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const errorSpy = vi.spyOn(debugLogger, 'warn').mockImplementation(() => {}); const config = createTestConfig(); const settings: Settings = { diff --git a/packages/cli/src/utils/sessionCleanup.test.ts b/packages/cli/src/utils/sessionCleanup.test.ts index d47be2fc00..e59ad4baf8 100644 --- a/packages/cli/src/utils/sessionCleanup.test.ts +++ b/packages/cli/src/utils/sessionCleanup.test.ts @@ -100,6 +100,8 @@ function createTestSessions(): SessionInfo[] { describe('Session Cleanup', () => { beforeEach(() => { vi.clearAllMocks(); + vi.spyOn(debugLogger, 'error').mockImplementation(() => {}); + vi.spyOn(debugLogger, 'warn').mockImplementation(() => {}); // By default, return all test sessions as valid const sessions = createTestSessions(); mockGetAllSessionFiles.mockResolvedValue( @@ -154,20 +156,16 @@ describe('Session Cleanup', () => { }, }; - const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - const result = await cleanupExpiredSessions(config, settings); expect(result.disabled).toBe(true); expect(result.scanned).toBe(0); expect(result.deleted).toBe(0); - expect(errorSpy).toHaveBeenCalledWith( + expect(debugLogger.warn).toHaveBeenCalledWith( expect.stringContaining( 'Session cleanup disabled: Error: Invalid retention period format', ), ); - - errorSpy.mockRestore(); }); it('should delete sessions older than maxAge', async () => { @@ -338,8 +336,6 @@ describe('Session Cleanup', () => { }, }; - const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - // Mock getSessionFiles to throw an error mockGetAllSessionFiles.mockRejectedValue( new Error('Directory access failed'), @@ -349,11 +345,9 @@ describe('Session Cleanup', () => { expect(result.disabled).toBe(false); expect(result.failed).toBe(1); - expect(errorSpy).toHaveBeenCalledWith( + expect(debugLogger.warn).toHaveBeenCalledWith( 'Session cleanup failed: Directory access failed', ); - - errorSpy.mockRestore(); }); it('should respect minRetention configuration', async () => { @@ -979,21 +973,17 @@ describe('Session Cleanup', () => { }, }; - const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - const result = await cleanupExpiredSessions(config, settings); expect(result.disabled).toBe(true); expect(result.scanned).toBe(0); - expect(errorSpy).toHaveBeenCalledWith( + expect(debugLogger.warn).toHaveBeenCalledWith( expect.stringContaining( input === '0d' ? 'Invalid retention period: 0d. Value must be greater than 0' : `Invalid retention period format: ${input}`, ), ); - - errorSpy.mockRestore(); }); // Test special case - empty string @@ -1010,18 +1000,14 @@ describe('Session Cleanup', () => { }, }; - const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - const result = await cleanupExpiredSessions(config, settings); expect(result.disabled).toBe(true); expect(result.scanned).toBe(0); // Empty string means no valid retention method specified - expect(errorSpy).toHaveBeenCalledWith( + expect(debugLogger.warn).toHaveBeenCalledWith( expect.stringContaining('Either maxAge or maxCount must be specified'), ); - - errorSpy.mockRestore(); }); // Test edge cases @@ -1082,17 +1068,13 @@ describe('Session Cleanup', () => { }, }; - const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - const result = await cleanupExpiredSessions(config, settings); expect(result.disabled).toBe(true); expect(result.scanned).toBe(0); - expect(errorSpy).toHaveBeenCalledWith( + expect(debugLogger.warn).toHaveBeenCalledWith( expect.stringContaining('Either maxAge or maxCount must be specified'), ); - - errorSpy.mockRestore(); }); it('should validate maxCount range', async () => { @@ -1108,17 +1090,13 @@ describe('Session Cleanup', () => { }, }; - const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - const result = await cleanupExpiredSessions(config, settings); expect(result.disabled).toBe(true); expect(result.scanned).toBe(0); - expect(errorSpy).toHaveBeenCalledWith( + expect(debugLogger.warn).toHaveBeenCalledWith( expect.stringContaining('maxCount must be at least 1'), ); - - errorSpy.mockRestore(); }); describe('maxAge format validation', () => { @@ -1135,21 +1113,14 @@ describe('Session Cleanup', () => { }, }; - const errorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); - const result = await cleanupExpiredSessions(config, settings); expect(result.disabled).toBe(true); expect(result.scanned).toBe(0); - expect(errorSpy).toHaveBeenCalledWith( + expect(debugLogger.warn).toHaveBeenCalledWith( expect.stringContaining('Invalid retention period format: 30'), ); - - errorSpy.mockRestore(); }); - it('should reject invalid maxAge format - invalid unit', async () => { const config = createMockConfig({ getDebugMode: vi.fn().mockReturnValue(true), @@ -1163,21 +1134,14 @@ describe('Session Cleanup', () => { }, }; - const errorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); - const result = await cleanupExpiredSessions(config, settings); expect(result.disabled).toBe(true); expect(result.scanned).toBe(0); - expect(errorSpy).toHaveBeenCalledWith( + expect(debugLogger.warn).toHaveBeenCalledWith( expect.stringContaining('Invalid retention period format: 30x'), ); - - errorSpy.mockRestore(); }); - it('should reject invalid maxAge format - no number', async () => { const config = createMockConfig({ getDebugMode: vi.fn().mockReturnValue(true), @@ -1191,21 +1155,14 @@ describe('Session Cleanup', () => { }, }; - const errorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); - const result = await cleanupExpiredSessions(config, settings); expect(result.disabled).toBe(true); expect(result.scanned).toBe(0); - expect(errorSpy).toHaveBeenCalledWith( + expect(debugLogger.warn).toHaveBeenCalledWith( expect.stringContaining('Invalid retention period format: d'), ); - - errorSpy.mockRestore(); }); - it('should reject invalid maxAge format - decimal number', async () => { const config = createMockConfig({ getDebugMode: vi.fn().mockReturnValue(true), @@ -1219,21 +1176,14 @@ describe('Session Cleanup', () => { }, }; - const errorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); - const result = await cleanupExpiredSessions(config, settings); expect(result.disabled).toBe(true); expect(result.scanned).toBe(0); - expect(errorSpy).toHaveBeenCalledWith( + expect(debugLogger.warn).toHaveBeenCalledWith( expect.stringContaining('Invalid retention period format: 1.5d'), ); - - errorSpy.mockRestore(); }); - it('should reject invalid maxAge format - negative number', async () => { const config = createMockConfig({ getDebugMode: vi.fn().mockReturnValue(true), @@ -1247,21 +1197,14 @@ describe('Session Cleanup', () => { }, }; - const errorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); - const result = await cleanupExpiredSessions(config, settings); expect(result.disabled).toBe(true); expect(result.scanned).toBe(0); - expect(errorSpy).toHaveBeenCalledWith( + expect(debugLogger.warn).toHaveBeenCalledWith( expect.stringContaining('Invalid retention period format: -5d'), ); - - errorSpy.mockRestore(); }); - it('should accept valid maxAge format - hours', async () => { const config = createMockConfig(); const settings: Settings = { @@ -1362,23 +1305,16 @@ describe('Session Cleanup', () => { }, }; - const errorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); - const result = await cleanupExpiredSessions(config, settings); expect(result.disabled).toBe(true); expect(result.scanned).toBe(0); - expect(errorSpy).toHaveBeenCalledWith( + expect(debugLogger.warn).toHaveBeenCalledWith( expect.stringContaining( 'maxAge cannot be less than minRetention (1d)', ), ); - - errorSpy.mockRestore(); }); - it('should reject maxAge less than custom minRetention', async () => { const config = createMockConfig({ getDebugMode: vi.fn().mockReturnValue(true), @@ -1393,23 +1329,16 @@ describe('Session Cleanup', () => { }, }; - const errorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); - const result = await cleanupExpiredSessions(config, settings); expect(result.disabled).toBe(true); expect(result.scanned).toBe(0); - expect(errorSpy).toHaveBeenCalledWith( + expect(debugLogger.warn).toHaveBeenCalledWith( expect.stringContaining( 'maxAge cannot be less than minRetention (3d)', ), ); - - errorSpy.mockRestore(); }); - it('should accept maxAge equal to minRetention', async () => { const config = createMockConfig(); const settings: Settings = { @@ -1537,21 +1466,14 @@ describe('Session Cleanup', () => { }, }; - const errorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); - const result = await cleanupExpiredSessions(config, settings); expect(result.disabled).toBe(true); expect(result.scanned).toBe(0); - expect(errorSpy).toHaveBeenCalledWith( + expect(debugLogger.warn).toHaveBeenCalledWith( expect.stringContaining('maxCount must be at least 1'), ); - - errorSpy.mockRestore(); }); - it('should accept valid maxCount in normal range', async () => { const config = createMockConfig(); const settings: Settings = { @@ -1611,22 +1533,15 @@ describe('Session Cleanup', () => { }, }; - const errorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); - const result = await cleanupExpiredSessions(config, settings); expect(result.disabled).toBe(true); expect(result.scanned).toBe(0); // Should fail on first validation error (maxAge format) - expect(errorSpy).toHaveBeenCalledWith( + expect(debugLogger.warn).toHaveBeenCalledWith( expect.stringContaining('Invalid retention period format'), ); - - errorSpy.mockRestore(); }); - it('should reject if maxAge is invalid even when maxCount is valid', async () => { const config = createMockConfig({ getDebugMode: vi.fn().mockReturnValue(true), @@ -1642,20 +1557,14 @@ describe('Session Cleanup', () => { }; // The validation logic rejects invalid maxAge format even if maxCount is valid - const errorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); - const result = await cleanupExpiredSessions(config, settings); // Should reject due to invalid maxAge format expect(result.disabled).toBe(true); expect(result.scanned).toBe(0); - expect(errorSpy).toHaveBeenCalledWith( + expect(debugLogger.warn).toHaveBeenCalledWith( expect.stringContaining('Invalid retention period format'), ); - - errorSpy.mockRestore(); }); }); diff --git a/packages/cli/src/utils/sessionCleanup.ts b/packages/cli/src/utils/sessionCleanup.ts index bb5e64e5f3..3cdce025c9 100644 --- a/packages/cli/src/utils/sessionCleanup.ts +++ b/packages/cli/src/utils/sessionCleanup.ts @@ -62,7 +62,7 @@ export async function cleanupExpiredSessions( ); if (validationErrorMessage) { // Log validation errors to console for visibility - console.error(`Session cleanup disabled: ${validationErrorMessage}`); + debugLogger.warn(`Session cleanup disabled: ${validationErrorMessage}`); return { ...result, disabled: true }; } @@ -114,7 +114,7 @@ export async function cleanupExpiredSessions( : sessionToDelete.sessionInfo.id; const errorMessage = error instanceof Error ? error.message : 'Unknown error'; - console.error( + debugLogger.warn( `Failed to delete session ${sessionId}: ${errorMessage}`, ); result.failed++; @@ -133,7 +133,7 @@ export async function cleanupExpiredSessions( // Global error handler - don't let cleanup failures break startup const errorMessage = error instanceof Error ? error.message : 'Unknown error'; - console.error(`Session cleanup failed: ${errorMessage}`); + debugLogger.warn(`Session cleanup failed: ${errorMessage}`); result.failed++; } @@ -273,7 +273,7 @@ function validateRetentionConfig( } catch (error) { // If minRetention format is invalid, fall back to default if (config.getDebugMode()) { - console.error(`Failed to parse minRetention: ${error}`); + debugLogger.warn(`Failed to parse minRetention: ${error}`); } minRetentionMs = parseRetentionPeriod(DEFAULT_MIN_RETENTION); } diff --git a/packages/cli/src/utils/sessions.test.ts b/packages/cli/src/utils/sessions.test.ts index fe7a0ea235..8fe22cebba 100644 --- a/packages/cli/src/utils/sessions.test.ts +++ b/packages/cli/src/utils/sessions.test.ts @@ -10,6 +10,11 @@ import { ChatRecordingService } from '@google/gemini-cli-core'; import { listSessions, deleteSession } from './sessions.js'; import { SessionSelector, type SessionInfo } from './sessionUtils.js'; +const mocks = vi.hoisted(() => ({ + writeToStdout: vi.fn(), + writeToStderr: vi.fn(), +})); + // Mock the SessionSelector and ChatRecordingService vi.mock('./sessionUtils.js', () => ({ SessionSelector: vi.fn(), @@ -22,13 +27,14 @@ vi.mock('@google/gemini-cli-core', async () => { ...actual, ChatRecordingService: vi.fn(), generateSummary: vi.fn().mockResolvedValue(undefined), + writeToStdout: mocks.writeToStdout, + writeToStderr: mocks.writeToStderr, }; }); describe('listSessions', () => { let mockConfig: Config; let mockListSessions: ReturnType; - let consoleLogSpy: ReturnType; beforeEach(() => { // Create mock config @@ -49,14 +55,12 @@ describe('listSessions', () => { listSessions: mockListSessions, }) as unknown as InstanceType, ); - - // Spy on console.log - consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); }); afterEach(() => { vi.clearAllMocks(); - consoleLogSpy.mockRestore(); + mocks.writeToStdout.mockClear(); + mocks.writeToStderr.mockClear(); }); it('should display message when no previous sessions were found', async () => { @@ -68,7 +72,7 @@ describe('listSessions', () => { // Assert expect(mockListSessions).toHaveBeenCalledOnce(); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( 'No previous sessions found for this project.', ); }); @@ -127,32 +131,32 @@ describe('listSessions', () => { expect(mockListSessions).toHaveBeenCalledOnce(); // Check that the header was displayed - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( '\nAvailable sessions for this project (3):\n', ); // Check that each session was logged - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( expect.stringContaining('1. First user message'), ); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( expect.stringContaining('[session-1]'), ); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( expect.stringContaining('2. Second user message'), ); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( expect.stringContaining('[session-2]'), ); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( expect.stringContaining('3. Current session'), ); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( expect.stringContaining(', current)'), ); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( expect.stringContaining('[current-session-id]'), ); }); @@ -209,7 +213,7 @@ describe('listSessions', () => { // Assert // Get all the session log calls (skip the header) - const sessionCalls = consoleLogSpy.mock.calls.filter( + const sessionCalls = mocks.writeToStdout.mock.calls.filter( (call): call is [string] => typeof call[0] === 'string' && call[0].includes('[session-') && @@ -246,13 +250,13 @@ describe('listSessions', () => { await listSessions(mockConfig); // Assert - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( expect.stringContaining('1. Test message'), ); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( expect.stringContaining('some time ago'), ); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( expect.stringContaining('[abc123def456]'), ); }); @@ -281,13 +285,13 @@ describe('listSessions', () => { await listSessions(mockConfig); // Assert - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( '\nAvailable sessions for this project (1):\n', ); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( expect.stringContaining('1. Only session'), ); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( expect.stringContaining(', current)'), ); }); @@ -318,10 +322,10 @@ describe('listSessions', () => { await listSessions(mockConfig); // Assert: Should show the summary (displayName), not the first user message - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( expect.stringContaining('1. Add dark mode to the app'), ); - expect(consoleLogSpy).not.toHaveBeenCalledWith( + expect(mocks.writeToStdout).not.toHaveBeenCalledWith( expect.stringContaining('How do I add dark mode to my React application'), ); }); @@ -331,8 +335,6 @@ describe('deleteSession', () => { let mockConfig: Config; let mockListSessions: ReturnType; let mockDeleteSession: ReturnType; - let consoleLogSpy: ReturnType; - let consoleErrorSpy: ReturnType; beforeEach(() => { // Create mock config @@ -362,16 +364,10 @@ describe('deleteSession', () => { deleteSession: mockDeleteSession, }) as unknown as InstanceType, ); - - // Spy on console methods - consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); - consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); }); afterEach(() => { vi.clearAllMocks(); - consoleLogSpy.mockRestore(); - consoleErrorSpy.mockRestore(); }); it('should display error when no sessions are found', async () => { @@ -383,7 +379,7 @@ describe('deleteSession', () => { // Assert expect(mockListSessions).toHaveBeenCalledOnce(); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(mocks.writeToStderr).toHaveBeenCalledWith( 'No sessions found for this project.', ); expect(mockDeleteSession).not.toHaveBeenCalled(); @@ -416,10 +412,10 @@ describe('deleteSession', () => { // Assert expect(mockListSessions).toHaveBeenCalledOnce(); expect(mockDeleteSession).toHaveBeenCalledWith('session-file-123'); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( 'Deleted session 1: Test session (some time ago)', ); - expect(consoleErrorSpy).not.toHaveBeenCalled(); + expect(mocks.writeToStderr).not.toHaveBeenCalled(); }); it('should delete session by index', async () => { @@ -463,7 +459,7 @@ describe('deleteSession', () => { // Assert expect(mockListSessions).toHaveBeenCalledOnce(); expect(mockDeleteSession).toHaveBeenCalledWith('session-file-2'); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( 'Deleted session 2: Second session (some time ago)', ); }); @@ -492,7 +488,7 @@ describe('deleteSession', () => { await deleteSession(mockConfig, 'invalid-id'); // Assert - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(mocks.writeToStderr).toHaveBeenCalledWith( 'Invalid session identifier "invalid-id". Use --list-sessions to see available sessions.', ); expect(mockDeleteSession).not.toHaveBeenCalled(); @@ -522,7 +518,7 @@ describe('deleteSession', () => { await deleteSession(mockConfig, '999'); // Assert - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(mocks.writeToStderr).toHaveBeenCalledWith( 'Invalid session identifier "999". Use --list-sessions to see available sessions.', ); expect(mockDeleteSession).not.toHaveBeenCalled(); @@ -552,7 +548,7 @@ describe('deleteSession', () => { await deleteSession(mockConfig, '0'); // Assert - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(mocks.writeToStderr).toHaveBeenCalledWith( 'Invalid session identifier "0". Use --list-sessions to see available sessions.', ); expect(mockDeleteSession).not.toHaveBeenCalled(); @@ -582,7 +578,7 @@ describe('deleteSession', () => { await deleteSession(mockConfig, '1'); // Assert - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(mocks.writeToStderr).toHaveBeenCalledWith( 'Cannot delete the current active session.', ); expect(mockDeleteSession).not.toHaveBeenCalled(); @@ -612,7 +608,7 @@ describe('deleteSession', () => { await deleteSession(mockConfig, 'current-session-id'); // Assert - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(mocks.writeToStderr).toHaveBeenCalledWith( 'Cannot delete the current active session.', ); expect(mockDeleteSession).not.toHaveBeenCalled(); @@ -646,7 +642,7 @@ describe('deleteSession', () => { // Assert expect(mockDeleteSession).toHaveBeenCalledWith('session-file-1'); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(mocks.writeToStderr).toHaveBeenCalledWith( 'Failed to delete session: File deletion failed', ); }); @@ -679,7 +675,7 @@ describe('deleteSession', () => { await deleteSession(mockConfig, '1'); // Assert - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(mocks.writeToStderr).toHaveBeenCalledWith( 'Failed to delete session: Unknown error', ); }); @@ -737,7 +733,7 @@ describe('deleteSession', () => { // Assert expect(mockDeleteSession).toHaveBeenCalledWith('session-file-1'); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mocks.writeToStdout).toHaveBeenCalledWith( expect.stringContaining('Oldest session'), ); }); diff --git a/packages/cli/src/utils/sessions.ts b/packages/cli/src/utils/sessions.ts index 3d9866f337..56f9f06a6a 100644 --- a/packages/cli/src/utils/sessions.ts +++ b/packages/cli/src/utils/sessions.ts @@ -7,6 +7,8 @@ import { ChatRecordingService, generateSummary, + writeToStderr, + writeToStdout, type Config, } from '@google/gemini-cli-core'; import { @@ -23,11 +25,13 @@ export async function listSessions(config: Config): Promise { const sessions = await sessionSelector.listSessions(); if (sessions.length === 0) { - console.log('No previous sessions found for this project.'); + writeToStdout('No previous sessions found for this project.'); return; } - console.log(`\nAvailable sessions for this project (${sessions.length}):\n`); + writeToStdout( + `\nAvailable sessions for this project (${sessions.length}):\n`, + ); sessions .sort( @@ -41,8 +45,8 @@ export async function listSessions(config: Config): Promise { session.displayName.length > 100 ? session.displayName.slice(0, 97) + '...' : session.displayName; - console.log( - ` ${index + 1}. ${title} (${time}${current}) [${session.id}]`, + writeToStdout( + ` ${index + 1}. ${title} (${time}${current}) [${session.id}]\n`, ); }); } @@ -55,7 +59,7 @@ export async function deleteSession( const sessions = await sessionSelector.listSessions(); if (sessions.length === 0) { - console.error('No sessions found for this project.'); + writeToStderr('No sessions found for this project.'); return; } @@ -76,7 +80,7 @@ export async function deleteSession( // Parse session index const index = parseInt(sessionIndex, 10); if (isNaN(index) || index < 1 || index > sessions.length) { - console.error( + writeToStderr( `Invalid session identifier "${sessionIndex}". Use --list-sessions to see available sessions.`, ); return; @@ -86,7 +90,7 @@ export async function deleteSession( // Prevent deleting the current session if (sessionToDelete.isCurrentSession) { - console.error('Cannot delete the current active session.'); + writeToStderr('Cannot delete the current active session.'); return; } @@ -96,11 +100,11 @@ export async function deleteSession( chatRecordingService.deleteSession(sessionToDelete.file); const time = formatRelativeTime(sessionToDelete.lastUpdated); - console.log( + writeToStdout( `Deleted session ${sessionToDelete.index}: ${sessionToDelete.firstUserMessage} (${time})`, ); } catch (error) { - console.error( + writeToStderr( `Failed to delete session: ${error instanceof Error ? error.message : 'Unknown error'}`, ); } diff --git a/packages/cli/src/validateNonInterActiveAuth.test.ts b/packages/cli/src/validateNonInterActiveAuth.test.ts index be5f4a14a2..e3cc33ea23 100644 --- a/packages/cli/src/validateNonInterActiveAuth.test.ts +++ b/packages/cli/src/validateNonInterActiveAuth.test.ts @@ -21,6 +21,7 @@ import { makeFakeConfig, debugLogger, ExitCodes, + coreEvents, } from '@google/gemini-cli-core'; import type { Config } from '@google/gemini-cli-core'; import * as auth from './config/auth.js'; @@ -36,8 +37,8 @@ describe('validateNonInterActiveAuth', () => { let originalEnvGeminiApiKey: string | undefined; let originalEnvVertexAi: string | undefined; let originalEnvGcp: string | undefined; - let consoleErrorSpy: ReturnType; let debugLoggerErrorSpy: ReturnType; + let coreEventsEmitFeedbackSpy: MockInstance; let processExitSpy: MockInstance; let refreshAuthMock: Mock; let mockSettings: LoadedSettings; @@ -49,10 +50,12 @@ describe('validateNonInterActiveAuth', () => { delete process.env['GEMINI_API_KEY']; delete process.env['GOOGLE_GENAI_USE_VERTEXAI']; delete process.env['GOOGLE_GENAI_USE_GCA']; - consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); debugLoggerErrorSpy = vi .spyOn(debugLogger, 'error') .mockImplementation(() => {}); + coreEventsEmitFeedbackSpy = vi + .spyOn(coreEvents, 'emitFeedback') + .mockImplementation(() => {}); processExitSpy = vi .spyOn(process, 'exit') .mockImplementation((code?: string | number | null | undefined) => { @@ -302,6 +305,7 @@ describe('validateNonInterActiveAuth', () => { expect(validateAuthMethodSpy).not.toHaveBeenCalled(); expect(debugLoggerErrorSpy).not.toHaveBeenCalled(); + expect(coreEventsEmitFeedbackSpy).not.toHaveBeenCalled(); expect(processExitSpy).not.toHaveBeenCalled(); // We still expect refreshAuth to be called with the (invalid) type expect(refreshAuthMock).toHaveBeenCalledWith('invalid-auth-type'); @@ -404,7 +408,8 @@ describe('validateNonInterActiveAuth', () => { expect(thrown?.message).toBe( `process.exit(${ExitCodes.FATAL_AUTHENTICATION_ERROR}) called`, ); - const errorArg = consoleErrorSpy.mock.calls[0]?.[0] as string; + // Checking coreEventsEmitFeedbackSpy arguments + const errorArg = coreEventsEmitFeedbackSpy.mock.calls[0]?.[1] as string; const payload = JSON.parse(errorArg); expect(payload.error.type).toBe('Error'); expect(payload.error.code).toBe(ExitCodes.FATAL_AUTHENTICATION_ERROR); @@ -439,7 +444,8 @@ describe('validateNonInterActiveAuth', () => { `process.exit(${ExitCodes.FATAL_AUTHENTICATION_ERROR}) called`, ); { - const errorArg = consoleErrorSpy.mock.calls[0]?.[0] as string; + // Checking coreEventsEmitFeedbackSpy arguments + const errorArg = coreEventsEmitFeedbackSpy.mock.calls[0]?.[1] as string; const payload = JSON.parse(errorArg); expect(payload.error.type).toBe('Error'); expect(payload.error.code).toBe(ExitCodes.FATAL_AUTHENTICATION_ERROR); @@ -477,7 +483,8 @@ describe('validateNonInterActiveAuth', () => { `process.exit(${ExitCodes.FATAL_AUTHENTICATION_ERROR}) called`, ); { - const errorArg = consoleErrorSpy.mock.calls[0]?.[0] as string; + // Checking coreEventsEmitFeedbackSpy arguments + const errorArg = coreEventsEmitFeedbackSpy.mock.calls[0]?.[1] as string; const payload = JSON.parse(errorArg); expect(payload.error.type).toBe('Error'); expect(payload.error.code).toBe(ExitCodes.FATAL_AUTHENTICATION_ERROR); diff --git a/packages/core/src/confirmation-bus/message-bus.ts b/packages/core/src/confirmation-bus/message-bus.ts index 8a6eae6025..11dab9ca23 100644 --- a/packages/core/src/confirmation-bus/message-bus.ts +++ b/packages/core/src/confirmation-bus/message-bus.ts @@ -14,6 +14,7 @@ import { type HookPolicyDecision, } from './types.js'; import { safeJsonStringify } from '../utils/safeJsonStringify.js'; +import { debugLogger } from '../utils/debugLogger.js'; export class MessageBus extends EventEmitter { constructor( @@ -45,7 +46,7 @@ export class MessageBus extends EventEmitter { async publish(message: Message): Promise { if (this.debug) { - console.debug(`[MESSAGE_BUS] publish: ${safeJsonStringify(message)}`); + debugLogger.debug(`[MESSAGE_BUS] publish: ${safeJsonStringify(message)}`); } try { if (!this.isValidMessage(message)) { diff --git a/packages/core/src/ide/ide-client.ts b/packages/core/src/ide/ide-client.ts index 1203610d40..d8e0577e28 100644 --- a/packages/core/src/ide/ide-client.ts +++ b/packages/core/src/ide/ide-client.ts @@ -30,7 +30,7 @@ const logger = { // eslint-disable-next-line @typescript-eslint/no-explicit-any debug: (...args: any[]) => debugLogger.debug('[DEBUG] [IDEClient]', ...args), // eslint-disable-next-line @typescript-eslint/no-explicit-any - error: (...args: any[]) => console.error('[ERROR] [IDEClient]', ...args), + error: (...args: any[]) => debugLogger.error('[ERROR] [IDEClient]', ...args), }; export type DiffUpdateResult = diff --git a/packages/core/src/mcp/oauth-utils.ts b/packages/core/src/mcp/oauth-utils.ts index 477374b596..de87838a2a 100644 --- a/packages/core/src/mcp/oauth-utils.ts +++ b/packages/core/src/mcp/oauth-utils.ts @@ -418,7 +418,10 @@ export class OAuthUtils { return payload.exp * 1000; // Convert seconds to milliseconds } } catch (e) { - console.error('Failed to parse ID token for expiry time with error:', e); + debugLogger.error( + 'Failed to parse ID token for expiry time with error:', + e, + ); } // Return undefined if try block fails or 'exp' is missing/invalid diff --git a/packages/core/src/mcp/token-storage/keychain-token-storage.ts b/packages/core/src/mcp/token-storage/keychain-token-storage.ts index b8ea7b689b..ac1d0266fc 100644 --- a/packages/core/src/mcp/token-storage/keychain-token-storage.ts +++ b/packages/core/src/mcp/token-storage/keychain-token-storage.ts @@ -324,7 +324,11 @@ export class KeychainTokenStorage .filter((cred) => cred.account.startsWith(SECRET_PREFIX)) .map((cred) => cred.account.substring(SECRET_PREFIX.length)); } catch (error) { - console.error('Failed to list secrets from keychain:', error); + coreEvents.emitFeedback( + 'error', + 'Failed to list secrets from keychain', + error, + ); return []; } } diff --git a/packages/core/src/policy/config.ts b/packages/core/src/policy/config.ts index c4888c4882..8d74589992 100644 --- a/packages/core/src/policy/config.ts +++ b/packages/core/src/policy/config.ts @@ -28,6 +28,7 @@ import { } from '../confirmation-bus/types.js'; import { type MessageBus } from '../confirmation-bus/message-bus.js'; import { coreEvents } from '../utils/events.js'; +import { debugLogger } from '../utils/debugLogger.js'; const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); @@ -312,7 +313,7 @@ export function createPolicyUpdater( existingData = toml.parse(fileContent) as { rule?: TomlRule[] }; } catch (error) { if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { - console.warn( + debugLogger.warn( `Failed to parse ${policyFile}, overwriting with new policy.`, error, ); diff --git a/packages/core/src/routing/strategies/compositeStrategy.test.ts b/packages/core/src/routing/strategies/compositeStrategy.test.ts index 7fb2c393b4..1be0b8a8e3 100644 --- a/packages/core/src/routing/strategies/compositeStrategy.test.ts +++ b/packages/core/src/routing/strategies/compositeStrategy.test.ts @@ -14,6 +14,14 @@ import type { } from '../routingStrategy.js'; import type { Config } from '../../config/config.js'; import type { BaseLlmClient } from '../../core/baseLlmClient.js'; +import { debugLogger } from '../../utils/debugLogger.js'; +import { coreEvents } from '../../utils/events.js'; + +vi.mock('../../utils/debugLogger.js', () => ({ + debugLogger: { + warn: vi.fn(), + }, +})); describe('CompositeStrategy', () => { let mockContext: RoutingContext; @@ -22,6 +30,7 @@ describe('CompositeStrategy', () => { let mockStrategy1: RoutingStrategy; let mockStrategy2: RoutingStrategy; let mockTerminalStrategy: TerminalStrategy; + let emitFeedbackSpy: ReturnType; beforeEach(() => { vi.clearAllMocks(); @@ -30,6 +39,8 @@ describe('CompositeStrategy', () => { mockConfig = {} as Config; mockBaseLlmClient = {} as BaseLlmClient; + emitFeedbackSpy = vi.spyOn(coreEvents, 'emitFeedback'); + mockStrategy1 = { name: 'strategy1', route: vi.fn().mockResolvedValue(null), @@ -112,9 +123,6 @@ describe('CompositeStrategy', () => { }); it('should handle errors in non-terminal strategies and continue', async () => { - const consoleErrorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); vi.spyOn(mockStrategy1, 'route').mockRejectedValue( new Error('Strategy 1 failed'), ); @@ -130,18 +138,14 @@ describe('CompositeStrategy', () => { mockBaseLlmClient, ); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLogger.warn).toHaveBeenCalledWith( "[Routing] Strategy 'strategy1' failed. Continuing to next strategy. Error:", expect.any(Error), ); expect(result.model).toBe('terminal-model'); - consoleErrorSpy.mockRestore(); }); it('should re-throw an error from the terminal strategy', async () => { - const consoleErrorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); const terminalError = new Error('Terminal strategy failed'); vi.spyOn(mockTerminalStrategy, 'route').mockRejectedValue(terminalError); @@ -151,11 +155,11 @@ describe('CompositeStrategy', () => { composite.route(mockContext, mockConfig, mockBaseLlmClient), ).rejects.toThrow(terminalError); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(emitFeedbackSpy).toHaveBeenCalledWith( + 'error', "[Routing] Critical Error: Terminal strategy 'terminal' failed. Routing cannot proceed. Error:", terminalError, ); - consoleErrorSpy.mockRestore(); }); it('should correctly finalize the decision metadata', async () => { diff --git a/packages/core/src/routing/strategies/compositeStrategy.ts b/packages/core/src/routing/strategies/compositeStrategy.ts index 6aadc439da..0b3856a4bd 100644 --- a/packages/core/src/routing/strategies/compositeStrategy.ts +++ b/packages/core/src/routing/strategies/compositeStrategy.ts @@ -6,6 +6,8 @@ import type { Config } from '../../config/config.js'; import type { BaseLlmClient } from '../../core/baseLlmClient.js'; +import { debugLogger } from '../../utils/debugLogger.js'; +import { coreEvents } from '../../utils/events.js'; import type { RoutingContext, RoutingDecision, @@ -59,7 +61,7 @@ export class CompositeStrategy implements TerminalStrategy { return this.finalizeDecision(decision, startTime); } } catch (error) { - console.error( + debugLogger.warn( `[Routing] Strategy '${strategy.name}' failed. Continuing to next strategy. Error:`, error, ); @@ -76,7 +78,8 @@ export class CompositeStrategy implements TerminalStrategy { return this.finalizeDecision(decision, startTime); } catch (error) { - console.error( + coreEvents.emitFeedback( + 'error', `[Routing] Critical Error: Terminal strategy '${terminalStrategy.name}' failed. Routing cannot proceed. Error:`, error, ); diff --git a/packages/core/src/telemetry/clearcut-logger/clearcut-logger.ts b/packages/core/src/telemetry/clearcut-logger/clearcut-logger.ts index e52669b3a3..4ac02642a7 100644 --- a/packages/core/src/telemetry/clearcut-logger/clearcut-logger.ts +++ b/packages/core/src/telemetry/clearcut-logger/clearcut-logger.ts @@ -277,7 +277,7 @@ export class ClearcutLogger { this.enqueueHelper(event); } catch (error) { if (this.config?.getDebugMode()) { - console.error('ClearcutLogger: Failed to enqueue log event.', error); + debugLogger.warn('ClearcutLogger: Failed to enqueue log event.', error); } } } @@ -301,9 +301,7 @@ export class ClearcutLogger { this.enqueueHelper(event); }); } catch (error) { - if (this.config?.getDebugMode()) { - console.error('ClearcutLogger: Failed to enqueue log event.', error); - } + debugLogger.warn('ClearcutLogger: Failed to enqueue log event.', error); } } @@ -435,7 +433,7 @@ export class ClearcutLogger { }; } else { if (this.config?.getDebugMode()) { - console.error( + debugLogger.warn( `Error flushing log events: HTTP ${response.status}: ${response.statusText}`, ); } @@ -445,7 +443,7 @@ export class ClearcutLogger { } } catch (e: unknown) { if (this.config?.getDebugMode()) { - console.error('Error flushing log events:', e as Error); + debugLogger.warn('Error flushing log events:', e as Error); } // Re-queue failed events for retry diff --git a/packages/core/src/telemetry/sdk.ts b/packages/core/src/telemetry/sdk.ts index 936e501d2c..5753cc5359 100644 --- a/packages/core/src/telemetry/sdk.ts +++ b/packages/core/src/telemetry/sdk.ts @@ -157,7 +157,6 @@ export async function initializeTelemetry( ) { const message = `Telemetry credentials have changed (from ${activeTelemetryEmail} to ${credentials.client_email}), but telemetry cannot be re-initialized in this process. Please restart the CLI to use the new account for telemetry.`; debugLogger.error(message); - console.error(message); } return; } @@ -309,7 +308,7 @@ export async function initializeTelemetry( initializeMetrics(config); void flushTelemetryBuffer(); } catch (error) { - console.error('Error starting OpenTelemetry SDK:', error); + debugLogger.error('Error starting OpenTelemetry SDK:', error); } // Note: We don't use process.on('exit') here because that callback is synchronous @@ -343,7 +342,7 @@ export async function flushTelemetry(config: Config): Promise { debugLogger.log('OpenTelemetry SDK flushed successfully.'); } } catch (error) { - console.error('Error flushing SDK:', error); + debugLogger.error('Error flushing SDK:', error); } } @@ -361,7 +360,7 @@ export async function shutdownTelemetry( debugLogger.log('OpenTelemetry SDK shut down successfully.'); } } catch (error) { - console.error('Error shutting down SDK:', error); + debugLogger.error('Error shutting down SDK:', error); } finally { telemetryInitialized = false; sdk = undefined; diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index e02abecaaf..52e3d8702a 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -1159,7 +1159,7 @@ describe('EditTool', () => { result.returnDisplay.diffStat?.model_removed_lines, ); } else if (result.error) { - console.error(`Edit failed for ${file.path}:`, result.error); + throw result.error; } } diff --git a/packages/core/src/tools/memoryTool.ts b/packages/core/src/tools/memoryTool.ts index 875dc8152f..3e38d6d294 100644 --- a/packages/core/src/tools/memoryTool.ts +++ b/packages/core/src/tools/memoryTool.ts @@ -277,9 +277,6 @@ class MemoryToolInvocation extends BaseToolInvocation< } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); - console.warn( - `[MemoryTool] Error executing save_memory for fact "${fact}": ${errorMessage}`, - ); return { llmContent: JSON.stringify({ success: false, @@ -367,10 +364,6 @@ export class MemoryTool await fsAdapter.writeFile(memoryFilePath, newContent, 'utf-8'); } catch (error) { - console.error( - `[MemoryTool] Error adding memory entry to ${memoryFilePath}:`, - error, - ); throw new Error( `[MemoryTool] Failed to add memory entry: ${error instanceof Error ? error.message : String(error)}`, ); diff --git a/packages/core/src/tools/smart-edit.test.ts b/packages/core/src/tools/smart-edit.test.ts index 01232d7db5..7c32c829c1 100644 --- a/packages/core/src/tools/smart-edit.test.ts +++ b/packages/core/src/tools/smart-edit.test.ts @@ -729,7 +729,7 @@ describe('SmartEditTool', () => { result.returnDisplay.diffStat?.model_removed_lines, ); } else if (result.error) { - console.error(`Edit failed for ${file.path}:`, result.error); + throw result.error; } } diff --git a/packages/core/src/tools/tool-registry.ts b/packages/core/src/tools/tool-registry.ts index 535bed7dea..40b9fc16dc 100644 --- a/packages/core/src/tools/tool-registry.ts +++ b/packages/core/src/tools/tool-registry.ts @@ -416,7 +416,7 @@ export class ToolRegistry { ); } } catch (e) { - console.error(`Tool discovery command "${discoveryCmd}" failed:`, e); + debugLogger.error(`Tool discovery command "${discoveryCmd}" failed:`, e); throw e; } } diff --git a/packages/core/src/tools/web-search.ts b/packages/core/src/tools/web-search.ts index c85ca02a6c..1d81bf96d6 100644 --- a/packages/core/src/tools/web-search.ts +++ b/packages/core/src/tools/web-search.ts @@ -14,6 +14,7 @@ import { ToolErrorType } from './tool-error.js'; import { getErrorMessage } from '../utils/errors.js'; import { type Config } from '../config/config.js'; import { getResponseText } from '../utils/partUtils.js'; +import { debugLogger } from '../utils/debugLogger.js'; interface GroundingChunkWeb { uri?: string; @@ -167,7 +168,7 @@ class WebSearchToolInvocation extends BaseToolInvocation< const errorMessage = `Error during web search for query "${ this.params.query }": ${getErrorMessage(error)}`; - console.error(errorMessage, error); + debugLogger.warn(errorMessage, error); return { llmContent: `Error: ${errorMessage}`, returnDisplay: `Error performing web search.`, diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 4749970977..bd832ac537 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -44,6 +44,7 @@ import { FileOperation } from '../telemetry/metrics.js'; import { getSpecificMimeType } from '../utils/fileUtils.js'; import { getLanguageFromFilePath } from '../utils/language-detection.js'; import type { MessageBus } from '../confirmation-bus/message-bus.js'; +import { debugLogger } from '../utils/debugLogger.js'; /** * Parameters for the WriteFile tool @@ -377,7 +378,7 @@ class WriteFileToolInvocation extends BaseToolInvocation< // Include stack trace in debug mode for better troubleshooting if (this.config.getDebugMode() && error.stack) { - console.error('Write file error stack:', error.stack); + debugLogger.error('Write file error stack:', error.stack); } } else if (error instanceof Error) { errorMsg = `Error writing to file: ${error.message}`; diff --git a/packages/core/src/utils/debugLogger.ts b/packages/core/src/utils/debugLogger.ts index 15ed154f95..9c5a82c123 100644 --- a/packages/core/src/utils/debugLogger.ts +++ b/packages/core/src/utils/debugLogger.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +/* eslint-disable no-console */ import * as fs from 'node:fs'; import * as util from 'node:util'; diff --git a/packages/core/src/utils/editCorrector.ts b/packages/core/src/utils/editCorrector.ts index 9c74d81f5a..99019e2b60 100644 --- a/packages/core/src/utils/editCorrector.ts +++ b/packages/core/src/utils/editCorrector.ts @@ -22,6 +22,7 @@ import { } from '../utils/messageInspectors.js'; import * as fs from 'node:fs'; import { promptIdContext } from './promptIdContext.js'; +import { debugLogger } from './debugLogger.js'; const CODE_CORRECTION_SYSTEM_PROMPT = ` You are an expert code-editing assistant. Your task is to analyze a failed edit attempt and provide a corrected version of the text snippets. @@ -434,7 +435,7 @@ Return ONLY the corrected target snippet in the specified JSON format with the k throw error; } - console.error( + debugLogger.warn( 'Error during LLM call for old string snippet correction:', error, ); @@ -523,7 +524,7 @@ Return ONLY the corrected string in the specified JSON format with the key 'corr throw error; } - console.error('Error during LLM call for new_string correction:', error); + debugLogger.warn('Error during LLM call for new_string correction:', error); return originalNewString; } } @@ -593,7 +594,7 @@ Return ONLY the corrected string in the specified JSON format with the key 'corr throw error; } - console.error( + debugLogger.warn( 'Error during LLM call for new_string escaping correction:', error, ); @@ -660,7 +661,7 @@ Return ONLY the corrected string in the specified JSON format with the key 'corr throw error; } - console.error( + debugLogger.warn( 'Error during LLM call for string escaping correction:', error, ); diff --git a/packages/core/src/utils/errorReporting.test.ts b/packages/core/src/utils/errorReporting.test.ts index 6b92c2499d..303fa84168 100644 --- a/packages/core/src/utils/errorReporting.test.ts +++ b/packages/core/src/utils/errorReporting.test.ts @@ -9,12 +9,13 @@ import fs from 'node:fs/promises'; import os from 'node:os'; import path from 'node:path'; import { reportError } from './errorReporting.js'; +import { debugLogger } from './debugLogger.js'; // Use a type alias for SpyInstance as it's not directly exported type SpyInstance = ReturnType; describe('reportError', () => { - let consoleErrorSpy: SpyInstance; + let debugLoggerErrorSpy: SpyInstance; let testDir: string; const MOCK_TIMESTAMP = '2025-01-01T00-00-00-000Z'; @@ -22,7 +23,9 @@ describe('reportError', () => { // Create a temporary directory for logs testDir = await fs.mkdtemp(path.join(os.tmpdir(), 'gemini-report-test-')); vi.resetAllMocks(); - consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + debugLoggerErrorSpy = vi + .spyOn(debugLogger, 'error') + .mockImplementation(() => {}); vi.spyOn(Date.prototype, 'toISOString').mockReturnValue(MOCK_TIMESTAMP); }); @@ -54,9 +57,10 @@ describe('reportError', () => { context, }); - // Verify the console log - expect(consoleErrorSpy).toHaveBeenCalledWith( + // Verify the user feedback + expect(debugLoggerErrorSpy).toHaveBeenCalledWith( `${baseMessage} Full report available at: ${expectedReportPath}`, + error, ); }); @@ -75,8 +79,9 @@ describe('reportError', () => { error: { message: 'Test plain object error' }, }); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerErrorSpy).toHaveBeenCalledWith( `${baseMessage} Full report available at: ${expectedReportPath}`, + error, ); }); @@ -95,8 +100,9 @@ describe('reportError', () => { error: { message: 'Just a string error' }, }); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerErrorSpy).toHaveBeenCalledWith( `${baseMessage} Full report available at: ${expectedReportPath}`, + error, ); }); @@ -109,15 +115,18 @@ describe('reportError', () => { await reportError(error, baseMessage, context, type, nonExistentDir); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerErrorSpy).toHaveBeenCalledWith( `${baseMessage} Additionally, failed to write detailed error report:`, expect.any(Error), // The actual write error ); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerErrorSpy).toHaveBeenCalledWith( 'Original error that triggered report generation:', error, ); - expect(consoleErrorSpy).toHaveBeenCalledWith('Original context:', context); + expect(debugLoggerErrorSpy).toHaveBeenCalledWith( + 'Original context:', + context, + ); }); it('should handle stringification failure of report content (e.g. BigInt in context)', async () => { @@ -146,15 +155,15 @@ describe('reportError', () => { await reportError(error, baseMessage, context, type, testDir); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerErrorSpy).toHaveBeenCalledWith( `${baseMessage} Could not stringify report content (likely due to context):`, stringifyError, ); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerErrorSpy).toHaveBeenCalledWith( 'Original error that triggered report generation:', error, ); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerErrorSpy).toHaveBeenCalledWith( 'Original context could not be stringified or included in report.', ); @@ -165,8 +174,9 @@ describe('reportError', () => { error: { message: error.message, stack: error.stack }, }); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerErrorSpy).toHaveBeenCalledWith( `${baseMessage} Partial report (excluding context) available at: ${expectedMinimalReportPath}`, + error, ); }); @@ -186,8 +196,9 @@ describe('reportError', () => { error: { message: 'Error without context', stack: 'No context stack' }, }); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerErrorSpy).toHaveBeenCalledWith( `${baseMessage} Full report available at: ${expectedReportPath}`, + error, ); }); }); diff --git a/packages/core/src/utils/errorReporting.ts b/packages/core/src/utils/errorReporting.ts index 9743d32411..d9eecbbdba 100644 --- a/packages/core/src/utils/errorReporting.ts +++ b/packages/core/src/utils/errorReporting.ts @@ -8,6 +8,7 @@ import fs from 'node:fs/promises'; import os from 'node:os'; import path from 'node:path'; import type { Content } from '@google/genai'; +import { debugLogger } from './debugLogger.js'; interface ErrorReportData { error: { message: string; stack?: string } | { message: string }; @@ -16,7 +17,7 @@ interface ErrorReportData { } /** - * Generates an error report, writes it to a temporary file, and logs information to console.error. + * Generates an error report, writes it to a temporary file, and logs information to user * @param error The error object. * @param context The relevant context (e.g., chat history, request contents). * @param type A string to identify the type of error (e.g., 'startChat', 'generateJson-api'). @@ -59,13 +60,16 @@ export async function reportError( stringifiedReportContent = JSON.stringify(reportContent, null, 2); } catch (stringifyError) { // This can happen if context contains something like BigInt - console.error( + debugLogger.error( `${baseMessage} Could not stringify report content (likely due to context):`, stringifyError, ); - console.error('Original error that triggered report generation:', error); + debugLogger.error( + 'Original error that triggered report generation:', + error, + ); if (context) { - console.error( + debugLogger.error( 'Original context could not be stringified or included in report.', ); } @@ -75,11 +79,12 @@ export async function reportError( stringifiedReportContent = JSON.stringify(minimalReportContent, null, 2); // Still try to write the minimal report await fs.writeFile(reportPath, stringifiedReportContent); - console.error( + debugLogger.error( `${baseMessage} Partial report (excluding context) available at: ${reportPath}`, + error, ); } catch (minimalWriteError) { - console.error( + debugLogger.error( `${baseMessage} Failed to write even a minimal error report:`, minimalWriteError, ); @@ -89,28 +94,37 @@ export async function reportError( try { await fs.writeFile(reportPath, stringifiedReportContent); - console.error(`${baseMessage} Full report available at: ${reportPath}`); + debugLogger.error( + `${baseMessage} Full report available at: ${reportPath}`, + error, + ); } catch (writeError) { - console.error( + debugLogger.error( `${baseMessage} Additionally, failed to write detailed error report:`, writeError, ); // Log the original error as a fallback if report writing fails - console.error('Original error that triggered report generation:', error); + debugLogger.error( + 'Original error that triggered report generation:', + error, + ); + if (context) { // Context was stringifiable, but writing the file failed. // We already have stringifiedReportContent, but it might be too large for console. // So, we try to log the original context object, and if that fails, its stringified version (truncated). try { - console.error('Original context:', context); + debugLogger.error('Original context:', context); } catch { try { - console.error( + debugLogger.error( 'Original context (stringified, truncated):', JSON.stringify(context).substring(0, 1000), ); } catch { - console.error('Original context could not be logged or stringified.'); + debugLogger.error( + 'Original context could not be logged or stringified.', + ); } } } diff --git a/packages/core/src/utils/getFolderStructure.ts b/packages/core/src/utils/getFolderStructure.ts index 141d4f542d..0544f4655d 100644 --- a/packages/core/src/utils/getFolderStructure.ts +++ b/packages/core/src/utils/getFolderStructure.ts @@ -342,7 +342,10 @@ export async function getFolderStructure( return `${summary}\n\n${resolvedPath}${path.sep}\n${structureLines.join('\n')}`; } catch (error: unknown) { - console.error(`Error getting folder structure for ${resolvedPath}:`, error); + debugLogger.warn( + `Error getting folder structure for ${resolvedPath}:`, + error, + ); return `Error processing directory "${resolvedPath}": ${getErrorMessage(error)}`; } } diff --git a/packages/core/src/utils/memoryDiscovery.ts b/packages/core/src/utils/memoryDiscovery.ts index b0fc766e05..88a6760fe0 100644 --- a/packages/core/src/utils/memoryDiscovery.ts +++ b/packages/core/src/utils/memoryDiscovery.ts @@ -31,7 +31,7 @@ const logger = { debugLogger.warn('[WARN] [MemoryDiscovery]', ...args), // eslint-disable-next-line @typescript-eslint/no-explicit-any error: (...args: any[]) => - console.error('[ERROR] [MemoryDiscovery]', ...args), + debugLogger.error('[ERROR] [MemoryDiscovery]', ...args), }; export interface GeminiFileContent { diff --git a/packages/core/src/utils/retry.ts b/packages/core/src/utils/retry.ts index fd91cbd2ff..fe5713287b 100644 --- a/packages/core/src/utils/retry.ts +++ b/packages/core/src/utils/retry.ts @@ -232,7 +232,7 @@ export async function retryWithBackoff( continue; } } catch (fallbackError) { - console.warn('Model fallback failed:', fallbackError); + debugLogger.warn('Model fallback failed:', fallbackError); } } throw classifiedError instanceof RetryableQuotaError @@ -241,7 +241,7 @@ export async function retryWithBackoff( } if (classifiedError instanceof RetryableQuotaError) { - console.warn( + debugLogger.warn( `Attempt ${attempt} failed: ${classifiedError.message}. Retrying after ${classifiedError.retryDelayMs}ms...`, ); await delay(classifiedError.retryDelayMs, signal); @@ -300,7 +300,7 @@ function logRetryAttempt( if (errorStatus === 429) { debugLogger.warn(message, error); } else if (errorStatus && errorStatus >= 500 && errorStatus < 600) { - console.error(message, error); + debugLogger.warn(message, error); } else if (error instanceof Error) { // Fallback for errors that might not have a status but have a message if (error.message.includes('429')) { @@ -309,7 +309,7 @@ function logRetryAttempt( error, ); } else if (error.message.match(/5\d{2}/)) { - console.error( + debugLogger.warn( `Attempt ${attempt} failed with 5xx error. Retrying with backoff...`, error, ); diff --git a/packages/core/src/utils/summarizer.test.ts b/packages/core/src/utils/summarizer.test.ts index e44196f9d1..83d30128a7 100644 --- a/packages/core/src/utils/summarizer.test.ts +++ b/packages/core/src/utils/summarizer.test.ts @@ -19,6 +19,7 @@ import type { ResolvedModelConfig, } from '../services/modelConfigService.js'; import { DEFAULT_GEMINI_MODEL } from '../config/models.js'; +import { debugLogger } from './debugLogger.js'; // Mock GeminiClient and Config constructor vi.mock('../core/client.js'); @@ -58,11 +59,11 @@ describe('summarizers', () => { (mockGeminiClient.generateContent as Mock) = vi.fn(); vi.spyOn(console, 'error').mockImplementation(() => {}); + vi.spyOn(debugLogger, 'warn').mockImplementation(() => {}); }); afterEach(() => { - vi.clearAllMocks(); - (console.error as Mock).mockRestore(); + vi.restoreAllMocks(); }); describe('summarizeToolOutput', () => {