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

6 files changed, 246 insertions, 32 deletions

diff --git a/packages/cli/index.ts b/packages/cli/index.ts
index 7df29cd0..5478f034 100644
--- a/packages/cli/index.ts
+++ b/packages/cli/index.ts
@@ -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);
+});
diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts
index 6c075b38..cfa5e2b5 100644
--- a/packages/cli/src/config/settings.test.ts
+++ b/packages/cli/src/config/settings.test.ts
@@ -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');
diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts
index 427fb898..d3a4dacb 100644
--- a/packages/cli/src/config/settings.ts
+++ b/packages/cli/src/config/settings.ts
@@ -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,
   );
 }
 
diff --git a/packages/cli/src/gemini.test.tsx b/packages/cli/src/gemini.test.tsx
new file mode 100644
index 00000000..f77f9577
--- /dev/null
+++ b/packages/cli/src/gemini.test.tsx
@@ -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);
+  });
+});
diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx
index f9b73074..e2169980 100644
--- a/packages/cli/src/gemini.tsx
+++ b/packages/cli/src/gemini.tsx
@@ -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,
diff --git a/packages/cli/src/ui/App.test.tsx b/packages/cli/src/ui/App.test.tsx
index c1430bb5..90adb24f 100644
--- a/packages/cli/src/ui/App.test.tsx
+++ b/packages/cli/src/ui/App.test.tsx
@@ -171,7 +171,7 @@ describe('App UI', () => {
       path: '/workspace/.gemini/settings.json',
       settings,
     };
-    return new LoadedSettings(userSettingsFile, workspaceSettingsFile);
+    return new LoadedSettings(userSettingsFile, workspaceSettingsFile, []);
   };
 
   beforeEach(() => {