From 5549fbf8e51b9ebb55f70aa6377f484474b71db5 Mon Sep 17 00:00:00 2001 From: Bryan Morgan Date: Sat, 31 Jan 2026 14:43:11 -0500 Subject: [PATCH] fix: address code review and resolve setting propagation bugs --- integration-tests/acp-env-auth.test.ts | 60 +++++++++++-------- packages/cli/src/config/settings.ts | 11 ++-- .../zed-integration/zedIntegration.test.ts | 29 ++++++++- .../cli/src/zed-integration/zedIntegration.ts | 52 +++++++++------- 4 files changed, 100 insertions(+), 52 deletions(-) diff --git a/integration-tests/acp-env-auth.test.ts b/integration-tests/acp-env-auth.test.ts index b7d4b3bbc7..9f51d2f690 100644 --- a/integration-tests/acp-env-auth.test.ts +++ b/integration-tests/acp-env-auth.test.ts @@ -41,11 +41,11 @@ describe('ACP Environment and Auth', () => { }); itMaybe( - 'should load .env from project directory and fail if API key is invalid', + '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 + // Create a project directory with a .env file containing a recognizable invalid key const projectDir = join(rig.testDir!, 'project'); mkdirSync(projectDir, { recursive: true }); writeFileSync( @@ -61,7 +61,8 @@ describe('ACP Environment and Auth', () => { env: { ...process.env, GEMINI_CLI_HOME: rig.homeDir!, - GEMINI_API_KEY: '', + GEMINI_API_KEY: undefined, + VERBOSE: 'true', }, }); @@ -80,27 +81,33 @@ describe('ACP Environment and Auth', () => { }, }); - try { - await connection.newSession({ - cwd: projectDir, - mcpServers: [], - }); - } catch (error) { - if (error instanceof Error) { - if (error.message.includes('Authentication required')) { - throw new Error( - `Expected session to be authenticated via .env, but got: ${error.message}`, - ); - } - } else { - const errorMsg = String(error); - if (errorMsg.includes('Authentication required')) { - throw new Error( - `Expected session to be authenticated via .env, but got: ${errorMsg}`, - ); - } - } - } + // 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(); }, @@ -114,12 +121,13 @@ describe('ACP Environment and Auth', () => { const bundlePath = join(import.meta.dirname, '..', 'bundle/gemini.js'); child = spawn('node', [bundlePath, '--experimental-acp'], { - cwd: rig.testDir!, + cwd: rig.homeDir!, stdio: ['pipe', 'pipe', 'inherit'], env: { ...process.env, GEMINI_CLI_HOME: rig.homeDir!, - GEMINI_API_KEY: '', + GEMINI_API_KEY: undefined, + VERBOSE: 'true', }, }); diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 1dba067f2d..9af0970dd3 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; @@ -437,8 +439,9 @@ export function loadEnvironment( workspaceDir: string, ): void { const envFilePath = findEnvFile(workspaceDir); + const trustResult = isWorkspaceTrusted(settings, workspaceDir); - if (isWorkspaceTrusted(settings, workspaceDir).isTrusted === false) { + if (trustResult.isTrusted === false) { return; } @@ -465,8 +468,8 @@ export function loadEnvironment( continue; } - // Load variable only if it's not already set in the environment. - if (!Object.hasOwn(process.env, key)) { + // Load variable only if it's not already set in the environment or if it's an empty string. + if (!process.env[key]) { process.env[key] = parsedEnv[key]; } } diff --git a/packages/cli/src/zed-integration/zedIntegration.test.ts b/packages/cli/src/zed-integration/zedIntegration.test.ts index 095bea4917..2f35b48a98 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', })); @@ -117,6 +129,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); }); @@ -163,10 +182,16 @@ describe('GeminiAgent', () => { }); 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, }); - mockSettings.merged.security.auth.selectedType = AuthType.USE_GEMINI; await expect( agent.newSession({ diff --git a/packages/cli/src/zed-integration/zedIntegration.ts b/packages/cli/src/zed-integration/zedIntegration.ts index f8c79ba784..f336337232 100644 --- a/packages/cli/src/zed-integration/zedIntegration.ts +++ b/packages/cli/src/zed-integration/zedIntegration.ts @@ -38,7 +38,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'; @@ -146,27 +146,34 @@ export class GeminiAgent { mcpServers, }: acp.NewSessionRequest): Promise { const sessionId = randomUUID(); - const config = await this.newSessionConfig(sessionId, cwd, mcpServers); + 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; - if (this.settings.merged.security.auth.selectedType) { - try { - await config.refreshAuth( - this.settings.merged.security.auth.selectedType, - ); - isAuthenticated = true; + try { + await config.refreshAuth(authType); + isAuthenticated = true; - // Extra validation for Gemini API key - if ( - this.settings.merged.security.auth.selectedType === - AuthType.USE_GEMINI && - !config.getContentGeneratorConfig().apiKey - ) { - isAuthenticated = false; - } - } catch (e) { - debugLogger.error(`Authentication failed: ${e}`); + // Extra validation for Gemini API key + const contentGeneratorConfig = config.getContentGeneratorConfig(); + if ( + authType === AuthType.USE_GEMINI && + (!contentGeneratorConfig || !contentGeneratorConfig.apiKey) + ) { + isAuthenticated = false; } + } catch (e) { + debugLogger.error( + `Authentication failed: ${e instanceof Error ? e.stack : e}`, + ); } if (!isAuthenticated) { @@ -197,8 +204,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 ( @@ -233,7 +242,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 });