fix(cli): minimal fix for settings persistence and comment preservation

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');
+  });
+});

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', () => ({

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;

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 '{}';
     });
 

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<string | symbol, unknown>;
+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<string, unknown>,
 ): 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<string, unknown>;
-  try {
-    // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
-    parsed = commentJsonParse(originalContent) as Record<string, unknown>;
-  } 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<string, unknown>,
-  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<string, unknown>,
-  desired: Record<string, unknown>,
-): 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<string, unknown>,
-        // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
-        nextVal as Record<string, unknown>,
-      );
-    } 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<string, unknown>,
-  updates: Record<string, unknown>,
-): Record<string, unknown> {
-  // Apply sync-by-omission semantics consistently at all levels
-  applyKeyDiff(current, updates);
-  return current;
-}