refactor(cli): fix settings merging so that settings using the new json format take priority over ones using the old format (#15116)

Author: jacob314

packages/cli/src/config/settings.test.ts

@@ -371,6 +371,37 @@ describe('Settings Loading and Merging', () => {
       expect((settings.merged as TestSettings)['allowedTools']).toBeUndefined();
     });
 
+    it('should allow V2 settings to override V1 settings when both are present (zombie setting fix)', () => {
+      (mockFsExistsSync as Mock).mockImplementation(
+        (p: fs.PathLike) => p === USER_SETTINGS_PATH,
+      );
+      const mixedSettingsContent = {
+        // V1 setting (migrates to ui.accessibility.screenReader = true)
+        accessibility: {
+          screenReader: true,
+        },
+        // V2 setting (explicitly set to false)
+        ui: {
+          accessibility: {
+            screenReader: false,
+          },
+        },
+      };
+
+      (fs.readFileSync as Mock).mockImplementation(
+        (p: fs.PathOrFileDescriptor) => {
+          if (p === USER_SETTINGS_PATH)
+            return JSON.stringify(mixedSettingsContent);
+          return '{}';
+        },
+      );
+
+      const settings = loadSettings(MOCK_WORKSPACE_DIR);
+
+      // We expect the V2 setting (false) to win, NOT the migrated V1 setting (true)
+      expect(settings.merged.ui?.accessibility?.screenReader).toBe(false);
+    });
+
     it('should correctly merge and migrate legacy array properties from multiple scopes', () => {
       (mockFsExistsSync as Mock).mockReturnValue(true);
       const legacyUserSettings = {

packages/cli/src/config/settings.ts

@@ -302,6 +302,19 @@ function migrateSettingsToV2(
 
   for (const [oldKey, newPath] of Object.entries(MIGRATION_MAP)) {
     if (flatKeys.has(oldKey)) {
+      // If the key exists and is a V2 container (like 'model'), and the value is an object,
+      // it is likely already migrated or partially migrated. We should not move it
+      // to the mapped V2 path (e.g. 'model' -> 'model.name').
+      // Instead, let it fall through to the "Carry over" section to be merged.
+      if (
+        KNOWN_V2_CONTAINERS.has(oldKey) &&
+        typeof flatSettings[oldKey] === 'object' &&
+        flatSettings[oldKey] !== null &&
+        !Array.isArray(flatSettings[oldKey])
+      ) {
+        continue;
+      }
+
       setNestedProperty(v2Settings, newPath, flatSettings[oldKey]);
       flatKeys.delete(oldKey);
     }
@@ -331,8 +344,8 @@ function migrateSettingsToV2(
       v2Settings[remainingKey] = customDeepMerge(
         pathAwareGetStrategy,
         {},
-        newValue as MergeableObject,
         existingValue as MergeableObject,
+        newValue as MergeableObject,
       );
     } else {
       v2Settings[remainingKey] = newValue;

packages/cli/src/config/settings_repro.test.ts

@@ -0,0 +1,200 @@
+/**
+ * @license
+ * Copyright 2025 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+/// <reference types="vitest/globals" />
+
+// Mock 'os' first.
+import * as osActual from 'node:os';
+
+vi.mock('os', async (importOriginal) => {
+  const actualOs = await importOriginal<typeof osActual>();
+  return {
+    ...actualOs,
+    homedir: vi.fn(() => '/mock/home/user'),
+    platform: vi.fn(() => 'linux'),
+  };
+});
+
+// Mock './settings.js' to ensure it uses the mocked 'os.homedir()' for its internal constants.
+vi.mock('./settings.js', async (importActual) => {
+  const originalModule = await importActual<typeof import('./settings.js')>();
+  return {
+    __esModule: true,
+    ...originalModule,
+  };
+});
+
+// Mock trustedFolders
+vi.mock('./trustedFolders.js', () => ({
+  isWorkspaceTrusted: vi
+    .fn()
+    .mockReturnValue({ isTrusted: true, source: 'file' }),
+}));
+
+import {
+  describe,
+  it,
+  expect,
+  vi,
+  beforeEach,
+  afterEach,
+  type Mocked,
+  type Mock,
+} from 'vitest';
+import * as fs from 'node:fs';
+import stripJsonComments from 'strip-json-comments';
+import { isWorkspaceTrusted } from './trustedFolders.js';
+
+import { loadSettings, USER_SETTINGS_PATH } from './settings.js';
+
+const MOCK_WORKSPACE_DIR = '/mock/workspace';
+
+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,
+  };
+});
+
+vi.mock('./extension.js');
+
+const mockCoreEvents = vi.hoisted(() => ({
+  emitFeedback: vi.fn(),
+}));
+
+vi.mock('@google/gemini-cli-core', async (importOriginal) => {
+  const actual =
+    await importOriginal<typeof import('@google/gemini-cli-core')>();
+  return {
+    ...actual,
+    coreEvents: mockCoreEvents,
+  };
+});
+
+vi.mock('../utils/commentJson.js', () => ({
+  updateSettingsFilePreservingFormat: vi.fn(),
+}));
+
+vi.mock('strip-json-comments', () => ({
+  default: vi.fn((content) => content),
+}));
+
+describe('Settings Repro', () => {
+  let mockFsExistsSync: Mocked<typeof fs.existsSync>;
+  let mockStripJsonComments: Mocked<typeof stripJsonComments>;
+  let mockFsMkdirSync: Mocked<typeof fs.mkdirSync>;
+
+  beforeEach(() => {
+    vi.resetAllMocks();
+
+    mockFsExistsSync = vi.mocked(fs.existsSync);
+    mockFsMkdirSync = vi.mocked(fs.mkdirSync);
+    mockStripJsonComments = vi.mocked(stripJsonComments);
+
+    vi.mocked(osActual.homedir).mockReturnValue('/mock/home/user');
+    (mockStripJsonComments as unknown as Mock).mockImplementation(
+      (jsonString: string) => jsonString,
+    );
+    (mockFsExistsSync as Mock).mockReturnValue(false);
+    (fs.readFileSync as Mock).mockReturnValue('{}');
+    (mockFsMkdirSync as Mock).mockImplementation(() => undefined);
+    vi.mocked(isWorkspaceTrusted).mockReturnValue({
+      isTrusted: true,
+      source: 'file',
+    });
+  });
+
+  afterEach(() => {
+    vi.restoreAllMocks();
+  });
+
+  it('should handle the problematic settings.json without crashing', () => {
+    (mockFsExistsSync as Mock).mockImplementation(
+      (p: fs.PathLike) => p === USER_SETTINGS_PATH,
+    );
+    const problemSettingsContent = {
+      accessibility: {
+        screenReader: true,
+      },
+      ide: {
+        enabled: false,
+        hasSeenNudge: true,
+      },
+      general: {
+        debugKeystrokeLogging: false,
+        enablePromptCompletion: false,
+        preferredEditor: 'vim',
+        vimMode: false,
+        previewFeatures: false,
+      },
+      security: {
+        auth: {
+          selectedType: 'gemini-api-key',
+        },
+        folderTrust: {
+          enabled: true,
+        },
+      },
+      tools: {
+        useRipgrep: true,
+        shell: {
+          showColor: true,
+          enableInteractiveShell: true,
+        },
+        enableMessageBusIntegration: true,
+      },
+      experimental: {
+        useModelRouter: false,
+        enableSubagents: false,
+        codebaseInvestigatorSettings: {
+          enabled: true,
+        },
+      },
+      ui: {
+        accessibility: {
+          screenReader: false,
+        },
+        showMemoryUsage: true,
+        showStatusInTitle: true,
+        showCitations: true,
+        useInkScrolling: true,
+        footer: {
+          hideContextPercentage: false,
+          hideModelInfo: false,
+        },
+      },
+      useWriteTodos: true,
+      output: {
+        format: 'text',
+      },
+      model: {
+        compressionThreshold: 0.8,
+      },
+    };
+
+    (fs.readFileSync as Mock).mockImplementation(
+      (p: fs.PathOrFileDescriptor) => {
+        if (p === USER_SETTINGS_PATH)
+          return JSON.stringify(problemSettingsContent);
+        return '{}';
+      },
+    );
+
+    const settings = loadSettings(MOCK_WORKSPACE_DIR);
+
+    // If it doesn't throw, check if it merged correctly.
+    // The model.compressionThreshold should be present.
+    // And model.name should probably be undefined or default, but certainly NOT { compressionThreshold: 0.8 }
+    expect(settings.merged.model?.compressionThreshold).toBe(0.8);
+    expect(typeof settings.merged.model?.name).not.toBe('object');
+  });
+});

packages/cli/src/utils/deepMerge.test.ts

@@ -199,4 +199,28 @@ describe('customDeepMerge', () => {
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     expect((result as any)['hooks']['disabled']).toEqual(['hook-a', 'hook-b']);
   });
+
+  it('should overwrite primitive with object', () => {
+    const target = { a: 1 };
+    const source = { a: { b: 2 } };
+    const getMergeStrategy = () => undefined;
+    const result = customDeepMerge(getMergeStrategy, target, source);
+    expect(result).toEqual({ a: { b: 2 } });
+  });
+
+  it('should overwrite object with primitive', () => {
+    const target = { a: { b: 2 } };
+    const source = { a: 1 };
+    const getMergeStrategy = () => undefined;
+    const result = customDeepMerge(getMergeStrategy, target, source);
+    expect(result).toEqual({ a: 1 });
+  });
+
+  it('should not overwrite with undefined', () => {
+    const target = { a: 1 };
+    const source = { a: undefined };
+    const getMergeStrategy = () => undefined;
+    const result = customDeepMerge(getMergeStrategy, target, source);
+    expect(result).toEqual({ a: 1 });
+  });
 });

packages/cli/src/utils/deepMerge.ts

@@ -28,11 +28,16 @@ function mergeRecursively(
   path: string[] = [],
 ) {
   for (const key of Object.keys(source)) {
-    if (key === '__proto__' || key === 'constructor' || key === 'prototype') {
+    // JSON.parse can create objects with __proto__ as an own property.
+    // We must skip it to prevent prototype pollution.
+    if (key === '__proto__') {
       continue;
     }
-    const newPath = [...path, key];
     const srcValue = source[key];
+    if (srcValue === undefined) {
+      continue;
+    }
+    const newPath = [...path, key];
     const objValue = target[key];
     const mergeStrategy = getMergeStrategyForPath(newPath);