diff --git a/packages/cli/src/config/settings-persistence.test.ts b/packages/cli/src/config/settings-persistence.test.ts new file mode 100644 index 0000000000..24e3cb129f --- /dev/null +++ b/packages/cli/src/config/settings-persistence.test.ts @@ -0,0 +1,73 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; +import * as fs from 'node:fs'; + +// Mock 'os' and 'fs' before importing settings +vi.mock('node:os', () => ({ + homedir: () => '/mock/home', + platform: () => 'linux', +})); + +vi.mock('node:fs', () => ({ + existsSync: vi.fn(), + readFileSync: vi.fn(), + writeFileSync: vi.fn(), + mkdirSync: vi.fn(), + renameSync: vi.fn(), + realpathSync: vi.fn((p) => p), + PathLike: {}, +})); + +import { loadSettings, USER_SETTINGS_PATH, SettingScope } from './settings.js'; + +describe('Settings Persistence', () => { + const MOCK_WORKSPACE_DIR = '/mock/workspace'; + + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('should persist a nested setting and preserve comments', () => { + const initialContent = `{ + // This is a comment + "ui": { + "theme": "dark" + } + }`; + + let currentContent = initialContent; + + (fs.existsSync as Mock).mockImplementation((p) => p === USER_SETTINGS_PATH); + (fs.readFileSync as Mock).mockImplementation((p) => { + if (p === USER_SETTINGS_PATH) return currentContent; + return '{}'; + }); + (fs.writeFileSync as Mock).mockImplementation((p, content) => { + if (p === USER_SETTINGS_PATH) currentContent = content; + }); + + // 1. Load + const settings = loadSettings(MOCK_WORKSPACE_DIR); + expect(settings.merged.ui?.theme).toBe('dark'); + + // 2. Modify + settings.setValue(SettingScope.User, 'ui.theme', 'light'); + + // 3. Verify content + expect(currentContent).toContain('"theme": "light"'); + expect(currentContent).toContain('// This is a comment'); + + // 4. Modify another key + settings.setValue(SettingScope.User, 'model.name', 'gemini-2.0-flash'); + + // 5. Verify both are present + expect(currentContent).toContain('"theme": "light"'); + expect(currentContent).toContain('"name": "gemini-2.0-flash"'); + expect(currentContent).toContain('// This is a comment'); + }); +}); diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index b60df49abc..848ca5d4a0 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -175,6 +175,9 @@ const { commentJsonParseMock } = await vi.hoisted(async () => { vi.mock('../utils/commentJson.js', () => ({ updateSettingsFilePreservingFormat: vi.fn(), parse: commentJsonParseMock, + stringify: vi.fn((obj, replacer, space) => + JSON.stringify(obj, replacer, space), + ), })); vi.mock('strip-json-comments', () => ({ diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index b94be5fe50..b8a4658051 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -851,14 +851,10 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { workspaceResult = load(workspaceSettingsPath); } - const systemOriginalSettings = structuredClone(systemResult.rawSettings); - const systemDefaultsOriginalSettings = structuredClone( - systemDefaultsResult.rawSettings, - ); - const userOriginalSettings = structuredClone(userResult.rawSettings); - const workspaceOriginalSettings = structuredClone( - workspaceResult.rawSettings, - ); + const systemOriginalSettings = systemResult.rawSettings; + const systemDefaultsOriginalSettings = systemDefaultsResult.rawSettings; + const userOriginalSettings = userResult.rawSettings; + const workspaceOriginalSettings = workspaceResult.rawSettings; // Environment variables for runtime use are already resolved and validated in load() systemSettings = systemResult.settings; diff --git a/packages/cli/src/config/settings_validation_warning.test.ts b/packages/cli/src/config/settings_validation_warning.test.ts index 5bb3b7729f..e3813a21b0 100644 --- a/packages/cli/src/config/settings_validation_warning.test.ts +++ b/packages/cli/src/config/settings_validation_warning.test.ts @@ -146,13 +146,28 @@ describe('Settings Validation Warning', () => { }).toThrow(); }); - it('should throw a fatal error when settings file contains invalid JSON', () => { + it('should NOT throw for trailing commas (now supported via comment-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"'; // Unclosed brace is invalid + if (p === USER_SETTINGS_PATH) return '{ "valid": "json", }'; // Trailing comma is allowed in JSONC + return '{}'; + }); + + expect(() => { + loadSettings(MOCK_WORKSPACE_DIR); + }).not.toThrow(); + }); + + it('should throw a fatal error when settings file is unparseable', () => { + (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"'; // Unclosed brace is truly invalid return '{}'; }); diff --git a/packages/cli/src/utils/commentJson.ts b/packages/cli/src/utils/commentJson.ts index fae0716df4..0b362404c4 100644 --- a/packages/cli/src/utils/commentJson.ts +++ b/packages/cli/src/utils/commentJson.ts @@ -6,165 +6,20 @@ import * as fs from 'node:fs'; import { parse as commentJsonParse, stringify } from 'comment-json'; -export { commentJsonParse as parse }; -import { coreEvents } from '@google/gemini-cli-core'; - -/** - * Type representing an object that may contain Symbol keys for comments. - */ -type CommentedRecord = Record; +export { commentJsonParse as parse, stringify }; /** * Updates a JSON file while preserving comments and formatting. + * + * This minimal version relies on the fact that the 'updates' object + * already contains the comment-json metadata (Symbols) because we + * avoided stripping them in settings.ts. */ export function updateSettingsFilePreservingFormat( filePath: string, updates: Record, ): void { - if (!fs.existsSync(filePath)) { - fs.writeFileSync(filePath, JSON.stringify(updates, null, 2), 'utf-8'); - return; - } - - const originalContent = fs.readFileSync(filePath, 'utf-8'); - - let parsed: Record; - try { - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - parsed = commentJsonParse(originalContent) as Record; - } catch (error) { - coreEvents.emitFeedback( - 'error', - 'Error parsing settings file. Please check the JSON syntax.', - error, - ); - return; - } - - const updatedStructure = applyUpdates(parsed, updates); - const updatedContent = stringify(updatedStructure, null, 2); - + // Directly stringify the updates object which should have metadata + const updatedContent = stringify(updates, null, 2); fs.writeFileSync(filePath, updatedContent, 'utf-8'); } - -/** - * When deleting a property from a comment-json parsed object, relocate any - * leading/trailing comments that were attached to that property so they are not lost. - * - * This function re-attaches comments to the next sibling's leading comments if - * available, otherwise to the previous sibling's trailing comments, otherwise - * to the container's leading/trailing comments. - */ -function preserveCommentsOnPropertyDeletion( - container: Record, - propName: string, -): void { - const target = container as CommentedRecord; - const beforeSym = Symbol.for(`before:${propName}`); - const afterSym = Symbol.for(`after:${propName}`); - - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - const beforeComments = target[beforeSym] as unknown[] | undefined; - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - const afterComments = target[afterSym] as unknown[] | undefined; - - if (!beforeComments && !afterComments) return; - - const keys = Object.getOwnPropertyNames(container); - const idx = keys.indexOf(propName); - const nextKey = idx >= 0 && idx + 1 < keys.length ? keys[idx + 1] : undefined; - const prevKey = idx > 0 ? keys[idx - 1] : undefined; - - function appendToSymbol(destSym: symbol, comments: unknown[]) { - if (!comments || comments.length === 0) return; - const existing = target[destSym]; - target[destSym] = Array.isArray(existing) - ? existing.concat(comments) - : comments; - } - - if (beforeComments && beforeComments.length > 0) { - if (nextKey) { - appendToSymbol(Symbol.for(`before:${nextKey}`), beforeComments); - } else if (prevKey) { - appendToSymbol(Symbol.for(`after:${prevKey}`), beforeComments); - } else { - appendToSymbol(Symbol.for('before'), beforeComments); - } - delete target[beforeSym]; - } - - if (afterComments && afterComments.length > 0) { - if (nextKey) { - appendToSymbol(Symbol.for(`before:${nextKey}`), afterComments); - } else if (prevKey) { - appendToSymbol(Symbol.for(`after:${prevKey}`), afterComments); - } else { - appendToSymbol(Symbol.for('after'), afterComments); - } - delete target[afterSym]; - } -} - -/** - * Applies sync-by-omission semantics: synchronizes base to match desired. - * - Adds/updates keys from desired - * - Removes keys from base that are not in desired - * - Recursively applies to nested objects - * - Preserves comments when deleting keys - */ -function applyKeyDiff( - base: Record, - desired: Record, -): void { - for (const existingKey of Object.getOwnPropertyNames(base)) { - if (!Object.prototype.hasOwnProperty.call(desired, existingKey)) { - preserveCommentsOnPropertyDeletion(base, existingKey); - delete base[existingKey]; - } - } - - for (const nextKey of Object.getOwnPropertyNames(desired)) { - const nextVal = desired[nextKey]; - const baseVal = base[nextKey]; - - const isObj = - typeof nextVal === 'object' && - nextVal !== null && - !Array.isArray(nextVal); - const isBaseObj = - typeof baseVal === 'object' && - baseVal !== null && - !Array.isArray(baseVal); - const isArr = Array.isArray(nextVal); - const isBaseArr = Array.isArray(baseVal); - - if (isObj && isBaseObj) { - applyKeyDiff( - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - baseVal as Record, - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - nextVal as Record, - ); - } else if (isArr && isBaseArr) { - // In-place mutate arrays to preserve array-level comments on CommentArray - const baseArr = baseVal as unknown[]; - const desiredArr = nextVal as unknown[]; - baseArr.length = 0; - for (const el of desiredArr) { - baseArr.push(el); - } - } else { - base[nextKey] = nextVal; - } - } -} - -function applyUpdates( - current: Record, - updates: Record, -): Record { - // Apply sync-by-omission semantics consistently at all levels - applyKeyDiff(current, updates); - return current; -}