MediaMetz/gemini-cli
1
0
Fork 0
mirror of https://github.com/google-gemini/gemini-cli.git synced 2026-03-10 22:21:22 -07:00
Code Issues Packages Projects Releases Wiki Activity

Move settings error throwing to loadSettings (#7605)

Browse Source
This commit is contained in:
Tommaso Sciortino
2025-09-03 10:41:53 -07:00
committed by GitHub
parent d2ae869bb4
commit e6e60861e5
14 changed files with 50 additions and 205 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
packages/cli/src/commands/mcp/add.ts
View File

@@ -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<MCPServerConfig> = {};

4
packages/cli/src/commands/mcp/list.ts
View File

@@ -20,8 +20,8 @@ const RESET_COLOR = '\u001b[0m';
async function getMcpServersFromConfig(): Promise<
Record<string, MCPServerConfig>
> {
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(

2
packages/cli/src/commands/mcp/remove.ts
View File

@@ -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 || {};

2
packages/cli/src/config/auth.ts
View File

@@ -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

4
packages/cli/src/config/extension.ts
View File

@@ -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()];

137
packages/cli/src/config/settings.test.ts
View File

@@ -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

23
packages/cli/src/config/settings.ts
View File

@@ -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<SettingScope>,
) {
@@ -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<SettingScope>;
@@ -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 {

44
packages/cli/src/gemini.test.tsx
View File

@@ -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<typeof vi.mocked<typeof loadSettings>>;
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');

20
packages/cli/src/gemini.tsx
View File

@@ -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

1
packages/cli/src/ui/App.test.tsx
View File

@@ -336,7 +336,6 @@ describe('App UI', () => {
systemDefaultsFile,
userSettingsFile,
workspaceSettingsFile,
[],
true,
new Set(),
);

10
packages/cli/src/ui/components/AuthDialog.test.tsx
View File

@@ -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(),
);

2
packages/cli/src/ui/components/SettingsDialog.test.tsx
View File

@@ -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(),
);

2
packages/cli/src/ui/utils/MarkdownDisplay.test.tsx
View File

@@ -23,7 +23,6 @@ describe('<MarkdownDisplay />', () => {
{ 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(),
);

2
packages/cli/src/utils/userStartupWarnings.ts
View File

@@ -60,7 +60,7 @@ const WARNING_CHECKS: readonly WarningCheck[] = [
];
export async function getUserStartupWarnings(
workspaceRoot: string,
workspaceRoot: string = process.cwd(),
): Promise<string[]> {
const results = await Promise.all(
WARNING_CHECKS.map((check) => check.check(workspaceRoot)),