fix: address code review and resolve setting propagation bugs

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',
         },
       });
 

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];
           }
         }

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<typeof import('../config/settings.js')>();
+  return {
+    ...actual,
+    loadSettings: vi.fn(),
+  };
+});
+
 vi.mock('node:crypto', () => ({
   randomUUID: () => 'test-session-id',
 }));
@@ -117,6 +129,13 @@ describe('GeminiAgent', () => {
     } as unknown as Mocked<acp.AgentSideConnection>;
 
     (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({

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<acp.NewSessionResponse> {
     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<Config> {
-    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 });