diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index a1ca034316..7028c36b82 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/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 = { diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 1983642231..3e41fbe87e 100644 --- a/packages/cli/src/config/settings.ts +++ b/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; diff --git a/packages/cli/src/config/settings_repro.test.ts b/packages/cli/src/config/settings_repro.test.ts new file mode 100644 index 0000000000..1f7ab5b943 --- /dev/null +++ b/packages/cli/src/config/settings_repro.test.ts @@ -0,0 +1,200 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/// + +// Mock 'os' first. +import * as osActual from 'node:os'; + +vi.mock('os', async (importOriginal) => { + const actualOs = await importOriginal(); + 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(); + 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(); + 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(); + 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; + let mockStripJsonComments: Mocked; + let mockFsMkdirSync: Mocked; + + 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'); + }); +}); diff --git a/packages/cli/src/utils/deepMerge.test.ts b/packages/cli/src/utils/deepMerge.test.ts index bc2e77140c..ee6cc7169c 100644 --- a/packages/cli/src/utils/deepMerge.test.ts +++ b/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 }); + }); }); diff --git a/packages/cli/src/utils/deepMerge.ts b/packages/cli/src/utils/deepMerge.ts index 85200ddbae..f4fec4d3c8 100644 --- a/packages/cli/src/utils/deepMerge.ts +++ b/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 srcValue = source[key]; + if (srcValue === undefined) { continue; } const newPath = [...path, key]; - const srcValue = source[key]; const objValue = target[key]; const mergeStrategy = getMergeStrategyForPath(newPath);