diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 5f37e1103..22a84a701 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -176,9 +176,6 @@ describe('Session', () => { }); it('swallows errors and does not throw', async () => { - const consoleErrorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => undefined); getAvailableCommandsSpy.mockRejectedValueOnce( new Error('Command discovery failed'), ); @@ -187,8 +184,6 @@ describe('Session', () => { session.sendAvailableCommandsUpdate(), ).resolves.toBeUndefined(); expect(mockClient.sessionUpdate).not.toHaveBeenCalled(); - expect(consoleErrorSpy).toHaveBeenCalled(); - consoleErrorSpy.mockRestore(); }); }); diff --git a/packages/cli/src/commands/extensions/disable.test.ts b/packages/cli/src/commands/extensions/disable.test.ts index 7bde3ee0a..6e54dd191 100644 --- a/packages/cli/src/commands/extensions/disable.test.ts +++ b/packages/cli/src/commands/extensions/disable.test.ts @@ -4,19 +4,14 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { - describe, - it, - expect, - vi, - beforeEach, - type MockInstance, -} from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import { disableCommand, handleDisable } from './disable.js'; import yargs from 'yargs'; import { SettingScope } from '../../config/settings.js'; const mockDisableExtension = vi.hoisted(() => vi.fn()); +const mockWriteStdoutLine = vi.hoisted(() => vi.fn()); +const mockWriteStderrLine = vi.hoisted(() => vi.fn()); vi.mock('./utils.js', () => ({ getExtensionManager: vi.fn().mockResolvedValue({ @@ -28,6 +23,12 @@ vi.mock('../../utils/errors.js', () => ({ getErrorMessage: vi.fn((error: Error) => error.message), })); +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: mockWriteStdoutLine, + writeStderrLine: mockWriteStderrLine, + clearScreen: vi.fn(), +})); + describe('extensions disable command', () => { it('should fail if no name is provided', () => { const validationParser = yargs([]) @@ -59,20 +60,15 @@ describe('extensions disable command', () => { }); describe('handleDisable', () => { - let consoleLogSpy: MockInstance; - let consoleErrorSpy: MockInstance; - let processExitSpy: MockInstance; - beforeEach(() => { - consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); - consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - processExitSpy = vi - .spyOn(process, 'exit') - .mockImplementation(() => undefined as never); vi.clearAllMocks(); }); it('should disable an extension with user scope', async () => { + const processExitSpy = vi + .spyOn(process, 'exit') + .mockImplementation(() => undefined as never); + await handleDisable({ name: 'test-extension', scope: 'user', @@ -82,12 +78,18 @@ describe('handleDisable', () => { 'test-extension', SettingScope.User, ); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "test-extension" successfully disabled for scope "user".', ); + + processExitSpy.mockRestore(); }); it('should disable an extension with workspace scope', async () => { + const processExitSpy = vi + .spyOn(process, 'exit') + .mockImplementation(() => undefined as never); + await handleDisable({ name: 'test-extension', scope: 'workspace', @@ -97,12 +99,18 @@ describe('handleDisable', () => { 'test-extension', SettingScope.Workspace, ); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "test-extension" successfully disabled for scope "workspace".', ); + + processExitSpy.mockRestore(); }); it('should default to user scope when no scope is provided', async () => { + const processExitSpy = vi + .spyOn(process, 'exit') + .mockImplementation(() => undefined as never); + await handleDisable({ name: 'test-extension', }); @@ -111,9 +119,15 @@ describe('handleDisable', () => { 'test-extension', SettingScope.User, ); + + processExitSpy.mockRestore(); }); it('should handle errors and exit with code 1', async () => { + const processExitSpy = vi + .spyOn(process, 'exit') + .mockImplementation(() => undefined as never); + mockDisableExtension.mockImplementationOnce(() => { throw new Error('Disable failed'); }); @@ -123,7 +137,9 @@ describe('handleDisable', () => { scope: 'user', }); - expect(consoleErrorSpy).toHaveBeenCalledWith('Disable failed'); + expect(mockWriteStderrLine).toHaveBeenCalledWith('Disable failed'); expect(processExitSpy).toHaveBeenCalledWith(1); + + processExitSpy.mockRestore(); }); }); diff --git a/packages/cli/src/commands/extensions/enable.test.ts b/packages/cli/src/commands/extensions/enable.test.ts index 374918e0a..3f77b0f53 100644 --- a/packages/cli/src/commands/extensions/enable.test.ts +++ b/packages/cli/src/commands/extensions/enable.test.ts @@ -4,19 +4,13 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { - describe, - it, - expect, - vi, - beforeEach, - type MockInstance, -} from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import { enableCommand, handleEnable } from './enable.js'; import yargs from 'yargs'; import { SettingScope } from '../../config/settings.js'; const mockEnableExtension = vi.hoisted(() => vi.fn()); +const mockWriteStdoutLine = vi.hoisted(() => vi.fn()); vi.mock('./utils.js', () => ({ getExtensionManager: vi.fn().mockResolvedValue({ @@ -39,6 +33,12 @@ vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { }; }); +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: mockWriteStdoutLine, + writeStderrLine: vi.fn(), + clearScreen: vi.fn(), +})); + describe('extensions enable command', () => { it('should fail if no name is provided', () => { const validationParser = yargs([]) @@ -70,10 +70,7 @@ describe('extensions enable command', () => { }); describe('handleEnable', () => { - let consoleLogSpy: MockInstance; - beforeEach(() => { - consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); vi.clearAllMocks(); }); @@ -87,7 +84,7 @@ describe('handleEnable', () => { 'test-extension', SettingScope.User, ); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "test-extension" successfully enabled for scope "user".', ); }); @@ -102,7 +99,7 @@ describe('handleEnable', () => { 'test-extension', SettingScope.Workspace, ); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "test-extension" successfully enabled for scope "workspace".', ); }); @@ -116,7 +113,7 @@ describe('handleEnable', () => { 'test-extension', SettingScope.User, ); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "test-extension" successfully enabled in all scopes.', ); }); diff --git a/packages/cli/src/commands/extensions/install.test.ts b/packages/cli/src/commands/extensions/install.test.ts index f002d1a12..f49c2d48a 100644 --- a/packages/cli/src/commands/extensions/install.test.ts +++ b/packages/cli/src/commands/extensions/install.test.ts @@ -4,15 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { - describe, - it, - expect, - vi, - beforeEach, - afterEach, - type MockInstance, -} from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { handleInstall, installCommand } from './install.js'; import yargs from 'yargs'; @@ -23,6 +15,8 @@ const mockRequestConsentNonInteractive = vi.hoisted(() => vi.fn()); const mockRequestConsentOrFail = vi.hoisted(() => vi.fn()); const mockIsWorkspaceTrusted = vi.hoisted(() => vi.fn()); const mockLoadSettings = vi.hoisted(() => vi.fn()); +const mockWriteStdoutLine = vi.hoisted(() => vi.fn()); +const mockWriteStderrLine = vi.hoisted(() => vi.fn()); vi.mock('@qwen-code/qwen-code-core', () => ({ ExtensionManager: vi.fn().mockImplementation(() => ({ @@ -50,6 +44,12 @@ vi.mock('../../utils/errors.js', () => ({ getErrorMessage: vi.fn((error: Error) => error.message), })); +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: mockWriteStdoutLine, + writeStderrLine: mockWriteStderrLine, + clearScreen: vi.fn(), +})); + describe('extensions install command', () => { it('should fail if no source is provided', () => { const validationParser = yargs([]) @@ -63,16 +63,7 @@ describe('extensions install command', () => { }); describe('handleInstall', () => { - let consoleLogSpy: MockInstance; - let consoleErrorSpy: MockInstance; - let processSpy: MockInstance; - beforeEach(() => { - consoleLogSpy = vi.spyOn(console, 'log'); - consoleErrorSpy = vi.spyOn(console, 'error'); - processSpy = vi - .spyOn(process, 'exit') - .mockImplementation(() => undefined as never); mockRefreshCache.mockResolvedValue(undefined); mockLoadSettings.mockReturnValue({ merged: {} }); mockIsWorkspaceTrusted.mockReturnValue(true); @@ -83,6 +74,10 @@ describe('handleInstall', () => { }); it('should install an extension from a http source', async () => { + const processSpy = vi + .spyOn(process, 'exit') + .mockImplementation(() => undefined as never); + mockParseInstallSource.mockResolvedValue({ type: 'http', url: 'http://google.com', @@ -93,12 +88,18 @@ describe('handleInstall', () => { source: 'http://google.com', }); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "http-extension" installed successfully and enabled.', ); + + processSpy.mockRestore(); }); it('should install an extension from a https source', async () => { + const processSpy = vi + .spyOn(process, 'exit') + .mockImplementation(() => undefined as never); + mockParseInstallSource.mockResolvedValue({ type: 'https', url: 'https://google.com', @@ -109,12 +110,18 @@ describe('handleInstall', () => { source: 'https://google.com', }); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "https-extension" installed successfully and enabled.', ); + + processSpy.mockRestore(); }); it('should install an extension from a git source', async () => { + const processSpy = vi + .spyOn(process, 'exit') + .mockImplementation(() => undefined as never); + mockParseInstallSource.mockResolvedValue({ type: 'git', url: 'git@some-url', @@ -125,12 +132,18 @@ describe('handleInstall', () => { source: 'git@some-url', }); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "git-extension" installed successfully and enabled.', ); + + processSpy.mockRestore(); }); it('throws an error from an unknown source', async () => { + const processSpy = vi + .spyOn(process, 'exit') + .mockImplementation(() => undefined as never); + mockParseInstallSource.mockRejectedValue( new Error('Install source not found.'), ); @@ -138,11 +151,19 @@ describe('handleInstall', () => { source: 'test://google.com', }); - expect(consoleErrorSpy).toHaveBeenCalledWith('Install source not found.'); + expect(mockWriteStderrLine).toHaveBeenCalledWith( + 'Install source not found.', + ); expect(processSpy).toHaveBeenCalledWith(1); + + processSpy.mockRestore(); }); it('should install an extension from a sso source', async () => { + const processSpy = vi + .spyOn(process, 'exit') + .mockImplementation(() => undefined as never); + mockParseInstallSource.mockResolvedValue({ type: 'sso', url: 'sso://google.com', @@ -153,12 +174,18 @@ describe('handleInstall', () => { source: 'sso://google.com', }); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "sso-extension" installed successfully and enabled.', ); + + processSpy.mockRestore(); }); it('should install an extension from a local path', async () => { + const processSpy = vi + .spyOn(process, 'exit') + .mockImplementation(() => undefined as never); + mockParseInstallSource.mockResolvedValue({ type: 'local', path: '/some/path', @@ -169,12 +196,18 @@ describe('handleInstall', () => { source: '/some/path', }); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "local-extension" installed successfully and enabled.', ); + + processSpy.mockRestore(); }); it('should throw an error if install extension fails', async () => { + const processSpy = vi + .spyOn(process, 'exit') + .mockImplementation(() => undefined as never); + mockParseInstallSource.mockResolvedValue({ type: 'git', url: 'git@some-url', @@ -185,7 +218,11 @@ describe('handleInstall', () => { await handleInstall({ source: 'git@some-url' }); - expect(consoleErrorSpy).toHaveBeenCalledWith('Install extension failed'); + expect(mockWriteStderrLine).toHaveBeenCalledWith( + 'Install extension failed', + ); expect(processSpy).toHaveBeenCalledWith(1); + + processSpy.mockRestore(); }); }); diff --git a/packages/cli/src/commands/extensions/link.test.ts b/packages/cli/src/commands/extensions/link.test.ts index babe4ce90..9aff17c8b 100644 --- a/packages/cli/src/commands/extensions/link.test.ts +++ b/packages/cli/src/commands/extensions/link.test.ts @@ -4,18 +4,13 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { - describe, - it, - expect, - vi, - beforeEach, - type MockInstance, -} from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import { linkCommand, handleLink } from './link.js'; import yargs from 'yargs'; const mockInstallExtension = vi.hoisted(() => vi.fn()); +const mockWriteStdoutLine = vi.hoisted(() => vi.fn()); +const mockWriteStderrLine = vi.hoisted(() => vi.fn()); vi.mock('./utils.js', () => ({ getExtensionManager: vi.fn().mockResolvedValue({ @@ -32,6 +27,12 @@ vi.mock('../../utils/errors.js', () => ({ getErrorMessage: vi.fn((error: Error) => error.message), })); +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: mockWriteStdoutLine, + writeStderrLine: mockWriteStderrLine, + clearScreen: vi.fn(), +})); + describe('extensions link command', () => { it('should fail if no path is provided', () => { const validationParser = yargs([]) @@ -50,20 +51,15 @@ describe('extensions link command', () => { }); describe('handleLink', () => { - let consoleLogSpy: MockInstance; - let consoleErrorSpy: MockInstance; - let processExitSpy: MockInstance; - beforeEach(() => { - consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); - consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - processExitSpy = vi - .spyOn(process, 'exit') - .mockImplementation(() => undefined as never); vi.clearAllMocks(); }); it('should link an extension from a local path', async () => { + const processExitSpy = vi + .spyOn(process, 'exit') + .mockImplementation(() => undefined as never); + mockInstallExtension.mockResolvedValueOnce({ name: 'linked-extension' }); await handleLink({ @@ -77,19 +73,27 @@ describe('handleLink', () => { }, expect.any(Function), ); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "linked-extension" linked successfully and enabled.', ); + + processExitSpy.mockRestore(); }); it('should handle errors and exit with code 1', async () => { + const processExitSpy = vi + .spyOn(process, 'exit') + .mockImplementation(() => undefined as never); + mockInstallExtension.mockRejectedValueOnce(new Error('Link failed')); await handleLink({ path: '/some/local/path', }); - expect(consoleErrorSpy).toHaveBeenCalledWith('Link failed'); + expect(mockWriteStderrLine).toHaveBeenCalledWith('Link failed'); expect(processExitSpy).toHaveBeenCalledWith(1); + + processExitSpy.mockRestore(); }); }); diff --git a/packages/cli/src/commands/extensions/list.test.ts b/packages/cli/src/commands/extensions/list.test.ts index 8c7a24951..96b7cf8d9 100644 --- a/packages/cli/src/commands/extensions/list.test.ts +++ b/packages/cli/src/commands/extensions/list.test.ts @@ -4,19 +4,14 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { - describe, - it, - expect, - vi, - beforeEach, - type MockInstance, -} from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import { listCommand, handleList } from './list.js'; import yargs from 'yargs'; const mockGetLoadedExtensions = vi.hoisted(() => vi.fn()); const mockToOutputString = vi.hoisted(() => vi.fn()); +const mockWriteStdoutLine = vi.hoisted(() => vi.fn()); +const mockWriteStderrLine = vi.hoisted(() => vi.fn()); vi.mock('./utils.js', () => ({ getExtensionManager: vi.fn().mockResolvedValue({ @@ -30,6 +25,12 @@ vi.mock('../../utils/errors.js', () => ({ getErrorMessage: vi.fn((error: Error) => error.message), })); +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: mockWriteStdoutLine, + writeStderrLine: mockWriteStderrLine, + clearScreen: vi.fn(), +})); + describe('extensions list command', () => { it('should parse the list command', () => { const parser = yargs([]).command(listCommand).fail(false).locale('en'); @@ -38,28 +39,31 @@ describe('extensions list command', () => { }); describe('handleList', () => { - let consoleLogSpy: MockInstance; - let consoleErrorSpy: MockInstance; - let processExitSpy: MockInstance; - beforeEach(() => { - consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); - consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - processExitSpy = vi - .spyOn(process, 'exit') - .mockImplementation(() => undefined as never); vi.clearAllMocks(); }); it('should display message when no extensions are installed', async () => { + const processExitSpy = vi + .spyOn(process, 'exit') + .mockImplementation(() => undefined as never); + mockGetLoadedExtensions.mockReturnValueOnce([]); await handleList(); - expect(consoleLogSpy).toHaveBeenCalledWith('No extensions installed.'); + expect(mockWriteStdoutLine).toHaveBeenCalledWith( + 'No extensions installed.', + ); + + processExitSpy.mockRestore(); }); it('should list installed extensions', async () => { + const processExitSpy = vi + .spyOn(process, 'exit') + .mockImplementation(() => undefined as never); + const mockExtensions = [ { name: 'extension-1', version: '1.0.0' }, { name: 'extension-2', version: '2.0.0' }, @@ -73,19 +77,27 @@ describe('handleList', () => { expect(mockGetLoadedExtensions).toHaveBeenCalled(); expect(mockToOutputString).toHaveBeenCalledTimes(2); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'extension-1 (1.0.0)\n\nextension-2 (2.0.0)', ); + + processExitSpy.mockRestore(); }); it('should handle errors and exit with code 1', async () => { + const processExitSpy = vi + .spyOn(process, 'exit') + .mockImplementation(() => undefined as never); + mockGetLoadedExtensions.mockImplementationOnce(() => { throw new Error('List failed'); }); await handleList(); - expect(consoleErrorSpy).toHaveBeenCalledWith('List failed'); + expect(mockWriteStderrLine).toHaveBeenCalledWith('List failed'); expect(processExitSpy).toHaveBeenCalledWith(1); + + processExitSpy.mockRestore(); }); }); diff --git a/packages/cli/src/commands/extensions/settings.test.ts b/packages/cli/src/commands/extensions/settings.test.ts index 042965eec..be043a2c2 100644 --- a/packages/cli/src/commands/extensions/settings.test.ts +++ b/packages/cli/src/commands/extensions/settings.test.ts @@ -4,14 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { - describe, - it, - expect, - vi, - beforeEach, - type MockInstance, -} from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import { settingsCommand } from './settings.js'; import yargs from 'yargs'; @@ -19,6 +12,13 @@ const mockGetLoadedExtensions = vi.hoisted(() => vi.fn()); const mockGetScopedEnvContents = vi.hoisted(() => vi.fn()); const mockUpdateSetting = vi.hoisted(() => vi.fn()); const mockPromptForSetting = vi.hoisted(() => vi.fn()); +const mockWriteStdoutLine = vi.hoisted(() => vi.fn()); + +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: mockWriteStdoutLine, + writeStderrLine: vi.fn(), + clearScreen: vi.fn(), +})); vi.mock('./utils.js', () => ({ getExtensionManager: vi.fn().mockResolvedValue({ @@ -89,10 +89,7 @@ describe('extensions settings command', () => { }); describe('settings set handler', () => { - let consoleLogSpy: MockInstance; - beforeEach(() => { - consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); vi.clearAllMocks(); }); @@ -123,7 +120,7 @@ describe('settings set handler', () => { const parser = yargs([]).command(settingsCommand); await parser.parseAsync('settings set my-extension API_KEY'); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "my-extension" not found.', ); expect(mockUpdateSetting).not.toHaveBeenCalled(); @@ -173,10 +170,7 @@ describe('settings set handler', () => { }); describe('settings list handler', () => { - let consoleLogSpy: MockInstance; - beforeEach(() => { - consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); vi.clearAllMocks(); }); @@ -207,7 +201,7 @@ describe('settings list handler', () => { const parser = yargs([]).command(settingsCommand); await parser.parseAsync('settings list my-extension'); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "my-extension" not found.', ); }); @@ -224,7 +218,7 @@ describe('settings list handler', () => { const parser = yargs([]).command(settingsCommand); await parser.parseAsync('settings list my-extension'); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "my-extension" has no settings to configure.', ); }); @@ -257,10 +251,16 @@ describe('settings list handler', () => { const parser = yargs([]).command(settingsCommand); await parser.parseAsync('settings list my-extension'); - expect(consoleLogSpy).toHaveBeenCalledWith('Settings for "my-extension":'); - expect(consoleLogSpy).toHaveBeenCalledWith('\n- API Key (API_KEY)'); - expect(consoleLogSpy).toHaveBeenCalledWith(' Description: Your API key'); - expect(consoleLogSpy).toHaveBeenCalledWith(' Value: my-api-key (user)'); + expect(mockWriteStdoutLine).toHaveBeenCalledWith( + 'Settings for "my-extension":', + ); + expect(mockWriteStdoutLine).toHaveBeenCalledWith('\n- API Key (API_KEY)'); + expect(mockWriteStdoutLine).toHaveBeenCalledWith( + ' Description: Your API key', + ); + expect(mockWriteStdoutLine).toHaveBeenCalledWith( + ' Value: my-api-key (user)', + ); }); it('should show workspace scope for workspace-scoped settings', async () => { @@ -286,7 +286,7 @@ describe('settings list handler', () => { await parser.parseAsync('settings list my-extension'); // Workspace should override user, and show (workspace) scope - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( ' Value: workspace-value (workspace)', ); }); @@ -313,7 +313,7 @@ describe('settings list handler', () => { const parser = yargs([]).command(settingsCommand); await parser.parseAsync('settings list my-extension'); - expect(consoleLogSpy).toHaveBeenCalledWith(' Value: [not set]'); + expect(mockWriteStdoutLine).toHaveBeenCalledWith(' Value: [not set]'); }); it('should show [value stored in keychain] for sensitive settings', async () => { @@ -338,7 +338,7 @@ describe('settings list handler', () => { const parser = yargs([]).command(settingsCommand); await parser.parseAsync('settings list my-extension'); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( ' Value: [value stored in keychain] (user)', ); }); diff --git a/packages/cli/src/commands/extensions/update.test.ts b/packages/cli/src/commands/extensions/update.test.ts index 7dfeec1d6..b78ea6608 100644 --- a/packages/cli/src/commands/extensions/update.test.ts +++ b/packages/cli/src/commands/extensions/update.test.ts @@ -4,14 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { - describe, - it, - expect, - vi, - beforeEach, - type MockInstance, -} from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import { updateCommand, handleUpdate } from './update.js'; import yargs from 'yargs'; import { ExtensionUpdateState } from '../../ui/state/extensions.js'; @@ -21,6 +14,8 @@ const mockUpdateExtension = vi.hoisted(() => vi.fn()); const mockCheckForAllExtensionUpdates = vi.hoisted(() => vi.fn()); const mockUpdateAllUpdatableExtensions = vi.hoisted(() => vi.fn()); const mockCheckForExtensionUpdate = vi.hoisted(() => vi.fn()); +const mockWriteStdoutLine = vi.hoisted(() => vi.fn()); +const mockWriteStderrLine = vi.hoisted(() => vi.fn()); vi.mock('./utils.js', () => ({ getExtensionManager: vi.fn().mockResolvedValue({ @@ -47,6 +42,12 @@ vi.mock('../../ui/state/extensions.js', () => ({ }, })); +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: mockWriteStdoutLine, + writeStderrLine: mockWriteStderrLine, + clearScreen: vi.fn(), +})); + describe('extensions update command', () => { it('should fail if neither name nor --all is provided', () => { const validationParser = yargs([]) @@ -80,12 +81,7 @@ describe('extensions update command', () => { }); describe('handleUpdate', () => { - let consoleLogSpy: MockInstance; - let consoleErrorSpy: MockInstance; - beforeEach(() => { - consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); - consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); vi.clearAllMocks(); }); @@ -95,7 +91,7 @@ describe('handleUpdate', () => { await handleUpdate({ name: 'non-existent-extension' }); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "non-existent-extension" not found.', ); }); @@ -107,7 +103,7 @@ describe('handleUpdate', () => { await handleUpdate({ name: 'test-extension' }); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Unable to install extension "test-extension" due to missing install metadata', ); }); @@ -124,7 +120,7 @@ describe('handleUpdate', () => { await handleUpdate({ name: 'test-extension' }); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "test-extension" is already up to date.', ); }); @@ -151,7 +147,7 @@ describe('handleUpdate', () => { ExtensionUpdateState.UPDATE_AVAILABLE, expect.any(Function), ); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "test-extension" successfully updated: 1.0.0 → 2.0.0.', ); }); @@ -173,7 +169,7 @@ describe('handleUpdate', () => { await handleUpdate({ name: 'test-extension' }); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "test-extension" is already up to date.', ); }); @@ -190,7 +186,7 @@ describe('handleUpdate', () => { await handleUpdate({ name: 'test-extension' }); - expect(consoleErrorSpy).toHaveBeenCalledWith('Update check failed'); + expect(mockWriteStderrLine).toHaveBeenCalledWith('Update check failed'); }); }); @@ -201,7 +197,9 @@ describe('handleUpdate', () => { await handleUpdate({ all: true }); - expect(consoleLogSpy).toHaveBeenCalledWith('No extensions to update.'); + expect(mockWriteStdoutLine).toHaveBeenCalledWith( + 'No extensions to update.', + ); }); it('should update all extensions with updates available', async () => { @@ -221,7 +219,7 @@ describe('handleUpdate', () => { await handleUpdate({ all: true }); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "extension-1" successfully updated: 1.0.0 → 2.0.0.\n' + 'Extension "extension-2" successfully updated: 1.0.0 → 1.5.0.', ); @@ -244,7 +242,7 @@ describe('handleUpdate', () => { await handleUpdate({ all: true }); - expect(consoleLogSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Extension "extension-1" successfully updated: 1.0.0 → 2.0.0.', ); }); @@ -256,7 +254,7 @@ describe('handleUpdate', () => { await handleUpdate({ all: true }); - expect(consoleErrorSpy).toHaveBeenCalledWith('Update all failed'); + expect(mockWriteStderrLine).toHaveBeenCalledWith('Update all failed'); }); }); }); diff --git a/packages/cli/src/commands/mcp/add.test.ts b/packages/cli/src/commands/mcp/add.test.ts index 94c85f656..19a069975 100644 --- a/packages/cli/src/commands/mcp/add.test.ts +++ b/packages/cli/src/commands/mcp/add.test.ts @@ -9,6 +9,15 @@ import { addCommand } from './add.js'; import { loadSettings, SettingScope } from '../../config/settings.js'; import { describe, it, expect, vi, beforeEach } from 'vitest'; +const mockWriteStdoutLine = vi.hoisted(() => vi.fn()); +const mockWriteStderrLine = vi.hoisted(() => vi.fn()); + +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: mockWriteStdoutLine, + writeStderrLine: mockWriteStderrLine, + clearScreen: vi.fn(), +})); + vi.mock('fs/promises', async (importOriginal) => { const actual = await importOriginal(); return { @@ -41,15 +50,13 @@ const mockedLoadSettings = loadSettings as vi.Mock; describe('mcp add command', () => { let parser: yargs.Argv; let mockSetValue: vi.Mock; - let mockConsoleError: vi.Mock; beforeEach(() => { vi.resetAllMocks(); const yargsInstance = yargs([]).command(addCommand); parser = yargsInstance; mockSetValue = vi.fn(); - mockConsoleError = vi.fn(); - vi.spyOn(console, 'error').mockImplementation(mockConsoleError); + mockWriteStderrLine.mockClear(); mockedLoadSettings.mockReturnValue({ forScope: () => ({ settings: {} }), setValue: mockSetValue, @@ -218,7 +225,7 @@ describe('mcp add command', () => { parser.parseAsync(`add ${serverName} ${command}`), ).rejects.toThrow('process.exit called'); - expect(mockConsoleError).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( 'Error: Please use --scope user to edit settings in the home directory.', ); expect(mockProcessExit).toHaveBeenCalledWith(1); @@ -236,7 +243,7 @@ describe('mcp add command', () => { parser.parseAsync(`add --scope project ${serverName} ${command}`), ).rejects.toThrow('process.exit called'); - expect(mockConsoleError).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( 'Error: Please use --scope user to edit settings in the home directory.', ); expect(mockProcessExit).toHaveBeenCalledWith(1); @@ -250,7 +257,7 @@ describe('mcp add command', () => { 'mcpServers', expect.any(Object), ); - expect(mockConsoleError).not.toHaveBeenCalled(); + expect(mockWriteStderrLine).not.toHaveBeenCalled(); }); }); diff --git a/packages/cli/src/commands/mcp/list.test.ts b/packages/cli/src/commands/mcp/list.test.ts index 438dcad59..ec7d184dc 100644 --- a/packages/cli/src/commands/mcp/list.test.ts +++ b/packages/cli/src/commands/mcp/list.test.ts @@ -4,13 +4,22 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { vi, describe, it, expect, beforeEach } from 'vitest'; import { listMcpServers } from './list.js'; import { loadSettings } from '../../config/settings.js'; import { isWorkspaceTrusted } from '../../config/trustedFolders.js'; import { createTransport, ExtensionManager } from '@qwen-code/qwen-code-core'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +const mockWriteStdoutLine = vi.hoisted(() => vi.fn()); +const mockWriteStderrLine = vi.hoisted(() => vi.fn()); + +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: mockWriteStdoutLine, + writeStderrLine: mockWriteStderrLine, + clearScreen: vi.fn(), +})); + vi.mock('../../config/settings.js', () => ({ loadSettings: vi.fn(), })); @@ -46,7 +55,6 @@ interface MockTransport { } describe('mcp list command', () => { - let consoleSpy: vi.SpyInstance; let mockClient: MockClient; let mockTransport: MockTransport; let mockExtensionManager: { @@ -56,8 +64,7 @@ describe('mcp list command', () => { beforeEach(() => { vi.resetAllMocks(); - - consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + mockWriteStdoutLine.mockClear(); mockTransport = { close: vi.fn() }; mockClient = { @@ -77,16 +84,14 @@ describe('mcp list command', () => { mockedIsWorkspaceTrusted.mockReturnValue(true); }); - afterEach(() => { - consoleSpy.mockRestore(); - }); - it('should display message when no servers configured', async () => { mockedLoadSettings.mockReturnValue({ merged: { mcpServers: {} } }); await listMcpServers(); - expect(consoleSpy).toHaveBeenCalledWith('No MCP servers configured.'); + expect(mockWriteStdoutLine).toHaveBeenCalledWith( + 'No MCP servers configured.', + ); }); it('should display different server types with connected status', async () => { @@ -105,18 +110,20 @@ describe('mcp list command', () => { await listMcpServers(); - expect(consoleSpy).toHaveBeenCalledWith('Configured MCP servers:\n'); - expect(consoleSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( + 'Configured MCP servers:\n', + ); + expect(mockWriteStdoutLine).toHaveBeenCalledWith( expect.stringContaining( 'stdio-server: /path/to/server arg1 (stdio) - Connected', ), ); - expect(consoleSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( expect.stringContaining( 'sse-server: https://example.com/sse (sse) - Connected', ), ); - expect(consoleSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( expect.stringContaining( 'http-server: https://example.com/http (http) - Connected', ), @@ -136,7 +143,7 @@ describe('mcp list command', () => { await listMcpServers(); - expect(consoleSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( expect.stringContaining( 'test-server: /test/server (stdio) - Disconnected', ), @@ -165,12 +172,12 @@ describe('mcp list command', () => { await listMcpServers(); - expect(consoleSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( expect.stringContaining( 'config-server: /config/server (stdio) - Connected', ), ); - expect(consoleSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( expect.stringContaining( 'extension-server: /ext/server (stdio) - Connected', ), diff --git a/packages/cli/src/commands/mcp/remove.test.ts b/packages/cli/src/commands/mcp/remove.test.ts index 0f3847e40..4bae8a6ed 100644 --- a/packages/cli/src/commands/mcp/remove.test.ts +++ b/packages/cli/src/commands/mcp/remove.test.ts @@ -9,6 +9,15 @@ import yargs from 'yargs'; import { loadSettings, SettingScope } from '../../config/settings.js'; import { removeCommand } from './remove.js'; +const mockWriteStdoutLine = vi.hoisted(() => vi.fn()); +const mockWriteStderrLine = vi.hoisted(() => vi.fn()); + +vi.mock('../../utils/stdioHelpers.js', () => ({ + writeStdoutLine: mockWriteStdoutLine, + writeStderrLine: mockWriteStderrLine, + clearScreen: vi.fn(), +})); + vi.mock('fs/promises', async (importOriginal) => { const actual = await importOriginal(); return { @@ -49,6 +58,7 @@ describe('mcp remove command', () => { forScope: () => ({ settings: mockSettings }), setValue: mockSetValue, }); + mockWriteStdoutLine.mockClear(); }); it('should remove a server from project settings', async () => { @@ -62,11 +72,10 @@ describe('mcp remove command', () => { }); it('should show a message if server not found', async () => { - const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); await parser.parseAsync('remove non-existent-server'); expect(mockSetValue).not.toHaveBeenCalled(); - expect(consoleSpy).toHaveBeenCalledWith( + expect(mockWriteStdoutLine).toHaveBeenCalledWith( 'Server "non-existent-server" not found in project settings.', ); }); diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index 67d3b114b..ce388fcb7 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -20,6 +20,15 @@ import type { Settings } from './settings.js'; import * as ServerConfig from '@qwen-code/qwen-code-core'; import { isWorkspaceTrusted } from './trustedFolders.js'; +const mockWriteStderrLine = vi.hoisted(() => vi.fn()); +const mockWriteStdoutLine = vi.hoisted(() => vi.fn()); + +vi.mock('../utils/stdioHelpers.js', () => ({ + writeStderrLine: mockWriteStderrLine, + writeStdoutLine: mockWriteStdoutLine, + clearScreen: vi.fn(), +})); + const createNativeLspServiceInstance = () => ({ discoverAndPrepare: vi.fn(), start: vi.fn(), @@ -158,23 +167,19 @@ describe('parseArguments', () => { const mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { throw new Error('process.exit called'); }); - - const mockConsoleError = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); + mockWriteStderrLine.mockClear(); await expect(parseArguments({} as Settings)).rejects.toThrow( 'process.exit called', ); - expect(mockConsoleError).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( expect.stringContaining( 'Cannot use both --prompt (-p) and --prompt-interactive (-i) together', ), ); mockExit.mockRestore(); - mockConsoleError.mockRestore(); }); it('should throw an error when using short flags -p and -i together', async () => { @@ -190,23 +195,19 @@ describe('parseArguments', () => { const mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { throw new Error('process.exit called'); }); - - const mockConsoleError = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); + mockWriteStderrLine.mockClear(); await expect(parseArguments({} as Settings)).rejects.toThrow( 'process.exit called', ); - expect(mockConsoleError).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( expect.stringContaining( 'Cannot use both --prompt (-p) and --prompt-interactive (-i) together', ), ); mockExit.mockRestore(); - mockConsoleError.mockRestore(); }); it('should allow --prompt without --prompt-interactive', async () => { @@ -389,23 +390,19 @@ describe('parseArguments', () => { const mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { throw new Error('process.exit called'); }); - - const mockConsoleError = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); + mockWriteStderrLine.mockClear(); await expect(parseArguments({} as Settings)).rejects.toThrow( 'process.exit called', ); - expect(mockConsoleError).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( expect.stringContaining( 'Cannot use both --yolo (-y) and --approval-mode together. Use --approval-mode=yolo instead.', ), ); mockExit.mockRestore(); - mockConsoleError.mockRestore(); }); it('should throw an error when using short flags -y and --approval-mode together', async () => { @@ -414,23 +411,19 @@ describe('parseArguments', () => { const mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { throw new Error('process.exit called'); }); - - const mockConsoleError = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); + mockWriteStderrLine.mockClear(); await expect(parseArguments({} as Settings)).rejects.toThrow( 'process.exit called', ); - expect(mockConsoleError).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( expect.stringContaining( 'Cannot use both --yolo (-y) and --approval-mode together. Use --approval-mode=yolo instead.', ), ); mockExit.mockRestore(); - mockConsoleError.mockRestore(); }); it('should throw an error when include-partial-messages is used without stream-json output', async () => { @@ -439,23 +432,19 @@ describe('parseArguments', () => { const mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { throw new Error('process.exit called'); }); - - const mockConsoleError = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); + mockWriteStderrLine.mockClear(); await expect(parseArguments({} as Settings)).rejects.toThrow( 'process.exit called', ); - expect(mockConsoleError).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( expect.stringContaining( '--include-partial-messages requires --output-format stream-json', ), ); mockExit.mockRestore(); - mockConsoleError.mockRestore(); }); it('should parse stream-json formats and include-partial-messages flag', async () => { @@ -496,21 +485,17 @@ describe('parseArguments', () => { const mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { throw new Error('process.exit called'); }); - - const mockConsoleError = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); + mockWriteStderrLine.mockClear(); await expect(parseArguments({} as Settings)).rejects.toThrow( 'process.exit called', ); - expect(mockConsoleError).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( expect.stringContaining('Invalid values:'), ); mockExit.mockRestore(); - mockConsoleError.mockRestore(); }); it('should support comma-separated values for --allowed-tools', async () => { @@ -900,21 +885,17 @@ describe('loadCliConfig telemetry', () => { const mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { throw new Error('process.exit called'); }); - - const mockConsoleError = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); + mockWriteStderrLine.mockClear(); await expect(parseArguments({} as Settings)).rejects.toThrow( 'process.exit called', ); - expect(mockConsoleError).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( expect.stringContaining('Invalid values:'), ); mockExit.mockRestore(); - mockConsoleError.mockRestore(); }); }); @@ -2138,17 +2119,14 @@ describe('Output format', () => { const mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { throw new Error('process.exit called'); }); - const mockConsoleError = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); + mockWriteStderrLine.mockClear(); await expect(parseArguments({} as Settings)).rejects.toThrow( 'process.exit called', ); - expect(mockConsoleError).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( expect.stringContaining('Invalid values:'), ); mockExit.mockRestore(); - mockConsoleError.mockRestore(); }); }); @@ -2172,23 +2150,19 @@ describe('parseArguments with positional prompt', () => { const mockExit = vi.spyOn(process, 'exit').mockImplementation(() => { throw new Error('process.exit called'); }); - - const mockConsoleError = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); + mockWriteStderrLine.mockClear(); await expect(parseArguments({} as Settings)).rejects.toThrow( 'process.exit called', ); - expect(mockConsoleError).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( expect.stringContaining( 'Cannot use both a positional prompt and the --prompt (-p) flag together', ), ); mockExit.mockRestore(); - mockConsoleError.mockRestore(); }); it('should correctly parse a positional prompt to query field', async () => { diff --git a/packages/cli/src/config/keyBindings.ts b/packages/cli/src/config/keyBindings.ts index dc53448d4..8737866ea 100644 --- a/packages/cli/src/config/keyBindings.ts +++ b/packages/cli/src/config/keyBindings.ts @@ -45,7 +45,6 @@ export enum Command { PASTE_CLIPBOARD_IMAGE = 'pasteClipboardImage', // App level bindings - SHOW_ERROR_DETAILS = 'showErrorDetails', TOGGLE_TOOL_DESCRIPTIONS = 'toggleToolDescriptions', TOGGLE_IDE_CONTEXT_DETAIL = 'toggleIDEContextDetail', QUIT = 'quit', @@ -156,7 +155,6 @@ export const defaultKeyBindings: KeyBindingConfig = { [Command.PASTE_CLIPBOARD_IMAGE]: [{ key: 'v', ctrl: true }], // App level bindings - [Command.SHOW_ERROR_DETAILS]: [{ key: 'o', ctrl: true }], [Command.TOGGLE_TOOL_DESCRIPTIONS]: [{ key: 't', ctrl: true }], [Command.TOGGLE_IDE_CONTEXT_DETAIL]: [{ key: 'g', ctrl: true }], [Command.QUIT]: [{ key: 'c', ctrl: true }], diff --git a/packages/cli/src/gemini.test.tsx b/packages/cli/src/gemini.test.tsx index 25db908c4..52cfce551 100644 --- a/packages/cli/src/gemini.test.tsx +++ b/packages/cli/src/gemini.test.tsx @@ -24,6 +24,8 @@ import { appEvents, AppEvent } from './utils/events.js'; import type { Config } from '@qwen-code/qwen-code-core'; import { OutputFormat } from '@qwen-code/qwen-code-core'; +const mockWriteStderrLine = vi.hoisted(() => vi.fn()); + // Custom error to identify mock process.exit calls class MockProcessExitError extends Error { constructor(readonly code?: string | number | null | undefined) { @@ -79,6 +81,12 @@ vi.mock('./utils/sandbox.js', () => ({ start_sandbox: vi.fn(() => Promise.resolve()), // Mock as an async function that resolves })); +vi.mock('./utils/stdioHelpers.js', () => ({ + writeStderrLine: mockWriteStderrLine, + writeStdoutLine: vi.fn(), + clearScreen: vi.fn(), +})); + vi.mock('./utils/relaunch.js', () => ({ relaunchAppInChildProcess: vi.fn(), })); @@ -501,34 +509,28 @@ describe('gemini.tsx main function kitty protocol', () => { }); describe('validateDnsResolutionOrder', () => { - let consoleWarnSpy: ReturnType; - beforeEach(() => { - consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); - }); - - afterEach(() => { - consoleWarnSpy.mockRestore(); + mockWriteStderrLine.mockClear(); }); it('should return "ipv4first" when the input is "ipv4first"', () => { expect(validateDnsResolutionOrder('ipv4first')).toBe('ipv4first'); - expect(consoleWarnSpy).not.toHaveBeenCalled(); + expect(mockWriteStderrLine).not.toHaveBeenCalled(); }); it('should return "verbatim" when the input is "verbatim"', () => { expect(validateDnsResolutionOrder('verbatim')).toBe('verbatim'); - expect(consoleWarnSpy).not.toHaveBeenCalled(); + expect(mockWriteStderrLine).not.toHaveBeenCalled(); }); it('should return the default "ipv4first" when the input is undefined', () => { expect(validateDnsResolutionOrder(undefined)).toBe('ipv4first'); - expect(consoleWarnSpy).not.toHaveBeenCalled(); + expect(mockWriteStderrLine).not.toHaveBeenCalled(); }); it('should return the default "ipv4first" and log a warning for an invalid string', () => { expect(validateDnsResolutionOrder('invalid-value')).toBe('ipv4first'); - expect(consoleWarnSpy).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( 'Invalid value for dnsResolutionOrder in settings: "invalid-value". Using default "ipv4first".', ); }); diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index d13c46536..dbe49f566 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -37,7 +37,6 @@ import { SettingsContext } from './ui/contexts/SettingsContext.js'; import { VimModeProvider } from './ui/contexts/VimModeContext.js'; import { useKittyKeyboardProtocol } from './ui/hooks/useKittyKeyboardProtocol.js'; import { themeManager } from './ui/themes/theme-manager.js'; -import { ConsolePatcher } from './ui/utils/ConsolePatcher.js'; import { detectAndEnableKittyProtocol } from './ui/utils/kittyProtocolDetector.js'; import { checkForUpdates } from './ui/utils/updateCheck.js'; import { @@ -359,15 +358,6 @@ export async function main() { // process.exit(0); // } - // Setup unified ConsolePatcher based on interactive mode - const isInteractive = config.isInteractive(); - const consolePatcher = new ConsolePatcher({ - stderr: isInteractive, - debugMode: isDebugMode, - }); - consolePatcher.patch(); - registerCleanup(consolePatcher.cleanup); - const wasRaw = process.stdin.isRaw; let kittyProtocolDetectionComplete: Promise | undefined; if (config.isInteractive() && !wasRaw && process.stdin.isTTY) { diff --git a/packages/cli/src/nonInteractive/control/ControlDispatcher.test.ts b/packages/cli/src/nonInteractive/control/ControlDispatcher.test.ts index b775b0a5e..ac42f3725 100644 --- a/packages/cli/src/nonInteractive/control/ControlDispatcher.test.ts +++ b/packages/cli/src/nonInteractive/control/ControlDispatcher.test.ts @@ -457,9 +457,6 @@ describe('ControlDispatcher', () => { it('should handle response for non-existent request in debug mode', () => { const context = createMockContext(true); - const consoleSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); const dispatcherWithDebug = new ControlDispatcher(context); const response: CLIControlResponse = { @@ -471,15 +468,10 @@ describe('ControlDispatcher', () => { }, }; - dispatcherWithDebug.handleControlResponse(response); - - expect(consoleSpy).toHaveBeenCalledWith( - expect.stringContaining( - '[ControlDispatcher] No pending outgoing request for: non-existent', - ), - ); - - consoleSpy.mockRestore(); + // Should not throw in debug mode + expect(() => + dispatcherWithDebug.handleControlResponse(response), + ).not.toThrow(); }); }); @@ -599,11 +591,8 @@ describe('ControlDispatcher', () => { expect(() => dispatcher.handleCancel('non-existent')).not.toThrow(); }); - it('should log cancellation in debug mode', () => { + it('should cancel request in debug mode without throwing', () => { const context = createMockContext(true); - const consoleSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); const dispatcherWithDebug = new ControlDispatcher(context); const requestId = 'cancel-req-debug'; @@ -626,15 +615,7 @@ describe('ControlDispatcher', () => { timeoutId, ); - dispatcherWithDebug.handleCancel(requestId); - - expect(consoleSpy).toHaveBeenCalledWith( - expect.stringContaining( - '[ControlDispatcher] Cancelled incoming request: cancel-req-debug', - ), - ); - - consoleSpy.mockRestore(); + expect(() => dispatcherWithDebug.handleCancel(requestId)).not.toThrow(); }); }); @@ -720,11 +701,8 @@ describe('ControlDispatcher', () => { expect(secondRejectCount).toBe(firstRejectCount); }); - it('should log input closure in debug mode', () => { + it('should mark input closed in debug mode without throwing', () => { const context = createMockContext(true); - const consoleSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); const dispatcherWithDebug = new ControlDispatcher(context); const requestId = 'reject-req-debug'; @@ -750,15 +728,7 @@ describe('ControlDispatcher', () => { timeoutId, ); - dispatcherWithDebug.markInputClosed(); - - expect(consoleSpy).toHaveBeenCalledWith( - expect.stringContaining( - '[ControlDispatcher] Input closed, rejecting 1 pending outgoing requests', - ), - ); - - consoleSpy.mockRestore(); + expect(() => dispatcherWithDebug.markInputClosed()).not.toThrow(); }); }); @@ -848,21 +818,12 @@ describe('ControlDispatcher', () => { expect(mockSystemController.cleanup).toHaveBeenCalled(); }); - it('should log shutdown in debug mode', () => { + it('should shutdown in debug mode without throwing', () => { const context = createMockContext(true); - const consoleSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); const dispatcherWithDebug = new ControlDispatcher(context); - dispatcherWithDebug.shutdown(); - - expect(consoleSpy).toHaveBeenCalledWith( - '[ControlDispatcher] Shutting down', - ); - - consoleSpy.mockRestore(); + expect(() => dispatcherWithDebug.shutdown()).not.toThrow(); }); }); diff --git a/packages/cli/src/nonInteractive/control/ControlDispatcher.ts b/packages/cli/src/nonInteractive/control/ControlDispatcher.ts index 01e3b4412..f2fb267c7 100644 --- a/packages/cli/src/nonInteractive/control/ControlDispatcher.ts +++ b/packages/cli/src/nonInteractive/control/ControlDispatcher.ts @@ -219,7 +219,7 @@ export class ControlDispatcher implements IPendingRequestRegistry { const requestIds = Array.from(this.pendingOutgoingRequests.keys()); if (this.context.debugMode) { - console.error( + debugLogger.debug( `[ControlDispatcher] Input closed, rejecting ${requestIds.length} pending outgoing requests`, ); } diff --git a/packages/cli/src/nonInteractive/session.test.ts b/packages/cli/src/nonInteractive/session.test.ts index 56fd7b3e0..e163f516a 100644 --- a/packages/cli/src/nonInteractive/session.test.ts +++ b/packages/cli/src/nonInteractive/session.test.ts @@ -18,7 +18,6 @@ import { StreamJsonOutputAdapter } from './io/StreamJsonOutputAdapter.js'; import { ControlDispatcher } from './control/ControlDispatcher.js'; import { ControlContext } from './control/ControlContext.js'; import { ControlService } from './control/ControlService.js'; -import { ConsolePatcher } from '../ui/utils/ConsolePatcher.js'; const runNonInteractiveMock = vi.fn(); @@ -47,10 +46,6 @@ vi.mock('./control/ControlService.js', () => ({ ControlService: vi.fn(), })); -vi.mock('../ui/utils/ConsolePatcher.js', () => ({ - ConsolePatcher: vi.fn(), -})); - interface ConfigOverrides { getSessionId?: () => string; getModel?: () => string; @@ -160,24 +155,11 @@ describe('runNonInteractiveStreamJson', () => { createSendSdkMcpMessage: ReturnType; }; }; - let mockConsolePatcher: { - patch: ReturnType; - cleanup: ReturnType; - }; - beforeEach(() => { config = createConfig(); runNonInteractiveMock.mockReset(); // Setup mocks - mockConsolePatcher = { - patch: vi.fn(), - cleanup: vi.fn(), - }; - (ConsolePatcher as unknown as ReturnType).mockImplementation( - () => mockConsolePatcher, - ); - mockOutputAdapter = { emitResult: vi.fn(), } as { @@ -236,9 +218,7 @@ describe('runNonInteractiveStreamJson', () => { await runNonInteractiveStreamJson(config, ''); - expect(mockConsolePatcher.patch).toHaveBeenCalledTimes(1); expect(mockDispatcher.dispatch).toHaveBeenCalledWith(initRequest); - expect(mockConsolePatcher.cleanup).toHaveBeenCalledTimes(1); }); it('processes user message when received as first message', async () => { @@ -489,8 +469,6 @@ describe('runNonInteractiveStreamJson', () => { await expect(runNonInteractiveStreamJson(config, '')).rejects.toThrow( 'Stream error', ); - - expect(mockConsolePatcher.cleanup).toHaveBeenCalled(); }); it('stops processing when abort signal is triggered', async () => { @@ -567,9 +545,6 @@ describe('runNonInteractiveStreamJson', () => { }; await runNonInteractiveStreamJson(config, ''); - - expect(mockConsolePatcher.patch).toHaveBeenCalledTimes(1); - expect(mockConsolePatcher.cleanup).toHaveBeenCalledTimes(1); }); it('cleans up output adapter on completion', async () => { @@ -598,7 +573,5 @@ describe('runNonInteractiveStreamJson', () => { }; await runNonInteractiveStreamJson(config, ''); - - expect(mockConsolePatcher.cleanup).toHaveBeenCalled(); }); }); diff --git a/packages/cli/src/nonInteractive/session.ts b/packages/cli/src/nonInteractive/session.ts index 42312fb29..ae04eb642 100644 --- a/packages/cli/src/nonInteractive/session.ts +++ b/packages/cli/src/nonInteractive/session.ts @@ -33,7 +33,6 @@ import { } from './types.js'; import { createMinimalSettings } from '../config/settings.js'; import { runNonInteractive } from '../nonInteractiveCli.js'; -import { ConsolePatcher } from '../ui/utils/ConsolePatcher.js'; const debugLogger = createDebugLogger('NON_INTERACTIVE_SESSION'); @@ -607,29 +606,20 @@ export async function runNonInteractiveStreamJson( config: Config, input: string, ): Promise { - const consolePatcher = new ConsolePatcher({ - debugMode: config.getDebugMode(), - }); - consolePatcher.patch(); - - try { - let initialPrompt: CLIUserMessage | undefined = undefined; - if (input && input.trim().length > 0) { - const sessionId = config.getSessionId(); - initialPrompt = { - type: 'user', - session_id: sessionId, - message: { - role: 'user', - content: input.trim(), - }, - parent_tool_use_id: null, - }; - } - - const manager = new Session(config, initialPrompt); - await manager.run(); - } finally { - consolePatcher.cleanup(); + let initialPrompt: CLIUserMessage | undefined = undefined; + if (input && input.trim().length > 0) { + const sessionId = config.getSessionId(); + initialPrompt = { + type: 'user', + session_id: sessionId, + message: { + role: 'user', + content: input.trim(), + }, + parent_tool_use_id: null, + }; } + + const manager = new Session(config, initialPrompt); + await manager.run(); } diff --git a/packages/cli/src/nonInteractiveCli.test.ts b/packages/cli/src/nonInteractiveCli.test.ts index 34598b70d..2931118fc 100644 --- a/packages/cli/src/nonInteractiveCli.test.ts +++ b/packages/cli/src/nonInteractiveCli.test.ts @@ -66,7 +66,6 @@ describe('runNonInteractive', () => { let mockToolRegistry: ToolRegistry; let mockCoreExecuteToolCall: Mock; let mockShutdownTelemetry: Mock; - let consoleErrorSpy: MockInstance; let processStdoutSpy: MockInstance; let processStderrSpy: MockInstance; let mockGeminiClient: { @@ -83,7 +82,6 @@ describe('runNonInteractive', () => { getCommands: mockGetCommands, }); - consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); processStdoutSpy = vi .spyOn(process.stdout, 'write') .mockImplementation(() => true); @@ -360,9 +358,6 @@ describe('runNonInteractive', () => { .mockReturnValueOnce(createStreamFromEvents([toolCallEvent])) .mockReturnValueOnce(createStreamFromEvents(finalResponse)); - // Enable debug mode so handleToolError logs to console.error - (mockConfig.getDebugMode as Mock).mockReturnValue(true); - await runNonInteractive( mockConfig, mockSettings, @@ -371,9 +366,6 @@ describe('runNonInteractive', () => { ); expect(mockCoreExecuteToolCall).toHaveBeenCalled(); - expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Error executing tool errorTool: Execution failed', - ); expect(mockGeminiClient.sendMessageStream).toHaveBeenCalledTimes(2); expect(mockGeminiClient.sendMessageStream).toHaveBeenNthCalledWith( 2, @@ -443,9 +435,6 @@ describe('runNonInteractive', () => { .mockReturnValueOnce(createStreamFromEvents([toolCallEvent])) .mockReturnValueOnce(createStreamFromEvents(finalResponse)); - // Enable debug mode so handleToolError logs to console.error - (mockConfig.getDebugMode as Mock).mockReturnValue(true); - await runNonInteractive( mockConfig, mockSettings, @@ -454,9 +443,6 @@ describe('runNonInteractive', () => { ); expect(mockCoreExecuteToolCall).toHaveBeenCalled(); - expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Error executing tool nonexistentTool: Tool "nonexistentTool" not found in registry.', - ); expect(mockGeminiClient.sendMessageStream).toHaveBeenCalledTimes(2); expect(processStdoutSpy).toHaveBeenCalledWith( "Sorry, I can't find that tool.", @@ -738,11 +724,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( @@ -760,19 +741,18 @@ describe('runNonInteractive', () => { // Should throw because of mocked process.exit expect(thrownError?.message).toBe('process.exit(1) called'); - expect(consoleErrorJsonSpy).toHaveBeenCalledWith( - JSON.stringify( - { - error: { - type: 'Error', - message: 'Invalid input provided', - code: 1, - }, + const jsonError = JSON.stringify( + { + error: { + type: 'Error', + message: 'Invalid input provided', + code: 1, }, - null, - 2, - ), + }, + null, + 2, ); + expect(processStderrSpy).toHaveBeenCalledWith(`${jsonError}\n`); }); it('should handle API errors in text mode and exit with error code', async () => { @@ -830,11 +810,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( @@ -852,19 +827,18 @@ describe('runNonInteractive', () => { // Should throw because of mocked process.exit with custom exit code expect(thrownError?.message).toBe('process.exit(42) called'); - expect(consoleErrorJsonSpy).toHaveBeenCalledWith( - JSON.stringify( - { - error: { - type: 'FatalInputError', - message: 'Invalid command syntax provided', - code: 42, - }, + const jsonError = JSON.stringify( + { + error: { + type: 'FatalInputError', + message: 'Invalid command syntax provided', + code: 42, }, - null, - 2, - ), + }, + null, + 2, ); + expect(processStderrSpy).toHaveBeenCalledWith(`${jsonError}\n`); }); it('should execute a slash command that returns a prompt', async () => { diff --git a/packages/cli/src/ui/AppContainer.test.tsx b/packages/cli/src/ui/AppContainer.test.tsx index 5e035d2be..1edec79f9 100644 --- a/packages/cli/src/ui/AppContainer.test.tsx +++ b/packages/cli/src/ui/AppContainer.test.tsx @@ -62,7 +62,6 @@ vi.mock('./hooks/useEditorSettings.js'); vi.mock('./hooks/useSettingsCommand.js'); vi.mock('./hooks/useModelCommand.js'); vi.mock('./hooks/slashCommandProcessor.js'); -vi.mock('./hooks/useConsoleMessages.js'); vi.mock('./hooks/useTerminalSize.js', () => ({ useTerminalSize: vi.fn(() => ({ columns: 80, rows: 24 })), })); @@ -85,7 +84,6 @@ vi.mock('./hooks/useLogger.js'); // Mock external utilities vi.mock('../utils/events.js'); vi.mock('../utils/handleAutoUpdate.js'); -vi.mock('./utils/ConsolePatcher.js'); vi.mock('../utils/cleanup.js'); import { useHistory } from './hooks/useHistoryManager.js'; @@ -95,7 +93,6 @@ import { useEditorSettings } from './hooks/useEditorSettings.js'; import { useSettingsCommand } from './hooks/useSettingsCommand.js'; import { useModelCommand } from './hooks/useModelCommand.js'; import { useSlashCommandProcessor } from './hooks/slashCommandProcessor.js'; -import { useConsoleMessages } from './hooks/useConsoleMessages.js'; import { useGeminiStream } from './hooks/useGeminiStream.js'; import { useVim } from './hooks/vim.js'; import { useFolderTrust } from './hooks/useFolderTrust.js'; @@ -125,7 +122,6 @@ describe('AppContainer State Management', () => { const mockedUseSettingsCommand = useSettingsCommand as Mock; const mockedUseModelCommand = useModelCommand as Mock; const mockedUseSlashCommandProcessor = useSlashCommandProcessor as Mock; - const mockedUseConsoleMessages = useConsoleMessages as Mock; const mockedUseGeminiStream = useGeminiStream as Mock; const mockedUseVim = useVim as Mock; const mockedUseFolderTrust = useFolderTrust as Mock; @@ -206,11 +202,6 @@ describe('AppContainer State Management', () => { shellConfirmationRequest: null, confirmationRequest: null, }); - mockedUseConsoleMessages.mockReturnValue({ - consoleMessages: [], - handleNewMessage: vi.fn(), - clearConsoleMessages: vi.fn(), - }); mockedUseGeminiStream.mockReturnValue({ streamingState: 'idle', submitQuery: vi.fn(), diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index e3703eb11..4f360b217 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -56,7 +56,6 @@ import { useApprovalModeCommand } from './hooks/useApprovalModeCommand.js'; import { useResumeCommand } from './hooks/useResumeCommand.js'; import { useSlashCommandProcessor } from './hooks/slashCommandProcessor.js'; import { useVimMode } from './contexts/VimModeContext.js'; -import { useConsoleMessages } from './hooks/useConsoleMessages.js'; import { useTerminalSize } from './hooks/useTerminalSize.js'; import { calculatePromptWidths } from './components/InputPrompt.js'; import { useStdin, useStdout } from 'ink'; @@ -82,10 +81,8 @@ import { type IdeIntegrationNudgeResult } from './IdeIntegrationNudge.js'; import { type CommandMigrationNudgeResult } from './CommandFormatMigrationNudge.js'; import { useCommandMigration } from './hooks/useCommandMigration.js'; import { migrateTomlCommands } from '../services/command-migration-tool.js'; -import { appEvents, AppEvent } from '../utils/events.js'; import { type UpdateObject } from './utils/updateCheck.js'; import { setUpdateHandler } from '../utils/handleAutoUpdate.js'; -import { ConsolePatcher } from './utils/ConsolePatcher.js'; import { registerCleanup, runExitCleanup } from '../utils/cleanup.js'; import { useMessageQueue } from './hooks/useMessageQueue.js'; import { useAutoAcceptIndicator } from './hooks/useAutoAcceptIndicator.js'; @@ -316,21 +313,6 @@ export const AppContainer = (props: AppContainerProps) => { return () => clearInterval(interval); }, [config, currentModel, getCurrentModel]); - const { - consoleMessages, - handleNewMessage, - clearConsoleMessages: clearConsoleMessagesState, - } = useConsoleMessages(); - - useEffect(() => { - const consolePatcher = new ConsolePatcher({ - onNewMessage: handleNewMessage, - debugMode: config.getDebugMode(), - }); - consolePatcher.patch(); - registerCleanup(consolePatcher.cleanup); - }, [handleNewMessage, config]); - // Derive widths for InputPrompt using shared helper const { inputWidth, suggestionsWidth } = useMemo(() => { const { inputWidth, suggestionsWidth } = @@ -772,10 +754,9 @@ export const AppContainer = (props: AppContainerProps) => { const handleClearScreen = useCallback(() => { historyManager.clearItems(); - clearConsoleMessagesState(); clearScreen(); refreshStatic(); - }, [historyManager, clearConsoleMessagesState, refreshStatic]); + }, [historyManager, refreshStatic]); const { handleInput: vimHandleInput } = useVim(buffer, handleFinalSubmit); @@ -903,7 +884,6 @@ export const AppContainer = (props: AppContainerProps) => { setShowMigrationNudge: setShowCommandMigrationNudge, } = useCommandMigration(settings, config.storage); - const [showErrorDetails, setShowErrorDetails] = useState(false); const [showToolDescriptions, setShowToolDescriptions] = useState(false); @@ -954,28 +934,6 @@ export const AppContainer = (props: AppContainerProps) => { return unsubscribe; }, []); - useEffect(() => { - const openDebugConsole = () => { - setShowErrorDetails(true); - setConstrainHeight(false); - }; - appEvents.on(AppEvent.OpenDebugConsole, openDebugConsole); - - const logErrorHandler = (errorMessage: unknown) => { - handleNewMessage({ - type: 'error', - content: String(errorMessage), - count: 1, - }); - }; - appEvents.on(AppEvent.LogError, logErrorHandler); - - return () => { - appEvents.off(AppEvent.OpenDebugConsole, openDebugConsole); - appEvents.off(AppEvent.LogError, logErrorHandler); - }; - }, [handleNewMessage]); - const handleEscapePromptChange = useCallback((showPrompt: boolean) => { setShowEscapePrompt(showPrompt); }, []); @@ -1217,9 +1175,7 @@ export const AppContainer = (props: AppContainerProps) => { setConstrainHeight(true); } - if (keyMatchers[Command.SHOW_ERROR_DETAILS](key)) { - setShowErrorDetails((prev) => !prev); - } else if (keyMatchers[Command.TOGGLE_TOOL_DESCRIPTIONS](key)) { + if (keyMatchers[Command.TOGGLE_TOOL_DESCRIPTIONS](key)) { const newValue = !showToolDescriptions; setShowToolDescriptions(newValue); @@ -1247,7 +1203,6 @@ export const AppContainer = (props: AppContainerProps) => { [ constrainHeight, setConstrainHeight, - setShowErrorDetails, showToolDescriptions, setShowToolDescriptions, config, @@ -1306,22 +1261,6 @@ export const AppContainer = (props: AppContainerProps) => { stdout, ]); - const filteredConsoleMessages = useMemo(() => { - if (config.getDebugMode()) { - return consoleMessages; - } - return consoleMessages.filter((msg) => msg.type !== 'debug'); - }, [consoleMessages, config]); - - // Computed values - const errorCount = useMemo( - () => - filteredConsoleMessages - .filter((msg) => msg.type === 'error') - .reduce((total, msg) => total + msg.count, 0), - [filteredConsoleMessages], - ); - const nightly = props.version.includes('nightly'); const dialogsVisible = @@ -1416,8 +1355,6 @@ export const AppContainer = (props: AppContainerProps) => { isFolderTrustDialogOpen: isFolderTrustDialogOpen ?? false, isTrustedFolder, constrainHeight, - showErrorDetails, - filteredConsoleMessages, ideContextState, showToolDescriptions, ctrlCPressedOnce, @@ -1431,7 +1368,6 @@ export const AppContainer = (props: AppContainerProps) => { showAutoAcceptIndicator, currentModel, contextFileNames, - errorCount, availableTerminalHeight, mainAreaWidth, staticAreaMaxItemHeight, @@ -1509,8 +1445,6 @@ export const AppContainer = (props: AppContainerProps) => { isFolderTrustDialogOpen, isTrustedFolder, constrainHeight, - showErrorDetails, - filteredConsoleMessages, ideContextState, showToolDescriptions, ctrlCPressedOnce, @@ -1523,7 +1457,6 @@ export const AppContainer = (props: AppContainerProps) => { messageQueue, showAutoAcceptIndicator, contextFileNames, - errorCount, availableTerminalHeight, mainAreaWidth, staticAreaMaxItemHeight, diff --git a/packages/cli/src/ui/commands/copyCommand.test.ts b/packages/cli/src/ui/commands/copyCommand.test.ts index de7509040..b07531967 100644 --- a/packages/cli/src/ui/commands/copyCommand.test.ts +++ b/packages/cli/src/ui/commands/copyCommand.test.ts @@ -34,6 +34,12 @@ describe('copyCommand', () => { getGeminiClient: () => ({ getChat: mockGetChat, }), + getDebugLogger: () => ({ + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + }), }, }, }); diff --git a/packages/cli/src/ui/components/Composer.test.tsx b/packages/cli/src/ui/components/Composer.test.tsx index a12855e4c..1db02d6f9 100644 --- a/packages/cli/src/ui/components/Composer.test.tsx +++ b/packages/cli/src/ui/components/Composer.test.tsx @@ -43,10 +43,6 @@ vi.mock('./ShellModeIndicator.js', () => ({ ShellModeIndicator: () => ShellModeIndicator, })); -vi.mock('./DetailedMessagesDisplay.js', () => ({ - DetailedMessagesDisplay: () => DetailedMessagesDisplay, -})); - vi.mock('./InputPrompt.js', () => ({ InputPrompt: () => InputPrompt, calculatePromptWidths: vi.fn(() => ({ @@ -60,10 +56,6 @@ vi.mock('./Footer.js', () => ({ Footer: () => Footer, })); -vi.mock('./ShowMoreLines.js', () => ({ - ShowMoreLines: () => ShowMoreLines, -})); - vi.mock('./QueuedMessageDisplay.js', () => ({ QueuedMessageDisplay: ({ messageQueue }: { messageQueue: string[] }) => { if (messageQueue.length === 0) { @@ -91,7 +83,6 @@ const createMockUIState = (overrides: Partial = {}): UIState => contextFileNames: [], showAutoAcceptIndicator: ApprovalMode.DEFAULT, messageQueue: [], - showErrorDetails: false, constrainHeight: false, isInputActive: true, buffer: '', @@ -111,7 +102,6 @@ const createMockUIState = (overrides: Partial = {}): UIState => ideContextState: null, geminiMdFileCount: 0, showToolDescriptions: false, - filteredConsoleMessages: [], sessionStats: { lastPromptTokenCount: 0, sessionTokenCount: 0, @@ -119,7 +109,6 @@ const createMockUIState = (overrides: Partial = {}): UIState => }, branchName: 'main', debugMessage: '', - errorCount: 0, nightly: false, isTrustedFolder: true, ...overrides, @@ -354,31 +343,4 @@ describe('Composer', () => { expect(lastFrame()).toContain('Footer'); }); }); - - describe('Error Details Display', () => { - it('shows DetailedMessagesDisplay when showErrorDetails is true', () => { - const uiState = createMockUIState({ - showErrorDetails: true, - filteredConsoleMessages: [ - { level: 'error', message: 'Test error', timestamp: new Date() }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ] as any, - }); - - const { lastFrame } = renderComposer(uiState); - - expect(lastFrame()).toContain('DetailedMessagesDisplay'); - expect(lastFrame()).toContain('ShowMoreLines'); - }); - - it('does not show error details when showErrorDetails is false', () => { - const uiState = createMockUIState({ - showErrorDetails: false, - }); - - const { lastFrame } = renderComposer(uiState); - - expect(lastFrame()).not.toContain('DetailedMessagesDisplay'); - }); - }); }); diff --git a/packages/cli/src/ui/components/Composer.tsx b/packages/cli/src/ui/components/Composer.tsx index 50b04a1d2..a1b4f6ec7 100644 --- a/packages/cli/src/ui/components/Composer.tsx +++ b/packages/cli/src/ui/components/Composer.tsx @@ -5,15 +5,12 @@ */ import { Box, useIsScreenReaderEnabled } from 'ink'; -import { useCallback, useMemo, useState } from 'react'; +import { useCallback, useState } from 'react'; import { LoadingIndicator } from './LoadingIndicator.js'; -import { DetailedMessagesDisplay } from './DetailedMessagesDisplay.js'; -import { InputPrompt, calculatePromptWidths } from './InputPrompt.js'; +import { InputPrompt } from './InputPrompt.js'; import { Footer } from './Footer.js'; -import { ShowMoreLines } from './ShowMoreLines.js'; import { QueuedMessageDisplay } from './QueuedMessageDisplay.js'; import { KeyboardShortcuts } from './KeyboardShortcuts.js'; -import { OverflowProvider } from '../contexts/OverflowContext.js'; import { useUIState } from '../contexts/UIStateContext.js'; import { useUIActions } from '../contexts/UIActionsContext.js'; import { useVimMode } from '../contexts/VimModeContext.js'; @@ -29,8 +26,6 @@ export const Composer = () => { const uiState = useUIState(); const uiActions = useUIActions(); const { vimEnabled } = useVimMode(); - const terminalWidth = process.stdout.columns; - const debugConsoleMaxHeight = Math.floor(Math.max(terminalWidth * 0.2, 5)); const { showAutoAcceptIndicator } = uiState; @@ -46,12 +41,6 @@ export const Composer = () => { setShowSuggestions(visible); }, []); - // Use the container width of InputPrompt for width of DetailedMessagesDisplay - const { containerWidth } = useMemo( - () => calculatePromptWidths(uiState.terminalWidth), - [uiState.terminalWidth], - ); - return ( {!uiState.embeddedShellFocused && ( @@ -75,21 +64,6 @@ export const Composer = () => { - {uiState.showErrorDetails && ( - - - - - - - )} - {uiState.isFeedbackDialogOpen && } {uiState.isInputActive && ( diff --git a/packages/cli/src/ui/components/ConsoleSummaryDisplay.tsx b/packages/cli/src/ui/components/ConsoleSummaryDisplay.tsx deleted file mode 100644 index 2f2f8a2a7..000000000 --- a/packages/cli/src/ui/components/ConsoleSummaryDisplay.tsx +++ /dev/null @@ -1,35 +0,0 @@ -/** - * @license - * Copyright 2025 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import type React from 'react'; -import { Box, Text } from 'ink'; -import { theme } from '../semantic-colors.js'; - -interface ConsoleSummaryDisplayProps { - errorCount: number; - // logCount is not currently in the plan to be displayed in summary -} - -export const ConsoleSummaryDisplay: React.FC = ({ - errorCount, -}) => { - if (errorCount === 0) { - return null; - } - - const errorIcon = '\u2716'; // Heavy multiplication x (✖) - - return ( - - {errorCount > 0 && ( - - {errorIcon} {errorCount} error{errorCount > 1 ? 's' : ''}{' '} - (ctrl+o for details) - - )} - - ); -}; diff --git a/packages/cli/src/ui/components/DetailedMessagesDisplay.tsx b/packages/cli/src/ui/components/DetailedMessagesDisplay.tsx deleted file mode 100644 index b31d08800..000000000 --- a/packages/cli/src/ui/components/DetailedMessagesDisplay.tsx +++ /dev/null @@ -1,83 +0,0 @@ -/** - * @license - * Copyright 2025 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import type React from 'react'; -import { Box, Text } from 'ink'; -import { theme } from '../semantic-colors.js'; -import type { ConsoleMessageItem } from '../types.js'; -import { MaxSizedBox } from './shared/MaxSizedBox.js'; - -interface DetailedMessagesDisplayProps { - messages: ConsoleMessageItem[]; - maxHeight: number | undefined; - width: number; - // debugMode is not needed here if App.tsx filters debug messages before passing them. - // If DetailedMessagesDisplay should handle filtering, add debugMode prop. -} - -export const DetailedMessagesDisplay: React.FC< - DetailedMessagesDisplayProps -> = ({ messages, maxHeight, width }) => { - if (messages.length === 0) { - return null; // Don't render anything if there are no messages - } - - const borderAndPadding = 4; - return ( - - - - Debug Console{' '} - (ctrl+o to close) - - - - {messages.map((msg, index) => { - let textColor = theme.text.primary; - let icon = '\u2139'; // Information source (ℹ) - - switch (msg.type) { - case 'warn': - textColor = theme.status.warning; - icon = '\u26A0'; // Warning sign (⚠) - break; - case 'error': - textColor = theme.status.error; - icon = '\u2716'; // Heavy multiplication x (✖) - break; - case 'debug': - textColor = theme.text.secondary; // Or theme.text.secondary - icon = '\u{1F50D}'; // Left-pointing magnifying glass (🔍) - break; - case 'log': - default: - // Default textColor and icon are already set - break; - } - - return ( - - {icon} - - {msg.content} - {msg.count && msg.count > 1 && ( - (x{msg.count}) - )} - - - ); - })} - - - ); -}; diff --git a/packages/cli/src/ui/components/Footer.tsx b/packages/cli/src/ui/components/Footer.tsx index b55923a84..af81f6a5d 100644 --- a/packages/cli/src/ui/components/Footer.tsx +++ b/packages/cli/src/ui/components/Footer.tsx @@ -7,7 +7,6 @@ import type React from 'react'; import { Box, Text } from 'ink'; import { theme } from '../semantic-colors.js'; -import { ConsoleSummaryDisplay } from './ConsoleSummaryDisplay.js'; import { ContextUsageDisplay } from './ContextUsageDisplay.js'; import { useTerminalSize } from '../hooks/useTerminalSize.js'; import { AutoAcceptIndicator } from './AutoAcceptIndicator.js'; @@ -25,20 +24,11 @@ export const Footer: React.FC = () => { const config = useConfig(); const { vimEnabled, vimMode } = useVimMode(); - const { - errorCount, - showErrorDetails, - promptTokenCount, - showAutoAcceptIndicator, - } = { - errorCount: uiState.errorCount, - showErrorDetails: uiState.showErrorDetails, + const { promptTokenCount, showAutoAcceptIndicator } = { promptTokenCount: uiState.sessionStats.lastPromptTokenCount, showAutoAcceptIndicator: uiState.showAutoAcceptIndicator, }; - const showErrorIndicator = !showErrorDetails && errorCount > 0; - const { columns: terminalWidth } = useTerminalSize(); const isNarrow = isNarrowWidth(terminalWidth); @@ -103,13 +93,6 @@ export const Footer: React.FC = () => { ), }); } - if (showErrorIndicator) { - rightItems.push({ - key: 'errors', - node: , - }); - } - return ( { expect(frameText).toContain("Press 'r' to restart Gemini"); }); - it('renders a generic message and logs an error for NONE reason', () => { - const consoleErrorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); + it('renders a generic message for NONE reason', () => { const { lastFrame } = renderWithProviders( , ); const frameText = lastFrame(); expect(frameText).toContain('Workspace trust has changed.'); - expect(consoleErrorSpy).toHaveBeenCalledWith( - 'IdeTrustChangeDialog rendered with unexpected reason "NONE"', - ); }); it('calls relaunchApp when "r" is pressed', () => { diff --git a/packages/cli/src/ui/components/InputPrompt.test.tsx b/packages/cli/src/ui/components/InputPrompt.test.tsx index 0ddeee83e..158c15275 100644 --- a/packages/cli/src/ui/components/InputPrompt.test.tsx +++ b/packages/cli/src/ui/components/InputPrompt.test.tsx @@ -475,10 +475,7 @@ describe('InputPrompt', () => { unmount(); }); - it('should handle errors during clipboard operations', async () => { - const consoleErrorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); + it('should handle errors during clipboard operations gracefully', async () => { vi.mocked(clipboardUtils.clipboardHasImage).mockRejectedValue( new Error('Clipboard error'), ); @@ -491,13 +488,9 @@ describe('InputPrompt', () => { stdin.write('\x16'); // Ctrl+V await wait(); - expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Error handling clipboard image:', - expect.any(Error), - ); + // Should not throw and should not set buffer text on error expect(mockBuffer.setText).not.toHaveBeenCalled(); - consoleErrorSpy.mockRestore(); unmount(); }); }); diff --git a/packages/cli/src/ui/components/QwenOAuthProgress.test.tsx b/packages/cli/src/ui/components/QwenOAuthProgress.test.tsx index a93db2b7a..29eb3712a 100644 --- a/packages/cli/src/ui/components/QwenOAuthProgress.test.tsx +++ b/packages/cli/src/ui/components/QwenOAuthProgress.test.tsx @@ -394,10 +394,6 @@ describe('QwenOAuthProgress', () => { it('should handle QR code generation errors gracefully', async () => { const qrcode = await import('qrcode-terminal'); const mockGenerate = vi.mocked(qrcode.default.generate); - const consoleErrorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); - mockGenerate.mockImplementation(() => { throw new Error('QR Code generation failed'); }); @@ -413,12 +409,6 @@ describe('QwenOAuthProgress', () => { // Should not crash and should not show QR code section since QR generation failed const output = lastFrame(); expect(output).not.toContain('Or scan the QR code below:'); - expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Failed to generate QR code:', - expect.any(Error), - ); - - consoleErrorSpy.mockRestore(); }); it('should not generate QR code when deviceAuth is null', async () => { diff --git a/packages/cli/src/ui/contexts/KeypressContext.test.tsx b/packages/cli/src/ui/contexts/KeypressContext.test.tsx index 1130f8352..c28cd9525 100644 --- a/packages/cli/src/ui/contexts/KeypressContext.test.tsx +++ b/packages/cli/src/ui/contexts/KeypressContext.test.tsx @@ -1190,20 +1190,7 @@ describe('KeypressContext - Kitty Protocol', () => { }); describe('debug keystroke logging', () => { - let consoleLogSpy: ReturnType; - let consoleWarnSpy: ReturnType; - - beforeEach(() => { - consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); - consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); - }); - - afterEach(() => { - consoleLogSpy.mockRestore(); - consoleWarnSpy.mockRestore(); - }); - - it('should not log keystrokes when debugKeystrokeLogging is false', async () => { + it('should handle kitty sequences when debugKeystrokeLogging is false', async () => { const keyHandler = vi.fn(); const wrapper = ({ children }: { children: React.ReactNode }) => ( @@ -1221,138 +1208,20 @@ describe('KeypressContext - Kitty Protocol', () => { result.current.subscribe(keyHandler); }); - // Send a kitty sequence + // Send a kitty sequence - should work without debug logging act(() => { stdin.sendKittySequence('\x1b[27u'); }); - expect(keyHandler).toHaveBeenCalled(); - expect(consoleLogSpy).not.toHaveBeenCalledWith( - expect.stringContaining('[DEBUG] Kitty'), - ); - }); - - it('should log kitty buffer accumulation when debugKeystrokeLogging is true', async () => { - const keyHandler = vi.fn(); - - const wrapper = ({ children }: { children: React.ReactNode }) => ( - - {children} - - ); - - const { result } = renderHook(() => useKeypressContext(), { wrapper }); - - act(() => { - result.current.subscribe(keyHandler); - }); - - // Send a complete kitty sequence for escape - act(() => { - stdin.sendKittySequence('\x1b[27u'); - }); - - expect(consoleLogSpy).toHaveBeenCalledWith( - '[DEBUG] Kitty buffer accumulating:', - expect.stringContaining('\x1b[27u'), - ); - const parsedCall = consoleLogSpy.mock.calls.find( - (args) => - typeof args[0] === 'string' && - args[0].includes('[DEBUG] Kitty sequence parsed successfully'), - ); - expect(parsedCall).toBeTruthy(); - expect(parsedCall?.[1]).toEqual(expect.stringContaining('\x1b[27u')); - }); - - it('should log kitty buffer overflow when debugKeystrokeLogging is true', async () => { - const keyHandler = vi.fn(); - - const wrapper = ({ children }: { children: React.ReactNode }) => ( - - {children} - - ); - - const { result } = renderHook(() => useKeypressContext(), { wrapper }); - - act(() => { - result.current.subscribe(keyHandler); - }); - - // Send an invalid long sequence to trigger overflow - const longInvalidSequence = '\x1b[' + 'x'.repeat(100); - act(() => { - stdin.sendKittySequence(longInvalidSequence); - }); - - expect(consoleLogSpy).toHaveBeenCalledWith( - '[DEBUG] Kitty buffer overflow, clearing:', - expect.any(String), - ); - }); - - it('should log kitty buffer clear on Ctrl+C when debugKeystrokeLogging is true', async () => { - const keyHandler = vi.fn(); - - const wrapper = ({ children }: { children: React.ReactNode }) => ( - - {children} - - ); - - const { result } = renderHook(() => useKeypressContext(), { wrapper }); - - act(() => { - result.current.subscribe(keyHandler); - }); - - // Send incomplete kitty sequence - act(() => { - stdin.pressKey({ - name: undefined, - ctrl: false, - meta: false, - shift: false, - sequence: '\x1b[1', - }); - }); - - // Send Ctrl+C - act(() => { - stdin.pressKey({ - name: 'c', - ctrl: true, - meta: false, - shift: false, - sequence: '\x03', - }); - }); - - expect(consoleLogSpy).toHaveBeenCalledWith( - '[DEBUG] Kitty buffer cleared on Ctrl+C:', - '\x1b[1', - ); - - // Verify Ctrl+C was handled expect(keyHandler).toHaveBeenCalledWith( expect.objectContaining({ - name: 'c', - ctrl: true, + name: 'escape', + kittyProtocol: true, }), ); }); - it('should show char codes when debugKeystrokeLogging is true even without debug mode', async () => { + it('should handle kitty sequences when debugKeystrokeLogging is true', async () => { const keyHandler = vi.fn(); const wrapper = ({ children }: { children: React.ReactNode }) => ( @@ -1370,29 +1239,44 @@ describe('KeypressContext - Kitty Protocol', () => { result.current.subscribe(keyHandler); }); - // Send incomplete kitty sequence - const sequence = '\x1b[12'; + // Send a complete kitty sequence for escape - should work with debug logging act(() => { - stdin.pressKey({ - name: undefined, - ctrl: false, - meta: false, - shift: false, - sequence, - }); + stdin.sendKittySequence('\x1b[27u'); }); - // Verify debug logging for accumulation - expect(consoleLogSpy).toHaveBeenCalledWith( - '[DEBUG] Kitty buffer accumulating:', - sequence, + expect(keyHandler).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'escape', + kittyProtocol: true, + }), + ); + }); + + it('should handle kitty buffer overflow without crashing', async () => { + const keyHandler = vi.fn(); + + const wrapper = ({ children }: { children: React.ReactNode }) => ( + + {children} + ); - // Verify warning for char codes - expect(consoleWarnSpy).toHaveBeenCalledWith( - 'Kitty sequence buffer has char codes:', - [27, 91, 49, 50], - ); + const { result } = renderHook(() => useKeypressContext(), { wrapper }); + + act(() => { + result.current.subscribe(keyHandler); + }); + + // Send an invalid long sequence to trigger overflow - should not crash + const longInvalidSequence = '\x1b[' + 'x'.repeat(100); + expect(() => { + act(() => { + stdin.sendKittySequence(longInvalidSequence); + }); + }).not.toThrow(); }); }); diff --git a/packages/cli/src/ui/contexts/UIStateContext.tsx b/packages/cli/src/ui/contexts/UIStateContext.tsx index f62819527..6a48d3eca 100644 --- a/packages/cli/src/ui/contexts/UIStateContext.tsx +++ b/packages/cli/src/ui/contexts/UIStateContext.tsx @@ -8,7 +8,6 @@ import { createContext, useContext } from 'react'; import type { HistoryItem, ThoughtSummary, - ConsoleMessageItem, ShellConfirmationRequest, ConfirmationRequest, LoopDetectionConfirmationRequest, @@ -81,8 +80,6 @@ export interface UIState { isFolderTrustDialogOpen: boolean; isTrustedFolder: boolean | undefined; constrainHeight: boolean; - showErrorDetails: boolean; - filteredConsoleMessages: ConsoleMessageItem[]; ideContextState: IdeContext | undefined; showToolDescriptions: boolean; ctrlCPressedOnce: boolean; @@ -96,7 +93,6 @@ export interface UIState { // Quota-related state currentModel: string; contextFileNames: string[]; - errorCount: number; availableTerminalHeight: number | undefined; mainAreaWidth: number; staticAreaMaxItemHeight: number; diff --git a/packages/cli/src/ui/hooks/useConsoleMessages.test.ts b/packages/cli/src/ui/hooks/useConsoleMessages.test.ts deleted file mode 100644 index b1d1acd66..000000000 --- a/packages/cli/src/ui/hooks/useConsoleMessages.test.ts +++ /dev/null @@ -1,147 +0,0 @@ -/** - * @license - * Copyright 2025 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import { act, renderHook } from '@testing-library/react'; -import { vi } from 'vitest'; -import { useConsoleMessages } from './useConsoleMessages'; -import { useCallback } from 'react'; - -describe('useConsoleMessages', () => { - beforeEach(() => { - vi.useFakeTimers(); - }); - - afterEach(() => { - vi.runOnlyPendingTimers(); - vi.useRealTimers(); - }); - - const useTestableConsoleMessages = () => { - const { handleNewMessage, ...rest } = useConsoleMessages(); - const log = useCallback( - (content: string) => handleNewMessage({ type: 'log', content, count: 1 }), - [handleNewMessage], - ); - const error = useCallback( - (content: string) => - handleNewMessage({ type: 'error', content, count: 1 }), - [handleNewMessage], - ); - return { - ...rest, - log, - error, - clearConsoleMessages: rest.clearConsoleMessages, - }; - }; - - it('should initialize with an empty array of console messages', () => { - const { result } = renderHook(() => useTestableConsoleMessages()); - expect(result.current.consoleMessages).toEqual([]); - }); - - it('should add a new message when log is called', async () => { - const { result } = renderHook(() => useTestableConsoleMessages()); - - act(() => { - result.current.log('Test message'); - }); - - await act(async () => { - await vi.advanceTimersByTimeAsync(20); - }); - - expect(result.current.consoleMessages).toEqual([ - { type: 'log', content: 'Test message', count: 1 }, - ]); - }); - - it('should batch and count identical consecutive messages', async () => { - const { result } = renderHook(() => useTestableConsoleMessages()); - - act(() => { - result.current.log('Test message'); - result.current.log('Test message'); - result.current.log('Test message'); - }); - - await act(async () => { - await vi.advanceTimersByTimeAsync(20); - }); - - expect(result.current.consoleMessages).toEqual([ - { type: 'log', content: 'Test message', count: 3 }, - ]); - }); - - it('should not batch different messages', async () => { - const { result } = renderHook(() => useTestableConsoleMessages()); - - act(() => { - result.current.log('First message'); - result.current.error('Second message'); - }); - - await act(async () => { - await vi.advanceTimersByTimeAsync(20); - }); - - expect(result.current.consoleMessages).toEqual([ - { type: 'log', content: 'First message', count: 1 }, - { type: 'error', content: 'Second message', count: 1 }, - ]); - }); - - it('should clear all messages when clearConsoleMessages is called', async () => { - const { result } = renderHook(() => useTestableConsoleMessages()); - - act(() => { - result.current.log('A message'); - }); - - await act(async () => { - await vi.advanceTimersByTimeAsync(20); - }); - - expect(result.current.consoleMessages).toHaveLength(1); - - act(() => { - result.current.clearConsoleMessages(); - }); - - expect(result.current.consoleMessages).toHaveLength(0); - }); - - it('should clear the pending timeout when clearConsoleMessages is called', () => { - const { result } = renderHook(() => useTestableConsoleMessages()); - const clearTimeoutSpy = vi.spyOn(global, 'clearTimeout'); - - act(() => { - result.current.log('A message'); - }); - - act(() => { - result.current.clearConsoleMessages(); - }); - - expect(clearTimeoutSpy).toHaveBeenCalled(); - clearTimeoutSpy.mockRestore(); - }); - - it('should clean up the timeout on unmount', () => { - const { result, unmount } = renderHook(() => useTestableConsoleMessages()); - const clearTimeoutSpy = vi.spyOn(global, 'clearTimeout'); - - act(() => { - result.current.log('A message'); - }); - - unmount(); - - expect(clearTimeoutSpy).toHaveBeenCalled(); - clearTimeoutSpy.mockRestore(); - }); -}); diff --git a/packages/cli/src/ui/hooks/useConsoleMessages.ts b/packages/cli/src/ui/hooks/useConsoleMessages.ts deleted file mode 100644 index af48fc5d7..000000000 --- a/packages/cli/src/ui/hooks/useConsoleMessages.ts +++ /dev/null @@ -1,110 +0,0 @@ -/** - * @license - * Copyright 2025 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import { - useCallback, - useEffect, - useReducer, - useRef, - useTransition, -} from 'react'; -import type { ConsoleMessageItem } from '../types.js'; - -export interface UseConsoleMessagesReturn { - consoleMessages: ConsoleMessageItem[]; - handleNewMessage: (message: ConsoleMessageItem) => void; - clearConsoleMessages: () => void; -} - -type Action = - | { type: 'ADD_MESSAGES'; payload: ConsoleMessageItem[] } - | { type: 'CLEAR' }; - -function consoleMessagesReducer( - state: ConsoleMessageItem[], - action: Action, -): ConsoleMessageItem[] { - switch (action.type) { - case 'ADD_MESSAGES': { - const newMessages = [...state]; - for (const queuedMessage of action.payload) { - const lastMessage = newMessages[newMessages.length - 1]; - if ( - lastMessage && - lastMessage.type === queuedMessage.type && - lastMessage.content === queuedMessage.content - ) { - // Create a new object for the last message to ensure React detects - // the change, preventing mutation of the existing state object. - newMessages[newMessages.length - 1] = { - ...lastMessage, - count: lastMessage.count + 1, - }; - } else { - newMessages.push({ ...queuedMessage, count: 1 }); - } - } - return newMessages; - } - case 'CLEAR': - return []; - default: - return state; - } -} - -export function useConsoleMessages(): UseConsoleMessagesReturn { - const [consoleMessages, dispatch] = useReducer(consoleMessagesReducer, []); - const messageQueueRef = useRef([]); - const timeoutRef = useRef(null); - const [, startTransition] = useTransition(); - - const processQueue = useCallback(() => { - if (messageQueueRef.current.length > 0) { - const messagesToProcess = messageQueueRef.current; - messageQueueRef.current = []; - startTransition(() => { - dispatch({ type: 'ADD_MESSAGES', payload: messagesToProcess }); - }); - } - timeoutRef.current = null; - }, []); - - const handleNewMessage = useCallback( - (message: ConsoleMessageItem) => { - messageQueueRef.current.push(message); - if (!timeoutRef.current) { - // Batch updates using a timeout. 16ms is a reasonable delay to batch - // rapid-fire messages without noticeable lag. - timeoutRef.current = setTimeout(processQueue, 16); - } - }, - [processQueue], - ); - - const clearConsoleMessages = useCallback(() => { - if (timeoutRef.current) { - clearTimeout(timeoutRef.current); - timeoutRef.current = null; - } - messageQueueRef.current = []; - startTransition(() => { - dispatch({ type: 'CLEAR' }); - }); - }, []); - - // Cleanup on unmount - useEffect( - () => () => { - if (timeoutRef.current) { - clearTimeout(timeoutRef.current); - } - }, - [], - ); - - return { consoleMessages, handleNewMessage, clearConsoleMessages }; -} diff --git a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx index d9aafa2e2..513519bac 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx +++ b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx @@ -1602,9 +1602,6 @@ describe('useGeminiStream', () => { }); it('should handle errors gracefully when auto-approving tool calls', async () => { - const consoleSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); const mockOnConfirmSuccess = vi.fn().mockResolvedValue(undefined); const mockOnConfirmError = vi .fn() @@ -1674,14 +1671,6 @@ describe('useGeminiStream', () => { // Both confirmation methods should be called expect(mockOnConfirmSuccess).toHaveBeenCalledTimes(1); expect(mockOnConfirmError).toHaveBeenCalledTimes(1); - - // Error should be logged - expect(consoleSpy).toHaveBeenCalledWith( - 'Failed to auto-approve tool call call2:', - expect.any(Error), - ); - - consoleSpy.mockRestore(); }); it('should skip tool calls without confirmationDetails', async () => { diff --git a/packages/cli/src/ui/hooks/useInputHistoryStore.test.ts b/packages/cli/src/ui/hooks/useInputHistoryStore.test.ts index 5404cefc0..49ccef42b 100644 --- a/packages/cli/src/ui/hooks/useInputHistoryStore.test.ts +++ b/packages/cli/src/ui/hooks/useInputHistoryStore.test.ts @@ -107,8 +107,6 @@ describe('useInputHistoryStore', () => { .mockRejectedValue(new Error('Logger error')), }; - const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); - const { result } = renderHook(() => useInputHistoryStore()); await act(async () => { @@ -116,12 +114,6 @@ describe('useInputHistoryStore', () => { }); expect(result.current.inputHistory).toEqual([]); - expect(consoleSpy).toHaveBeenCalledWith( - 'Failed to initialize input history from logger:', - expect.any(Error), - ); - - consoleSpy.mockRestore(); }); it('should initialize only once', async () => { diff --git a/packages/cli/src/ui/hooks/useShellHistory.test.ts b/packages/cli/src/ui/hooks/useShellHistory.test.ts index 8c4939345..4bfe6c9bb 100644 --- a/packages/cli/src/ui/hooks/useShellHistory.test.ts +++ b/packages/cli/src/ui/hooks/useShellHistory.test.ts @@ -43,6 +43,12 @@ vi.mock('@qwen-code/qwen-code-core', () => { return { isNodeError: (err: unknown): err is NodeJS.ErrnoException => typeof err === 'object' && err !== null && 'code' in err, + createDebugLogger: () => ({ + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + }), Storage, }; }); diff --git a/packages/cli/src/ui/keyMatchers.test.ts b/packages/cli/src/ui/keyMatchers.test.ts index a0a9b8279..7ca67117c 100644 --- a/packages/cli/src/ui/keyMatchers.test.ts +++ b/packages/cli/src/ui/keyMatchers.test.ts @@ -50,7 +50,6 @@ describe('keyMatchers', () => { [Command.OPEN_EXTERNAL_EDITOR]: (key: Key) => key.ctrl && (key.name === 'x' || key.sequence === '\x18'), [Command.PASTE_CLIPBOARD_IMAGE]: (key: Key) => key.ctrl && key.name === 'v', - [Command.SHOW_ERROR_DETAILS]: (key: Key) => key.ctrl && key.name === 'o', [Command.TOGGLE_TOOL_DESCRIPTIONS]: (key: Key) => key.ctrl && key.name === 't', [Command.TOGGLE_IDE_CONTEXT_DETAIL]: (key: Key) => @@ -222,11 +221,6 @@ describe('keyMatchers', () => { }, // App level bindings - { - command: Command.SHOW_ERROR_DETAILS, - positive: [createKey('o', { ctrl: true })], - negative: [createKey('o'), createKey('e', { ctrl: true })], - }, { command: Command.TOGGLE_TOOL_DESCRIPTIONS, positive: [createKey('t', { ctrl: true })], diff --git a/packages/cli/src/ui/themes/theme-manager.test.ts b/packages/cli/src/ui/themes/theme-manager.test.ts index 52e03b011..75c6b761d 100644 --- a/packages/cli/src/ui/themes/theme-manager.test.ts +++ b/packages/cli/src/ui/themes/theme-manager.test.ts @@ -160,22 +160,14 @@ describe('ThemeManager', () => { expect(themeManager.getActiveTheme().name).toBe(DEFAULT_THEME.name); }); - it('should not load a theme from an untrusted file path and log a message', () => { + it('should not load a theme from an untrusted file path', () => { vi.spyOn(fs, 'existsSync').mockReturnValue(true); vi.spyOn(fs, 'readFileSync').mockReturnValue(JSON.stringify(mockTheme)); - const consoleWarnSpy = vi - .spyOn(console, 'warn') - .mockImplementation(() => {}); const result = themeManager.setActiveTheme('/untrusted/my-theme.json'); expect(result).toBe(false); expect(themeManager.getActiveTheme().name).toBe(DEFAULT_THEME.name); - expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining('is outside your home directory'), - ); - - consoleWarnSpy.mockRestore(); }); }); }); diff --git a/packages/cli/src/ui/utils/ConsolePatcher.ts b/packages/cli/src/ui/utils/ConsolePatcher.ts deleted file mode 100644 index b0dd048e6..000000000 --- a/packages/cli/src/ui/utils/ConsolePatcher.ts +++ /dev/null @@ -1,71 +0,0 @@ -/** - * @license - * Copyright 2025 Google LLC - * SPDX-License-Identifier: Apache-2.0 - */ - -import util from 'node:util'; -import type { ConsoleMessageItem } from '../types.js'; - -interface ConsolePatcherParams { - onNewMessage?: (message: Omit) => void; - debugMode: boolean; - stderr?: boolean; -} - -export class ConsolePatcher { - private originalConsoleLog = console.log; - private originalConsoleWarn = console.warn; - private originalConsoleError = console.error; - private originalConsoleDebug = console.debug; - private originalConsoleInfo = console.info; - - private params: ConsolePatcherParams; - - constructor(params: ConsolePatcherParams) { - this.params = params; - } - - patch() { - console.log = this.patchConsoleMethod('log', this.originalConsoleLog); - console.warn = this.patchConsoleMethod('warn', this.originalConsoleWarn); - console.error = this.patchConsoleMethod('error', this.originalConsoleError); - console.debug = this.patchConsoleMethod('debug', this.originalConsoleDebug); - console.info = this.patchConsoleMethod('info', this.originalConsoleInfo); - } - - cleanup = () => { - console.log = this.originalConsoleLog; - console.warn = this.originalConsoleWarn; - console.error = this.originalConsoleError; - console.debug = this.originalConsoleDebug; - console.info = this.originalConsoleInfo; - }; - - private formatArgs = (args: unknown[]): string => util.format(...args); - - private patchConsoleMethod = - ( - type: 'log' | 'warn' | 'error' | 'debug' | 'info', - originalMethod: (...args: unknown[]) => void, - ) => - (...args: unknown[]) => { - if (this.params.stderr) { - if (type !== 'debug' || this.params.debugMode) { - this.originalConsoleError(this.formatArgs(args)); - } - } else { - if (this.params.debugMode) { - originalMethod.apply(console, args); - } - - if (type !== 'debug' || this.params.debugMode) { - this.params.onNewMessage?.({ - type, - content: this.formatArgs(args), - count: 1, - }); - } - } - }; -} diff --git a/packages/cli/src/utils/commentJson.test.ts b/packages/cli/src/utils/commentJson.test.ts index fcf2501cd..0cba17cb6 100644 --- a/packages/cli/src/utils/commentJson.test.ts +++ b/packages/cli/src/utils/commentJson.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import * as fs from 'node:fs'; import * as path from 'node:path'; import * as os from 'node:os'; @@ -158,28 +158,14 @@ describe('commentJson', () => { fs.writeFileSync(testFilePath, corruptedContent, 'utf-8'); - const consoleSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); - expect(() => { updateSettingsFilePreservingFormat(testFilePath, { model: 'gemini-2.5-flash', }); }).not.toThrow(); - expect(consoleSpy).toHaveBeenCalledWith( - 'Error parsing settings file:', - expect.any(Error), - ); - expect(consoleSpy).toHaveBeenCalledWith( - 'Settings file may be corrupted. Please check the JSON syntax.', - ); - const unchangedContent = fs.readFileSync(testFilePath, 'utf-8'); expect(unchangedContent).toBe(corruptedContent); - - consoleSpy.mockRestore(); }); }); }); diff --git a/packages/cli/src/utils/errors.test.ts b/packages/cli/src/utils/errors.test.ts index 9a6ee1c63..1dfa0deda 100644 --- a/packages/cli/src/utils/errors.test.ts +++ b/packages/cli/src/utils/errors.test.ts @@ -19,6 +19,14 @@ import { handleMaxTurnsExceededError, } from './errors.js'; +const mockWriteStderrLine = vi.hoisted(() => vi.fn()); +const debugLoggerSpy = vi.hoisted(() => ({ + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), +})); + // Mock the core modules vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { const original = @@ -26,6 +34,12 @@ vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { return { ...original, + createDebugLogger: () => ({ + debug: debugLoggerSpy.debug, + info: debugLoggerSpy.info, + warn: debugLoggerSpy.warn, + error: debugLoggerSpy.error, + }), parseAndFormatApiError: vi.fn((error: unknown) => { if (error instanceof Error) { return `API Error: ${error.message}`; @@ -66,18 +80,25 @@ vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { }; }); +vi.mock('./stdioHelpers.js', () => ({ + writeStderrLine: mockWriteStderrLine, + writeStdoutLine: vi.fn(), + clearScreen: vi.fn(), +})); + describe('errors', () => { let mockConfig: Config; let processExitSpy: MockInstance; let processStderrWriteSpy: MockInstance; - let consoleErrorSpy: MockInstance; beforeEach(() => { // Reset mocks vi.clearAllMocks(); - - // Mock console.error - consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + mockWriteStderrLine.mockClear(); + debugLoggerSpy.debug.mockClear(); + debugLoggerSpy.info.mockClear(); + debugLoggerSpy.warn.mockClear(); + debugLoggerSpy.error.mockClear(); // Mock process.stderr.write processStderrWriteSpy = vi @@ -99,7 +120,6 @@ describe('errors', () => { }); afterEach(() => { - consoleErrorSpy.mockRestore(); processStderrWriteSpy.mockRestore(); processExitSpy.mockRestore(); }); @@ -163,7 +183,9 @@ describe('errors', () => { handleError(testError, mockConfig); }).toThrow(testError); - expect(consoleErrorSpy).toHaveBeenCalledWith('API Error: Test error'); + expect(mockWriteStderrLine).toHaveBeenCalledWith( + 'API Error: Test error', + ); }); it('should handle non-Error objects', () => { @@ -173,7 +195,9 @@ describe('errors', () => { handleError(testError, mockConfig); }).toThrow(testError); - expect(consoleErrorSpy).toHaveBeenCalledWith('API Error: String error'); + expect(mockWriteStderrLine).toHaveBeenCalledWith( + 'API Error: String error', + ); }); }); @@ -191,7 +215,7 @@ describe('errors', () => { handleError(testError, mockConfig); }).toThrow('process.exit called with code: 1'); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( JSON.stringify( { error: { @@ -213,7 +237,7 @@ describe('errors', () => { handleError(testError, mockConfig, 42); }).toThrow('process.exit called with code: 42'); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( JSON.stringify( { error: { @@ -235,7 +259,7 @@ describe('errors', () => { handleError(fatalError, mockConfig); }).toThrow('process.exit called with code: 42'); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( JSON.stringify( { error: { @@ -271,7 +295,7 @@ describe('errors', () => { handleError(errorWithStatus, mockConfig); }).toThrow('process.exit called with code: 1'); // string codes become 1 - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( JSON.stringify( { error: { @@ -307,7 +331,7 @@ describe('errors', () => { it('should log error message to stderr and not exit', () => { handleToolError(toolName, toolError, mockConfig); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerSpy.error).toHaveBeenCalledWith( 'Error executing tool test-tool: Tool failed', ); expect(processExitSpy).not.toHaveBeenCalled(); @@ -322,7 +346,7 @@ describe('errors', () => { 'Custom display message', ); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerSpy.error).toHaveBeenCalledWith( 'Error executing tool test-tool: Custom display message', ); expect(processExitSpy).not.toHaveBeenCalled(); @@ -340,7 +364,7 @@ describe('errors', () => { handleToolError(toolName, toolError, mockConfig); // In JSON mode, should not exit (just log to stderr when debug mode is on) - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerSpy.error).toHaveBeenCalledWith( 'Error executing tool test-tool: Tool failed', ); expect(processExitSpy).not.toHaveBeenCalled(); @@ -350,7 +374,7 @@ describe('errors', () => { handleToolError(toolName, toolError, mockConfig, 'CUSTOM_TOOL_ERROR'); // In JSON mode, should not exit (just log to stderr when debug mode is on) - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerSpy.error).toHaveBeenCalledWith( 'Error executing tool test-tool: Tool failed', ); expect(processExitSpy).not.toHaveBeenCalled(); @@ -360,7 +384,7 @@ describe('errors', () => { handleToolError(toolName, toolError, mockConfig, 500); // In JSON mode, should not exit (just log to stderr when debug mode is on) - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerSpy.error).toHaveBeenCalledWith( 'Error executing tool test-tool: Tool failed', ); expect(processExitSpy).not.toHaveBeenCalled(); @@ -376,7 +400,7 @@ describe('errors', () => { ); // In JSON mode, should not exit (just log to stderr when debug mode is on) - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerSpy.error).toHaveBeenCalledWith( 'Error executing tool test-tool: Display message', ); expect(processExitSpy).not.toHaveBeenCalled(); @@ -394,7 +418,7 @@ describe('errors', () => { handleToolError(toolName, toolError, mockConfig); // Should not exit in STREAM_JSON mode (just log to stderr when debug mode is on) - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(debugLoggerSpy.error).toHaveBeenCalledWith( 'Error executing tool test-tool: Tool failed', ); expect(processExitSpy).not.toHaveBeenCalled(); @@ -407,36 +431,42 @@ describe('errors', () => { (mockConfig.getDebugMode as Mock).mockReturnValue(false); }); - it('should not log and not exit in text mode', () => { + it('should log error and not exit in text mode', () => { ( mockConfig.getOutputFormat as ReturnType ).mockReturnValue(OutputFormat.TEXT); handleToolError(toolName, toolError, mockConfig); - expect(consoleErrorSpy).not.toHaveBeenCalled(); + expect(debugLoggerSpy.error).toHaveBeenCalledWith( + 'Error executing tool test-tool: Tool failed', + ); expect(processExitSpy).not.toHaveBeenCalled(); }); - it('should not log and not exit in JSON mode', () => { + it('should log error and not exit in JSON mode', () => { ( mockConfig.getOutputFormat as ReturnType ).mockReturnValue(OutputFormat.JSON); handleToolError(toolName, toolError, mockConfig); - expect(consoleErrorSpy).not.toHaveBeenCalled(); + expect(debugLoggerSpy.error).toHaveBeenCalledWith( + 'Error executing tool test-tool: Tool failed', + ); expect(processExitSpy).not.toHaveBeenCalled(); }); - it('should not log and not exit in STREAM_JSON mode', () => { + it('should log error and not exit in STREAM_JSON mode', () => { ( mockConfig.getOutputFormat as ReturnType ).mockReturnValue(OutputFormat.STREAM_JSON); handleToolError(toolName, toolError, mockConfig); - expect(consoleErrorSpy).not.toHaveBeenCalled(); + expect(debugLoggerSpy.error).toHaveBeenCalledWith( + 'Error executing tool test-tool: Tool failed', + ); expect(processExitSpy).not.toHaveBeenCalled(); }); }); @@ -565,7 +595,9 @@ describe('errors', () => { handleCancellationError(mockConfig); }).toThrow('process.exit called with code: 130'); - expect(consoleErrorSpy).toHaveBeenCalledWith('Operation cancelled.'); + expect(mockWriteStderrLine).toHaveBeenCalledWith( + 'Operation cancelled.', + ); }); }); @@ -581,7 +613,7 @@ describe('errors', () => { handleCancellationError(mockConfig); }).toThrow('process.exit called with code: 130'); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( JSON.stringify( { error: { @@ -611,7 +643,7 @@ describe('errors', () => { handleMaxTurnsExceededError(mockConfig); }).toThrow('process.exit called with code: 53'); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( 'Reached max session turns for this session. Increase the number of turns by specifying maxSessionTurns in settings.json.', ); }); @@ -629,7 +661,7 @@ describe('errors', () => { handleMaxTurnsExceededError(mockConfig); }).toThrow('process.exit called with code: 53'); - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( JSON.stringify( { error: { diff --git a/packages/cli/src/utils/installationInfo.test.ts b/packages/cli/src/utils/installationInfo.test.ts index 34a70dae2..2267287a3 100644 --- a/packages/cli/src/utils/installationInfo.test.ts +++ b/packages/cli/src/utils/installationInfo.test.ts @@ -11,9 +11,14 @@ import * as path from 'node:path'; import * as childProcess from 'node:child_process'; import { isGitRepository } from '@qwen-code/qwen-code-core'; -vi.mock('@qwen-code/qwen-code-core', () => ({ - isGitRepository: vi.fn(), -})); +vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + isGitRepository: vi.fn(), + }; +}); vi.mock('fs', async (importOriginal) => { const actualFs = await importOriginal(); @@ -58,8 +63,7 @@ describe('getInstallationInfo', () => { expect(info.packageManager).toBe(PackageManager.UNKNOWN); }); - it('should return UNKNOWN and log error if realpathSync fails', () => { - const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + it('should return UNKNOWN if realpathSync fails', () => { process.argv[1] = '/path/to/cli'; const error = new Error('realpath failed'); mockedRealPathSync.mockImplementation(() => { @@ -69,8 +73,6 @@ describe('getInstallationInfo', () => { const info = getInstallationInfo(projectRoot, false); expect(info.packageManager).toBe(PackageManager.UNKNOWN); - expect(consoleSpy).toHaveBeenCalledWith(error); - consoleSpy.mockRestore(); }); it('should detect running from a local git clone', () => { diff --git a/packages/cli/src/utils/modelConfigUtils.test.ts b/packages/cli/src/utils/modelConfigUtils.test.ts index 28812e8d4..0d39ed06e 100644 --- a/packages/cli/src/utils/modelConfigUtils.test.ts +++ b/packages/cli/src/utils/modelConfigUtils.test.ts @@ -16,6 +16,8 @@ import { } from './modelConfigUtils.js'; import type { Settings } from '../config/settings.js'; +const mockWriteStderrLine = vi.hoisted(() => vi.fn()); + vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { const original = await importOriginal(); @@ -25,6 +27,12 @@ vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { }; }); +vi.mock('./stdioHelpers.js', () => ({ + writeStderrLine: mockWriteStderrLine, + writeStdoutLine: vi.fn(), + clearScreen: vi.fn(), +})); + describe('modelConfigUtils', () => { describe('getAuthTypeFromEnv', () => { const originalEnv = process.env; @@ -122,17 +130,15 @@ describe('modelConfigUtils', () => { describe('resolveCliGenerationConfig', () => { const originalEnv = process.env; - const originalConsoleWarn = console.warn; beforeEach(() => { vi.resetModules(); process.env = { ...originalEnv }; - console.warn = vi.fn(); + mockWriteStderrLine.mockClear(); }); afterEach(() => { process.env = originalEnv; - console.warn = originalConsoleWarn; vi.clearAllMocks(); }); @@ -521,8 +527,8 @@ describe('modelConfigUtils', () => { selectedAuthType, }); - expect(console.warn).toHaveBeenCalledWith('Warning 1'); - expect(console.warn).toHaveBeenCalledWith('Warning 2'); + expect(mockWriteStderrLine).toHaveBeenCalledWith('Warning 1'); + expect(mockWriteStderrLine).toHaveBeenCalledWith('Warning 2'); }); it('should use custom env when provided', () => { diff --git a/packages/cli/src/utils/relaunch.test.ts b/packages/cli/src/utils/relaunch.test.ts index 0ed8c4856..1d137bced 100644 --- a/packages/cli/src/utils/relaunch.test.ts +++ b/packages/cli/src/utils/relaunch.test.ts @@ -33,14 +33,12 @@ 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); @@ -49,7 +47,6 @@ describe('relaunchOnExitCode', () => { afterEach(() => { processExitSpy.mockRestore(); - consoleErrorSpy.mockRestore(); stdinResumeSpy.mockRestore(); }); @@ -90,10 +87,6 @@ describe('relaunchOnExitCode', () => { ); expect(runner).toHaveBeenCalledTimes(1); - expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Fatal error: Failed to relaunch the CLI process.', - error, - ); expect(stdinResumeSpy).toHaveBeenCalled(); expect(processExitSpy).toHaveBeenCalledWith(1); }); @@ -101,7 +94,6 @@ describe('relaunchOnExitCode', () => { describe('relaunchAppInChildProcess', () => { let processExitSpy: MockInstance; - let consoleErrorSpy: MockInstance; let stdinPauseSpy: MockInstance; let stdinResumeSpy: MockInstance; @@ -124,7 +116,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 +131,6 @@ describe('relaunchAppInChildProcess', () => { process.execPath = originalExecPath; processExitSpy.mockRestore(); - consoleErrorSpy.mockRestore(); stdinPauseSpy.mockRestore(); stdinResumeSpy.mockRestore(); }); diff --git a/packages/cli/src/utils/sandbox.ts b/packages/cli/src/utils/sandbox.ts index ae5cc82af..25b07185e 100644 --- a/packages/cli/src/utils/sandbox.ts +++ b/packages/cli/src/utils/sandbox.ts @@ -17,7 +17,6 @@ import { import { promisify } from 'node:util'; import type { Config, SandboxConfig } from '@qwen-code/qwen-code-core'; import { FatalSandboxError } from '@qwen-code/qwen-code-core'; -import { ConsolePatcher } from '../ui/utils/ConsolePatcher.js'; import { randomBytes } from 'node:crypto'; import { writeStdoutLine, writeStderrLine } from './stdioHelpers.js'; @@ -179,625 +178,124 @@ export async function start_sandbox( cliConfig?: Config, cliArgs: string[] = [], ): Promise { - const patcher = new ConsolePatcher({ - debugMode: cliConfig?.getDebugMode() || !!process.env['DEBUG'], - stderr: true, - }); - patcher.patch(); - - try { - if (config.command === 'sandbox-exec') { - // disallow BUILD_SANDBOX - if (process.env['BUILD_SANDBOX']) { - throw new FatalSandboxError( - 'Cannot BUILD_SANDBOX when using macOS Seatbelt', - ); - } - - const profile = (process.env['SEATBELT_PROFILE'] ??= 'permissive-open'); - let profileFile = fileURLToPath( - new URL(`sandbox-macos-${profile}.sb`, import.meta.url), - ); - // if profile name is not recognized, then look for file under project settings directory - if (!BUILTIN_SEATBELT_PROFILES.includes(profile)) { - profileFile = path.join( - SETTINGS_DIRECTORY_NAME, - `sandbox-macos-${profile}.sb`, - ); - } - if (!fs.existsSync(profileFile)) { - throw new FatalSandboxError( - `Missing macos seatbelt profile file '${profileFile}'`, - ); - } - // Log on STDERR so it doesn't clutter the output on STDOUT - writeStderrLine(`using macos seatbelt (profile: ${profile}) ...`); - // if DEBUG is set, convert to --inspect-brk in NODE_OPTIONS - const nodeOptions = [ - ...(process.env['DEBUG'] ? ['--inspect-brk'] : []), - ...nodeArgs, - ].join(' '); - - const args = [ - '-D', - `TARGET_DIR=${fs.realpathSync(process.cwd())}`, - '-D', - `TMP_DIR=${fs.realpathSync(os.tmpdir())}`, - '-D', - `HOME_DIR=${fs.realpathSync(os.homedir())}`, - '-D', - `CACHE_DIR=${fs.realpathSync(execSync(`getconf DARWIN_USER_CACHE_DIR`).toString().trim())}`, - ]; - - // Add included directories from the workspace context - // Always add 5 INCLUDE_DIR parameters to ensure .sb files can reference them - const MAX_INCLUDE_DIRS = 5; - const targetDir = fs.realpathSync(cliConfig?.getTargetDir() || ''); - const includedDirs: string[] = []; - - if (cliConfig) { - const workspaceContext = cliConfig.getWorkspaceContext(); - const directories = workspaceContext.getDirectories(); - - // Filter out TARGET_DIR - for (const dir of directories) { - const realDir = fs.realpathSync(dir); - if (realDir !== targetDir) { - includedDirs.push(realDir); - } - } - } - - for (let i = 0; i < MAX_INCLUDE_DIRS; i++) { - let dirPath = '/dev/null'; // Default to a safe path that won't cause issues - - if (i < includedDirs.length) { - dirPath = includedDirs[i]; - } - - args.push('-D', `INCLUDE_DIR_${i}=${dirPath}`); - } - - const finalArgv = cliArgs; - - args.push( - '-f', - profileFile, - 'sh', - '-c', - [ - `SANDBOX=sandbox-exec`, - `NODE_OPTIONS="${nodeOptions}"`, - ...finalArgv.map((arg) => quote([arg])), - ].join(' '), - ); - // start and set up proxy if GEMINI_SANDBOX_PROXY_COMMAND is set - const proxyCommand = process.env['GEMINI_SANDBOX_PROXY_COMMAND']; - let proxyProcess: ChildProcess | undefined = undefined; - let sandboxProcess: ChildProcess | undefined = undefined; - const sandboxEnv = { ...process.env }; - if (proxyCommand) { - const proxy = - process.env['HTTPS_PROXY'] || - process.env['https_proxy'] || - process.env['HTTP_PROXY'] || - process.env['http_proxy'] || - 'http://localhost:8877'; - sandboxEnv['HTTPS_PROXY'] = proxy; - sandboxEnv['https_proxy'] = proxy; // lower-case can be required, e.g. for curl - sandboxEnv['HTTP_PROXY'] = proxy; - sandboxEnv['http_proxy'] = proxy; - const noProxy = process.env['NO_PROXY'] || process.env['no_proxy']; - if (noProxy) { - sandboxEnv['NO_PROXY'] = noProxy; - sandboxEnv['no_proxy'] = noProxy; - } - // Note: CodeQL flags this as js/shell-command-injection-from-environment. - // This is intentional - CLI tool executes user-provided proxy commands. - proxyProcess = spawn('bash', ['-c', proxyCommand], { - stdio: ['ignore', 'pipe', 'pipe'], - detached: true, - }); - // install handlers to stop proxy on exit/signal - const stopProxy = () => { - writeStdoutLine('stopping proxy ...'); - if (proxyProcess?.pid) { - process.kill(-proxyProcess.pid, 'SIGTERM'); - } - }; - process.on('exit', stopProxy); - process.on('SIGINT', stopProxy); - process.on('SIGTERM', stopProxy); - - // commented out as it disrupts ink rendering - // proxyProcess.stdout?.on('data', (data) => { - // console.info(data.toString()); - // }); - proxyProcess.stderr?.on('data', (data) => { - writeStderrLine(data.toString()); - }); - proxyProcess.on('close', (code, signal) => { - if (sandboxProcess?.pid) { - process.kill(-sandboxProcess.pid, 'SIGTERM'); - } - throw new FatalSandboxError( - `Proxy command '${proxyCommand}' exited with code ${code}, signal ${signal}`, - ); - }); - writeStdoutLine('waiting for proxy to start ...'); - await execAsync( - `until timeout 0.25 curl -s http://localhost:8877; do sleep 0.25; done`, - ); - } - // spawn child and let it inherit stdio - process.stdin.pause(); - sandboxProcess = spawn(config.command, args, { - stdio: 'inherit', - }); - return new Promise((resolve, reject) => { - sandboxProcess?.on('error', reject); - sandboxProcess?.on('close', (code) => { - process.stdin.resume(); - resolve(code ?? 1); - }); - }); - } - - writeStderrLine(`hopping into sandbox (command: ${config.command}) ...`); - - // determine full path for gemini-cli to distinguish linked vs installed setting - const gcPath = fs.realpathSync(process.argv[1]); - - const projectSandboxDockerfile = path.join( - SETTINGS_DIRECTORY_NAME, - 'sandbox.Dockerfile', - ); - const isCustomProjectSandbox = fs.existsSync(projectSandboxDockerfile); - - const image = config.image; - const workdir = path.resolve(process.cwd()); - const containerWorkdir = getContainerPath(workdir); - - // if BUILD_SANDBOX is set, then call scripts/build_sandbox.js under gemini-cli repo - // - // note this can only be done with binary linked from gemini-cli repo + if (config.command === 'sandbox-exec') { + // disallow BUILD_SANDBOX if (process.env['BUILD_SANDBOX']) { - if (!gcPath.includes('qwen-code/packages/')) { - throw new FatalSandboxError( - 'Cannot build sandbox using installed Qwen Code binary; ' + - 'run `npm link ./packages/cli` under QwenCode-cli repo to switch to linked binary.', - ); - } else { - writeStderrLine('building sandbox ...'); - const gcRoot = gcPath.split('/packages/')[0]; - // if project folder has sandbox.Dockerfile under project settings folder, use that - let buildArgs = ''; - const projectSandboxDockerfile = path.join( - SETTINGS_DIRECTORY_NAME, - 'sandbox.Dockerfile', - ); - if (isCustomProjectSandbox) { - writeStderrLine(`using ${projectSandboxDockerfile} for sandbox`); - buildArgs += `-f ${path.resolve(projectSandboxDockerfile)} -i ${image}`; - } - execSync( - `cd ${gcRoot} && node scripts/build_sandbox.js -s ${buildArgs}`, - { - stdio: 'inherit', - env: { - ...process.env, - GEMINI_SANDBOX: config.command, // in case sandbox is enabled via flags (see config.ts under cli package) - }, - }, - ); - } - } - - // stop if image is missing - if (!(await ensureSandboxImageIsPresent(config.command, image))) { - 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.' - : 'Please check the image name, your network connection, or notify gemini-cli-dev@google.com if the issue persists.'; throw new FatalSandboxError( - `Sandbox image '${image}' is missing or could not be pulled. ${remedy}`, + 'Cannot BUILD_SANDBOX when using macOS Seatbelt', ); } - // use interactive mode and auto-remove container on exit - // run init binary inside container to forward signals & reap zombies - const args = ['run', '-i', '--rm', '--init', '--workdir', containerWorkdir]; - - // add custom flags from SANDBOX_FLAGS - if (process.env['SANDBOX_FLAGS']) { - const flags = parse(process.env['SANDBOX_FLAGS'], process.env).filter( - (f): f is string => typeof f === 'string', - ); - args.push(...flags); - } - - // add TTY only if stdin is TTY as well, i.e. for piped input don't init TTY in container - if (process.stdin.isTTY) { - args.push('-t'); - } - - // allow access to host.docker.internal - args.push('--add-host', 'host.docker.internal:host-gateway'); - - // mount current directory as working directory in sandbox (set via --workdir) - args.push('--volume', `${workdir}:${containerWorkdir}`); - - // mount user settings directory inside container, after creating if missing - // note user/home changes inside sandbox and we mount at BOTH paths for consistency - const userSettingsDirOnHost = USER_SETTINGS_DIR; - const userSettingsDirInSandbox = getContainerPath( - `/home/node/${SETTINGS_DIRECTORY_NAME}`, + const profile = (process.env['SEATBELT_PROFILE'] ??= 'permissive-open'); + let profileFile = fileURLToPath( + new URL(`sandbox-macos-${profile}.sb`, import.meta.url), ); - if (!fs.existsSync(userSettingsDirOnHost)) { - fs.mkdirSync(userSettingsDirOnHost); - } - args.push( - '--volume', - `${userSettingsDirOnHost}:${userSettingsDirInSandbox}`, - ); - if (userSettingsDirInSandbox !== userSettingsDirOnHost) { - args.push( - '--volume', - `${userSettingsDirOnHost}:${getContainerPath(userSettingsDirOnHost)}`, + // if profile name is not recognized, then look for file under project settings directory + if (!BUILTIN_SEATBELT_PROFILES.includes(profile)) { + profileFile = path.join( + SETTINGS_DIRECTORY_NAME, + `sandbox-macos-${profile}.sb`, ); } - - // mount os.tmpdir() as os.tmpdir() inside container - args.push('--volume', `${os.tmpdir()}:${getContainerPath(os.tmpdir())}`); - - // mount gcloud config directory if it exists - const gcloudConfigDir = path.join(os.homedir(), '.config', 'gcloud'); - if (fs.existsSync(gcloudConfigDir)) { - args.push( - '--volume', - `${gcloudConfigDir}:${getContainerPath(gcloudConfigDir)}:ro`, + if (!fs.existsSync(profileFile)) { + throw new FatalSandboxError( + `Missing macos seatbelt profile file '${profileFile}'`, ); } + // Log on STDERR so it doesn't clutter the output on STDOUT + writeStderrLine(`using macos seatbelt (profile: ${profile}) ...`); + // if DEBUG is set, convert to --inspect-brk in NODE_OPTIONS + const nodeOptions = [ + ...(process.env['DEBUG'] ? ['--inspect-brk'] : []), + ...nodeArgs, + ].join(' '); - // mount ADC file if GOOGLE_APPLICATION_CREDENTIALS is set - if (process.env['GOOGLE_APPLICATION_CREDENTIALS']) { - const adcFile = process.env['GOOGLE_APPLICATION_CREDENTIALS']; - if (fs.existsSync(adcFile)) { - args.push('--volume', `${adcFile}:${getContainerPath(adcFile)}:ro`); - args.push( - '--env', - `GOOGLE_APPLICATION_CREDENTIALS=${getContainerPath(adcFile)}`, - ); - } - } + const args = [ + '-D', + `TARGET_DIR=${fs.realpathSync(process.cwd())}`, + '-D', + `TMP_DIR=${fs.realpathSync(os.tmpdir())}`, + '-D', + `HOME_DIR=${fs.realpathSync(os.homedir())}`, + '-D', + `CACHE_DIR=${fs.realpathSync(execSync(`getconf DARWIN_USER_CACHE_DIR`).toString().trim())}`, + ]; - // mount paths listed in SANDBOX_MOUNTS - if (process.env['SANDBOX_MOUNTS']) { - for (let mount of process.env['SANDBOX_MOUNTS'].split(',')) { - if (mount.trim()) { - // parse mount as from:to:opts - let [from, to, opts] = mount.trim().split(':'); - to = to || from; // default to mount at same path inside container - opts = opts || 'ro'; // default to read-only - mount = `${from}:${to}:${opts}`; - // check that from path is absolute - if (!path.isAbsolute(from)) { - throw new FatalSandboxError( - `Path '${from}' listed in SANDBOX_MOUNTS must be absolute`, - ); - } - // check that from path exists on host - if (!fs.existsSync(from)) { - throw new FatalSandboxError( - `Missing mount path '${from}' listed in SANDBOX_MOUNTS`, - ); - } - writeStderrLine(`SANDBOX_MOUNTS: ${from} -> ${to} (${opts})`); - args.push('--volume', mount); + // Add included directories from the workspace context + // Always add 5 INCLUDE_DIR parameters to ensure .sb files can reference them + const MAX_INCLUDE_DIRS = 5; + const targetDir = fs.realpathSync(cliConfig?.getTargetDir() || ''); + const includedDirs: string[] = []; + + if (cliConfig) { + const workspaceContext = cliConfig.getWorkspaceContext(); + const directories = workspaceContext.getDirectories(); + + // Filter out TARGET_DIR + for (const dir of directories) { + const realDir = fs.realpathSync(dir); + if (realDir !== targetDir) { + includedDirs.push(realDir); } } } - // expose env-specified ports on the sandbox - ports().forEach((p) => args.push('--publish', `${p}:${p}`)); + for (let i = 0; i < MAX_INCLUDE_DIRS; i++) { + let dirPath = '/dev/null'; // Default to a safe path that won't cause issues - // if DEBUG is set, expose debugging port - if (process.env['DEBUG']) { - const debugPort = process.env['DEBUG_PORT'] || '9229'; - args.push(`--publish`, `${debugPort}:${debugPort}`); + if (i < includedDirs.length) { + dirPath = includedDirs[i]; + } + + args.push('-D', `INCLUDE_DIR_${i}=${dirPath}`); } - // copy proxy environment variables, replacing localhost with SANDBOX_PROXY_NAME - // copy as both upper-case and lower-case as is required by some utilities - // GEMINI_SANDBOX_PROXY_COMMAND implies HTTPS_PROXY unless HTTP_PROXY is set - const proxyCommand = process.env['GEMINI_SANDBOX_PROXY_COMMAND']; + const finalArgv = cliArgs; + args.push( + '-f', + profileFile, + 'sh', + '-c', + [ + `SANDBOX=sandbox-exec`, + `NODE_OPTIONS="${nodeOptions}"`, + ...finalArgv.map((arg) => quote([arg])), + ].join(' '), + ); + // start and set up proxy if GEMINI_SANDBOX_PROXY_COMMAND is set + const proxyCommand = process.env['GEMINI_SANDBOX_PROXY_COMMAND']; + let proxyProcess: ChildProcess | undefined = undefined; + let sandboxProcess: ChildProcess | undefined = undefined; + const sandboxEnv = { ...process.env }; if (proxyCommand) { - let proxy = + const proxy = process.env['HTTPS_PROXY'] || process.env['https_proxy'] || process.env['HTTP_PROXY'] || process.env['http_proxy'] || 'http://localhost:8877'; - proxy = proxy.replace('localhost', SANDBOX_PROXY_NAME); - if (proxy) { - args.push('--env', `HTTPS_PROXY=${proxy}`); - args.push('--env', `https_proxy=${proxy}`); // lower-case can be required, e.g. for curl - args.push('--env', `HTTP_PROXY=${proxy}`); - args.push('--env', `http_proxy=${proxy}`); - } + sandboxEnv['HTTPS_PROXY'] = proxy; + sandboxEnv['https_proxy'] = proxy; // lower-case can be required, e.g. for curl + sandboxEnv['HTTP_PROXY'] = proxy; + sandboxEnv['http_proxy'] = proxy; const noProxy = process.env['NO_PROXY'] || process.env['no_proxy']; if (noProxy) { - args.push('--env', `NO_PROXY=${noProxy}`); - args.push('--env', `no_proxy=${noProxy}`); + sandboxEnv['NO_PROXY'] = noProxy; + sandboxEnv['no_proxy'] = noProxy; } - - // if using proxy, switch to internal networking through proxy - if (proxy) { - execSync( - `${config.command} network inspect ${SANDBOX_NETWORK_NAME} || ${config.command} network create --internal ${SANDBOX_NETWORK_NAME}`, - ); - args.push('--network', SANDBOX_NETWORK_NAME); - // if proxy command is set, create a separate network w/ host access (i.e. non-internal) - // we will run proxy in its own container connected to both host network and internal network - // this allows proxy to work even on rootless podman on macos with host<->vm<->container isolation - if (proxyCommand) { - execSync( - `${config.command} network inspect ${SANDBOX_PROXY_NAME} || ${config.command} network create ${SANDBOX_PROXY_NAME}`, - ); - } - } - } - - // name container after image, plus random suffix to avoid conflicts - const imageName = parseImageName(image); - const isIntegrationTest = - process.env['GEMINI_CLI_INTEGRATION_TEST'] === 'true'; - let containerName; - if (isIntegrationTest) { - containerName = `gemini-cli-integration-test-${randomBytes(4).toString( - 'hex', - )}`; - writeStdoutLine(`ContainerName: ${containerName}`); - } else { - let index = 0; - const containerNameCheck = execSync( - `${config.command} ps -a --format "{{.Names}}"`, - ) - .toString() - .trim(); - while (containerNameCheck.includes(`${imageName}-${index}`)) { - index++; - } - containerName = `${imageName}-${index}`; - writeStdoutLine(`ContainerName (regular): ${containerName}`); - } - args.push('--name', containerName, '--hostname', containerName); - - // copy GEMINI_CLI_TEST_VAR for integration tests - if (process.env['GEMINI_CLI_TEST_VAR']) { - args.push( - '--env', - `GEMINI_CLI_TEST_VAR=${process.env['GEMINI_CLI_TEST_VAR']}`, - ); - } - - // copy GEMINI_API_KEY(s) - if (process.env['GEMINI_API_KEY']) { - args.push('--env', `GEMINI_API_KEY=${process.env['GEMINI_API_KEY']}`); - } - if (process.env['GOOGLE_API_KEY']) { - args.push('--env', `GOOGLE_API_KEY=${process.env['GOOGLE_API_KEY']}`); - } - - // copy OPENAI_API_KEY and related env vars for Qwen - if (process.env['OPENAI_API_KEY']) { - args.push('--env', `OPENAI_API_KEY=${process.env['OPENAI_API_KEY']}`); - } - // copy TAVILY_API_KEY for web search tool - if (process.env['TAVILY_API_KEY']) { - args.push('--env', `TAVILY_API_KEY=${process.env['TAVILY_API_KEY']}`); - } - if (process.env['OPENAI_BASE_URL']) { - args.push('--env', `OPENAI_BASE_URL=${process.env['OPENAI_BASE_URL']}`); - } - if (process.env['OPENAI_MODEL']) { - args.push('--env', `OPENAI_MODEL=${process.env['OPENAI_MODEL']}`); - } - - // copy GOOGLE_GENAI_USE_VERTEXAI - if (process.env['GOOGLE_GENAI_USE_VERTEXAI']) { - args.push( - '--env', - `GOOGLE_GENAI_USE_VERTEXAI=${process.env['GOOGLE_GENAI_USE_VERTEXAI']}`, - ); - } - - // copy GOOGLE_GENAI_USE_GCA - if (process.env['GOOGLE_GENAI_USE_GCA']) { - args.push( - '--env', - `GOOGLE_GENAI_USE_GCA=${process.env['GOOGLE_GENAI_USE_GCA']}`, - ); - } - - // copy GOOGLE_CLOUD_PROJECT - if (process.env['GOOGLE_CLOUD_PROJECT']) { - args.push( - '--env', - `GOOGLE_CLOUD_PROJECT=${process.env['GOOGLE_CLOUD_PROJECT']}`, - ); - } - - // copy GOOGLE_CLOUD_LOCATION - if (process.env['GOOGLE_CLOUD_LOCATION']) { - args.push( - '--env', - `GOOGLE_CLOUD_LOCATION=${process.env['GOOGLE_CLOUD_LOCATION']}`, - ); - } - - // copy GEMINI_MODEL - if (process.env['GEMINI_MODEL']) { - args.push('--env', `GEMINI_MODEL=${process.env['GEMINI_MODEL']}`); - } - - // copy TERM and COLORTERM to try to maintain terminal setup - if (process.env['TERM']) { - args.push('--env', `TERM=${process.env['TERM']}`); - } - if (process.env['COLORTERM']) { - args.push('--env', `COLORTERM=${process.env['COLORTERM']}`); - } - - // Pass through IDE mode environment variables - for (const envVar of [ - 'QWEN_CODE_IDE_SERVER_PORT', - 'QWEN_CODE_IDE_WORKSPACE_PATH', - 'TERM_PROGRAM', - ]) { - if (process.env[envVar]) { - args.push('--env', `${envVar}=${process.env[envVar]}`); - } - } - - // copy VIRTUAL_ENV if under working directory - // also mount-replace VIRTUAL_ENV directory with /sandbox.venv - // sandbox can then set up this new VIRTUAL_ENV directory using sandbox.bashrc (see below) - // directory will be empty if not set up, which is still preferable to having host binaries - if ( - process.env['VIRTUAL_ENV'] - ?.toLowerCase() - .startsWith(workdir.toLowerCase()) - ) { - const sandboxVenvPath = path.resolve( - SETTINGS_DIRECTORY_NAME, - 'sandbox.venv', - ); - if (!fs.existsSync(sandboxVenvPath)) { - fs.mkdirSync(sandboxVenvPath, { recursive: true }); - } - args.push( - '--volume', - `${sandboxVenvPath}:${getContainerPath(process.env['VIRTUAL_ENV'])}`, - ); - args.push( - '--env', - `VIRTUAL_ENV=${getContainerPath(process.env['VIRTUAL_ENV'])}`, - ); - } - - // copy additional environment variables from SANDBOX_ENV - if (process.env['SANDBOX_ENV']) { - for (let env of process.env['SANDBOX_ENV'].split(',')) { - if ((env = env.trim())) { - if (env.includes('=')) { - writeStderrLine(`SANDBOX_ENV: ${env}`); - args.push('--env', env); - } else { - throw new FatalSandboxError( - 'SANDBOX_ENV must be a comma-separated list of key=value pairs', - ); - } - } - } - } - - // copy NODE_OPTIONS - const existingNodeOptions = process.env['NODE_OPTIONS'] || ''; - const allNodeOptions = [ - ...(existingNodeOptions ? [existingNodeOptions] : []), - ...nodeArgs, - ].join(' '); - - if (allNodeOptions.length > 0) { - args.push('--env', `NODE_OPTIONS="${allNodeOptions}"`); - } - - // set SANDBOX as container name - args.push('--env', `SANDBOX=${containerName}`); - - // for podman only, use empty --authfile to skip unnecessary auth refresh overhead - if (config.command === 'podman') { - const emptyAuthFilePath = path.join(os.tmpdir(), 'empty_auth.json'); - fs.writeFileSync(emptyAuthFilePath, '{}', 'utf-8'); - args.push('--authfile', emptyAuthFilePath); - } - - // Determine if the current user's UID/GID should be passed to the sandbox. - // See shouldUseCurrentUserInSandbox for more details. - let userFlag = ''; - const finalEntrypoint = entrypoint(workdir, cliArgs); - - if (process.env['GEMINI_CLI_INTEGRATION_TEST'] === 'true') { - args.push('--user', 'root'); - userFlag = '--user root'; - } else if (await shouldUseCurrentUserInSandbox()) { - // For the user-creation logic to work, the container must start as root. - // The entrypoint script then handles dropping privileges to the correct user. - args.push('--user', 'root'); - - const uid = execSync('id -u').toString().trim(); - const gid = execSync('id -g').toString().trim(); - - // Instead of passing --user to the main sandbox container, we let it - // start as root, then create a user with the host's UID/GID, and - // finally switch to that user to run the gemini process. This is - // necessary on Linux to ensure the user exists within the - // container's /etc/passwd file, which is required by os.userInfo(). - const username = 'gemini'; - const homeDir = getContainerPath(os.homedir()); - - const setupUserCommands = [ - // Use -f with groupadd to avoid errors if the group already exists. - `groupadd -f -g ${gid} ${username}`, - // Create user only if it doesn't exist. Use -o for non-unique UID. - `id -u ${username} &>/dev/null || useradd -o -u ${uid} -g ${gid} -d ${homeDir} -s /bin/bash ${username}`, - ].join(' && '); - - const originalCommand = finalEntrypoint[2]; - const escapedOriginalCommand = originalCommand.replace(/'/g, "'\\''"); - - // Use `su -p` to preserve the environment. - const suCommand = `su -p ${username} -c '${escapedOriginalCommand}'`; - - // The entrypoint is always `['bash', '-c', '']`, so we modify the command part. - finalEntrypoint[2] = `${setupUserCommands} && ${suCommand}`; - - // We still need userFlag for the simpler proxy container, which does not have this issue. - userFlag = `--user ${uid}:${gid}`; - // When forcing a UID in the sandbox, $HOME can be reset to '/', so we copy $HOME as well. - args.push('--env', `HOME=${os.homedir()}`); - } - - // push container image name - args.push(image); - - // push container entrypoint (including args) - args.push(...finalEntrypoint); - - // start and set up proxy if GEMINI_SANDBOX_PROXY_COMMAND is set - let proxyProcess: ChildProcess | undefined = undefined; - let sandboxProcess: ChildProcess | undefined = undefined; - - if (proxyCommand) { - // run proxyCommand in its own container - const proxyContainerCommand = `${config.command} run --rm --init ${userFlag} --name ${SANDBOX_PROXY_NAME} --network ${SANDBOX_PROXY_NAME} -p 8877:8877 -v ${process.cwd()}:${workdir} --workdir ${workdir} ${image} ${proxyCommand}`; - const isWindows = os.platform() === 'win32'; - const proxyShell = isWindows ? 'cmd.exe' : 'bash'; - const proxyShellArgs = isWindows - ? ['/c', proxyContainerCommand] - : ['-c', proxyContainerCommand]; // Note: CodeQL flags this as js/shell-command-injection-from-environment. - // This is intentional - CLI tool executes user-provided proxy commands in container. - proxyProcess = spawn(proxyShell, proxyShellArgs, { + // This is intentional - CLI tool executes user-provided proxy commands. + proxyProcess = spawn('bash', ['-c', proxyCommand], { stdio: ['ignore', 'pipe', 'pipe'], detached: true, }); // install handlers to stop proxy on exit/signal const stopProxy = () => { - writeStdoutLine('stopping proxy container ...'); - execSync(`${config.command} rm -f ${SANDBOX_PROXY_NAME}`); + writeStdoutLine('stopping proxy ...'); + if (proxyProcess?.pid) { + process.kill(-proxyProcess.pid, 'SIGTERM'); + } }; process.on('exit', stopProxy); process.on('SIGINT', stopProxy); @@ -808,52 +306,538 @@ export async function start_sandbox( // console.info(data.toString()); // }); proxyProcess.stderr?.on('data', (data) => { - writeStderrLine(data.toString().trim()); + writeStderrLine(data.toString()); }); proxyProcess.on('close', (code, signal) => { if (sandboxProcess?.pid) { process.kill(-sandboxProcess.pid, 'SIGTERM'); } throw new FatalSandboxError( - `Proxy container command '${proxyContainerCommand}' exited with code ${code}, signal ${signal}`, + `Proxy command '${proxyCommand}' exited with code ${code}, signal ${signal}`, ); }); writeStdoutLine('waiting for proxy to start ...'); await execAsync( `until timeout 0.25 curl -s http://localhost:8877; do sleep 0.25; done`, ); - // connect proxy container to sandbox network - // (workaround for older versions of docker that don't support multiple --network args) - await execAsync( - `${config.command} network connect ${SANDBOX_NETWORK_NAME} ${SANDBOX_PROXY_NAME}`, - ); } - // spawn child and let it inherit stdio process.stdin.pause(); sandboxProcess = spawn(config.command, args, { stdio: 'inherit', }); - - return new Promise((resolve, reject) => { - sandboxProcess.on('error', (err) => { - writeStderrLine(`Sandbox process error: ${err}`); - reject(err); - }); - - sandboxProcess?.on('close', (code, signal) => { + return new Promise((resolve, reject) => { + sandboxProcess?.on('error', reject); + sandboxProcess?.on('close', (code) => { process.stdin.resume(); - if (code !== 0 && code !== null) { - writeStderrLine( - `Sandbox process exited with code: ${code}, signal: ${signal}`, - ); - } resolve(code ?? 1); }); }); - } finally { - patcher.cleanup(); } + + writeStderrLine(`hopping into sandbox (command: ${config.command}) ...`); + + // determine full path for gemini-cli to distinguish linked vs installed setting + const gcPath = fs.realpathSync(process.argv[1]); + + const projectSandboxDockerfile = path.join( + SETTINGS_DIRECTORY_NAME, + 'sandbox.Dockerfile', + ); + const isCustomProjectSandbox = fs.existsSync(projectSandboxDockerfile); + + const image = config.image; + const workdir = path.resolve(process.cwd()); + const containerWorkdir = getContainerPath(workdir); + + // if BUILD_SANDBOX is set, then call scripts/build_sandbox.js under gemini-cli repo + // + // note this can only be done with binary linked from gemini-cli repo + if (process.env['BUILD_SANDBOX']) { + if (!gcPath.includes('qwen-code/packages/')) { + throw new FatalSandboxError( + 'Cannot build sandbox using installed Qwen Code binary; ' + + 'run `npm link ./packages/cli` under QwenCode-cli repo to switch to linked binary.', + ); + } else { + writeStderrLine('building sandbox ...'); + const gcRoot = gcPath.split('/packages/')[0]; + // if project folder has sandbox.Dockerfile under project settings folder, use that + let buildArgs = ''; + const projectSandboxDockerfile = path.join( + SETTINGS_DIRECTORY_NAME, + 'sandbox.Dockerfile', + ); + if (isCustomProjectSandbox) { + writeStderrLine(`using ${projectSandboxDockerfile} for sandbox`); + buildArgs += `-f ${path.resolve(projectSandboxDockerfile)} -i ${image}`; + } + execSync( + `cd ${gcRoot} && node scripts/build_sandbox.js -s ${buildArgs}`, + { + stdio: 'inherit', + env: { + ...process.env, + GEMINI_SANDBOX: config.command, // in case sandbox is enabled via flags (see config.ts under cli package) + }, + }, + ); + } + } + + // stop if image is missing + if (!(await ensureSandboxImageIsPresent(config.command, image))) { + 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.' + : 'Please check the image name, your network connection, or notify gemini-cli-dev@google.com if the issue persists.'; + throw new FatalSandboxError( + `Sandbox image '${image}' is missing or could not be pulled. ${remedy}`, + ); + } + + // use interactive mode and auto-remove container on exit + // run init binary inside container to forward signals & reap zombies + const args = ['run', '-i', '--rm', '--init', '--workdir', containerWorkdir]; + + // add custom flags from SANDBOX_FLAGS + if (process.env['SANDBOX_FLAGS']) { + const flags = parse(process.env['SANDBOX_FLAGS'], process.env).filter( + (f): f is string => typeof f === 'string', + ); + args.push(...flags); + } + + // add TTY only if stdin is TTY as well, i.e. for piped input don't init TTY in container + if (process.stdin.isTTY) { + args.push('-t'); + } + + // allow access to host.docker.internal + args.push('--add-host', 'host.docker.internal:host-gateway'); + + // mount current directory as working directory in sandbox (set via --workdir) + args.push('--volume', `${workdir}:${containerWorkdir}`); + + // mount user settings directory inside container, after creating if missing + // note user/home changes inside sandbox and we mount at BOTH paths for consistency + const userSettingsDirOnHost = USER_SETTINGS_DIR; + const userSettingsDirInSandbox = getContainerPath( + `/home/node/${SETTINGS_DIRECTORY_NAME}`, + ); + if (!fs.existsSync(userSettingsDirOnHost)) { + fs.mkdirSync(userSettingsDirOnHost); + } + args.push('--volume', `${userSettingsDirOnHost}:${userSettingsDirInSandbox}`); + if (userSettingsDirInSandbox !== userSettingsDirOnHost) { + args.push( + '--volume', + `${userSettingsDirOnHost}:${getContainerPath(userSettingsDirOnHost)}`, + ); + } + + // mount os.tmpdir() as os.tmpdir() inside container + args.push('--volume', `${os.tmpdir()}:${getContainerPath(os.tmpdir())}`); + + // mount gcloud config directory if it exists + const gcloudConfigDir = path.join(os.homedir(), '.config', 'gcloud'); + if (fs.existsSync(gcloudConfigDir)) { + args.push( + '--volume', + `${gcloudConfigDir}:${getContainerPath(gcloudConfigDir)}:ro`, + ); + } + + // mount ADC file if GOOGLE_APPLICATION_CREDENTIALS is set + if (process.env['GOOGLE_APPLICATION_CREDENTIALS']) { + const adcFile = process.env['GOOGLE_APPLICATION_CREDENTIALS']; + if (fs.existsSync(adcFile)) { + args.push('--volume', `${adcFile}:${getContainerPath(adcFile)}:ro`); + args.push( + '--env', + `GOOGLE_APPLICATION_CREDENTIALS=${getContainerPath(adcFile)}`, + ); + } + } + + // mount paths listed in SANDBOX_MOUNTS + if (process.env['SANDBOX_MOUNTS']) { + for (let mount of process.env['SANDBOX_MOUNTS'].split(',')) { + if (mount.trim()) { + // parse mount as from:to:opts + let [from, to, opts] = mount.trim().split(':'); + to = to || from; // default to mount at same path inside container + opts = opts || 'ro'; // default to read-only + mount = `${from}:${to}:${opts}`; + // check that from path is absolute + if (!path.isAbsolute(from)) { + throw new FatalSandboxError( + `Path '${from}' listed in SANDBOX_MOUNTS must be absolute`, + ); + } + // check that from path exists on host + if (!fs.existsSync(from)) { + throw new FatalSandboxError( + `Missing mount path '${from}' listed in SANDBOX_MOUNTS`, + ); + } + writeStderrLine(`SANDBOX_MOUNTS: ${from} -> ${to} (${opts})`); + args.push('--volume', mount); + } + } + } + + // expose env-specified ports on the sandbox + ports().forEach((p) => args.push('--publish', `${p}:${p}`)); + + // if DEBUG is set, expose debugging port + if (process.env['DEBUG']) { + const debugPort = process.env['DEBUG_PORT'] || '9229'; + args.push(`--publish`, `${debugPort}:${debugPort}`); + } + + // copy proxy environment variables, replacing localhost with SANDBOX_PROXY_NAME + // copy as both upper-case and lower-case as is required by some utilities + // GEMINI_SANDBOX_PROXY_COMMAND implies HTTPS_PROXY unless HTTP_PROXY is set + const proxyCommand = process.env['GEMINI_SANDBOX_PROXY_COMMAND']; + + if (proxyCommand) { + let proxy = + process.env['HTTPS_PROXY'] || + process.env['https_proxy'] || + process.env['HTTP_PROXY'] || + process.env['http_proxy'] || + 'http://localhost:8877'; + proxy = proxy.replace('localhost', SANDBOX_PROXY_NAME); + if (proxy) { + args.push('--env', `HTTPS_PROXY=${proxy}`); + args.push('--env', `https_proxy=${proxy}`); // lower-case can be required, e.g. for curl + args.push('--env', `HTTP_PROXY=${proxy}`); + args.push('--env', `http_proxy=${proxy}`); + } + const noProxy = process.env['NO_PROXY'] || process.env['no_proxy']; + if (noProxy) { + args.push('--env', `NO_PROXY=${noProxy}`); + args.push('--env', `no_proxy=${noProxy}`); + } + + // if using proxy, switch to internal networking through proxy + if (proxy) { + execSync( + `${config.command} network inspect ${SANDBOX_NETWORK_NAME} || ${config.command} network create --internal ${SANDBOX_NETWORK_NAME}`, + ); + args.push('--network', SANDBOX_NETWORK_NAME); + // if proxy command is set, create a separate network w/ host access (i.e. non-internal) + // we will run proxy in its own container connected to both host network and internal network + // this allows proxy to work even on rootless podman on macos with host<->vm<->container isolation + if (proxyCommand) { + execSync( + `${config.command} network inspect ${SANDBOX_PROXY_NAME} || ${config.command} network create ${SANDBOX_PROXY_NAME}`, + ); + } + } + } + + // name container after image, plus random suffix to avoid conflicts + const imageName = parseImageName(image); + const isIntegrationTest = + process.env['GEMINI_CLI_INTEGRATION_TEST'] === 'true'; + let containerName; + if (isIntegrationTest) { + containerName = `gemini-cli-integration-test-${randomBytes(4).toString( + 'hex', + )}`; + writeStdoutLine(`ContainerName: ${containerName}`); + } else { + let index = 0; + const containerNameCheck = execSync( + `${config.command} ps -a --format "{{.Names}}"`, + ) + .toString() + .trim(); + while (containerNameCheck.includes(`${imageName}-${index}`)) { + index++; + } + containerName = `${imageName}-${index}`; + writeStdoutLine(`ContainerName (regular): ${containerName}`); + } + args.push('--name', containerName, '--hostname', containerName); + + // copy GEMINI_CLI_TEST_VAR for integration tests + if (process.env['GEMINI_CLI_TEST_VAR']) { + args.push( + '--env', + `GEMINI_CLI_TEST_VAR=${process.env['GEMINI_CLI_TEST_VAR']}`, + ); + } + + // copy GEMINI_API_KEY(s) + if (process.env['GEMINI_API_KEY']) { + args.push('--env', `GEMINI_API_KEY=${process.env['GEMINI_API_KEY']}`); + } + if (process.env['GOOGLE_API_KEY']) { + args.push('--env', `GOOGLE_API_KEY=${process.env['GOOGLE_API_KEY']}`); + } + + // copy OPENAI_API_KEY and related env vars for Qwen + if (process.env['OPENAI_API_KEY']) { + args.push('--env', `OPENAI_API_KEY=${process.env['OPENAI_API_KEY']}`); + } + // copy TAVILY_API_KEY for web search tool + if (process.env['TAVILY_API_KEY']) { + args.push('--env', `TAVILY_API_KEY=${process.env['TAVILY_API_KEY']}`); + } + if (process.env['OPENAI_BASE_URL']) { + args.push('--env', `OPENAI_BASE_URL=${process.env['OPENAI_BASE_URL']}`); + } + if (process.env['OPENAI_MODEL']) { + args.push('--env', `OPENAI_MODEL=${process.env['OPENAI_MODEL']}`); + } + + // copy GOOGLE_GENAI_USE_VERTEXAI + if (process.env['GOOGLE_GENAI_USE_VERTEXAI']) { + args.push( + '--env', + `GOOGLE_GENAI_USE_VERTEXAI=${process.env['GOOGLE_GENAI_USE_VERTEXAI']}`, + ); + } + + // copy GOOGLE_GENAI_USE_GCA + if (process.env['GOOGLE_GENAI_USE_GCA']) { + args.push( + '--env', + `GOOGLE_GENAI_USE_GCA=${process.env['GOOGLE_GENAI_USE_GCA']}`, + ); + } + + // copy GOOGLE_CLOUD_PROJECT + if (process.env['GOOGLE_CLOUD_PROJECT']) { + args.push( + '--env', + `GOOGLE_CLOUD_PROJECT=${process.env['GOOGLE_CLOUD_PROJECT']}`, + ); + } + + // copy GOOGLE_CLOUD_LOCATION + if (process.env['GOOGLE_CLOUD_LOCATION']) { + args.push( + '--env', + `GOOGLE_CLOUD_LOCATION=${process.env['GOOGLE_CLOUD_LOCATION']}`, + ); + } + + // copy GEMINI_MODEL + if (process.env['GEMINI_MODEL']) { + args.push('--env', `GEMINI_MODEL=${process.env['GEMINI_MODEL']}`); + } + + // copy TERM and COLORTERM to try to maintain terminal setup + if (process.env['TERM']) { + args.push('--env', `TERM=${process.env['TERM']}`); + } + if (process.env['COLORTERM']) { + args.push('--env', `COLORTERM=${process.env['COLORTERM']}`); + } + + // Pass through IDE mode environment variables + for (const envVar of [ + 'QWEN_CODE_IDE_SERVER_PORT', + 'QWEN_CODE_IDE_WORKSPACE_PATH', + 'TERM_PROGRAM', + ]) { + if (process.env[envVar]) { + args.push('--env', `${envVar}=${process.env[envVar]}`); + } + } + + // copy VIRTUAL_ENV if under working directory + // also mount-replace VIRTUAL_ENV directory with /sandbox.venv + // sandbox can then set up this new VIRTUAL_ENV directory using sandbox.bashrc (see below) + // directory will be empty if not set up, which is still preferable to having host binaries + if ( + process.env['VIRTUAL_ENV']?.toLowerCase().startsWith(workdir.toLowerCase()) + ) { + const sandboxVenvPath = path.resolve( + SETTINGS_DIRECTORY_NAME, + 'sandbox.venv', + ); + if (!fs.existsSync(sandboxVenvPath)) { + fs.mkdirSync(sandboxVenvPath, { recursive: true }); + } + args.push( + '--volume', + `${sandboxVenvPath}:${getContainerPath(process.env['VIRTUAL_ENV'])}`, + ); + args.push( + '--env', + `VIRTUAL_ENV=${getContainerPath(process.env['VIRTUAL_ENV'])}`, + ); + } + + // copy additional environment variables from SANDBOX_ENV + if (process.env['SANDBOX_ENV']) { + for (let env of process.env['SANDBOX_ENV'].split(',')) { + if ((env = env.trim())) { + if (env.includes('=')) { + writeStderrLine(`SANDBOX_ENV: ${env}`); + args.push('--env', env); + } else { + throw new FatalSandboxError( + 'SANDBOX_ENV must be a comma-separated list of key=value pairs', + ); + } + } + } + } + + // copy NODE_OPTIONS + const existingNodeOptions = process.env['NODE_OPTIONS'] || ''; + const allNodeOptions = [ + ...(existingNodeOptions ? [existingNodeOptions] : []), + ...nodeArgs, + ].join(' '); + + if (allNodeOptions.length > 0) { + args.push('--env', `NODE_OPTIONS="${allNodeOptions}"`); + } + + // set SANDBOX as container name + args.push('--env', `SANDBOX=${containerName}`); + + // for podman only, use empty --authfile to skip unnecessary auth refresh overhead + if (config.command === 'podman') { + const emptyAuthFilePath = path.join(os.tmpdir(), 'empty_auth.json'); + fs.writeFileSync(emptyAuthFilePath, '{}', 'utf-8'); + args.push('--authfile', emptyAuthFilePath); + } + + // Determine if the current user's UID/GID should be passed to the sandbox. + // See shouldUseCurrentUserInSandbox for more details. + let userFlag = ''; + const finalEntrypoint = entrypoint(workdir, cliArgs); + + if (process.env['GEMINI_CLI_INTEGRATION_TEST'] === 'true') { + args.push('--user', 'root'); + userFlag = '--user root'; + } else if (await shouldUseCurrentUserInSandbox()) { + // For the user-creation logic to work, the container must start as root. + // The entrypoint script then handles dropping privileges to the correct user. + args.push('--user', 'root'); + + const uid = execSync('id -u').toString().trim(); + const gid = execSync('id -g').toString().trim(); + + // Instead of passing --user to the main sandbox container, we let it + // start as root, then create a user with the host's UID/GID, and + // finally switch to that user to run the gemini process. This is + // necessary on Linux to ensure the user exists within the + // container's /etc/passwd file, which is required by os.userInfo(). + const username = 'gemini'; + const homeDir = getContainerPath(os.homedir()); + + const setupUserCommands = [ + // Use -f with groupadd to avoid errors if the group already exists. + `groupadd -f -g ${gid} ${username}`, + // Create user only if it doesn't exist. Use -o for non-unique UID. + `id -u ${username} &>/dev/null || useradd -o -u ${uid} -g ${gid} -d ${homeDir} -s /bin/bash ${username}`, + ].join(' && '); + + const originalCommand = finalEntrypoint[2]; + const escapedOriginalCommand = originalCommand.replace(/'/g, "'\\''"); + + // Use `su -p` to preserve the environment. + const suCommand = `su -p ${username} -c '${escapedOriginalCommand}'`; + + // The entrypoint is always `['bash', '-c', '']`, so we modify the command part. + finalEntrypoint[2] = `${setupUserCommands} && ${suCommand}`; + + // We still need userFlag for the simpler proxy container, which does not have this issue. + userFlag = `--user ${uid}:${gid}`; + // When forcing a UID in the sandbox, $HOME can be reset to '/', so we copy $HOME as well. + args.push('--env', `HOME=${os.homedir()}`); + } + + // push container image name + args.push(image); + + // push container entrypoint (including args) + args.push(...finalEntrypoint); + + // start and set up proxy if GEMINI_SANDBOX_PROXY_COMMAND is set + let proxyProcess: ChildProcess | undefined = undefined; + let sandboxProcess: ChildProcess | undefined = undefined; + + if (proxyCommand) { + // run proxyCommand in its own container + const proxyContainerCommand = `${config.command} run --rm --init ${userFlag} --name ${SANDBOX_PROXY_NAME} --network ${SANDBOX_PROXY_NAME} -p 8877:8877 -v ${process.cwd()}:${workdir} --workdir ${workdir} ${image} ${proxyCommand}`; + const isWindows = os.platform() === 'win32'; + const proxyShell = isWindows ? 'cmd.exe' : 'bash'; + const proxyShellArgs = isWindows + ? ['/c', proxyContainerCommand] + : ['-c', proxyContainerCommand]; + // Note: CodeQL flags this as js/shell-command-injection-from-environment. + // This is intentional - CLI tool executes user-provided proxy commands in container. + proxyProcess = spawn(proxyShell, proxyShellArgs, { + stdio: ['ignore', 'pipe', 'pipe'], + detached: true, + }); + // install handlers to stop proxy on exit/signal + const stopProxy = () => { + writeStdoutLine('stopping proxy container ...'); + execSync(`${config.command} rm -f ${SANDBOX_PROXY_NAME}`); + }; + process.on('exit', stopProxy); + process.on('SIGINT', stopProxy); + process.on('SIGTERM', stopProxy); + + // commented out as it disrupts ink rendering + // proxyProcess.stdout?.on('data', (data) => { + // console.info(data.toString()); + // }); + proxyProcess.stderr?.on('data', (data) => { + writeStderrLine(data.toString().trim()); + }); + proxyProcess.on('close', (code, signal) => { + if (sandboxProcess?.pid) { + process.kill(-sandboxProcess.pid, 'SIGTERM'); + } + throw new FatalSandboxError( + `Proxy container command '${proxyContainerCommand}' exited with code ${code}, signal ${signal}`, + ); + }); + writeStdoutLine('waiting for proxy to start ...'); + await execAsync( + `until timeout 0.25 curl -s http://localhost:8877; do sleep 0.25; done`, + ); + // connect proxy container to sandbox network + // (workaround for older versions of docker that don't support multiple --network args) + await execAsync( + `${config.command} network connect ${SANDBOX_NETWORK_NAME} ${SANDBOX_PROXY_NAME}`, + ); + } + + // spawn child and let it inherit stdio + process.stdin.pause(); + sandboxProcess = spawn(config.command, args, { + stdio: 'inherit', + }); + + return new Promise((resolve, reject) => { + sandboxProcess.on('error', (err) => { + writeStderrLine(`Sandbox process error: ${err}`); + reject(err); + }); + + sandboxProcess?.on('close', (code, signal) => { + process.stdin.resume(); + if (code !== 0 && code !== null) { + writeStderrLine( + `Sandbox process exited with code: ${code}, signal: ${signal}`, + ); + } + resolve(code ?? 1); + }); + }); } // Helper functions to ensure sandbox image is present diff --git a/packages/cli/src/validateNonInterActiveAuth.test.ts b/packages/cli/src/validateNonInterActiveAuth.test.ts index 11dd3289f..2df4156ca 100644 --- a/packages/cli/src/validateNonInterActiveAuth.test.ts +++ b/packages/cli/src/validateNonInterActiveAuth.test.ts @@ -14,6 +14,14 @@ import * as JsonOutputAdapterModule from './nonInteractive/io/JsonOutputAdapter. import * as StreamJsonOutputAdapterModule from './nonInteractive/io/StreamJsonOutputAdapter.js'; import * as cleanupModule from './utils/cleanup.js'; +const mockWriteStderrLine = vi.hoisted(() => vi.fn()); + +vi.mock('./utils/stdioHelpers.js', () => ({ + writeStderrLine: mockWriteStderrLine, + writeStdoutLine: vi.fn(), + clearScreen: vi.fn(), +})); + type ModelsConfig = ReturnType; // Helper to create a mock Config with modelsConfig @@ -42,7 +50,6 @@ describe('validateNonInterActiveAuth', () => { let originalEnvQwenOauth: string | undefined; let originalEnvGoogleApiKey: string | undefined; let originalEnvAnthropicApiKey: string | undefined; - let consoleErrorSpy: ReturnType; let processExitSpy: ReturnType>; let refreshAuthMock: ReturnType; let mockSettings: LoadedSettings; @@ -62,7 +69,7 @@ describe('validateNonInterActiveAuth', () => { delete process.env['QWEN_OAUTH']; delete process.env['GOOGLE_API_KEY']; delete process.env['ANTHROPIC_API_KEY']; - consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + mockWriteStderrLine.mockClear(); processExitSpy = vi.spyOn(process, 'exit').mockImplementation((code) => { throw new Error(`process.exit(${code}) called`); }) as ReturnType>; @@ -149,7 +156,7 @@ describe('validateNonInterActiveAuth', () => { } catch (e) { expect((e as Error).message).toContain('process.exit(1) called'); } - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( expect.stringContaining('Missing API key'), ); expect(processExitSpy).toHaveBeenCalledWith(1); @@ -204,7 +211,7 @@ describe('validateNonInterActiveAuth', () => { } catch (e) { expect((e as Error).message).toContain('process.exit(1) called'); } - expect(consoleErrorSpy).toHaveBeenCalledWith('Auth error!'); + expect(mockWriteStderrLine).toHaveBeenCalledWith('Auth error!'); expect(processExitSpy).toHaveBeenCalledWith(1); }); @@ -226,7 +233,7 @@ describe('validateNonInterActiveAuth', () => { ); expect(validateAuthMethodSpy).not.toHaveBeenCalled(); - expect(consoleErrorSpy).not.toHaveBeenCalled(); + expect(mockWriteStderrLine).not.toHaveBeenCalled(); expect(processExitSpy).not.toHaveBeenCalled(); // refreshAuth is called with the authType from config.getModelsConfig().getCurrentAuthType() expect(refreshAuthMock).toHaveBeenCalledWith(AuthType.QWEN_OAUTH); @@ -272,7 +279,7 @@ describe('validateNonInterActiveAuth', () => { } catch (e) { expect((e as Error).message).toContain('process.exit(1) called'); } - expect(consoleErrorSpy).toHaveBeenCalledWith( + expect(mockWriteStderrLine).toHaveBeenCalledWith( 'The configured auth type is qwen-oauth, but the current auth type is openai. Please re-authenticate with the correct type.', ); expect(processExitSpy).toHaveBeenCalledWith(1); @@ -330,7 +337,7 @@ describe('validateNonInterActiveAuth', () => { }); expect(runExitCleanupMock).toHaveBeenCalled(); expect(processExitSpy).toHaveBeenCalledWith(1); - expect(consoleErrorSpy).not.toHaveBeenCalled(); + expect(mockWriteStderrLine).not.toHaveBeenCalled(); }); it('emits error result and exits when enforced auth mismatches current auth', async () => { @@ -369,7 +376,7 @@ describe('validateNonInterActiveAuth', () => { }); expect(runExitCleanupMock).toHaveBeenCalled(); expect(processExitSpy).toHaveBeenCalledWith(1); - expect(consoleErrorSpy).not.toHaveBeenCalled(); + expect(mockWriteStderrLine).not.toHaveBeenCalled(); }); it('emits error result and exits when API key validation fails', async () => { @@ -406,7 +413,7 @@ describe('validateNonInterActiveAuth', () => { }); expect(runExitCleanupMock).toHaveBeenCalled(); expect(processExitSpy).toHaveBeenCalledWith(1); - expect(consoleErrorSpy).not.toHaveBeenCalled(); + expect(mockWriteStderrLine).not.toHaveBeenCalled(); }); }); @@ -466,7 +473,7 @@ describe('validateNonInterActiveAuth', () => { }); expect(runExitCleanupMock).toHaveBeenCalled(); expect(processExitSpy).toHaveBeenCalledWith(1); - expect(consoleErrorSpy).not.toHaveBeenCalled(); + expect(mockWriteStderrLine).not.toHaveBeenCalled(); }); it('emits error result and exits when enforced auth mismatches current auth', async () => { @@ -506,7 +513,7 @@ describe('validateNonInterActiveAuth', () => { }); expect(runExitCleanupMock).toHaveBeenCalled(); expect(processExitSpy).toHaveBeenCalledWith(1); - expect(consoleErrorSpy).not.toHaveBeenCalled(); + expect(mockWriteStderrLine).not.toHaveBeenCalled(); }); it('emits error result and exits when API key validation fails', async () => { @@ -544,7 +551,7 @@ describe('validateNonInterActiveAuth', () => { }); expect(runExitCleanupMock).toHaveBeenCalled(); expect(processExitSpy).toHaveBeenCalledWith(1); - expect(consoleErrorSpy).not.toHaveBeenCalled(); + expect(mockWriteStderrLine).not.toHaveBeenCalled(); }); }); }); diff --git a/packages/core/src/core/logger.test.ts b/packages/core/src/core/logger.test.ts index 07b8a9e8a..de3fc3f78 100644 --- a/packages/core/src/core/logger.test.ts +++ b/packages/core/src/core/logger.test.ts @@ -35,22 +35,23 @@ const CHECKPOINT_FILE_NAME = 'checkpoint.json'; const projectDir = process.cwd(); const hash = crypto.createHash('sha256').update(projectDir).digest('hex'); -const TEST_GEMINI_DIR = path.join( - os.homedir(), - GEMINI_DIR_NAME, - TMP_DIR_NAME, - hash, -); +const TEST_HOME_DIR = path.join(os.tmpdir(), 'qwen-core-logger-home'); -const TEST_LOG_FILE_PATH = path.join(TEST_GEMINI_DIR, LOG_FILE_NAME); -const TEST_CHECKPOINT_FILE_PATH = path.join( - TEST_GEMINI_DIR, - CHECKPOINT_FILE_NAME, -); +let originalHome: string | undefined; +let testGeminiDir: string; +let testLogFilePath: string; +let testCheckpointFilePath: string; + +const setTestPaths = () => { + testGeminiDir = path.join(os.homedir(), GEMINI_DIR_NAME, TMP_DIR_NAME, hash); + testLogFilePath = path.join(testGeminiDir, LOG_FILE_NAME); + testCheckpointFilePath = path.join(testGeminiDir, CHECKPOINT_FILE_NAME); +}; async function cleanupLogAndCheckpointFiles() { try { - await fs.rm(TEST_GEMINI_DIR, { recursive: true, force: true }); + if (!testGeminiDir) return; + await fs.rm(testGeminiDir, { recursive: true, force: true }); } catch (_error) { // Ignore errors, as the directory may not exist, which is fine. } @@ -58,7 +59,7 @@ async function cleanupLogAndCheckpointFiles() { async function readLogFile(): Promise { try { - const content = await fs.readFile(TEST_LOG_FILE_PATH, 'utf-8'); + const content = await fs.readFile(testLogFilePath, 'utf-8'); return JSON.parse(content) as LogEntry[]; } catch (error) { if ((error as NodeJS.ErrnoException).code === 'ENOENT') { @@ -94,10 +95,13 @@ describe('Logger', () => { vi.resetAllMocks(); vi.useFakeTimers(); vi.setSystemTime(new Date('2025-01-01T12:00:00.000Z')); + originalHome = process.env['HOME']; + process.env['HOME'] = TEST_HOME_DIR; + setTestPaths(); // Clean up before the test await cleanupLogAndCheckpointFiles(); // Ensure the directory exists for the test - await fs.mkdir(TEST_GEMINI_DIR, { recursive: true }); + await fs.mkdir(testGeminiDir, { recursive: true }); logger = new Logger(testSessionId, new Storage(process.cwd())); await logger.initialize(); }); @@ -110,6 +114,11 @@ describe('Logger', () => { await cleanupLogAndCheckpointFiles(); vi.useRealTimers(); vi.restoreAllMocks(); + if (originalHome === undefined) { + delete process.env['HOME']; + } else { + process.env['HOME'] = originalHome; + } }); afterAll(async () => { @@ -120,13 +129,13 @@ describe('Logger', () => { describe('initialize', () => { it('should create .gemini directory and an empty log file if none exist', async () => { const dirExists = await fs - .access(TEST_GEMINI_DIR) + .access(testGeminiDir) .then(() => true) .catch(() => false); expect(dirExists).toBe(true); const fileExists = await fs - .access(TEST_LOG_FILE_PATH) + .access(testLogFilePath) .then(() => true) .catch(() => false); expect(fileExists).toBe(true); @@ -162,7 +171,7 @@ describe('Logger', () => { }, ]; await fs.writeFile( - TEST_LOG_FILE_PATH, + testLogFilePath, JSON.stringify(existingLogs, null, 2), ); const newLogger = new Logger( @@ -186,7 +195,7 @@ describe('Logger', () => { }, ]; await fs.writeFile( - TEST_LOG_FILE_PATH, + testLogFilePath, JSON.stringify(existingLogs, null, 2), ); const newLogger = new Logger('a-new-session', new Storage(process.cwd())); @@ -209,21 +218,14 @@ describe('Logger', () => { }); it('should handle invalid JSON in log file by backing it up and starting fresh', async () => { - await fs.writeFile(TEST_LOG_FILE_PATH, 'invalid json'); - const consoleDebugSpy = vi - .spyOn(console, 'debug') - .mockImplementation(() => {}); + await fs.writeFile(testLogFilePath, 'invalid json'); const newLogger = new Logger(testSessionId, new Storage(process.cwd())); await newLogger.initialize(); - expect(consoleDebugSpy).toHaveBeenCalledWith( - expect.stringContaining('Invalid JSON in log file'), - expect.any(SyntaxError), - ); const logContent = await readLogFile(); expect(logContent).toEqual([]); - const dirContents = await fs.readdir(TEST_GEMINI_DIR); + const dirContents = await fs.readdir(testGeminiDir); expect( dirContents.some( (f) => @@ -234,23 +236,14 @@ describe('Logger', () => { }); it('should handle non-array JSON in log file by backing it up and starting fresh', async () => { - await fs.writeFile( - TEST_LOG_FILE_PATH, - JSON.stringify({ not: 'an array' }), - ); - const consoleDebugSpy = vi - .spyOn(console, 'debug') - .mockImplementation(() => {}); + await fs.writeFile(testLogFilePath, JSON.stringify({ not: 'an array' })); const newLogger = new Logger(testSessionId, new Storage(process.cwd())); await newLogger.initialize(); - expect(consoleDebugSpy).toHaveBeenCalledWith( - `Log file at ${TEST_LOG_FILE_PATH} is not a valid JSON array. Starting with empty logs.`, - ); const logContent = await readLogFile(); expect(logContent).toEqual([]); - const dirContents = await fs.readdir(TEST_GEMINI_DIR); + const dirContents = await fs.readdir(testGeminiDir); expect( dirContents.some( (f) => @@ -297,13 +290,7 @@ describe('Logger', () => { new Storage(process.cwd()), ); uninitializedLogger.close(); // Ensure it's treated as uninitialized - const consoleDebugSpy = vi - .spyOn(console, 'debug') - .mockImplementation(() => {}); await uninitializedLogger.logMessage(MessageSenderType.USER, 'test'); - expect(consoleDebugSpy).toHaveBeenCalledWith( - 'Logger not initialized or session ID missing. Cannot log message.', - ); expect((await readLogFile()).length).toBe(0); uninitializedLogger.close(); }); @@ -353,18 +340,11 @@ describe('Logger', () => { it('should not throw, not increment messageId, and log error if writing to file fails', async () => { vi.spyOn(fs, 'writeFile').mockRejectedValueOnce(new Error('Disk full')); - const consoleDebugSpy = vi - .spyOn(console, 'debug') - .mockImplementation(() => {}); const initialMessageId = logger['messageId']; const initialLogCount = logger['logs'].length; await logger.logMessage(MessageSenderType.USER, 'test fail write'); - expect(consoleDebugSpy).toHaveBeenCalledWith( - 'Error writing to log file:', - expect.any(Error), - ); expect(logger['messageId']).toBe(initialMessageId); // Not incremented expect(logger['logs'].length).toBe(initialLogCount); // Log not added to in-memory cache }); @@ -453,7 +433,7 @@ describe('Logger', () => { ])('should save a checkpoint', async ({ tag, encodedTag }) => { await logger.saveCheckpoint(conversation, tag); const taggedFilePath = path.join( - TEST_GEMINI_DIR, + testGeminiDir, `checkpoint-${encodedTag}.json`, ); const fileContent = await fs.readFile(taggedFilePath, 'utf-8'); @@ -466,16 +446,10 @@ describe('Logger', () => { new Storage(process.cwd()), ); uninitializedLogger.close(); - const consoleErrorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); await expect( uninitializedLogger.saveCheckpoint(conversation, 'tag'), ).resolves.not.toThrow(); - expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Logger not initialized or checkpoint file path not set. Cannot save a checkpoint.', - ); }); }); @@ -487,7 +461,7 @@ describe('Logger', () => { beforeEach(async () => { await fs.writeFile( - TEST_CHECKPOINT_FILE_PATH, + testCheckpointFilePath, JSON.stringify(conversation, null, 2), ); }); @@ -516,7 +490,7 @@ describe('Logger', () => { { role: 'user', parts: [{ text: 'hello' }] }, ]; const taggedFilePath = path.join( - TEST_GEMINI_DIR, + testGeminiDir, `checkpoint-${encodedTag}.json`, ); await fs.writeFile( @@ -536,7 +510,7 @@ describe('Logger', () => { }); it('should return an empty array if the checkpoint file does not exist', async () => { - await fs.unlink(TEST_CHECKPOINT_FILE_PATH); // Ensure it's gone + await fs.unlink(testCheckpointFilePath); // Ensure it's gone const loaded = await logger.loadCheckpoint('missing'); expect(loaded).toEqual([]); }); @@ -545,19 +519,12 @@ describe('Logger', () => { const tag = 'invalid-json-tag'; const encodedTag = 'invalid-json-tag'; const taggedFilePath = path.join( - TEST_GEMINI_DIR, + testGeminiDir, `checkpoint-${encodedTag}.json`, ); await fs.writeFile(taggedFilePath, 'invalid json'); - const consoleErrorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); const loadedCheckpoint = await logger.loadCheckpoint(tag); expect(loadedCheckpoint).toEqual([]); - expect(consoleErrorSpy).toHaveBeenCalledWith( - expect.stringContaining('Failed to read or parse checkpoint file'), - expect.any(Error), - ); }); it('should return an empty array if logger is not initialized', async () => { @@ -566,14 +533,8 @@ describe('Logger', () => { new Storage(process.cwd()), ); uninitializedLogger.close(); - const consoleErrorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); const loadedCheckpoint = await uninitializedLogger.loadCheckpoint('tag'); expect(loadedCheckpoint).toEqual([]); - expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Logger not initialized or checkpoint file path not set. Cannot load checkpoint.', - ); }); }); @@ -587,7 +548,7 @@ describe('Logger', () => { beforeEach(async () => { taggedFilePath = path.join( - TEST_GEMINI_DIR, + testGeminiDir, `checkpoint-${encodedTag}.json`, ); // Create a file to be deleted @@ -605,7 +566,7 @@ describe('Logger', () => { it('should delete both new and old checkpoint files if they exist', async () => { const oldTag = 'delete-me(old)'; const oldStylePath = path.join( - TEST_GEMINI_DIR, + testGeminiDir, `checkpoint-${oldTag}.json`, ); const newStylePath = logger['_checkpointPath'](oldTag); @@ -638,17 +599,10 @@ describe('Logger', () => { code: 'EACCES', }), ); - const consoleErrorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); await expect(logger.deleteCheckpoint(tag)).rejects.toThrow( 'EACCES: permission denied', ); - expect(consoleErrorSpy).toHaveBeenCalledWith( - `Failed to delete checkpoint file ${taggedFilePath}:`, - expect.any(Error), - ); }); it('should return false if logger is not initialized', async () => { @@ -657,15 +611,9 @@ describe('Logger', () => { new Storage(process.cwd()), ); uninitializedLogger.close(); - const consoleErrorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); const result = await uninitializedLogger.deleteCheckpoint(tag); expect(result).toBe(false); - expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Logger not initialized or checkpoint file path not set. Cannot delete checkpoint.', - ); }); }); @@ -676,7 +624,7 @@ describe('Logger', () => { beforeEach(() => { taggedFilePath = path.join( - TEST_GEMINI_DIR, + testGeminiDir, `checkpoint-${encodedTag}.json`, ); }); @@ -710,17 +658,10 @@ describe('Logger', () => { code: 'EACCES', }), ); - const consoleErrorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); await expect(logger.checkpointExists(tag)).rejects.toThrow( 'EACCES: permission denied', ); - expect(consoleErrorSpy).toHaveBeenCalledWith( - `Failed to check checkpoint existence for path for tag "${tag}":`, - expect.any(Error), - ); }); }); @@ -735,10 +676,7 @@ describe('Logger', () => { { role: 'user', parts: [{ text: 'hello' }] }, ]; const tag = 'special(char)'; - const taggedFilePath = path.join( - TEST_GEMINI_DIR, - `checkpoint-${tag}.json`, - ); + const taggedFilePath = path.join(testGeminiDir, `checkpoint-${tag}.json`); await fs.writeFile( taggedFilePath, JSON.stringify(taggedConversation, null, 2), @@ -753,13 +691,7 @@ describe('Logger', () => { it('should reset logger state', async () => { await logger.logMessage(MessageSenderType.USER, 'A message'); logger.close(); - const consoleDebugSpy = vi - .spyOn(console, 'debug') - .mockImplementation(() => {}); await logger.logMessage(MessageSenderType.USER, 'Another message'); - expect(consoleDebugSpy).toHaveBeenCalledWith( - 'Logger not initialized or session ID missing. Cannot log message.', - ); const messages = await logger.getPreviousUserMessages(); expect(messages).toEqual([]); expect(logger['initialized']).toBe(false); diff --git a/packages/core/src/core/openaiContentGenerator/errorHandler.test.ts b/packages/core/src/core/openaiContentGenerator/errorHandler.test.ts index e124d92a2..a75b43ca8 100644 --- a/packages/core/src/core/openaiContentGenerator/errorHandler.test.ts +++ b/packages/core/src/core/openaiContentGenerator/errorHandler.test.ts @@ -11,13 +11,10 @@ import type { RequestContext } from './errorHandler.js'; describe('EnhancedErrorHandler', () => { let errorHandler: EnhancedErrorHandler; - let mockConsoleError: ReturnType; let mockContext: RequestContext; let mockRequest: GenerateContentParameters; beforeEach(() => { - mockConsoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); - mockContext = { userPromptId: 'test-prompt-id', model: 'test-model', @@ -63,33 +60,6 @@ describe('EnhancedErrorHandler', () => { }).toThrow(originalError); }); - it('should log error message for non-timeout errors', () => { - const originalError = new Error('Test error'); - - expect(() => { - errorHandler.handle(originalError, mockContext, mockRequest); - }).toThrow(); - - expect(mockConsoleError).toHaveBeenCalledWith( - 'OpenAI API Error:', - 'Test error', - ); - }); - - it('should log streaming error message for streaming requests', () => { - const streamingContext = { ...mockContext, isStreaming: true }; - const originalError = new Error('Test streaming error'); - - expect(() => { - errorHandler.handle(originalError, streamingContext, mockRequest); - }).toThrow(); - - expect(mockConsoleError).toHaveBeenCalledWith( - 'OpenAI API Streaming Error:', - 'Test streaming error', - ); - }); - it('should throw enhanced error message for timeout errors', () => { const timeoutError = new Error('Request timeout'); @@ -98,7 +68,7 @@ describe('EnhancedErrorHandler', () => { }).toThrow(/Request timeout after 5s.*Troubleshooting tips:/s); }); - it('should not log error when suppression is enabled', () => { + it('should use custom suppression function', () => { const suppressLogging = vi.fn(() => true); errorHandler = new EnhancedErrorHandler(suppressLogging); const originalError = new Error('Test error'); @@ -107,7 +77,6 @@ describe('EnhancedErrorHandler', () => { errorHandler.handle(originalError, mockContext, mockRequest); }).toThrow(); - expect(mockConsoleError).not.toHaveBeenCalled(); expect(suppressLogging).toHaveBeenCalledWith(originalError, mockRequest); }); @@ -117,11 +86,6 @@ describe('EnhancedErrorHandler', () => { expect(() => { errorHandler.handle(stringError, mockContext, mockRequest); }).toThrow(stringError); - - expect(mockConsoleError).toHaveBeenCalledWith( - 'OpenAI API Error:', - 'String error message', - ); }); it('should handle null/undefined errors', () => { @@ -378,8 +342,6 @@ describe('EnhancedErrorHandler', () => { expect(() => { errorHandler.handle(emptyError, mockContext, mockRequest); }).toThrow(emptyError); - - expect(mockConsoleError).toHaveBeenCalledWith('OpenAI API Error:', ''); }); it('should handle error with only whitespace message', () => { diff --git a/packages/core/src/core/prompts.test.ts b/packages/core/src/core/prompts.test.ts index a232b50f7..bc1445d4d 100644 --- a/packages/core/src/core/prompts.test.ts +++ b/packages/core/src/core/prompts.test.ts @@ -603,7 +603,6 @@ describe('resolvePathFromEnv helper function', () => { vi.spyOn(os, 'homedir').mockImplementation(() => { throw new Error('Cannot resolve home directory'); }); - const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); const result = resolvePathFromEnv('~/documents/file.txt'); expect(result).toEqual({ @@ -611,12 +610,6 @@ describe('resolvePathFromEnv helper function', () => { value: null, isDisabled: false, }); - expect(consoleSpy).toHaveBeenCalledWith( - 'Could not resolve home directory for path: ~/documents/file.txt', - expect.any(Error), - ); - - consoleSpy.mockRestore(); }); }); }); diff --git a/packages/core/src/models/modelRegistry.test.ts b/packages/core/src/models/modelRegistry.test.ts index 7574ae5d8..3a5993603 100644 --- a/packages/core/src/models/modelRegistry.test.ts +++ b/packages/core/src/models/modelRegistry.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { describe, it, expect, beforeEach } from 'vitest'; import { ModelRegistry, QWEN_OAUTH_MODELS } from './modelRegistry.js'; import { AuthType } from '../core/contentGenerator.js'; import type { ModelProvidersConfig } from './types.js'; @@ -270,41 +270,21 @@ describe('ModelRegistry', () => { expect(geminiModels[0].id).toBe('gemini-pro'); }); - it('should skip invalid authType keys with warning', () => { - const consoleWarnSpy = vi - .spyOn(console, 'warn') - .mockImplementation(() => undefined); - + it('should skip invalid authType keys', () => { const registry = new ModelRegistry({ openai: [{ id: 'gpt-4', name: 'GPT-4' }], 'invalid-key': [{ id: 'some-model', name: 'Some Model' }], } as unknown as ModelProvidersConfig); - expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining('[ModelRegistry] Invalid authType key'), - ); - expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining('invalid-key'), - ); - expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining('Expected one of:'), - ); - // Valid key should be registered expect(registry.getModelsForAuthType(AuthType.USE_OPENAI).length).toBe(1); // Invalid key should be skipped (no crash) const openaiModels = registry.getModelsForAuthType(AuthType.USE_OPENAI); expect(openaiModels.length).toBe(1); - - consoleWarnSpy.mockRestore(); }); it('should handle mixed valid and invalid keys', () => { - const consoleWarnSpy = vi - .spyOn(console, 'warn') - .mockImplementation(() => undefined); - const registry = new ModelRegistry({ openai: [{ id: 'gpt-4', name: 'GPT-4' }], 'bad-key-1': [{ id: 'model-1', name: 'Model 1' }], @@ -312,15 +292,6 @@ describe('ModelRegistry', () => { 'bad-key-2': [{ id: 'model-2', name: 'Model 2' }], } as unknown as ModelProvidersConfig); - // Should warn twice for the two invalid keys - expect(consoleWarnSpy).toHaveBeenCalledTimes(2); - expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining('bad-key-1'), - ); - expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining('bad-key-2'), - ); - // Valid keys should be registered expect(registry.getModelsForAuthType(AuthType.USE_OPENAI).length).toBe(1); expect(registry.getModelsForAuthType(AuthType.USE_GEMINI).length).toBe(1); @@ -331,43 +302,9 @@ describe('ModelRegistry', () => { const geminiModels = registry.getModelsForAuthType(AuthType.USE_GEMINI); expect(geminiModels.length).toBe(1); - - consoleWarnSpy.mockRestore(); - }); - - it('should list all valid AuthType values in warning message', () => { - const consoleWarnSpy = vi - .spyOn(console, 'warn') - .mockImplementation(() => undefined); - - new ModelRegistry({ - 'invalid-auth': [{ id: 'model', name: 'Model' }], - } as unknown as ModelProvidersConfig); - - expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining('openai'), - ); - expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining('qwen-oauth'), - ); - expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining('gemini'), - ); - expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining('vertex-ai'), - ); - expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining('anthropic'), - ); - - consoleWarnSpy.mockRestore(); }); it('should work correctly with getModelsForAuthType after validation', () => { - const consoleWarnSpy = vi - .spyOn(console, 'warn') - .mockImplementation(() => undefined); - const registry = new ModelRegistry({ openai: [ { id: 'gpt-4', name: 'GPT-4' }, @@ -381,8 +318,6 @@ describe('ModelRegistry', () => { expect(models.find((m) => m.id === 'gpt-4')).toBeDefined(); expect(models.find((m) => m.id === 'gpt-3.5')).toBeDefined(); expect(models.find((m) => m.id === 'invalid-model')).toBeUndefined(); - - consoleWarnSpy.mockRestore(); }); }); }); diff --git a/packages/core/src/qwen/qwenOAuth2.ts b/packages/core/src/qwen/qwenOAuth2.ts index 3427492de..4dfe7ff53 100644 --- a/packages/core/src/qwen/qwenOAuth2.ts +++ b/packages/core/src/qwen/qwenOAuth2.ts @@ -567,10 +567,10 @@ export async function getQwenOAuthClient( /** * Displays a formatted box with OAuth device authorization URL. - * Uses process.stderr.write() to bypass ConsolePatcher and ensure the auth URL - * is always visible to users, especially in non-interactive mode. - * Using stderr prevents corruption of structured JSON output (which goes to stdout) - * and follows the standard Unix convention of user-facing messages to stderr. + * Uses process.stderr.write() to ensure the auth URL is always visible to users, + * especially in non-interactive mode. Using stderr prevents corruption of + * structured JSON output (which goes to stdout) and follows the standard Unix + * convention of user-facing messages to stderr. */ function showFallbackMessage(verificationUriComplete: string): void { const title = 'Qwen OAuth Device Authorization'; diff --git a/packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts b/packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts index 0f80e87ac..784a9d40a 100644 --- a/packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts +++ b/packages/core/src/telemetry/qwen-logger/qwen-logger.test.ts @@ -26,12 +26,12 @@ import { } from '../types.js'; import type { RumEvent, RumPayload } from './event-types.js'; -const debugLoggerSpy = { +const debugLoggerSpy = vi.hoisted(() => ({ debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn(), -}; +})); // Mock dependencies vi.mock('../../utils/user_id.js', () => ({ @@ -47,7 +47,12 @@ vi.mock('../../utils/debugLogger.js', async (importOriginal) => { await importOriginal(); return { ...original, - createDebugLogger: () => debugLoggerSpy, + createDebugLogger: () => ({ + debug: debugLoggerSpy.debug, + info: debugLoggerSpy.info, + warn: debugLoggerSpy.warn, + error: debugLoggerSpy.error, + }), }; }); @@ -158,11 +163,11 @@ describe('QwenLogger', () => { }); } - // Should have logged debug messages about dropping events - expect(debugLoggerSpy.debug).toHaveBeenCalledWith( - expect.stringContaining( - 'QwenLogger: Dropped old event to prevent memory leak', - ), + const events = logger['events'].toArray() as RumEvent[]; + expect(logger['events'].size).toBe(TEST_ONLY.MAX_EVENTS); + expect(events[0]?.name).toBe('test-event-10'); + expect(events[events.length - 1]?.name).toBe( + `test-event-${TEST_ONLY.MAX_EVENTS + 9}`, ); }); @@ -182,10 +187,7 @@ describe('QwenLogger', () => { name: 'test-event', }); - expect(debugLoggerSpy.error).toHaveBeenCalledWith( - 'QwenLogger: Failed to enqueue log event.', - expect.any(Error), - ); + expect(logger['events'].size).toBe(0); // Restore original method logger['events'].push = originalPush; @@ -202,12 +204,7 @@ describe('QwenLogger', () => { // Try to flush while another flush is in progress const result = logger.flushToRum(); - // Should have logged about pending flush - expect(debugLoggerSpy.debug).toHaveBeenCalledWith( - expect.stringContaining( - 'QwenLogger: Flush already in progress, marking pending flush', - ), - ); + expect(logger['pendingFlush']).toBe(true); // Should return a resolved promise expect(result).toBeInstanceOf(Promise); @@ -235,10 +232,7 @@ describe('QwenLogger', () => { // eslint-disable-next-line @typescript-eslint/no-explicit-any (logger as any).requeueFailedEvents(failedEvents); - // Should have logged about dropping events due to retry limit - expect(debugLoggerSpy.debug).toHaveBeenCalledWith( - expect.stringContaining('QwenLogger: Re-queued'), - ); + expect(logger['events'].size).toBe(TEST_ONLY.MAX_RETRY_EVENTS); }); it('should handle empty retry queue gracefully', () => { @@ -267,9 +261,7 @@ describe('QwenLogger', () => { // eslint-disable-next-line @typescript-eslint/no-explicit-any (logger as any).requeueFailedEvents(failedEvents); - expect(debugLoggerSpy.debug).toHaveBeenCalledWith( - expect.stringContaining('QwenLogger: No events re-queued'), - ); + expect(logger['events'].size).toBe(TEST_ONLY.MAX_EVENTS); }); }); diff --git a/packages/core/src/tools/mcp-client.test.ts b/packages/core/src/tools/mcp-client.test.ts index fd9f9d503..37dc94b1f 100644 --- a/packages/core/src/tools/mcp-client.test.ts +++ b/packages/core/src/tools/mcp-client.test.ts @@ -78,9 +78,6 @@ describe('mcp-client', () => { }); it('should not skip tools even if a parameter is missing a type', async () => { - const consoleWarnSpy = vi - .spyOn(console, 'warn') - .mockImplementation(() => {}); const mockedClient = { connect: vi.fn(), discover: vi.fn(), @@ -137,14 +134,9 @@ describe('mcp-client', () => { await client.connect(); await client.discover({} as Config); expect(mockedToolRegistry.registerTool).toHaveBeenCalledTimes(2); - expect(consoleWarnSpy).not.toHaveBeenCalled(); - consoleWarnSpy.mockRestore(); }); it('should handle errors when discovering prompts', async () => { - const consoleErrorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); const mockedClient = { connect: vi.fn(), discover: vi.fn(), @@ -178,10 +170,6 @@ describe('mcp-client', () => { await expect(client.discover({} as Config)).rejects.toThrow( 'No prompts or tools found on the server.', ); - expect(consoleErrorSpy).toHaveBeenCalledWith( - `Error discovering prompts from test-server: Test error`, - ); - consoleErrorSpy.mockRestore(); }); }); describe('appendMcpServerCommand', () => { diff --git a/packages/core/src/utils/editor.test.ts b/packages/core/src/utils/editor.test.ts index dd3202b8f..a2ac74680 100644 --- a/packages/core/src/utils/editor.test.ts +++ b/packages/core/src/utils/editor.test.ts @@ -393,15 +393,10 @@ describe('editor utils', () => { }); } - it('should log an error if diff command is not available', async () => { - const consoleErrorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); + it('should handle unsupported editor gracefully', async () => { // @ts-expect-error Testing unsupported editor await openDiff('old.txt', 'new.txt', 'foobar', () => {}); - expect(consoleErrorSpy).toHaveBeenCalledWith( - 'No diff tool available. Install a supported editor.', - ); + // Function should complete without throwing (logs error to debugLogger) }); describe('onEditorClose callback', () => { diff --git a/packages/core/src/utils/errorReporting.test.ts b/packages/core/src/utils/errorReporting.test.ts index 3a9f92baa..585a0b976 100644 --- a/packages/core/src/utils/errorReporting.test.ts +++ b/packages/core/src/utils/errorReporting.test.ts @@ -5,58 +5,48 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import fs from 'node:fs/promises'; -import os from 'node:os'; -import path from 'node:path'; import { reportError } from './errorReporting.js'; -// Use a type alias for SpyInstance as it's not directly exported -type SpyInstance = ReturnType; +const debugLoggerSpy = vi.hoisted(() => ({ + error: vi.fn(), + warn: vi.fn(), + info: vi.fn(), + debug: vi.fn(), +})); + +// Mock the debugLogger +vi.mock('./debugLogger.js', () => ({ + createDebugLogger: () => ({ + error: debugLoggerSpy.error, + warn: debugLoggerSpy.warn, + info: debugLoggerSpy.info, + debug: debugLoggerSpy.debug, + }), +})); describe('reportError', () => { - let consoleErrorSpy: SpyInstance; - let testDir: string; - const MOCK_TIMESTAMP = '2025-01-01T00-00-00-000Z'; - - beforeEach(async () => { - // Create a temporary directory for logs - testDir = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-report-test-')); + beforeEach(() => { vi.resetAllMocks(); - consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - vi.spyOn(Date.prototype, 'toISOString').mockReturnValue(MOCK_TIMESTAMP); }); - afterEach(async () => { + afterEach(() => { vi.restoreAllMocks(); - // Clean up the temporary directory - await fs.rm(testDir, { recursive: true, force: true }); }); - const getExpectedReportPath = (type: string) => - path.join(testDir, `qwen-client-error-${type}-${MOCK_TIMESTAMP}.json`); - - it('should generate a report and log the path', async () => { + it('should not throw when called with a standard error', async () => { const error = new Error('Test error'); error.stack = 'Test stack'; const baseMessage = 'An error occurred.'; const context = { data: 'test context' }; const type = 'test-type'; - const expectedReportPath = getExpectedReportPath(type); - await reportError(error, baseMessage, context, type, testDir); - - // Verify the file was written - const reportContent = await fs.readFile(expectedReportPath, 'utf-8'); - const parsedReport = JSON.parse(reportContent); - - expect(parsedReport).toEqual({ - error: { message: 'Test error', stack: 'Test stack' }, - context, - }); - - // Verify the console log - expect(consoleErrorSpy).toHaveBeenCalledWith( - `${baseMessage} Full report available at: ${expectedReportPath}`, + await expect( + reportError(error, baseMessage, context, type), + ).resolves.not.toThrow(); + expect(debugLoggerSpy.error).toHaveBeenCalled(); + expect(debugLoggerSpy.error).toHaveBeenCalledWith( + `${baseMessage} [${type}]`, + expect.any(String), ); }); @@ -64,19 +54,13 @@ describe('reportError', () => { const error = { message: 'Test plain object error' }; const baseMessage = 'Another error.'; const type = 'general'; - const expectedReportPath = getExpectedReportPath(type); - await reportError(error, baseMessage, undefined, type, testDir); - - const reportContent = await fs.readFile(expectedReportPath, 'utf-8'); - const parsedReport = JSON.parse(reportContent); - - expect(parsedReport).toEqual({ - error: { message: 'Test plain object error' }, - }); - - expect(consoleErrorSpy).toHaveBeenCalledWith( - `${baseMessage} Full report available at: ${expectedReportPath}`, + await expect( + reportError(error, baseMessage, undefined, type), + ).resolves.not.toThrow(); + expect(debugLoggerSpy.error).toHaveBeenCalledWith( + `${baseMessage} [${type}]`, + expect.any(String), ); }); @@ -84,52 +68,21 @@ describe('reportError', () => { const error = 'Just a string error'; const baseMessage = 'String error occurred.'; const type = 'general'; - const expectedReportPath = getExpectedReportPath(type); - await reportError(error, baseMessage, undefined, type, testDir); - - const reportContent = await fs.readFile(expectedReportPath, 'utf-8'); - const parsedReport = JSON.parse(reportContent); - - expect(parsedReport).toEqual({ - error: { message: 'Just a string error' }, - }); - - expect(consoleErrorSpy).toHaveBeenCalledWith( - `${baseMessage} Full report available at: ${expectedReportPath}`, + await expect( + reportError(error, baseMessage, undefined, type), + ).resolves.not.toThrow(); + expect(debugLoggerSpy.error).toHaveBeenCalledWith( + `${baseMessage} [${type}]`, + expect.any(String), ); }); - it('should log fallback message if writing report fails', async () => { - const error = new Error('Main error'); - const baseMessage = 'Failed operation.'; - const context = ['some context']; - const type = 'general'; - const nonExistentDir = path.join(testDir, 'non-existent-dir'); - - await reportError(error, baseMessage, context, type, nonExistentDir); - - expect(consoleErrorSpy).toHaveBeenCalledWith( - `${baseMessage} Additionally, failed to write detailed error report:`, - expect.any(Error), // The actual write error - ); - expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Original error that triggered report generation:', - error, - ); - expect(consoleErrorSpy).toHaveBeenCalledWith('Original context:', context); - }); - it('should handle stringification failure of report content (e.g. BigInt in context)', async () => { const error = new Error('Main error'); error.stack = 'Main stack'; const baseMessage = 'Failed operation with BigInt.'; const context = { a: BigInt(1) }; // BigInt cannot be stringified by JSON.stringify - const type = 'bigint-fail'; - const stringifyError = new TypeError( - 'Do not know how to serialize a BigInt', - ); - const expectedMinimalReportPath = getExpectedReportPath(type); // Simulate JSON.stringify throwing an error for the full report const originalJsonStringify = JSON.stringify; @@ -138,35 +91,23 @@ describe('reportError', () => { callCount++; if (callCount === 1) { // First call is for the full report content - throw stringifyError; + throw new TypeError('Do not know how to serialize a BigInt'); } // Subsequent calls (for minimal report) should succeed return originalJsonStringify(value, replacer, space); }); - await reportError(error, baseMessage, context, type, testDir); - - expect(consoleErrorSpy).toHaveBeenCalledWith( - `${baseMessage} Could not stringify report content (likely due to context):`, - stringifyError, - ); - expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Original error that triggered report generation:', + await expect( + reportError(error, baseMessage, context, 'bigint-fail'), + ).resolves.not.toThrow(); + expect(debugLoggerSpy.error).toHaveBeenCalledWith( + `${baseMessage} [bigint-fail] Could not stringify report content (likely due to context):`, + expect.any(TypeError), error, ); - expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Original context could not be stringified or included in report.', - ); - - // Check that it writes a minimal report - const reportContent = await fs.readFile(expectedMinimalReportPath, 'utf-8'); - const parsedReport = JSON.parse(reportContent); - expect(parsedReport).toEqual({ - error: { message: error.message, stack: error.stack }, - }); - - expect(consoleErrorSpy).toHaveBeenCalledWith( - `${baseMessage} Partial report (excluding context) available at: ${expectedMinimalReportPath}`, + expect(debugLoggerSpy.error).toHaveBeenCalledWith( + `${baseMessage} [bigint-fail]`, + expect.any(String), ); }); @@ -175,19 +116,13 @@ describe('reportError', () => { error.stack = 'No context stack'; const baseMessage = 'Simple error.'; const type = 'general'; - const expectedReportPath = getExpectedReportPath(type); - await reportError(error, baseMessage, undefined, type, testDir); - - const reportContent = await fs.readFile(expectedReportPath, 'utf-8'); - const parsedReport = JSON.parse(reportContent); - - expect(parsedReport).toEqual({ - error: { message: 'Error without context', stack: 'No context stack' }, - }); - - expect(consoleErrorSpy).toHaveBeenCalledWith( - `${baseMessage} Full report available at: ${expectedReportPath}`, + await expect( + reportError(error, baseMessage, undefined, type), + ).resolves.not.toThrow(); + expect(debugLoggerSpy.error).toHaveBeenCalledWith( + `${baseMessage} [${type}]`, + expect.any(String), ); }); }); diff --git a/packages/core/src/utils/errorReporting.ts b/packages/core/src/utils/errorReporting.ts index 3e137ef61..fd162b5a6 100644 --- a/packages/core/src/utils/errorReporting.ts +++ b/packages/core/src/utils/errorReporting.ts @@ -4,10 +4,10 @@ * SPDX-License-Identifier: Apache-2.0 */ -import fs from 'node:fs/promises'; -import os from 'node:os'; -import path from 'node:path'; import type { Content } from '@google/genai'; +import { createDebugLogger } from './debugLogger.js'; + +const debugLogger = createDebugLogger('ERROR_REPORT'); interface ErrorReportData { error: { message: string; stack?: string } | { message: string }; @@ -16,23 +16,18 @@ interface ErrorReportData { } /** - * Generates an error report, writes it to a temporary file, and logs information to console.error. + * Generates an error report and writes it to the debug log. * @param error The error object. + * @param baseMessage The base message describing the error context. * @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'). - * @param baseMessage The initial message to log to console.error before the report path. */ export async function reportError( error: Error | unknown, baseMessage: string, context?: Content[] | Record | unknown[], type = 'general', - reportingDir = os.tmpdir(), // for testing ): Promise { - const timestamp = new Date().toISOString().replace(/[:.]/g, '-'); - const reportFileName = `qwen-client-error-${type}-${timestamp}.json`; - const reportPath = path.join(reportingDir, reportFileName); - let errorToReport: { message: string; stack?: string }; if (error instanceof Error) { errorToReport = { message: error.message, stack: error.stack }; @@ -54,65 +49,32 @@ export async function reportError( reportContent.context = context; } + const reportLabel = `${baseMessage} [${type}]`; let stringifiedReportContent: string; try { stringifiedReportContent = JSON.stringify(reportContent, null, 2); } catch (stringifyError) { // This can happen if context contains something like BigInt - console.error( - `${baseMessage} Could not stringify report content (likely due to context):`, + debugLogger.error( + `${reportLabel} Could not stringify report content (likely due to context):`, stringifyError, + error, ); - console.error('Original error that triggered report generation:', error); - if (context) { - console.error( - 'Original context could not be stringified or included in report.', - ); - } // Fallback: try to report only the error if context was the issue try { const minimalReportContent = { error: errorToReport }; stringifiedReportContent = JSON.stringify(minimalReportContent, null, 2); - // Still try to write the minimal report - await fs.writeFile(reportPath, stringifiedReportContent); - console.error( - `${baseMessage} Partial report (excluding context) available at: ${reportPath}`, - ); - } catch (minimalWriteError) { - console.error( - `${baseMessage} Failed to write even a minimal error report:`, - minimalWriteError, + debugLogger.error(reportLabel, stringifiedReportContent); + } catch (minimalStringifyError) { + debugLogger.error( + `${reportLabel} Failed to stringify minimal error report:`, + minimalStringifyError, + error, ); } return; } - try { - await fs.writeFile(reportPath, stringifiedReportContent); - console.error(`${baseMessage} Full report available at: ${reportPath}`); - } catch (writeError) { - console.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); - 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); - } catch { - try { - console.error( - 'Original context (stringified, truncated):', - JSON.stringify(context).substring(0, 1000), - ); - } catch { - console.error('Original context could not be logged or stringified.'); - } - } - } - } + // Write to debug log instead of separate file + debugLogger.error(reportLabel, stringifiedReportContent); } diff --git a/packages/core/src/utils/installationManager.test.ts b/packages/core/src/utils/installationManager.test.ts index 1e6263667..09575ae41 100644 --- a/packages/core/src/utils/installationManager.test.ts +++ b/packages/core/src/utils/installationManager.test.ts @@ -90,14 +90,10 @@ describe('InstallationManager', () => { readSpy.mockImplementationOnce(() => { throw new Error('Read error'); }); - const consoleErrorSpy = vi - .spyOn(console, 'error') - .mockImplementation(() => {}); const id = installationManager.getInstallationId(); expect(id).toBe('123456789'); - expect(consoleErrorSpy).toHaveBeenCalled(); }); }); }); diff --git a/packages/core/src/utils/llm-edit-fixer.test.ts b/packages/core/src/utils/llm-edit-fixer.test.ts index 222bf2289..0f136e6d0 100644 --- a/packages/core/src/utils/llm-edit-fixer.test.ts +++ b/packages/core/src/utils/llm-edit-fixer.test.ts @@ -71,9 +71,6 @@ describe('FixLLMEditWithInstruction', () => { it('should generate and use a fallback promptId when context is not available', async () => { mockGenerateJson.mockResolvedValue(mockApiResponse); - const consoleWarnSpy = vi - .spyOn(console, 'warn') - .mockImplementation(() => {}); // Run the function outside of any context await FixLLMEditWithInstruction( @@ -86,13 +83,6 @@ describe('FixLLMEditWithInstruction', () => { abortSignal, ); - // Verify the warning was logged - expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining( - 'Could not find promptId in context. This is unexpected. Using a fallback ID: llm-fixer-fallback-', - ), - ); - // Verify that generateJson was called with the generated fallback promptId expect(mockGenerateJson).toHaveBeenCalledTimes(1); expect(mockGenerateJson).toHaveBeenCalledWith( @@ -100,9 +90,6 @@ describe('FixLLMEditWithInstruction', () => { promptId: expect.stringContaining('llm-fixer-fallback-'), }), ); - - // Restore mocks - consoleWarnSpy.mockRestore(); }); it('should construct the user prompt correctly', async () => { diff --git a/packages/core/src/utils/openaiLogger.test.ts b/packages/core/src/utils/openaiLogger.test.ts index 17a074865..9d3387e4b 100644 --- a/packages/core/src/utils/openaiLogger.test.ts +++ b/packages/core/src/utils/openaiLogger.test.ts @@ -12,11 +12,15 @@ import { OpenAILogger } from './openaiLogger.js'; describe('OpenAILogger', () => { let originalCwd: string; + let originalHome: string | undefined; let testTempDir: string; const createdDirs: string[] = []; + const testHomeDir = path.join(os.tmpdir(), 'openai-logger-home'); beforeEach(() => { originalCwd = process.cwd(); + originalHome = process.env['HOME']; + process.env['HOME'] = testHomeDir; testTempDir = path.join(os.tmpdir(), `openai-logger-test-${Date.now()}`); createdDirs.length = 0; // Clear array }); @@ -41,6 +45,11 @@ describe('OpenAILogger', () => { await Promise.all(cleanupPromises); process.chdir(originalCwd); + if (originalHome === undefined) { + delete process.env['HOME']; + } else { + process.env['HOME'] = originalHome; + } }); describe('constructor', () => { diff --git a/packages/core/src/utils/summarizer.test.ts b/packages/core/src/utils/summarizer.test.ts index 33fa9bc02..6098e77b7 100644 --- a/packages/core/src/utils/summarizer.test.ts +++ b/packages/core/src/utils/summarizer.test.ts @@ -41,13 +41,10 @@ describe('summarizers', () => { mockGeminiClient = new GeminiClient(mockConfigInstance); (mockGeminiClient.generateContent as Mock) = vi.fn(); - - vi.spyOn(console, 'error').mockImplementation(() => {}); }); afterEach(() => { vi.clearAllMocks(); - (console.error as Mock).mockRestore(); }); describe('summarizeToolOutput', () => { @@ -107,10 +104,6 @@ describe('summarizers', () => { expect(mockGeminiClient.generateContent).toHaveBeenCalledTimes(1); expect(result).toBe(longText); - expect(console.error).toHaveBeenCalledWith( - 'Failed to summarize tool output.', - error, - ); }); it('should construct the correct prompt for summarization', async () => { diff --git a/packages/core/src/utils/systemEncoding.test.ts b/packages/core/src/utils/systemEncoding.test.ts index 61558f38b..6b6ce693f 100644 --- a/packages/core/src/utils/systemEncoding.test.ts +++ b/packages/core/src/utils/systemEncoding.test.ts @@ -24,13 +24,11 @@ import { } from './systemEncoding.js'; describe('Shell Command Processor - Encoding Functions', () => { - let consoleWarnSpy: ReturnType; let mockedExecSync: ReturnType>; let mockedOsPlatform: ReturnType string>>; let mockedChardetDetect: ReturnType>; beforeEach(() => { - consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); mockedExecSync = vi.mocked(execSync); mockedOsPlatform = vi.mocked(os.platform); mockedChardetDetect = vi.mocked(chardetDetect); @@ -65,9 +63,6 @@ describe('Shell Command Processor - Encoding Functions', () => { it('should return null for unmapped code pages and warn', () => { expect(windowsCodePageToEncoding(99999)).toBe(null); - expect(consoleWarnSpy).toHaveBeenCalledWith( - 'Unable to determine encoding for windows code page 99999.', - ); }); it('should handle all Windows-specific code pages', () => { @@ -109,10 +104,6 @@ describe('Shell Command Processor - Encoding Functions', () => { const result = detectEncodingFromBuffer(buffer); expect(result).toBe(null); - expect(consoleWarnSpy).toHaveBeenCalledWith( - 'Failed to detect encoding with chardet:', - expect.any(Error), - ); }); it('should return null when chardet returns null', () => { @@ -169,11 +160,6 @@ describe('Shell Command Processor - Encoding Functions', () => { const result = getSystemEncoding(); expect(result).toBe(null); - expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining( - "Failed to get Windows code page using 'chcp' command", - ), - ); }); it('should return null when chcp output cannot be parsed', () => { @@ -181,11 +167,6 @@ describe('Shell Command Processor - Encoding Functions', () => { const result = getSystemEncoding(); expect(result).toBe(null); - expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining( - "Failed to get Windows code page using 'chcp' command", - ), - ); }); it('should return null when code page is not a number', () => { @@ -193,11 +174,6 @@ describe('Shell Command Processor - Encoding Functions', () => { const result = getSystemEncoding(); expect(result).toBe(null); - expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining( - "Failed to get Windows code page using 'chcp' command", - ), - ); }); it('should return null when code page maps to null', () => { @@ -205,10 +181,6 @@ describe('Shell Command Processor - Encoding Functions', () => { const result = getSystemEncoding(); expect(result).toBe(null); - // Should warn about unknown code page from windowsCodePageToEncoding - expect(consoleWarnSpy).toHaveBeenCalledWith( - 'Unable to determine encoding for windows code page 99999.', - ); }); }); @@ -262,9 +234,6 @@ describe('Shell Command Processor - Encoding Functions', () => { const result = getSystemEncoding(); expect(result).toBe(null); - expect(consoleWarnSpy).toHaveBeenCalledWith( - 'Failed to get locale charmap.', - ); }); it('should handle locale without encoding (no dot)', () => { diff --git a/packages/core/src/utils/workspaceContext.test.ts b/packages/core/src/utils/workspaceContext.test.ts index c93dffe47..686c50ba3 100644 --- a/packages/core/src/utils/workspaceContext.test.ts +++ b/packages/core/src/utils/workspaceContext.test.ts @@ -391,32 +391,24 @@ describe('WorkspaceContext with optional directories', () => { fs.mkdirSync(cwd, { recursive: true }); fs.mkdirSync(existingDir1, { recursive: true }); fs.mkdirSync(existingDir2, { recursive: true }); - - vi.spyOn(console, 'warn').mockImplementation(() => {}); }); afterEach(() => { fs.rmSync(tempDir, { recursive: true, force: true }); - vi.restoreAllMocks(); }); - it('should skip a missing optional directory and log a warning', () => { + it('should skip a missing optional directory', () => { const workspaceContext = new WorkspaceContext(cwd, [ nonExistentDir, existingDir1, ]); const directories = workspaceContext.getDirectories(); expect(directories).toEqual([cwd, existingDir1]); - expect(console.warn).toHaveBeenCalledTimes(1); - expect(console.warn).toHaveBeenCalledWith( - `[WARN] Skipping unreadable directory: ${nonExistentDir} (Directory does not exist: ${nonExistentDir})`, - ); }); it('should include an existing optional directory', () => { const workspaceContext = new WorkspaceContext(cwd, [existingDir1]); const directories = workspaceContext.getDirectories(); expect(directories).toEqual([cwd, existingDir1]); - expect(console.warn).not.toHaveBeenCalled(); }); });