From b6b0727e28b796790c8bf9b8721e04b749875f85 Mon Sep 17 00:00:00 2001 From: Jacob Richman Date: Tue, 23 Dec 2025 10:26:37 -0800 Subject: [PATCH] Make schema validation errors non-fatal (#15487) --- .../src/config/settings-validation.test.ts | 4 +- .../cli/src/config/settings-validation.ts | 2 +- packages/cli/src/config/settings.ts | 25 ++- .../settings_validation_warning.test.ts | 156 ++++++++++++++++++ packages/cli/src/gemini.tsx | 17 ++ 5 files changed, 191 insertions(+), 13 deletions(-) create mode 100644 packages/cli/src/config/settings_validation_warning.test.ts diff --git a/packages/cli/src/config/settings-validation.test.ts b/packages/cli/src/config/settings-validation.test.ts index 908da8e01c..4c9579e5f0 100644 --- a/packages/cli/src/config/settings-validation.test.ts +++ b/packages/cli/src/config/settings-validation.test.ts @@ -323,9 +323,7 @@ describe('settings-validation', () => { expect(formatted).toContain('/path/to/settings.json'); expect(formatted).toContain('model.name'); expect(formatted).toContain('Expected: string, but received: object'); - expect(formatted).toContain( - 'Please fix the configuration and try again.', - ); + expect(formatted).toContain('Please fix the configuration.'); expect(formatted).toContain( 'https://github.com/google-gemini/gemini-cli', ); diff --git a/packages/cli/src/config/settings-validation.ts b/packages/cli/src/config/settings-validation.ts index f7f267a410..da06cf082e 100644 --- a/packages/cli/src/config/settings-validation.ts +++ b/packages/cli/src/config/settings-validation.ts @@ -322,7 +322,7 @@ export function formatValidationError( lines.push(''); } - lines.push('Please fix the configuration and try again.'); + lines.push('Please fix the configuration.'); lines.push( 'See: https://github.com/google-gemini/gemini-cli/blob/main/docs/get-started/configuration.md', ); diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 5506ff8dba..5c7294b601 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -233,6 +233,7 @@ export interface SessionRetentionSettings { export interface SettingsError { message: string; path: string; + severity: 'error' | 'warning'; } export interface SettingsFile { @@ -456,6 +457,7 @@ export class LoadedSettings { workspace: SettingsFile, isTrusted: boolean, migratedInMemoryScopes: Set, + errors: SettingsError[] = [], ) { this.system = system; this.systemDefaults = systemDefaults; @@ -463,6 +465,7 @@ export class LoadedSettings { this.workspace = workspace; this.isTrusted = isTrusted; this.migratedInMemoryScopes = migratedInMemoryScopes; + this.errors = errors; this._merged = this.computeMergedSettings(); } @@ -472,6 +475,7 @@ export class LoadedSettings { readonly workspace: SettingsFile; readonly isTrusted: boolean; readonly migratedInMemoryScopes: Set; + readonly errors: SettingsError[]; private _merged: Settings; @@ -658,6 +662,7 @@ export function loadSettings( settingsErrors.push({ message: 'Settings file is not a valid JSON object.', path: filePath, + severity: 'error', }); return { settings: {} }; } @@ -695,19 +700,20 @@ export function loadSettings( validationResult.error, filePath, ); - throw new FatalConfigError(errorMessage); + settingsErrors.push({ + message: errorMessage, + path: filePath, + severity: 'warning', + }); } return { settings: settingsObject as Settings, rawJson: content }; } } catch (error: unknown) { - // Preserve FatalConfigError with formatted validation messages - if (error instanceof FatalConfigError) { - throw error; - } settingsErrors.push({ message: getErrorMessage(error), path: filePath, + severity: 'error', }); } return { settings: {} }; @@ -779,10 +785,10 @@ export function loadSettings( // the settings to avoid a cycle loadEnvironment(tempMergedSettings); - // Create LoadedSettings first - - if (settingsErrors.length > 0) { - const errorMessages = settingsErrors.map( + // Check for any fatal errors before proceeding + const fatalErrors = settingsErrors.filter((e) => e.severity === 'error'); + if (fatalErrors.length > 0) { + const errorMessages = fatalErrors.map( (error) => `Error in ${error.path}: ${error.message}`, ); throw new FatalConfigError( @@ -817,6 +823,7 @@ export function loadSettings( }, isTrusted, migratedInMemoryScopes, + settingsErrors, ); } diff --git a/packages/cli/src/config/settings_validation_warning.test.ts b/packages/cli/src/config/settings_validation_warning.test.ts new file mode 100644 index 0000000000..67212bf0bc --- /dev/null +++ b/packages/cli/src/config/settings_validation_warning.test.ts @@ -0,0 +1,156 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/// + +import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; +import * as fs from 'node:fs'; + +const mockCoreEvents = vi.hoisted(() => ({ + emitFeedback: vi.fn(), + emitConsoleLog: vi.fn(), + emitOutput: vi.fn(), + emitModelChanged: vi.fn(), + drainBacklogs: vi.fn(), +})); + +const mockIsWorkspaceTrusted = vi.hoisted(() => + vi.fn().mockReturnValue({ isTrusted: true, source: 'file' }), +); + +vi.mock('@google/gemini-cli-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + coreEvents: mockCoreEvents, + Storage: class extends actual.Storage { + static override getGlobalSettingsPath = () => + '/mock/home/user/.gemini/settings.json'; + override getWorkspaceSettingsPath = () => + '/mock/workspace/.gemini/settings.json'; + static override getGlobalGeminiDir = () => '/mock/home/user/.gemini'; + }, + }; +}); + +vi.mock('./trustedFolders.js', () => ({ + isWorkspaceTrusted: mockIsWorkspaceTrusted, + loadTrustedFolders: vi.fn().mockReturnValue({ + isPathTrusted: vi.fn().mockReturnValue(true), + user: { config: {} }, + errors: [], + }), + isFolderTrustEnabled: vi.fn().mockReturnValue(false), + TrustLevel: { + TRUST_FOLDER: 'TRUST_FOLDER', + TRUST_PARENT: 'TRUST_PARENT', + DO_NOT_TRUST: 'DO_NOT_TRUST', + }, +})); + +vi.mock('os', () => ({ + homedir: () => '/mock/home/user', + platform: () => 'linux', + totalmem: () => 16 * 1024 * 1024 * 1024, +})); + +vi.mock('fs', async (importOriginal) => { + const actualFs = await importOriginal(); + return { + ...actualFs, + existsSync: vi.fn(), + readFileSync: vi.fn(), + writeFileSync: vi.fn(), + mkdirSync: vi.fn(), + renameSync: vi.fn(), + realpathSync: (p: string) => p, + }; +}); + +// Import loadSettings after all mocks are defined +import { + loadSettings, + USER_SETTINGS_PATH, + type LoadedSettings, +} from './settings.js'; + +const MOCK_WORKSPACE_DIR = '/mock/workspace'; + +describe('Settings Validation Warning', () => { + beforeEach(() => { + vi.clearAllMocks(); + (fs.readFileSync as Mock).mockReturnValue('{}'); + (fs.existsSync as Mock).mockReturnValue(false); + }); + + it('should emit a warning and NOT throw when settings are invalid', () => { + (fs.existsSync as Mock).mockImplementation( + (p: string) => p === USER_SETTINGS_PATH, + ); + + const invalidSettingsContent = { + ui: { + customThemes: { + terafox: { + name: 'terafox', + type: 'custom', + DiffModified: '#ffffff', // Invalid key + }, + }, + }, + }; + + (fs.readFileSync as Mock).mockImplementation((p: string) => { + if (p === USER_SETTINGS_PATH) + return JSON.stringify(invalidSettingsContent); + return '{}'; + }); + + // Should NOT throw + let settings: LoadedSettings | undefined; + expect(() => { + settings = loadSettings(MOCK_WORKSPACE_DIR); + }).not.toThrow(); + + // Should have recorded a warning in the settings object + expect( + settings?.errors.some((e) => + e.message.includes("Unrecognized key(s) in object: 'DiffModified'"), + ), + ).toBe(true); + }); + + it('should throw a fatal error when settings file is not a valid JSON object', () => { + (fs.existsSync as Mock).mockImplementation( + (p: string) => p === USER_SETTINGS_PATH, + ); + + (fs.readFileSync as Mock).mockImplementation((p: string) => { + if (p === USER_SETTINGS_PATH) return '[]'; + return '{}'; + }); + + expect(() => { + loadSettings(MOCK_WORKSPACE_DIR); + }).toThrow(); + }); + + it('should throw a fatal error when settings file contains invalid JSON', () => { + (fs.existsSync as Mock).mockImplementation( + (p: string) => p === USER_SETTINGS_PATH, + ); + + (fs.readFileSync as Mock).mockImplementation((p: string) => { + if (p === USER_SETTINGS_PATH) return '{ "invalid": "json", }'; // Trailing comma is invalid in standard JSON + return '{}'; + }); + + expect(() => { + loadSettings(MOCK_WORKSPACE_DIR); + }).toThrow(); + }); +}); diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index da5de6f5e2..adc6e171b8 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -16,6 +16,10 @@ import os from 'node:os'; import dns from 'node:dns'; import { start_sandbox } from './utils/sandbox.js'; import type { DnsResolutionOrder, LoadedSettings } from './config/settings.js'; +import { + loadTrustedFolders, + type TrustedFoldersError, +} from './config/trustedFolders.js'; import { loadSettings, migrateDeprecatedSettings, @@ -299,6 +303,19 @@ export async function main() { const settings = loadSettings(); loadSettingsHandle?.end(); + // Report settings errors once during startup + settings.errors.forEach((error) => { + coreEvents.emitFeedback('warning', error.message); + }); + + const trustedFolders = loadTrustedFolders(); + trustedFolders.errors.forEach((error: TrustedFoldersError) => { + coreEvents.emitFeedback( + 'warning', + `Error in ${error.path}: ${error.message}`, + ); + }); + const migrateHandle = startupProfiler.start('migrate_settings'); migrateDeprecatedSettings( settings,