Make schema validation errors non-fatal (#15487)

This commit is contained in:
Jacob Richman
2025-12-23 10:26:37 -08:00
committed by GitHub
parent e9a601c1fe
commit b6b0727e28
5 changed files with 191 additions and 13 deletions

View File

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

View File

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

View File

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

View File

@@ -0,0 +1,156 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
/// <reference types="vitest/globals" />
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<typeof import('@google/gemini-cli-core')>();
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<typeof fs>();
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();
});
});

View File

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