diff --git a/integration-tests/acp-env-auth.test.ts b/integration-tests/acp-env-auth.test.ts new file mode 100644 index 0000000000..78eec9cd56 --- /dev/null +++ b/integration-tests/acp-env-auth.test.ts @@ -0,0 +1,163 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { TestRig } from './test-helper.js'; +import { spawn, ChildProcess } from 'node:child_process'; +import { join, resolve } from 'node:path'; +import { writeFileSync, mkdirSync } from 'node:fs'; +import { Writable, Readable } from 'node:stream'; +import { env } from 'node:process'; +import * as acp from '@agentclientprotocol/sdk'; + +const sandboxEnv = env['GEMINI_SANDBOX']; +const itMaybe = sandboxEnv && sandboxEnv !== 'false' ? it.skip : it; + +class MockClient implements acp.Client { + updates: acp.SessionNotification[] = []; + sessionUpdate = async (params: acp.SessionNotification) => { + this.updates.push(params); + }; + requestPermission = async (): Promise => { + throw new Error('unexpected'); + }; +} + +describe('ACP Environment and Auth', () => { + let rig: TestRig; + let child: ChildProcess | undefined; + + beforeEach(() => { + rig = new TestRig(); + }); + + afterEach(async () => { + child?.kill(); + child = undefined; + await rig.cleanup(); + }); + + itMaybe( + 'should load .env from project directory and use the provided API key', + async () => { + rig.setup('acp-env-loading'); + + // Create a project directory with a .env file containing a recognizable invalid key + const projectDir = resolve(join(rig.testDir!, 'project')); + mkdirSync(projectDir, { recursive: true }); + writeFileSync( + join(projectDir, '.env'), + 'GEMINI_API_KEY=test-key-from-env\n', + ); + + const bundlePath = join(import.meta.dirname, '..', 'bundle/gemini.js'); + + child = spawn('node', [bundlePath, '--experimental-acp'], { + cwd: rig.homeDir!, + stdio: ['pipe', 'pipe', 'inherit'], + env: { + ...process.env, + GEMINI_CLI_HOME: rig.homeDir!, + GEMINI_API_KEY: undefined, + VERBOSE: 'true', + }, + }); + + const input = Writable.toWeb(child.stdin!); + const output = Readable.toWeb( + child.stdout!, + ) as ReadableStream; + const testClient = new MockClient(); + const stream = acp.ndJsonStream(input, output); + const connection = new acp.ClientSideConnection(() => testClient, stream); + + await connection.initialize({ + protocolVersion: acp.PROTOCOL_VERSION, + clientCapabilities: { + fs: { readTextFile: false, writeTextFile: false }, + }, + }); + + // 1. newSession should succeed because it finds the key in .env + const { sessionId } = await connection.newSession({ + cwd: projectDir, + mcpServers: [], + }); + + expect(sessionId).toBeDefined(); + + // 2. prompt should fail because the key is invalid, + // but the error should come from the API, not the internal auth check. + await expect( + connection.prompt({ + sessionId, + prompt: [{ type: 'text', text: 'hello' }], + }), + ).rejects.toSatisfy((error: unknown) => { + const acpError = error as acp.RequestError; + const errorData = acpError.data as + | { error?: { message?: string } } + | undefined; + const message = String(errorData?.error?.message || acpError.message); + // It should NOT be our internal "Authentication required" message + expect(message).not.toContain('Authentication required'); + // It SHOULD be an API error mentioning the invalid key + expect(message).toContain('API key not valid'); + return true; + }); + + child.stdin!.end(); + }, + ); + + itMaybe( + 'should fail with authRequired when no API key is found', + async () => { + rig.setup('acp-auth-failure'); + + const bundlePath = join(import.meta.dirname, '..', 'bundle/gemini.js'); + + child = spawn('node', [bundlePath, '--experimental-acp'], { + cwd: rig.homeDir!, + stdio: ['pipe', 'pipe', 'inherit'], + env: { + ...process.env, + GEMINI_CLI_HOME: rig.homeDir!, + GEMINI_API_KEY: undefined, + VERBOSE: 'true', + }, + }); + + const input = Writable.toWeb(child.stdin!); + const output = Readable.toWeb( + child.stdout!, + ) as ReadableStream; + const testClient = new MockClient(); + const stream = acp.ndJsonStream(input, output); + const connection = new acp.ClientSideConnection(() => testClient, stream); + + await connection.initialize({ + protocolVersion: acp.PROTOCOL_VERSION, + clientCapabilities: { + fs: { readTextFile: false, writeTextFile: false }, + }, + }); + + await expect( + connection.newSession({ + cwd: resolve(rig.testDir!), + mcpServers: [], + }), + ).rejects.toMatchObject({ + message: expect.stringContaining( + 'Gemini API key is missing or not configured.', + ), + }); + + child.stdin!.end(); + }, + ); +}); diff --git a/packages/cli/src/config/auth.ts b/packages/cli/src/config/auth.ts index a3cfea7d77..b1f32b6b28 100644 --- a/packages/cli/src/config/auth.ts +++ b/packages/cli/src/config/auth.ts @@ -8,7 +8,7 @@ import { AuthType } from '@google/gemini-cli-core'; import { loadEnvironment, loadSettings } from './settings.js'; export function validateAuthMethod(authMethod: string): string | null { - loadEnvironment(loadSettings().merged); + loadEnvironment(loadSettings().merged, process.cwd()); if ( authMethod === AuthType.LOGIN_WITH_GOOGLE || authMethod === AuthType.COMPUTE_ADC diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index c8e1e8c975..7890d7e08d 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -433,7 +433,7 @@ export async function loadCliConfig( const ideMode = settings.ide?.enabled ?? false; const folderTrust = settings.security?.folderTrust?.enabled ?? false; - const trustedFolder = isWorkspaceTrusted(settings)?.isTrusted ?? false; + const trustedFolder = isWorkspaceTrusted(settings, cwd)?.isTrusted ?? false; // Set the context filename in the server's memoryTool module BEFORE loading memory // TODO(b/343434939): This is a bit of a hack. The contextFileName should ideally be passed diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 15cc99ebd6..8b4d6f64c5 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -29,10 +29,11 @@ vi.mock('./settings.js', async (importActual) => { }); // Mock trustedFolders +import * as trustedFolders from './trustedFolders.js'; vi.mock('./trustedFolders.js', () => ({ - isWorkspaceTrusted: vi - .fn() - .mockReturnValue({ isTrusted: true, source: 'file' }), + isWorkspaceTrusted: vi.fn(), + isFolderTrustEnabled: vi.fn(), + loadTrustedFolders: vi.fn(), })); vi.mock('./settingsSchema.js', async (importOriginal) => { @@ -151,7 +152,7 @@ describe('Settings Loading and Merging', () => { (mockFsExistsSync as Mock).mockReturnValue(false); (fs.readFileSync as Mock).mockReturnValue('{}'); // Return valid empty JSON (mockFsMkdirSync as Mock).mockImplementation(() => undefined); - vi.mocked(isWorkspaceTrusted).mockReturnValue({ + vi.spyOn(trustedFolders, 'isWorkspaceTrusted').mockReturnValue({ isTrusted: true, source: 'file', }); @@ -1635,7 +1636,7 @@ describe('Settings Loading and Merging', () => { }); it('should NOT merge workspace settings when workspace is not trusted', () => { - vi.mocked(isWorkspaceTrusted).mockReturnValue({ + vi.spyOn(trustedFolders, 'isWorkspaceTrusted').mockReturnValue({ isTrusted: false, source: 'file', }); @@ -1666,23 +1667,60 @@ describe('Settings Loading and Merging', () => { expect(settings.merged.context?.fileName).toBe('USER.md'); // User setting expect(settings.merged.ui?.theme).toBe('dark'); // User setting }); + + it('should NOT merge workspace settings when workspace trust is undefined', () => { + vi.spyOn(trustedFolders, 'isWorkspaceTrusted').mockReturnValue({ + isTrusted: undefined, + source: undefined, + }); + (mockFsExistsSync as Mock).mockReturnValue(true); + const userSettingsContent = { + ui: { theme: 'dark' }, + tools: { sandbox: false }, + context: { fileName: 'USER.md' }, + }; + const workspaceSettingsContent = { + tools: { sandbox: true }, + context: { fileName: 'WORKSPACE.md' }, + }; + + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if (p === USER_SETTINGS_PATH) + return JSON.stringify(userSettingsContent); + if (p === MOCK_WORKSPACE_SETTINGS_PATH) + return JSON.stringify(workspaceSettingsContent); + return '{}'; + }, + ); + + const settings = loadSettings(MOCK_WORKSPACE_DIR); + + expect(settings.merged.tools?.sandbox).toBe(false); // User setting + expect(settings.merged.context?.fileName).toBe('USER.md'); // User setting + }); }); describe('loadEnvironment', () => { function setup({ isFolderTrustEnabled = true, - isWorkspaceTrustedValue = true, + isWorkspaceTrustedValue = true as boolean | undefined, }) { delete process.env['TESTTEST']; // reset - const geminiEnvPath = path.resolve(path.join(GEMINI_DIR, '.env')); + const geminiEnvPath = path.resolve( + path.join(MOCK_WORKSPACE_DIR, GEMINI_DIR, '.env'), + ); - vi.mocked(isWorkspaceTrusted).mockReturnValue({ + vi.spyOn(trustedFolders, 'isWorkspaceTrusted').mockReturnValue({ isTrusted: isWorkspaceTrustedValue, source: 'file', }); - (mockFsExistsSync as Mock).mockImplementation((p: fs.PathLike) => - [USER_SETTINGS_PATH, geminiEnvPath].includes(p.toString()), - ); + (mockFsExistsSync as Mock).mockImplementation((p: fs.PathLike) => { + const normalizedP = path.resolve(p.toString()); + return [path.resolve(USER_SETTINGS_PATH), geminiEnvPath].includes( + normalizedP, + ); + }); const userSettingsContent: Settings = { ui: { theme: 'dark', @@ -1698,9 +1736,10 @@ describe('Settings Loading and Merging', () => { }; (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + const normalizedP = path.resolve(p.toString()); + if (normalizedP === path.resolve(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === geminiEnvPath) return 'TESTTEST=1234'; + if (normalizedP === geminiEnvPath) return 'TESTTEST=1234'; return '{}'; }, ); @@ -1708,14 +1747,34 @@ describe('Settings Loading and Merging', () => { it('sets environment variables from .env files', () => { setup({ isFolderTrustEnabled: false, isWorkspaceTrustedValue: true }); - loadEnvironment(loadSettings(MOCK_WORKSPACE_DIR).merged); + const settings = { + security: { folderTrust: { enabled: false } }, + } as Settings; + loadEnvironment(settings, MOCK_WORKSPACE_DIR, isWorkspaceTrusted); expect(process.env['TESTTEST']).toEqual('1234'); }); it('does not load env files from untrusted spaces', () => { setup({ isFolderTrustEnabled: true, isWorkspaceTrustedValue: false }); - loadEnvironment(loadSettings(MOCK_WORKSPACE_DIR).merged); + const settings = { + security: { folderTrust: { enabled: true } }, + } as Settings; + loadEnvironment(settings, MOCK_WORKSPACE_DIR, isWorkspaceTrusted); + + expect(process.env['TESTTEST']).not.toEqual('1234'); + }); + + it('does not load env files when trust is undefined', () => { + delete process.env['TESTTEST']; + // isWorkspaceTrusted returns {isTrusted: undefined} for matched rules with no trust value, or no matching rules. + setup({ isFolderTrustEnabled: true, isWorkspaceTrustedValue: undefined }); + const settings = { + security: { folderTrust: { enabled: true } }, + } as Settings; + + const mockTrustFn = vi.fn().mockReturnValue({ isTrusted: undefined }); + loadEnvironment(settings, MOCK_WORKSPACE_DIR, mockTrustFn); expect(process.env['TESTTEST']).not.toEqual('1234'); }); @@ -1731,7 +1790,7 @@ describe('Settings Loading and Merging', () => { mockFsExistsSync.mockReturnValue(true); mockFsReadFileSync = vi.mocked(fs.readFileSync); mockFsReadFileSync.mockReturnValue('{}'); - vi.mocked(isWorkspaceTrusted).mockReturnValue({ + vi.spyOn(trustedFolders, 'isWorkspaceTrusted').mockReturnValue({ isTrusted: true, source: undefined, }); diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index b2544650d3..b1dc449219 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -50,7 +50,9 @@ import { formatValidationError, } from './settings-validation.js'; -function getMergeStrategyForPath(path: string[]): MergeStrategy | undefined { +export function getMergeStrategyForPath( + path: string[], +): MergeStrategy | undefined { let current: SettingDefinition | undefined = undefined; let currentSchema: SettingsSchema | undefined = getSettingsSchema(); let parent: SettingDefinition | undefined = undefined; @@ -432,10 +434,15 @@ export function setUpCloudShellEnvironment(envFilePath: string | null): void { } } -export function loadEnvironment(settings: Settings): void { - const envFilePath = findEnvFile(process.cwd()); +export function loadEnvironment( + settings: Settings, + workspaceDir: string, + isWorkspaceTrustedFn = isWorkspaceTrusted, +): void { + const envFilePath = findEnvFile(workspaceDir); + const trustResult = isWorkspaceTrustedFn(settings, workspaceDir); - if (!isWorkspaceTrusted(settings).isTrusted) { + if (trustResult.isTrusted !== true) { return; } @@ -600,7 +607,8 @@ export function loadSettings( userSettings, ); const isTrusted = - isWorkspaceTrusted(initialTrustCheckSettings as Settings).isTrusted ?? true; + isWorkspaceTrusted(initialTrustCheckSettings as Settings, workspaceDir) + .isTrusted ?? false; // Create a temporary merged settings object to pass to loadEnvironment. const tempMergedSettings = mergeSettings( @@ -613,7 +621,7 @@ export function loadSettings( // loadEnvironment depends on settings so we have to create a temp version of // the settings to avoid a cycle - loadEnvironment(tempMergedSettings); + loadEnvironment(tempMergedSettings, workspaceDir); // Check for any fatal errors before proceeding const fatalErrors = settingsErrors.filter((e) => e.severity === 'error'); diff --git a/packages/cli/src/config/trustedFolders.test.ts b/packages/cli/src/config/trustedFolders.test.ts index 9bd4cef9f6..1c5268167c 100644 --- a/packages/cli/src/config/trustedFolders.test.ts +++ b/packages/cli/src/config/trustedFolders.test.ts @@ -326,6 +326,19 @@ describe('isWorkspaceTrusted', () => { }); }); + it('should use workspaceDir instead of process.cwd() when provided', () => { + mockCwd = '/home/user/untrusted'; + const workspaceDir = '/home/user/projectA'; + mockRules['/home/user/projectA'] = TrustLevel.TRUST_FOLDER; + mockRules['/home/user/untrusted'] = TrustLevel.DO_NOT_TRUST; + + // process.cwd() is untrusted, but workspaceDir is trusted + expect(isWorkspaceTrusted(mockSettings, workspaceDir)).toEqual({ + isTrusted: true, + source: 'file', + }); + }); + it('should handle path normalization', () => { mockCwd = '/home/user/projectA'; mockRules[`/home/user/../user/${path.basename('/home/user/projectA')}`] = diff --git a/packages/cli/src/config/trustedFolders.ts b/packages/cli/src/config/trustedFolders.ts index 3057a7d3ec..c462460d06 100644 --- a/packages/cli/src/config/trustedFolders.ts +++ b/packages/cli/src/config/trustedFolders.ts @@ -227,6 +227,7 @@ export function isFolderTrustEnabled(settings: Settings): boolean { } function getWorkspaceTrustFromLocalConfig( + workspaceDir: string, trustConfig?: Record, ): TrustResult { const folders = loadTrustedFolders(); @@ -241,7 +242,7 @@ function getWorkspaceTrustFromLocalConfig( ); } - const isTrusted = folders.isPathTrusted(process.cwd(), configToUse); + const isTrusted = folders.isPathTrusted(workspaceDir, configToUse); return { isTrusted, source: isTrusted !== undefined ? 'file' : undefined, @@ -250,6 +251,7 @@ function getWorkspaceTrustFromLocalConfig( export function isWorkspaceTrusted( settings: Settings, + workspaceDir: string = process.cwd(), trustConfig?: Record, ): TrustResult { if (!isFolderTrustEnabled(settings)) { @@ -262,5 +264,5 @@ export function isWorkspaceTrusted( } // Fall back to the local user configuration - return getWorkspaceTrustFromLocalConfig(trustConfig); + return getWorkspaceTrustFromLocalConfig(workspaceDir, trustConfig); } diff --git a/packages/cli/src/ui/hooks/usePermissionsModifyTrust.ts b/packages/cli/src/ui/hooks/usePermissionsModifyTrust.ts index 5d97ffb36d..7c3fbd8616 100644 --- a/packages/cli/src/ui/hooks/usePermissionsModifyTrust.ts +++ b/packages/cli/src/ui/hooks/usePermissionsModifyTrust.ts @@ -41,7 +41,10 @@ function getInitialTrustState( }; } - const { isTrusted, source } = isWorkspaceTrusted(settings.merged); + const { isTrusted, source } = isWorkspaceTrusted( + settings.merged, + process.cwd(), + ); const isInheritedTrust = isTrusted && @@ -99,7 +102,10 @@ export const usePermissionsModifyTrust = ( } // All logic below only applies when editing the current workspace. - const wasTrusted = isWorkspaceTrusted(settings.merged).isTrusted; + const wasTrusted = isWorkspaceTrusted( + settings.merged, + process.cwd(), + ).isTrusted; // Create a temporary config to check the new trust status without writing const currentConfig = loadTrustedFolders().user.config; @@ -107,6 +113,7 @@ export const usePermissionsModifyTrust = ( const { isTrusted, source } = isWorkspaceTrusted( settings.merged, + process.cwd(), newConfig, ); diff --git a/packages/cli/src/zed-integration/zedIntegration.test.ts b/packages/cli/src/zed-integration/zedIntegration.test.ts index fe20c3b577..41a0958f56 100644 --- a/packages/cli/src/zed-integration/zedIntegration.test.ts +++ b/packages/cli/src/zed-integration/zedIntegration.test.ts @@ -26,7 +26,11 @@ import { type Config, type MessageBus, } from '@google/gemini-cli-core'; -import { SettingScope, type LoadedSettings } from '../config/settings.js'; +import { + SettingScope, + type LoadedSettings, + loadSettings, +} from '../config/settings.js'; import { loadCliConfig, type CliArgs } from '../config/config.js'; import * as fs from 'node:fs/promises'; import * as path from 'node:path'; @@ -35,6 +39,14 @@ vi.mock('../config/config.js', () => ({ loadCliConfig: vi.fn(), })); +vi.mock('../config/settings.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + loadSettings: vi.fn(), + }; +}); + vi.mock('node:crypto', () => ({ randomUUID: () => 'test-session-id', })); @@ -95,6 +107,10 @@ describe('GeminiAgent', () => { initialize: vi.fn(), getFileSystemService: vi.fn(), setFileSystemService: vi.fn(), + getContentGeneratorConfig: vi.fn(), + getActiveModel: vi.fn().mockReturnValue('gemini-pro'), + getModel: vi.fn().mockReturnValue('gemini-pro'), + getPreviewFeatures: vi.fn().mockReturnValue({}), getGeminiClient: vi.fn().mockReturnValue({ startChat: vi.fn().mockResolvedValue({}), }), @@ -117,6 +133,13 @@ describe('GeminiAgent', () => { } as unknown as Mocked; (loadCliConfig as unknown as Mock).mockResolvedValue(mockConfig); + (loadSettings as unknown as Mock).mockImplementation(() => ({ + merged: { + security: { auth: { selectedType: AuthType.LOGIN_WITH_GOOGLE } }, + mcpServers: {}, + }, + setValue: vi.fn(), + })); agent = new GeminiAgent(mockConfig, mockSettings, mockArgv, mockConnection); }); @@ -148,6 +171,9 @@ describe('GeminiAgent', () => { }); it('should create a new session', async () => { + mockConfig.getContentGeneratorConfig = vi.fn().mockReturnValue({ + apiKey: 'test-key', + }); const response = await agent.newSession({ cwd: '/tmp', mcpServers: [], @@ -159,6 +185,28 @@ describe('GeminiAgent', () => { expect(mockConfig.getGeminiClient).toHaveBeenCalled(); }); + it('should fail session creation if Gemini API key is missing', async () => { + (loadSettings as unknown as Mock).mockImplementation(() => ({ + merged: { + security: { auth: { selectedType: AuthType.USE_GEMINI } }, + mcpServers: {}, + }, + setValue: vi.fn(), + })); + mockConfig.getContentGeneratorConfig = vi.fn().mockReturnValue({ + apiKey: undefined, + }); + + await expect( + agent.newSession({ + cwd: '/tmp', + mcpServers: [], + }), + ).rejects.toMatchObject({ + message: 'Gemini API key is missing or not configured.', + }); + }); + it('should create a new session with mcp servers', async () => { const mcpServers = [ { @@ -194,14 +242,14 @@ describe('GeminiAgent', () => { mockConfig.refreshAuth.mockRejectedValue(new Error('Auth failed')); const debugSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - // Should throw RequestError.authRequired() + // Should throw RequestError with custom message await expect( agent.newSession({ cwd: '/tmp', mcpServers: [], }), ).rejects.toMatchObject({ - message: 'Authentication required', + message: 'Auth failed', }); debugSpy.mockRestore(); diff --git a/packages/cli/src/zed-integration/zedIntegration.ts b/packages/cli/src/zed-integration/zedIntegration.ts index ac33e50e96..e48e3dbf01 100644 --- a/packages/cli/src/zed-integration/zedIntegration.ts +++ b/packages/cli/src/zed-integration/zedIntegration.ts @@ -40,7 +40,7 @@ import { AcpFileSystemService } from './fileSystemService.js'; import { Readable, Writable } from 'node:stream'; import type { Content, Part, FunctionCall } from '@google/genai'; import type { LoadedSettings } from '../config/settings.js'; -import { SettingScope } from '../config/settings.js'; +import { SettingScope, loadSettings } from '../config/settings.js'; import * as fs from 'node:fs/promises'; import * as path from 'node:path'; import { z } from 'zod'; @@ -139,7 +139,11 @@ export class GeminiAgent { // Refresh auth with the requested method // This will reuse existing credentials if they're valid, // or perform new authentication if needed - await this.config.refreshAuth(method); + try { + await this.config.refreshAuth(method); + } catch (e) { + throw new acp.RequestError(getErrorStatus(e) || 401, getErrorMessage(e)); + } this.settings.setValue( SettingScope.User, 'security.auth.selectedType', @@ -152,12 +156,47 @@ export class GeminiAgent { mcpServers, }: acp.NewSessionRequest): Promise { const sessionId = randomUUID(); - const config = await this.initializeSessionConfig( + const loadedSettings = loadSettings(cwd); + const config = await this.newSessionConfig( sessionId, cwd, mcpServers, + loadedSettings, ); + const authType = + loadedSettings.merged.security.auth.selectedType || AuthType.USE_GEMINI; + + let isAuthenticated = false; + let authErrorMessage = ''; + try { + await config.refreshAuth(authType); + isAuthenticated = true; + + // Extra validation for Gemini API key + const contentGeneratorConfig = config.getContentGeneratorConfig(); + if ( + authType === AuthType.USE_GEMINI && + (!contentGeneratorConfig || !contentGeneratorConfig.apiKey) + ) { + isAuthenticated = false; + authErrorMessage = 'Gemini API key is missing or not configured.'; + } + } catch (e) { + isAuthenticated = false; + authErrorMessage = getErrorMessage(e); + debugLogger.error( + `Authentication failed: ${e instanceof Error ? e.stack : e}`, + ); + } + + if (!isAuthenticated) { + throw new acp.RequestError( + 401, + authErrorMessage || 'Authentication required.', + ); + } + if (this.clientCapabilities?.fs) { const acpFileSystemService = new AcpFileSystemService( this.connection, @@ -168,6 +207,9 @@ export class GeminiAgent { config.setFileSystemService(acpFileSystemService); } + await config.initialize(); + startupProfiler.flush(config); + const geminiClient = config.getGeminiClient(); const chat = await geminiClient.startChat(); const session = new Session(sessionId, chat, config, this.connection); @@ -264,8 +306,10 @@ export class GeminiAgent { sessionId: string, cwd: string, mcpServers: acp.McpServer[], + loadedSettings?: LoadedSettings, ): Promise { - const mergedMcpServers = { ...this.settings.merged.mcpServers }; + const currentSettings = loadedSettings || this.settings; + const mergedMcpServers = { ...currentSettings.merged.mcpServers }; for (const server of mcpServers) { if ( @@ -300,7 +344,10 @@ export class GeminiAgent { } } - const settings = { ...this.settings.merged, mcpServers: mergedMcpServers }; + const settings = { + ...currentSettings.merged, + mcpServers: mergedMcpServers, + }; const config = await loadCliConfig(settings, sessionId, this.argv, { cwd }); @@ -497,7 +544,10 @@ export class Session { return { stopReason: 'cancelled' }; } - throw error; + throw new acp.RequestError( + getErrorStatus(error) || 500, + getErrorMessage(error), + ); } if (functionCalls.length > 0) { diff --git a/packages/core/src/utils/errors.test.ts b/packages/core/src/utils/errors.test.ts index 8ee1bcfd7a..e94805ea27 100644 --- a/packages/core/src/utils/errors.test.ts +++ b/packages/core/src/utils/errors.test.ts @@ -11,8 +11,65 @@ import { toFriendlyError, BadRequestError, ForbiddenError, + getErrorMessage, } from './errors.js'; +describe('getErrorMessage', () => { + it('should return plain error message', () => { + expect(getErrorMessage(new Error('plain error'))).toBe('plain error'); + }); + + it('should parse simple JSON error response', () => { + const json = JSON.stringify({ error: { message: 'json error' } }); + expect(getErrorMessage(new Error(json))).toBe('json error'); + }); + + it('should parse double-encoded JSON error response', () => { + const innerJson = JSON.stringify({ error: { message: 'nested error' } }); + const outerJson = JSON.stringify({ error: { message: innerJson } }); + expect(getErrorMessage(new Error(outerJson))).toBe('nested error'); + }); + + it('should parse array-style JSON error response', () => { + const json = JSON.stringify([{ error: { message: 'array error' } }]); + expect(getErrorMessage(new Error(json))).toBe('array error'); + }); + + it('should parse JSON with top-level message field', () => { + const json = JSON.stringify({ message: 'top-level message' }); + expect(getErrorMessage(new Error(json))).toBe('top-level message'); + }); + + it('should handle JSON with trailing newline', () => { + const json = JSON.stringify({ error: { message: 'newline error' } }) + '\n'; + expect(getErrorMessage(new Error(json))).toBe('newline error'); + }); + + it('should return original message if JSON parsing fails', () => { + const invalidJson = '{ not-json }'; + expect(getErrorMessage(new Error(invalidJson))).toBe(invalidJson); + }); + + it('should handle non-Error inputs', () => { + expect(getErrorMessage('string error')).toBe('string error'); + expect(getErrorMessage(123)).toBe('123'); + }); + + it('should handle structured HTTP errors via toFriendlyError', () => { + const error = { + response: { + data: { + error: { + code: 400, + message: 'Bad Request Message', + }, + }, + }, + }; + expect(getErrorMessage(error)).toBe('Bad Request Message'); + }); +}); + describe('isAuthenticationError', () => { it('should detect error with code: 401 property (MCP SDK style)', () => { const error = { code: 401, message: 'Unauthorized' }; diff --git a/packages/core/src/utils/errors.ts b/packages/core/src/utils/errors.ts index 86f1cc9b86..f833d9aad6 100644 --- a/packages/core/src/utils/errors.ts +++ b/packages/core/src/utils/errors.ts @@ -15,11 +15,41 @@ export function isNodeError(error: unknown): error is NodeJS.ErrnoException { } export function getErrorMessage(error: unknown): string { - if (error instanceof Error) { - return error.message; + const friendlyError = toFriendlyError(error); + return extractMessage(friendlyError); +} + +function extractMessage(input: unknown): string { + if (input instanceof Error) { + return extractMessage(input.message); } + + if (typeof input === 'string') { + const trimmed = input.trim(); + // Attempt to parse JSON error responses (common in Google API errors) + if ( + (trimmed.startsWith('{') && trimmed.endsWith('}')) || + (trimmed.startsWith('[') && trimmed.endsWith(']')) + ) { + try { + const parsed = JSON.parse(trimmed); + const next = + parsed?.error?.message || + parsed?.[0]?.error?.message || + parsed?.message; + + if (next && next !== input) { + return extractMessage(next); + } + } catch { + // Fall back to original string if parsing fails + } + } + return input; + } + try { - return String(error); + return String(input); } catch { return 'Failed to get error details'; }