fix(cli): resolve environment loading and auth validation issues in ACP mode (#18025)

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
This commit is contained in:
Bryan Morgan
2026-02-03 00:54:10 -05:00
committed by GitHub
parent 1b274b081d
commit e7bfd2bf83
12 changed files with 477 additions and 40 deletions
+163
View File
@@ -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<acp.RequestPermissionResponse> => {
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<Uint8Array>;
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<Uint8Array>;
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();
},
);
});
+1 -1
View File
@@ -8,7 +8,7 @@ import { AuthType } from '@google/gemini-cli-core';
import { loadEnvironment, loadSettings } from './settings.js'; import { loadEnvironment, loadSettings } from './settings.js';
export function validateAuthMethod(authMethod: string): string | null { export function validateAuthMethod(authMethod: string): string | null {
loadEnvironment(loadSettings().merged); loadEnvironment(loadSettings().merged, process.cwd());
if ( if (
authMethod === AuthType.LOGIN_WITH_GOOGLE || authMethod === AuthType.LOGIN_WITH_GOOGLE ||
authMethod === AuthType.COMPUTE_ADC authMethod === AuthType.COMPUTE_ADC
+1 -1
View File
@@ -433,7 +433,7 @@ export async function loadCliConfig(
const ideMode = settings.ide?.enabled ?? false; const ideMode = settings.ide?.enabled ?? false;
const folderTrust = settings.security?.folderTrust?.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 // 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 // TODO(b/343434939): This is a bit of a hack. The contextFileName should ideally be passed
+75 -16
View File
@@ -29,10 +29,11 @@ vi.mock('./settings.js', async (importActual) => {
}); });
// Mock trustedFolders // Mock trustedFolders
import * as trustedFolders from './trustedFolders.js';
vi.mock('./trustedFolders.js', () => ({ vi.mock('./trustedFolders.js', () => ({
isWorkspaceTrusted: vi isWorkspaceTrusted: vi.fn(),
.fn() isFolderTrustEnabled: vi.fn(),
.mockReturnValue({ isTrusted: true, source: 'file' }), loadTrustedFolders: vi.fn(),
})); }));
vi.mock('./settingsSchema.js', async (importOriginal) => { vi.mock('./settingsSchema.js', async (importOriginal) => {
@@ -151,7 +152,7 @@ describe('Settings Loading and Merging', () => {
(mockFsExistsSync as Mock).mockReturnValue(false); (mockFsExistsSync as Mock).mockReturnValue(false);
(fs.readFileSync as Mock).mockReturnValue('{}'); // Return valid empty JSON (fs.readFileSync as Mock).mockReturnValue('{}'); // Return valid empty JSON
(mockFsMkdirSync as Mock).mockImplementation(() => undefined); (mockFsMkdirSync as Mock).mockImplementation(() => undefined);
vi.mocked(isWorkspaceTrusted).mockReturnValue({ vi.spyOn(trustedFolders, 'isWorkspaceTrusted').mockReturnValue({
isTrusted: true, isTrusted: true,
source: 'file', source: 'file',
}); });
@@ -1635,7 +1636,7 @@ describe('Settings Loading and Merging', () => {
}); });
it('should NOT merge workspace settings when workspace is not trusted', () => { it('should NOT merge workspace settings when workspace is not trusted', () => {
vi.mocked(isWorkspaceTrusted).mockReturnValue({ vi.spyOn(trustedFolders, 'isWorkspaceTrusted').mockReturnValue({
isTrusted: false, isTrusted: false,
source: 'file', source: 'file',
}); });
@@ -1666,23 +1667,60 @@ describe('Settings Loading and Merging', () => {
expect(settings.merged.context?.fileName).toBe('USER.md'); // User setting expect(settings.merged.context?.fileName).toBe('USER.md'); // User setting
expect(settings.merged.ui?.theme).toBe('dark'); // 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', () => { describe('loadEnvironment', () => {
function setup({ function setup({
isFolderTrustEnabled = true, isFolderTrustEnabled = true,
isWorkspaceTrustedValue = true, isWorkspaceTrustedValue = true as boolean | undefined,
}) { }) {
delete process.env['TESTTEST']; // reset 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, isTrusted: isWorkspaceTrustedValue,
source: 'file', source: 'file',
}); });
(mockFsExistsSync as Mock).mockImplementation((p: fs.PathLike) => (mockFsExistsSync as Mock).mockImplementation((p: fs.PathLike) => {
[USER_SETTINGS_PATH, geminiEnvPath].includes(p.toString()), const normalizedP = path.resolve(p.toString());
); return [path.resolve(USER_SETTINGS_PATH), geminiEnvPath].includes(
normalizedP,
);
});
const userSettingsContent: Settings = { const userSettingsContent: Settings = {
ui: { ui: {
theme: 'dark', theme: 'dark',
@@ -1698,9 +1736,10 @@ describe('Settings Loading and Merging', () => {
}; };
(fs.readFileSync as Mock).mockImplementation( (fs.readFileSync as Mock).mockImplementation(
(p: fs.PathOrFileDescriptor) => { (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); return JSON.stringify(userSettingsContent);
if (p === geminiEnvPath) return 'TESTTEST=1234'; if (normalizedP === geminiEnvPath) return 'TESTTEST=1234';
return '{}'; return '{}';
}, },
); );
@@ -1708,14 +1747,34 @@ describe('Settings Loading and Merging', () => {
it('sets environment variables from .env files', () => { it('sets environment variables from .env files', () => {
setup({ isFolderTrustEnabled: false, isWorkspaceTrustedValue: true }); 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'); expect(process.env['TESTTEST']).toEqual('1234');
}); });
it('does not load env files from untrusted spaces', () => { it('does not load env files from untrusted spaces', () => {
setup({ isFolderTrustEnabled: true, isWorkspaceTrustedValue: false }); 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'); expect(process.env['TESTTEST']).not.toEqual('1234');
}); });
@@ -1731,7 +1790,7 @@ describe('Settings Loading and Merging', () => {
mockFsExistsSync.mockReturnValue(true); mockFsExistsSync.mockReturnValue(true);
mockFsReadFileSync = vi.mocked(fs.readFileSync); mockFsReadFileSync = vi.mocked(fs.readFileSync);
mockFsReadFileSync.mockReturnValue('{}'); mockFsReadFileSync.mockReturnValue('{}');
vi.mocked(isWorkspaceTrusted).mockReturnValue({ vi.spyOn(trustedFolders, 'isWorkspaceTrusted').mockReturnValue({
isTrusted: true, isTrusted: true,
source: undefined, source: undefined,
}); });
+14 -6
View File
@@ -50,7 +50,9 @@ import {
formatValidationError, formatValidationError,
} from './settings-validation.js'; } from './settings-validation.js';
function getMergeStrategyForPath(path: string[]): MergeStrategy | undefined { export function getMergeStrategyForPath(
path: string[],
): MergeStrategy | undefined {
let current: SettingDefinition | undefined = undefined; let current: SettingDefinition | undefined = undefined;
let currentSchema: SettingsSchema | undefined = getSettingsSchema(); let currentSchema: SettingsSchema | undefined = getSettingsSchema();
let parent: SettingDefinition | undefined = undefined; let parent: SettingDefinition | undefined = undefined;
@@ -432,10 +434,15 @@ export function setUpCloudShellEnvironment(envFilePath: string | null): void {
} }
} }
export function loadEnvironment(settings: Settings): void { export function loadEnvironment(
const envFilePath = findEnvFile(process.cwd()); 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; return;
} }
@@ -600,7 +607,8 @@ export function loadSettings(
userSettings, userSettings,
); );
const isTrusted = 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. // Create a temporary merged settings object to pass to loadEnvironment.
const tempMergedSettings = mergeSettings( const tempMergedSettings = mergeSettings(
@@ -613,7 +621,7 @@ export function loadSettings(
// loadEnvironment depends on settings so we have to create a temp version of // loadEnvironment depends on settings so we have to create a temp version of
// the settings to avoid a cycle // the settings to avoid a cycle
loadEnvironment(tempMergedSettings); loadEnvironment(tempMergedSettings, workspaceDir);
// Check for any fatal errors before proceeding // Check for any fatal errors before proceeding
const fatalErrors = settingsErrors.filter((e) => e.severity === 'error'); const fatalErrors = settingsErrors.filter((e) => e.severity === 'error');
@@ -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', () => { it('should handle path normalization', () => {
mockCwd = '/home/user/projectA'; mockCwd = '/home/user/projectA';
mockRules[`/home/user/../user/${path.basename('/home/user/projectA')}`] = mockRules[`/home/user/../user/${path.basename('/home/user/projectA')}`] =
+4 -2
View File
@@ -227,6 +227,7 @@ export function isFolderTrustEnabled(settings: Settings): boolean {
} }
function getWorkspaceTrustFromLocalConfig( function getWorkspaceTrustFromLocalConfig(
workspaceDir: string,
trustConfig?: Record<string, TrustLevel>, trustConfig?: Record<string, TrustLevel>,
): TrustResult { ): TrustResult {
const folders = loadTrustedFolders(); const folders = loadTrustedFolders();
@@ -241,7 +242,7 @@ function getWorkspaceTrustFromLocalConfig(
); );
} }
const isTrusted = folders.isPathTrusted(process.cwd(), configToUse); const isTrusted = folders.isPathTrusted(workspaceDir, configToUse);
return { return {
isTrusted, isTrusted,
source: isTrusted !== undefined ? 'file' : undefined, source: isTrusted !== undefined ? 'file' : undefined,
@@ -250,6 +251,7 @@ function getWorkspaceTrustFromLocalConfig(
export function isWorkspaceTrusted( export function isWorkspaceTrusted(
settings: Settings, settings: Settings,
workspaceDir: string = process.cwd(),
trustConfig?: Record<string, TrustLevel>, trustConfig?: Record<string, TrustLevel>,
): TrustResult { ): TrustResult {
if (!isFolderTrustEnabled(settings)) { if (!isFolderTrustEnabled(settings)) {
@@ -262,5 +264,5 @@ export function isWorkspaceTrusted(
} }
// Fall back to the local user configuration // Fall back to the local user configuration
return getWorkspaceTrustFromLocalConfig(trustConfig); return getWorkspaceTrustFromLocalConfig(workspaceDir, trustConfig);
} }
@@ -41,7 +41,10 @@ function getInitialTrustState(
}; };
} }
const { isTrusted, source } = isWorkspaceTrusted(settings.merged); const { isTrusted, source } = isWorkspaceTrusted(
settings.merged,
process.cwd(),
);
const isInheritedTrust = const isInheritedTrust =
isTrusted && isTrusted &&
@@ -99,7 +102,10 @@ export const usePermissionsModifyTrust = (
} }
// All logic below only applies when editing the current workspace. // 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 // Create a temporary config to check the new trust status without writing
const currentConfig = loadTrustedFolders().user.config; const currentConfig = loadTrustedFolders().user.config;
@@ -107,6 +113,7 @@ export const usePermissionsModifyTrust = (
const { isTrusted, source } = isWorkspaceTrusted( const { isTrusted, source } = isWorkspaceTrusted(
settings.merged, settings.merged,
process.cwd(),
newConfig, newConfig,
); );
@@ -26,7 +26,11 @@ import {
type Config, type Config,
type MessageBus, type MessageBus,
} from '@google/gemini-cli-core'; } 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 { loadCliConfig, type CliArgs } from '../config/config.js';
import * as fs from 'node:fs/promises'; import * as fs from 'node:fs/promises';
import * as path from 'node:path'; import * as path from 'node:path';
@@ -35,6 +39,14 @@ vi.mock('../config/config.js', () => ({
loadCliConfig: vi.fn(), loadCliConfig: vi.fn(),
})); }));
vi.mock('../config/settings.js', async (importOriginal) => {
const actual = await importOriginal<typeof import('../config/settings.js')>();
return {
...actual,
loadSettings: vi.fn(),
};
});
vi.mock('node:crypto', () => ({ vi.mock('node:crypto', () => ({
randomUUID: () => 'test-session-id', randomUUID: () => 'test-session-id',
})); }));
@@ -95,6 +107,10 @@ describe('GeminiAgent', () => {
initialize: vi.fn(), initialize: vi.fn(),
getFileSystemService: vi.fn(), getFileSystemService: vi.fn(),
setFileSystemService: 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({ getGeminiClient: vi.fn().mockReturnValue({
startChat: vi.fn().mockResolvedValue({}), startChat: vi.fn().mockResolvedValue({}),
}), }),
@@ -117,6 +133,13 @@ describe('GeminiAgent', () => {
} as unknown as Mocked<acp.AgentSideConnection>; } as unknown as Mocked<acp.AgentSideConnection>;
(loadCliConfig as unknown as Mock).mockResolvedValue(mockConfig); (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); agent = new GeminiAgent(mockConfig, mockSettings, mockArgv, mockConnection);
}); });
@@ -148,6 +171,9 @@ describe('GeminiAgent', () => {
}); });
it('should create a new session', async () => { it('should create a new session', async () => {
mockConfig.getContentGeneratorConfig = vi.fn().mockReturnValue({
apiKey: 'test-key',
});
const response = await agent.newSession({ const response = await agent.newSession({
cwd: '/tmp', cwd: '/tmp',
mcpServers: [], mcpServers: [],
@@ -159,6 +185,28 @@ describe('GeminiAgent', () => {
expect(mockConfig.getGeminiClient).toHaveBeenCalled(); 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 () => { it('should create a new session with mcp servers', async () => {
const mcpServers = [ const mcpServers = [
{ {
@@ -194,14 +242,14 @@ describe('GeminiAgent', () => {
mockConfig.refreshAuth.mockRejectedValue(new Error('Auth failed')); mockConfig.refreshAuth.mockRejectedValue(new Error('Auth failed'));
const debugSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); const debugSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
// Should throw RequestError.authRequired() // Should throw RequestError with custom message
await expect( await expect(
agent.newSession({ agent.newSession({
cwd: '/tmp', cwd: '/tmp',
mcpServers: [], mcpServers: [],
}), }),
).rejects.toMatchObject({ ).rejects.toMatchObject({
message: 'Authentication required', message: 'Auth failed',
}); });
debugSpy.mockRestore(); debugSpy.mockRestore();
@@ -40,7 +40,7 @@ import { AcpFileSystemService } from './fileSystemService.js';
import { Readable, Writable } from 'node:stream'; import { Readable, Writable } from 'node:stream';
import type { Content, Part, FunctionCall } from '@google/genai'; import type { Content, Part, FunctionCall } from '@google/genai';
import type { LoadedSettings } from '../config/settings.js'; 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 fs from 'node:fs/promises';
import * as path from 'node:path'; import * as path from 'node:path';
import { z } from 'zod'; import { z } from 'zod';
@@ -139,7 +139,11 @@ export class GeminiAgent {
// Refresh auth with the requested method // Refresh auth with the requested method
// This will reuse existing credentials if they're valid, // This will reuse existing credentials if they're valid,
// or perform new authentication if needed // 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( this.settings.setValue(
SettingScope.User, SettingScope.User,
'security.auth.selectedType', 'security.auth.selectedType',
@@ -152,12 +156,47 @@ export class GeminiAgent {
mcpServers, mcpServers,
}: acp.NewSessionRequest): Promise<acp.NewSessionResponse> { }: acp.NewSessionRequest): Promise<acp.NewSessionResponse> {
const sessionId = randomUUID(); const sessionId = randomUUID();
const config = await this.initializeSessionConfig( const loadedSettings = loadSettings(cwd);
const config = await this.newSessionConfig(
sessionId, sessionId,
cwd, cwd,
mcpServers, 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) { if (this.clientCapabilities?.fs) {
const acpFileSystemService = new AcpFileSystemService( const acpFileSystemService = new AcpFileSystemService(
this.connection, this.connection,
@@ -168,6 +207,9 @@ export class GeminiAgent {
config.setFileSystemService(acpFileSystemService); config.setFileSystemService(acpFileSystemService);
} }
await config.initialize();
startupProfiler.flush(config);
const geminiClient = config.getGeminiClient(); const geminiClient = config.getGeminiClient();
const chat = await geminiClient.startChat(); const chat = await geminiClient.startChat();
const session = new Session(sessionId, chat, config, this.connection); const session = new Session(sessionId, chat, config, this.connection);
@@ -264,8 +306,10 @@ export class GeminiAgent {
sessionId: string, sessionId: string,
cwd: string, cwd: string,
mcpServers: acp.McpServer[], mcpServers: acp.McpServer[],
loadedSettings?: LoadedSettings,
): Promise<Config> { ): Promise<Config> {
const mergedMcpServers = { ...this.settings.merged.mcpServers }; const currentSettings = loadedSettings || this.settings;
const mergedMcpServers = { ...currentSettings.merged.mcpServers };
for (const server of mcpServers) { for (const server of mcpServers) {
if ( 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 }); const config = await loadCliConfig(settings, sessionId, this.argv, { cwd });
@@ -497,7 +544,10 @@ export class Session {
return { stopReason: 'cancelled' }; return { stopReason: 'cancelled' };
} }
throw error; throw new acp.RequestError(
getErrorStatus(error) || 500,
getErrorMessage(error),
);
} }
if (functionCalls.length > 0) { if (functionCalls.length > 0) {
+57
View File
@@ -11,8 +11,65 @@ import {
toFriendlyError, toFriendlyError,
BadRequestError, BadRequestError,
ForbiddenError, ForbiddenError,
getErrorMessage,
} from './errors.js'; } 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', () => { describe('isAuthenticationError', () => {
it('should detect error with code: 401 property (MCP SDK style)', () => { it('should detect error with code: 401 property (MCP SDK style)', () => {
const error = { code: 401, message: 'Unauthorized' }; const error = { code: 401, message: 'Unauthorized' };
+33 -3
View File
@@ -15,11 +15,41 @@ export function isNodeError(error: unknown): error is NodeJS.ErrnoException {
} }
export function getErrorMessage(error: unknown): string { export function getErrorMessage(error: unknown): string {
if (error instanceof Error) { const friendlyError = toFriendlyError(error);
return error.message; 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 { try {
return String(error); return String(input);
} catch { } catch {
return 'Failed to get error details'; return 'Failed to get error details';
} }