Exit with an error message if parsing settings.json fails. (#747)

This commit is contained in:
Jacob Richman 2025-06-06 09:56:45 -07:00 committed by GitHub
parent b4a6b16227
commit 89aca349cf
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 246 additions and 32 deletions

View File

@ -7,3 +7,15 @@
*/
import './src/gemini.js';
import { main } from './src/gemini.js';
// --- Global Entry Point ---
main().catch((error) => {
console.error('An unexpected critical error occurred:');
if (error instanceof Error) {
console.error(error.message);
} else {
console.error(String(error));
}
process.exit(1);
});

View File

@ -96,6 +96,7 @@ describe('Settings Loading and Merging', () => {
expect(settings.user.settings).toEqual({});
expect(settings.workspace.settings).toEqual({});
expect(settings.merged).toEqual({});
expect(settings.errors.length).toBe(0);
});
it('should load user settings if only user file exists', () => {
@ -300,27 +301,61 @@ describe('Settings Loading and Merging', () => {
});
it('should handle JSON parsing errors gracefully', () => {
(mockFsExistsSync as Mock).mockReturnValue(true);
(mockFsExistsSync as Mock).mockReturnValue(true); // Both files "exist"
const invalidJsonContent = 'invalid json';
const userReadError = new SyntaxError(
"Expected ',' or '}' after property value in JSON at position 10",
);
const workspaceReadError = new SyntaxError(
'Unexpected token i in JSON at position 0',
);
(fs.readFileSync as Mock).mockImplementation(
(p: fs.PathOrFileDescriptor) => {
// Make it return invalid json for the paths it will try to read
if (p === USER_SETTINGS_PATH || p === MOCK_WORKSPACE_SETTINGS_PATH)
return 'invalid json';
return '';
if (p === USER_SETTINGS_PATH) {
// Simulate JSON.parse throwing for user settings
vi.spyOn(JSON, 'parse').mockImplementationOnce(() => {
throw userReadError;
});
return invalidJsonContent; // Content that would cause JSON.parse to throw
}
if (p === MOCK_WORKSPACE_SETTINGS_PATH) {
// Simulate JSON.parse throwing for workspace settings
vi.spyOn(JSON, 'parse').mockImplementationOnce(() => {
throw workspaceReadError;
});
return invalidJsonContent;
}
return '{}'; // Default for other reads
},
);
const consoleErrorSpy = vi
.spyOn(console, 'error')
.mockImplementation(() => {});
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({});
expect(consoleErrorSpy).toHaveBeenCalledTimes(2);
consoleErrorSpy.mockRestore();
// 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);
// Restore JSON.parse mock if it was spied on specifically for this test
vi.restoreAllMocks(); // Or more targeted restore if needed
});
it('should resolve environment variables in user settings', () => {
@ -585,13 +620,14 @@ describe('Settings Loading and Merging', () => {
'MY_AGENTS.md',
);
expect(loadedSettings.merged.contextFileName).toBe('MY_AGENTS.md');
expect(loadedSettings.merged.theme).toBe('matrix');
expect(loadedSettings.merged.theme).toBe('matrix'); // User setting should still be there
expect(fs.writeFileSync).toHaveBeenCalledWith(
MOCK_WORKSPACE_SETTINGS_PATH,
JSON.stringify({ contextFileName: 'MY_AGENTS.md' }, null, 2),
'utf-8',
);
// Workspace theme overrides user theme
loadedSettings.setValue(SettingScope.Workspace, 'theme', 'ocean');
expect(loadedSettings.workspace.settings.theme).toBe('ocean');

View File

@ -7,7 +7,7 @@
import * as fs from 'fs';
import * as path from 'path';
import { homedir } from 'os';
import { MCPServerConfig } from '@gemini-code/core';
import { MCPServerConfig, getErrorMessage } from '@gemini-code/core';
import stripJsonComments from 'strip-json-comments';
import { DefaultLight } from '../ui/themes/default-light.js';
import { DefaultDark } from '../ui/themes/default.js';
@ -47,19 +47,30 @@ export interface Settings {
// Add other settings here.
}
export interface SettingsError {
message: string;
path: string;
}
export interface SettingsFile {
settings: Settings;
path: string;
}
export class LoadedSettings {
constructor(user: SettingsFile, workspace: SettingsFile) {
constructor(
user: SettingsFile,
workspace: SettingsFile,
errors: SettingsError[],
) {
this.user = user;
this.workspace = workspace;
this.errors = errors;
this._merged = this.computeMergedSettings();
}
readonly user: SettingsFile;
readonly workspace: SettingsFile;
readonly errors: SettingsError[];
private _merged: Settings;
@ -147,6 +158,7 @@ function resolveEnvVarsInObject<T>(obj: T): T {
export function loadSettings(workspaceDir: string): LoadedSettings {
let userSettings: Settings = {};
let workspaceSettings: Settings = {};
const settingsErrors: SettingsError[] = [];
// Load user settings
try {
@ -163,8 +175,11 @@ export function loadSettings(workspaceDir: string): LoadedSettings {
userSettings.theme = DefaultDark.name;
}
}
} catch (error) {
console.error('Error reading user settings file:', error);
} catch (error: unknown) {
settingsErrors.push({
message: getErrorMessage(error),
path: USER_SETTINGS_PATH,
});
}
const workspaceSettingsPath = path.join(
@ -190,13 +205,23 @@ export function loadSettings(workspaceDir: string): LoadedSettings {
workspaceSettings.theme = DefaultDark.name;
}
}
} catch (error) {
console.error('Error reading workspace settings file:', error);
} catch (error: unknown) {
settingsErrors.push({
message: getErrorMessage(error),
path: workspaceSettingsPath,
});
}
return new LoadedSettings(
{ path: USER_SETTINGS_PATH, settings: userSettings },
{ path: workspaceSettingsPath, settings: workspaceSettings },
{
path: USER_SETTINGS_PATH,
settings: userSettings,
},
{
path: workspaceSettingsPath,
settings: workspaceSettings,
},
settingsErrors,
);
}

View File

@ -0,0 +1,140 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
import stripAnsi from 'strip-ansi';
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import { main } from './gemini.js';
import {
LoadedSettings,
SettingsFile,
loadSettings,
} from './config/settings.js';
// Custom error to identify mock process.exit calls
class MockProcessExitError extends Error {
constructor(readonly code?: string | number | null | undefined) {
super('PROCESS_EXIT_MOCKED');
this.name = 'MockProcessExitError';
}
}
// Mock dependencies
vi.mock('./config/settings.js', async (importOriginal) => {
const actual = await importOriginal<typeof import('./config/settings.js')>();
return {
...actual,
loadSettings: vi.fn(),
};
});
vi.mock('./config/config.js', () => ({
loadCliConfig: vi.fn().mockResolvedValue({
config: {
getSandbox: vi.fn(() => false),
getQuestion: vi.fn(() => ''),
},
modelWasSwitched: false,
originalModelBeforeSwitch: null,
finalModel: 'test-model',
}),
}));
vi.mock('read-package-up', () => ({
readPackageUp: vi.fn().mockResolvedValue({
packageJson: { version: 'test-version' },
path: '/fake/path/package.json',
}),
}));
vi.mock('./utils/sandbox.js', () => ({
sandbox_command: vi.fn(() => ''), // Default to no sandbox command
start_sandbox: vi.fn(() => Promise.resolve()), // Mock as an async function that resolves
}));
describe('gemini.tsx main function', () => {
let consoleErrorSpy: ReturnType<typeof vi.spyOn>;
let loadSettingsMock: ReturnType<typeof vi.mocked<typeof loadSettings>>;
let originalEnvGeminiSandbox: string | undefined;
let originalEnvSandbox: string | undefined;
const processExitSpy = vi
.spyOn(process, 'exit')
.mockImplementation((code) => {
throw new MockProcessExitError(code);
});
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;
delete process.env.GEMINI_SANDBOX;
delete process.env.SANDBOX;
consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
});
afterEach(() => {
// Restore original env variables
if (originalEnvGeminiSandbox !== undefined) {
process.env.GEMINI_SANDBOX = originalEnvGeminiSandbox;
} else {
delete process.env.GEMINI_SANDBOX;
}
if (originalEnvSandbox !== undefined) {
process.env.SANDBOX = originalEnvSandbox;
} else {
delete process.env.SANDBOX;
}
vi.restoreAllMocks();
});
it('should call process.exit(1) 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 mockLoadedSettings = new LoadedSettings(
userSettingsFile,
workspaceSettingsFile,
[settingsError],
);
loadSettingsMock.mockReturnValue(mockLoadedSettings);
try {
await main();
// If main completes without throwing, the test should fail because process.exit was expected
expect.fail('main function did not exit as expected');
} catch (error) {
expect(error).toBeInstanceOf(MockProcessExitError);
if (error instanceof MockProcessExitError) {
expect(error.code).toBe(1);
}
}
// Verify console.error was called with the error message
expect(consoleErrorSpy).toHaveBeenCalledTimes(2);
expect(stripAnsi(String(consoleErrorSpy.mock.calls[0][0]))).toBe(
'Error in /test/settings.json: Test settings error',
);
expect(stripAnsi(String(consoleErrorSpy.mock.calls[1][0]))).toBe(
'Please fix /test/settings.json and try again.',
);
// Verify process.exit was called (indirectly, via the thrown error)
expect(processExitSpy).toHaveBeenCalledWith(1);
});
});

View File

@ -37,7 +37,7 @@ import {
const __filename = fileURLToPath(import.meta.url);
const __dirname = dirname(__filename);
async function main() {
export async function main() {
// warn about deprecated environment variables
if (process.env.GEMINI_CODE_MODEL) {
console.warn('GEMINI_CODE_MODEL is deprecated. Use GEMINI_MODEL instead.');
@ -61,6 +61,18 @@ async function main() {
const settings = loadSettings(workspaceRoot);
const geminiIgnorePatterns = loadGeminiIgnorePatterns(workspaceRoot);
if (settings.errors.length > 0) {
for (const error of settings.errors) {
let errorMessage = `Error in ${error.path}: ${error.message}`;
if (!process.env.NO_COLOR) {
errorMessage = `\x1b[31m${errorMessage}\x1b[0m`;
}
console.error(errorMessage);
console.error(`Please fix ${error.path} and try again.`);
}
process.exit(1);
}
const { config, modelWasSwitched, originalModelBeforeSwitch, finalModel } =
await loadCliConfig(settings.merged, geminiIgnorePatterns);
@ -144,17 +156,6 @@ process.on('unhandledRejection', (reason, _promise) => {
process.exit(1);
});
// --- Global Entry Point ---
main().catch((error) => {
console.error('An unexpected critical error occurred:');
if (error instanceof Error) {
console.error(error.message);
} else {
console.error(String(error));
}
process.exit(1);
});
async function loadNonInteractiveConfig(
config: Config,
settings: LoadedSettings,

View File

@ -171,7 +171,7 @@ describe('App UI', () => {
path: '/workspace/.gemini/settings.json',
settings,
};
return new LoadedSettings(userSettingsFile, workspaceSettingsFile);
return new LoadedSettings(userSettingsFile, workspaceSettingsFile, []);
};
beforeEach(() => {