diff --git a/packages/cli/src/commands/mcp/add.ts b/packages/cli/src/commands/mcp/add.ts index 47c481a29b..ecfc1b77c1 100644 --- a/packages/cli/src/commands/mcp/add.ts +++ b/packages/cli/src/commands/mcp/add.ts @@ -38,7 +38,7 @@ async function addMcpServer( } = options; const settingsScope = scope === 'user' ? SettingScope.User : SettingScope.Workspace; - const settings = loadSettings(process.cwd()); + const settings = loadSettings(); let newServer: Partial = {}; diff --git a/packages/cli/src/commands/mcp/list.ts b/packages/cli/src/commands/mcp/list.ts index 3d0f6e27cb..547f8631dd 100644 --- a/packages/cli/src/commands/mcp/list.ts +++ b/packages/cli/src/commands/mcp/list.ts @@ -20,8 +20,8 @@ const RESET_COLOR = '\u001b[0m'; async function getMcpServersFromConfig(): Promise< Record > { - const settings = loadSettings(process.cwd()); - const extensions = loadExtensions(process.cwd()); + const settings = loadSettings(); + const extensions = loadExtensions(); const mcpServers = { ...(settings.merged.mcpServers || {}) }; for (const extension of extensions) { Object.entries(extension.config.mcpServers || {}).forEach( diff --git a/packages/cli/src/commands/mcp/remove.ts b/packages/cli/src/commands/mcp/remove.ts index e05478e37e..bcaa5ad4bb 100644 --- a/packages/cli/src/commands/mcp/remove.ts +++ b/packages/cli/src/commands/mcp/remove.ts @@ -17,7 +17,7 @@ async function removeMcpServer( const { scope } = options; const settingsScope = scope === 'user' ? SettingScope.User : SettingScope.Workspace; - const settings = loadSettings(process.cwd()); + const settings = loadSettings(); const existingSettings = settings.forScope(settingsScope).settings; const mcpServers = existingSettings.mcpServers || {}; diff --git a/packages/cli/src/config/auth.ts b/packages/cli/src/config/auth.ts index 234a4d907a..8917ba8717 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(process.cwd()).merged); + loadEnvironment(loadSettings().merged); if ( authMethod === AuthType.LOGIN_WITH_GOOGLE || authMethod === AuthType.CLOUD_SHELL diff --git a/packages/cli/src/config/extension.ts b/packages/cli/src/config/extension.ts index 9e0ca4f358..e92bd65699 100644 --- a/packages/cli/src/config/extension.ts +++ b/packages/cli/src/config/extension.ts @@ -110,7 +110,9 @@ export async function performWorkspaceExtensionMigration( return failedInstallNames; } -export function loadExtensions(workspaceDir: string): Extension[] { +export function loadExtensions( + workspaceDir: string = process.cwd(), +): Extension[] { const settings = loadSettings(workspaceDir).merged; const disabledExtensions = settings.extensions?.disabled ?? []; const allExtensions = [...loadUserExtensions()]; diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 012f84fe9a..8f2c31bceb 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -60,7 +60,7 @@ import { type Settings, loadEnvironment, } from './settings.js'; -import { GEMINI_DIR } from '@google/gemini-cli-core'; +import { FatalConfigError, GEMINI_DIR } from '@google/gemini-cli-core'; const MOCK_WORKSPACE_DIR = '/mock/workspace'; // Use the (mocked) SETTINGS_DIRECTORY_NAME for consistency @@ -146,7 +146,6 @@ describe('Settings Loading and Merging', () => { }, security: {}, }); - expect(settings.errors.length).toBe(0); }); it('should load system settings if only system file exists', () => { @@ -317,75 +316,13 @@ describe('Settings Loading and Merging', () => { }); }); - it('should merge user and workspace settings, with workspace taking precedence', () => { - (mockFsExistsSync as Mock).mockReturnValue(true); - const userSettingsContent = { - ui: { - theme: 'dark', - }, - tools: { - sandbox: false, - }, - context: { - fileName: 'USER_CONTEXT.md', - }, - }; - const workspaceSettingsContent = { - tools: { - sandbox: true, - core: ['tool1'], - }, - context: { - fileName: 'WORKSPACE_CONTEXT.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.user.settings).toEqual(userSettingsContent); - expect(settings.workspace.settings).toEqual(workspaceSettingsContent); - expect(settings.merged).toEqual({ - general: {}, - ui: { - theme: 'dark', - customThemes: {}, - }, - tools: { - sandbox: true, - core: ['tool1'], - }, - context: { - fileName: 'WORKSPACE_CONTEXT.md', - includeDirectories: [], - }, - advanced: { - excludedEnvVars: [], - }, - extensions: { - disabled: [], - workspacesWithMigrationNudge: [], - }, - mcp: {}, - mcpServers: {}, - model: { - chatCompression: {}, - }, - security: {}, - }); - }); - it('should merge system, user and workspace settings, with system taking precedence over workspace, and workspace over user', () => { - (mockFsExistsSync as Mock).mockReturnValue(true); + (mockFsExistsSync as Mock).mockImplementation( + (p: fs.PathLike) => + p === getSystemSettingsPath() || + p === USER_SETTINGS_PATH || + p === MOCK_WORKSPACE_SETTINGS_PATH, + ); const systemSettingsContent = { ui: { theme: 'system-theme', @@ -869,7 +806,10 @@ describe('Settings Loading and Merging', () => { }); it('should merge excludedProjectEnvVars with workspace taking precedence over user', () => { - (mockFsExistsSync as Mock).mockReturnValue(true); + (mockFsExistsSync as Mock).mockImplementation( + (p: fs.PathLike) => + p === USER_SETTINGS_PATH || p === MOCK_WORKSPACE_SETTINGS_PATH, + ); const userSettingsContent = { general: {}, advanced: { excludedEnvVars: ['DEBUG', 'NODE_ENV', 'USER_VAR'] }, @@ -910,7 +850,10 @@ describe('Settings Loading and Merging', () => { }); it('should default contextFileName to undefined if not in any settings file', () => { - (mockFsExistsSync as Mock).mockReturnValue(true); + (mockFsExistsSync as Mock).mockImplementation( + (p: fs.PathLike) => + p === USER_SETTINGS_PATH || p === MOCK_WORKSPACE_SETTINGS_PATH, + ); const userSettingsContent = { ui: { theme: 'dark' } }; const workspaceSettingsContent = { tools: { sandbox: true } }; (fs.readFileSync as Mock).mockImplementation( @@ -986,7 +929,10 @@ describe('Settings Loading and Merging', () => { }); it('should merge MCP servers correctly, with workspace taking precedence', () => { - (mockFsExistsSync as Mock).mockReturnValue(true); + (mockFsExistsSync as Mock).mockImplementation( + (p: fs.PathLike) => + p === USER_SETTINGS_PATH || p === MOCK_WORKSPACE_SETTINGS_PATH, + ); const userSettingsContent = { mcpServers: { 'user-server': { @@ -1405,50 +1351,7 @@ describe('Settings Loading and Merging', () => { }, ); - const settings = loadSettings(MOCK_WORKSPACE_DIR); - - // Check that settings are empty due to parsing errors - expect(settings.user.settings).toEqual({}); - expect(settings.workspace.settings).toEqual({}); - expect(settings.merged).toEqual({ - general: {}, - ui: { - customThemes: {}, - }, - mcp: {}, - mcpServers: {}, - context: { - includeDirectories: [], - }, - model: { - chatCompression: {}, - }, - advanced: { - excludedEnvVars: [], - }, - extensions: { - disabled: [], - workspacesWithMigrationNudge: [], - }, - security: {}, - }); - - // Check that error objects are populated in settings.errors - expect(settings.errors).toBeDefined(); - // Assuming both user and workspace files cause errors and are added in order - expect(settings.errors.length).toEqual(2); - - const userError = settings.errors.find( - (e) => e.path === USER_SETTINGS_PATH, - ); - expect(userError).toBeDefined(); - expect(userError?.message).toBe(userReadError.message); - - const workspaceError = settings.errors.find( - (e) => e.path === MOCK_WORKSPACE_SETTINGS_PATH, - ); - expect(workspaceError).toBeDefined(); - expect(workspaceError?.message).toBe(workspaceReadError.message); + expect(() => loadSettings(MOCK_WORKSPACE_DIR)).toThrow(FatalConfigError); // Restore JSON.parse mock if it was spied on specifically for this test vi.restoreAllMocks(); // Or more targeted restore if needed diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index b7bc25f321..c42736528e 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -10,6 +10,7 @@ import { homedir, platform } from 'node:os'; import * as dotenv from 'dotenv'; import process from 'node:process'; import { + FatalConfigError, GEMINI_CONFIG_DIR as GEMINI_DIR, getErrorMessage, Storage, @@ -412,7 +413,6 @@ export class LoadedSettings { systemDefaults: SettingsFile, user: SettingsFile, workspace: SettingsFile, - errors: SettingsError[], isTrusted: boolean, migratedInMemorScopes: Set, ) { @@ -420,7 +420,6 @@ export class LoadedSettings { this.systemDefaults = systemDefaults; this.user = user; this.workspace = workspace; - this.errors = errors; this.isTrusted = isTrusted; this.migratedInMemorScopes = migratedInMemorScopes; this._merged = this.computeMergedSettings(); @@ -430,7 +429,6 @@ export class LoadedSettings { readonly systemDefaults: SettingsFile; readonly user: SettingsFile; readonly workspace: SettingsFile; - readonly errors: SettingsError[]; readonly isTrusted: boolean; readonly migratedInMemorScopes: Set; @@ -570,7 +568,9 @@ export function loadEnvironment(settings: Settings): void { * Loads settings from user and workspace directories. * Project settings override user settings. */ -export function loadSettings(workspaceDir: string): LoadedSettings { +export function loadSettings( + workspaceDir: string = process.cwd(), +): LoadedSettings { let systemSettings: Settings = {}; let systemDefaultSettings: Settings = {}; let userSettings: Settings = {}; @@ -703,7 +703,17 @@ export function loadSettings(workspaceDir: string): LoadedSettings { workspaceSettings = resolveEnvVarsInObject(workspaceSettings); // Create LoadedSettings first - const loadedSettings = new LoadedSettings( + + if (settingsErrors.length > 0) { + const errorMessages = settingsErrors.map( + (error) => `Error in ${error.path}: ${error.message}`, + ); + throw new FatalConfigError( + `${errorMessages.join('\n')}\nPlease fix the configuration file(s) and try again.`, + ); + } + + return new LoadedSettings( { path: systemSettingsPath, settings: systemSettings, @@ -720,12 +730,9 @@ export function loadSettings(workspaceDir: string): LoadedSettings { path: workspaceSettingsPath, settings: workspaceSettings, }, - settingsErrors, isTrusted, migratedInMemorScopes, ); - - return loadedSettings; } export function saveSettings(settingsFile: SettingsFile): void { diff --git a/packages/cli/src/gemini.test.tsx b/packages/cli/src/gemini.test.tsx index 5b7cd7edcc..bb002adb71 100644 --- a/packages/cli/src/gemini.test.tsx +++ b/packages/cli/src/gemini.test.tsx @@ -6,16 +6,13 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { - main, setupUnhandledRejectionHandler, validateDnsResolutionOrder, startInteractiveUI, } from './gemini.js'; -import type { SettingsFile } from './config/settings.js'; -import { LoadedSettings, loadSettings } from './config/settings.js'; +import { type LoadedSettings } from './config/settings.js'; import { appEvents, AppEvent } from './utils/events.js'; import type { Config } from '@google/gemini-cli-core'; -import { FatalConfigError } from '@google/gemini-cli-core'; // Custom error to identify mock process.exit calls class MockProcessExitError extends Error { @@ -75,7 +72,6 @@ vi.mock('./utils/sandbox.js', () => ({ })); describe('gemini.tsx main function', () => { - let loadSettingsMock: ReturnType>; let originalEnvGeminiSandbox: string | undefined; let originalEnvSandbox: string | undefined; let initialUnhandledRejectionListeners: NodeJS.UnhandledRejectionListener[] = @@ -88,8 +84,6 @@ describe('gemini.tsx main function', () => { }); beforeEach(() => { - loadSettingsMock = vi.mocked(loadSettings); - // Store and clear sandbox-related env variables to ensure a consistent test environment originalEnvGeminiSandbox = process.env['GEMINI_SANDBOX']; originalEnvSandbox = process.env['SANDBOX']; @@ -124,42 +118,6 @@ describe('gemini.tsx main function', () => { vi.restoreAllMocks(); }); - it('should throw InvalidConfigurationError if settings have errors', async () => { - const settingsError = { - message: 'Test settings error', - path: '/test/settings.json', - }; - const userSettingsFile: SettingsFile = { - path: '/user/settings.json', - settings: {}, - }; - const workspaceSettingsFile: SettingsFile = { - path: '/workspace/.gemini/settings.json', - settings: {}, - }; - const systemSettingsFile: SettingsFile = { - path: '/system/settings.json', - settings: {}, - }; - const systemDefaultsFile: SettingsFile = { - path: '/system/system-defaults.json', - settings: {}, - }; - const mockLoadedSettings = new LoadedSettings( - systemSettingsFile, - systemDefaultsFile, - userSettingsFile, - workspaceSettingsFile, - [settingsError], - true, - new Set(), - ); - - loadSettingsMock.mockReturnValue(mockLoadedSettings); - - await expect(main()).rejects.toThrow(FatalConfigError); - }); - it('should log unhandled promise rejections and open debug console on first error', async () => { const appEventsMock = vi.mocked(appEvents); const rejectionError = new Error('Test unhandled rejection'); diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index 51291d6600..a358b83bdf 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -39,7 +39,6 @@ import { logIdeConnection, IdeConnectionEvent, IdeConnectionType, - FatalConfigError, uiTelemetryService, } from '@google/gemini-cli-core'; import { validateAuthMethod } from './config/auth.js'; @@ -173,7 +172,7 @@ export async function startInteractiveUI( config: Config, settings: LoadedSettings, startupWarnings: string[], - workspaceRoot: string, + workspaceRoot: string = process.cwd(), ) { const version = await getCliVersion(); // Detect and enable Kitty keyboard protocol once at startup @@ -209,21 +208,12 @@ export async function startInteractiveUI( export async function main() { setupUnhandledRejectionHandler(); - const workspaceRoot = process.cwd(); - const settings = loadSettings(workspaceRoot); + const settings = loadSettings(); await cleanupCheckpoints(); - if (settings.errors.length > 0) { - const errorMessages = settings.errors.map( - (error) => `Error in ${error.path}: ${error.message}`, - ); - throw new FatalConfigError( - `${errorMessages.join('\n')}\nPlease fix the configuration file(s) and try again.`, - ); - } const argv = await parseArguments(settings.merged); - const extensions = loadExtensions(workspaceRoot); + const extensions = loadExtensions(); const config = await loadCliConfig( settings.merged, extensions, @@ -398,12 +388,12 @@ export async function main() { let input = config.getQuestion(); const startupWarnings = [ ...(await getStartupWarnings()), - ...(await getUserStartupWarnings(workspaceRoot)), + ...(await getUserStartupWarnings()), ]; // Render UI, passing necessary config values. Check that there is no command line question. if (config.isInteractive()) { - await startInteractiveUI(config, settings, startupWarnings, workspaceRoot); + await startInteractiveUI(config, settings, startupWarnings); return; } // If not a TTY, read from stdin diff --git a/packages/cli/src/ui/App.test.tsx b/packages/cli/src/ui/App.test.tsx index 12779bdb0d..bfc62322bb 100644 --- a/packages/cli/src/ui/App.test.tsx +++ b/packages/cli/src/ui/App.test.tsx @@ -336,7 +336,6 @@ describe('App UI', () => { systemDefaultsFile, userSettingsFile, workspaceSettingsFile, - [], true, new Set(), ); diff --git a/packages/cli/src/ui/components/AuthDialog.test.tsx b/packages/cli/src/ui/components/AuthDialog.test.tsx index 845ae7dbb6..dc5d6090d3 100644 --- a/packages/cli/src/ui/components/AuthDialog.test.tsx +++ b/packages/cli/src/ui/components/AuthDialog.test.tsx @@ -52,7 +52,6 @@ describe('AuthDialog', () => { settings: { ui: { customThemes: {} }, mcpServers: {} }, path: '', }, - [], true, new Set(), ); @@ -95,7 +94,6 @@ describe('AuthDialog', () => { settings: { ui: { customThemes: {} }, mcpServers: {} }, path: '', }, - [], true, new Set(), ); @@ -134,7 +132,6 @@ describe('AuthDialog', () => { settings: { ui: { customThemes: {} }, mcpServers: {} }, path: '', }, - [], true, new Set(), ); @@ -173,7 +170,6 @@ describe('AuthDialog', () => { settings: { ui: { customThemes: {} }, mcpServers: {} }, path: '', }, - [], true, new Set(), ); @@ -213,7 +209,6 @@ describe('AuthDialog', () => { settings: { ui: { customThemes: {} }, mcpServers: {} }, path: '', }, - [], true, new Set(), ); @@ -248,7 +243,6 @@ describe('AuthDialog', () => { settings: { ui: { customThemes: {} }, mcpServers: {} }, path: '', }, - [], true, new Set(), ); @@ -285,7 +279,6 @@ describe('AuthDialog', () => { settings: { ui: { customThemes: {} }, mcpServers: {} }, path: '', }, - [], true, new Set(), ); @@ -326,7 +319,6 @@ describe('AuthDialog', () => { settings: { ui: { customThemes: {} }, mcpServers: {} }, path: '', }, - [], true, new Set(), ); @@ -371,7 +363,6 @@ describe('AuthDialog', () => { settings: { ui: { customThemes: {} }, mcpServers: {} }, path: '', }, - [], true, new Set(), ); @@ -419,7 +410,6 @@ describe('AuthDialog', () => { settings: { ui: { customThemes: {} }, mcpServers: {} }, path: '', }, - [], true, new Set(), ); diff --git a/packages/cli/src/ui/components/SettingsDialog.test.tsx b/packages/cli/src/ui/components/SettingsDialog.test.tsx index fe546f03f9..e5fc79f1c5 100644 --- a/packages/cli/src/ui/components/SettingsDialog.test.tsx +++ b/packages/cli/src/ui/components/SettingsDialog.test.tsx @@ -63,7 +63,6 @@ const createMockSettings = ( }, path: '/workspace/settings.json', }, - [], true, new Set(), ); @@ -188,7 +187,6 @@ describe('SettingsDialog', () => { }, path: '/workspace/settings.json', }, - [], true, new Set(), ); diff --git a/packages/cli/src/ui/utils/MarkdownDisplay.test.tsx b/packages/cli/src/ui/utils/MarkdownDisplay.test.tsx index 78da567b40..d93437bb0a 100644 --- a/packages/cli/src/ui/utils/MarkdownDisplay.test.tsx +++ b/packages/cli/src/ui/utils/MarkdownDisplay.test.tsx @@ -23,7 +23,6 @@ describe('', () => { { path: '', settings: {} }, { path: '', settings: {} }, { path: '', settings: {} }, - [], true, new Set(), ); @@ -227,7 +226,6 @@ Another paragraph. { path: '', settings: {} }, { path: '', settings: { ui: { showLineNumbers: false } } }, { path: '', settings: {} }, - [], true, new Set(), ); diff --git a/packages/cli/src/utils/userStartupWarnings.ts b/packages/cli/src/utils/userStartupWarnings.ts index 165f6e2e68..d355290789 100644 --- a/packages/cli/src/utils/userStartupWarnings.ts +++ b/packages/cli/src/utils/userStartupWarnings.ts @@ -60,7 +60,7 @@ const WARNING_CHECKS: readonly WarningCheck[] = [ ]; export async function getUserStartupWarnings( - workspaceRoot: string, + workspaceRoot: string = process.cwd(), ): Promise { const results = await Promise.all( WARNING_CHECKS.map((check) => check.check(workspaceRoot)),