diff --git a/npm-shrinkwrap.json b/npm-shrinkwrap.json index 0661811812..be7ed4277a 100644 --- a/npm-shrinkwrap.json +++ b/npm-shrinkwrap.json @@ -5201,6 +5201,12 @@ "url": "https://github.com/sponsors/ljharb" } }, + "node_modules/array-timsort": { + "version": "1.0.3", + "resolved": "https://registry.npmjs.org/array-timsort/-/array-timsort-1.0.3.tgz", + "integrity": "sha512-/+3GRL7dDAGEfM6TseQk/U+mi18TU2Ms9I3UlLdUMhz2hbvGNTKdj9xniwXfUqgYhHxRx0+8UnKkvlNwVU+cWQ==", + "license": "MIT" + }, "node_modules/array.prototype.findlast": { "version": "1.2.5", "resolved": "https://registry.npmjs.org/array.prototype.findlast/-/array.prototype.findlast-1.2.5.tgz", @@ -6293,6 +6299,22 @@ "node": ">=18" } }, + "node_modules/comment-json": { + "version": "4.2.5", + "resolved": "https://registry.npmjs.org/comment-json/-/comment-json-4.2.5.tgz", + "integrity": "sha512-bKw/r35jR3HGt5PEPm1ljsQQGyCrR8sFGNiN5L+ykDHdpO8Smxkrkla9Yi6NkQyUrb8V54PGhfMs6NrIwtxtdw==", + "license": "MIT", + "dependencies": { + "array-timsort": "^1.0.3", + "core-util-is": "^1.0.3", + "esprima": "^4.0.1", + "has-own-prop": "^2.0.0", + "repeat-string": "^1.6.1" + }, + "engines": { + "node": ">= 6" + } + }, "node_modules/component-emitter": { "version": "1.3.1", "resolved": "https://registry.npmjs.org/component-emitter/-/component-emitter-1.3.1.tgz", @@ -6398,6 +6420,12 @@ "dev": true, "license": "MIT" }, + "node_modules/core-util-is": { + "version": "1.0.3", + "resolved": "https://registry.npmjs.org/core-util-is/-/core-util-is-1.0.3.tgz", + "integrity": "sha512-ZQBvi1DcpJ4GDqanjucZ2Hj3wEO5pZDS89BWbkcrvdxksJorwUDDZamX9ldFkp9aw2lmBDLgkObEA4DWNJ9FYQ==", + "license": "MIT" + }, "node_modules/cors": { "version": "2.8.5", "resolved": "https://registry.npmjs.org/cors/-/cors-2.8.5.tgz", @@ -7652,7 +7680,6 @@ "version": "4.0.1", "resolved": "https://registry.npmjs.org/esprima/-/esprima-4.0.1.tgz", "integrity": "sha512-eGuFFw7Upda+g4p+QHvnW0RyTX/SVeJBDM/gCtMARO0cLuT2HcEKnTPvhjV6aGeqrCB/sbNop0Kszm0jsaWU4A==", - "dev": true, "license": "BSD-2-Clause", "bin": { "esparse": "bin/esparse.js", @@ -8906,6 +8933,15 @@ "node": ">=8" } }, + "node_modules/has-own-prop": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/has-own-prop/-/has-own-prop-2.0.0.tgz", + "integrity": "sha512-Pq0h+hvsVm6dDEa8x82GnLSYHOzNDt7f0ddFa3FqcQlgzEiptPqL+XrOJNavjOzSYiYWIrgeVYYgGlLmnxwilQ==", + "license": "MIT", + "engines": { + "node": ">=8" + } + }, "node_modules/has-property-descriptors": { "version": "1.0.2", "resolved": "https://registry.npmjs.org/has-property-descriptors/-/has-property-descriptors-1.0.2.tgz", @@ -12678,6 +12714,15 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/repeat-string": { + "version": "1.6.1", + "resolved": "https://registry.npmjs.org/repeat-string/-/repeat-string-1.6.1.tgz", + "integrity": "sha512-PV0dzCYDNfRi1jCDbJzpW7jNNDRuCOG/jI5ctQcGKt/clZD+YcPS3yIlWuTJMmESC8aevCFmWJy5wjAFgNqN6w==", + "license": "MIT", + "engines": { + "node": ">=0.10" + } + }, "node_modules/require-directory": { "version": "2.1.1", "resolved": "https://registry.npmjs.org/require-directory/-/require-directory-2.1.1.tgz", @@ -16220,6 +16265,7 @@ "@modelcontextprotocol/sdk": "^1.15.1", "@types/update-notifier": "^6.0.8", "command-exists": "^1.2.9", + "comment-json": "^4.2.5", "diff": "^7.0.0", "dotenv": "^17.1.0", "fzf": "^0.5.2", diff --git a/packages/cli/package.json b/packages/cli/package.json index c2c8a5e9dc..693b183d40 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -34,6 +34,7 @@ "@modelcontextprotocol/sdk": "^1.15.1", "@types/update-notifier": "^6.0.8", "command-exists": "^1.2.9", + "comment-json": "^4.2.5", "diff": "^7.0.0", "dotenv": "^17.1.0", "fzf": "^0.5.2", diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index e1a4538fc8..42e01f32b3 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -29,6 +29,7 @@ import { } from './settingsSchema.js'; import { resolveEnvVarsInObject } from '../utils/envVarResolver.js'; import { customDeepMerge } from '../utils/deepMerge.js'; +import { updateSettingsFilePreservingFormat } from '../utils/commentJson.js'; function getMergeStrategyForPath(path: string[]): MergeStrategy | undefined { let current: SettingDefinition | undefined = undefined; @@ -172,7 +173,9 @@ export interface SettingsError { export interface SettingsFile { settings: Settings; + originalSettings: Settings; path: string; + rawJson?: string; } function setNestedProperty( @@ -406,6 +409,7 @@ export class LoadedSettings { setValue(scope: SettingScope, key: string, value: unknown): void { const settingsFile = this.forScope(scope); setNestedProperty(settingsFile.settings, key, value); + setNestedProperty(settingsFile.originalSettings, key, value); this._merged = this.computeMergedSettings(); saveSettings(settingsFile); } @@ -539,7 +543,10 @@ export function loadSettings( workspaceDir, ).getWorkspaceSettingsPath(); - const loadAndMigrate = (filePath: string, scope: SettingScope): Settings => { + const loadAndMigrate = ( + filePath: string, + scope: SettingScope, + ): { settings: Settings; rawJson?: string } => { try { if (fs.existsSync(filePath)) { const content = fs.readFileSync(filePath, 'utf-8'); @@ -554,7 +561,7 @@ export function loadSettings( message: 'Settings file is not a valid JSON object.', path: filePath, }); - return {}; + return { settings: {} }; } let settingsObject = rawSettings as Record; @@ -582,7 +589,7 @@ export function loadSettings( settingsObject = migratedSettings; } } - return settingsObject as Settings; + return { settings: settingsObject as Settings, rawJson: content }; } } catch (error: unknown) { settingsErrors.push({ @@ -590,23 +597,40 @@ export function loadSettings( path: filePath, }); } - return {}; + return { settings: {} }; }; - systemSettings = loadAndMigrate(systemSettingsPath, SettingScope.System); - systemDefaultSettings = loadAndMigrate( + const systemResult = loadAndMigrate(systemSettingsPath, SettingScope.System); + const systemDefaultsResult = loadAndMigrate( systemDefaultsPath, SettingScope.SystemDefaults, ); - userSettings = loadAndMigrate(USER_SETTINGS_PATH, SettingScope.User); + const userResult = loadAndMigrate(USER_SETTINGS_PATH, SettingScope.User); + let workspaceResult: { settings: Settings; rawJson?: string } = { + settings: {} as Settings, + rawJson: undefined, + }; if (realWorkspaceDir !== realHomeDir) { - workspaceSettings = loadAndMigrate( + workspaceResult = loadAndMigrate( workspaceSettingsPath, SettingScope.Workspace, ); } + const systemOriginalSettings = structuredClone(systemResult.settings); + const systemDefaultsOriginalSettings = structuredClone( + systemDefaultsResult.settings, + ); + const userOriginalSettings = structuredClone(userResult.settings); + const workspaceOriginalSettings = structuredClone(workspaceResult.settings); + + // Environment variables for runtime use + systemSettings = resolveEnvVarsInObject(systemResult.settings); + systemDefaultSettings = resolveEnvVarsInObject(systemDefaultsResult.settings); + userSettings = resolveEnvVarsInObject(userResult.settings); + workspaceSettings = resolveEnvVarsInObject(workspaceResult.settings); + // Support legacy theme names if (userSettings.ui?.theme === 'VS') { userSettings.ui.theme = DefaultLight.name; @@ -642,11 +666,6 @@ export function loadSettings( // the settings to avoid a cycle loadEnvironment(tempMergedSettings); - // Now that the environment is loaded, resolve variables in the settings. - systemSettings = resolveEnvVarsInObject(systemSettings); - userSettings = resolveEnvVarsInObject(userSettings); - workspaceSettings = resolveEnvVarsInObject(workspaceSettings); - // Create LoadedSettings first if (settingsErrors.length > 0) { @@ -662,18 +681,26 @@ export function loadSettings( { path: systemSettingsPath, settings: systemSettings, + originalSettings: systemOriginalSettings, + rawJson: systemResult.rawJson, }, { path: systemDefaultsPath, settings: systemDefaultSettings, + originalSettings: systemDefaultsOriginalSettings, + rawJson: systemDefaultsResult.rawJson, }, { path: USER_SETTINGS_PATH, settings: userSettings, + originalSettings: userOriginalSettings, + rawJson: userResult.rawJson, }, { path: workspaceSettingsPath, settings: workspaceSettings, + originalSettings: workspaceOriginalSettings, + rawJson: workspaceResult.rawJson, }, isTrusted, migratedInMemorScopes, @@ -688,17 +715,17 @@ export function saveSettings(settingsFile: SettingsFile): void { fs.mkdirSync(dirPath, { recursive: true }); } - let settingsToSave = settingsFile.settings; + let settingsToSave = settingsFile.originalSettings; if (!MIGRATE_V2_OVERWRITE) { settingsToSave = migrateSettingsToV1( settingsToSave as Record, ) as Settings; } - fs.writeFileSync( + // Use the format-preserving update function + updateSettingsFilePreservingFormat( settingsFile.path, - JSON.stringify(settingsToSave, null, 2), - 'utf-8', + settingsToSave as Record, ); } catch (error) { console.error('Error saving user settings file:', error); diff --git a/packages/cli/src/ui/components/SettingsDialog.test.tsx b/packages/cli/src/ui/components/SettingsDialog.test.tsx index 31a1a344fd..1062442a2a 100644 --- a/packages/cli/src/ui/components/SettingsDialog.test.tsx +++ b/packages/cli/src/ui/components/SettingsDialog.test.tsx @@ -58,10 +58,16 @@ const createMockSettings = ( new LoadedSettings( { settings: { ui: { customThemes: {} }, mcpServers: {}, ...systemSettings }, + originalSettings: { + ui: { customThemes: {} }, + mcpServers: {}, + ...systemSettings, + }, path: '/system/settings.json', }, { settings: {}, + originalSettings: {}, path: '/system/system-defaults.json', }, { @@ -70,6 +76,11 @@ const createMockSettings = ( mcpServers: {}, ...userSettings, }, + originalSettings: { + ui: { customThemes: {} }, + mcpServers: {}, + ...userSettings, + }, path: '/user/settings.json', }, { @@ -78,6 +89,11 @@ const createMockSettings = ( mcpServers: {}, ...workspaceSettings, }, + originalSettings: { + ui: { customThemes: {} }, + mcpServers: {}, + ...workspaceSettings, + }, path: '/workspace/settings.json', }, true, diff --git a/packages/cli/src/ui/components/ThemeDialog.test.tsx b/packages/cli/src/ui/components/ThemeDialog.test.tsx index f2899e94a2..0a9a3d0c0d 100644 --- a/packages/cli/src/ui/components/ThemeDialog.test.tsx +++ b/packages/cli/src/ui/components/ThemeDialog.test.tsx @@ -21,10 +21,12 @@ const createMockSettings = ( new LoadedSettings( { settings: { ui: { customThemes: {} }, ...systemSettings }, + originalSettings: { ui: { customThemes: {} }, ...systemSettings }, path: '/system/settings.json', }, { settings: {}, + originalSettings: {}, path: '/system/system-defaults.json', }, { @@ -32,6 +34,10 @@ const createMockSettings = ( ui: { customThemes: {} }, ...userSettings, }, + originalSettings: { + ui: { customThemes: {} }, + ...userSettings, + }, path: '/user/settings.json', }, { @@ -39,6 +45,10 @@ const createMockSettings = ( ui: { customThemes: {} }, ...workspaceSettings, }, + originalSettings: { + ui: { customThemes: {} }, + ...workspaceSettings, + }, path: '/workspace/settings.json', }, true, diff --git a/packages/cli/src/ui/utils/MarkdownDisplay.test.tsx b/packages/cli/src/ui/utils/MarkdownDisplay.test.tsx index d93437bb0a..80a339b3b1 100644 --- a/packages/cli/src/ui/utils/MarkdownDisplay.test.tsx +++ b/packages/cli/src/ui/utils/MarkdownDisplay.test.tsx @@ -19,10 +19,10 @@ describe('', () => { }; const mockSettings = new LoadedSettings( - { path: '', settings: {} }, - { path: '', settings: {} }, - { path: '', settings: {} }, - { path: '', settings: {} }, + { path: '', settings: {}, originalSettings: {} }, + { path: '', settings: {}, originalSettings: {} }, + { path: '', settings: {}, originalSettings: {} }, + { path: '', settings: {}, originalSettings: {} }, true, new Set(), ); @@ -222,10 +222,14 @@ Another paragraph. it('hides line numbers in code blocks when showLineNumbers is false', () => { const text = '```javascript\nconst x = 1;\n```'.replace(/\n/g, EOL); const settings = new LoadedSettings( - { path: '', settings: {} }, - { path: '', settings: {} }, - { path: '', settings: { ui: { showLineNumbers: false } } }, - { path: '', settings: {} }, + { path: '', settings: {}, originalSettings: {} }, + { path: '', settings: {}, originalSettings: {} }, + { + path: '', + settings: { ui: { showLineNumbers: false } }, + originalSettings: { ui: { showLineNumbers: false } }, + }, + { path: '', settings: {}, originalSettings: {} }, true, new Set(), ); diff --git a/packages/cli/src/utils/commentJson.test.ts b/packages/cli/src/utils/commentJson.test.ts new file mode 100644 index 0000000000..f996087657 --- /dev/null +++ b/packages/cli/src/utils/commentJson.test.ts @@ -0,0 +1,182 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import * as os from 'node:os'; +import { updateSettingsFilePreservingFormat } from './commentJson.js'; + +describe('commentJson', () => { + let tempDir: string; + let testFilePath: string; + + beforeEach(() => { + // Create a temporary directory for test files + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'preserve-format-test-')); + testFilePath = path.join(tempDir, 'settings.json'); + }); + + afterEach(() => { + // Clean up temporary directory + if (fs.existsSync(tempDir)) { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + + describe('updateSettingsFilePreservingFormat', () => { + it('should preserve comments when updating settings', () => { + const originalContent = `{ + // Model configuration + "model": "gemini-2.5-pro", + "ui": { + // Theme setting + "theme": "dark" + } + }`; + + fs.writeFileSync(testFilePath, originalContent, 'utf-8'); + + updateSettingsFilePreservingFormat(testFilePath, { + model: 'gemini-3.0-ultra', + }); + + const updatedContent = fs.readFileSync(testFilePath, 'utf-8'); + + expect(updatedContent).toContain('// Model configuration'); + expect(updatedContent).toContain('// Theme setting'); + expect(updatedContent).toContain('"model": "gemini-3.0-ultra"'); + expect(updatedContent).toContain('"theme": "dark"'); + }); + + it('should handle nested object updates', () => { + const originalContent = `{ + "ui": { + "theme": "dark", + "showLineNumbers": true + } + }`; + + fs.writeFileSync(testFilePath, originalContent, 'utf-8'); + + updateSettingsFilePreservingFormat(testFilePath, { + ui: { + theme: 'light', + showLineNumbers: true, + }, + }); + + const updatedContent = fs.readFileSync(testFilePath, 'utf-8'); + expect(updatedContent).toContain('"theme": "light"'); + expect(updatedContent).toContain('"showLineNumbers": true'); + }); + + it('should add new fields while preserving existing structure', () => { + const originalContent = `{ + // Existing config + "model": "gemini-2.5-pro" + }`; + + fs.writeFileSync(testFilePath, originalContent, 'utf-8'); + + updateSettingsFilePreservingFormat(testFilePath, { + model: 'gemini-2.5-pro', + newField: 'newValue', + }); + + const updatedContent = fs.readFileSync(testFilePath, 'utf-8'); + expect(updatedContent).toContain('// Existing config'); + expect(updatedContent).toContain('"newField": "newValue"'); + }); + + it('should create file if it does not exist', () => { + updateSettingsFilePreservingFormat(testFilePath, { + model: 'gemini-2.5-pro', + }); + + expect(fs.existsSync(testFilePath)).toBe(true); + const content = fs.readFileSync(testFilePath, 'utf-8'); + expect(content).toContain('"model": "gemini-2.5-pro"'); + }); + + it('should handle complex real-world scenario', () => { + const complexContent = `{ + // Settings + "model": "gemini-2.5-pro", + "mcpServers": { + // Active server + "context7": { + "headers": { + "API_KEY": "test-key" // API key + } + } + } + }`; + + fs.writeFileSync(testFilePath, complexContent, 'utf-8'); + + updateSettingsFilePreservingFormat(testFilePath, { + model: 'gemini-3.0-ultra', + mcpServers: { + context7: { + headers: { + API_KEY: 'new-test-key', + }, + }, + }, + newSection: { + setting: 'value', + }, + }); + + const updatedContent = fs.readFileSync(testFilePath, 'utf-8'); + + // Verify comments preserved + expect(updatedContent).toContain('// Settings'); + expect(updatedContent).toContain('// Active server'); + expect(updatedContent).toContain('// API key'); + + // Verify updates applied + expect(updatedContent).toContain('"model": "gemini-3.0-ultra"'); + expect(updatedContent).toContain('"newSection"'); + expect(updatedContent).toContain('"API_KEY": "new-test-key"'); + }); + + it('should handle corrupted JSON files gracefully', () => { + const corruptedContent = `{ + "model": "gemini-2.5-pro", + "ui": { + "theme": "dark" + // Missing closing brace + `; + + fs.writeFileSync(testFilePath, corruptedContent, 'utf-8'); + + const consoleSpy = vi + .spyOn(console, 'error') + .mockImplementation(() => {}); + + expect(() => { + updateSettingsFilePreservingFormat(testFilePath, { + model: 'gemini-3.0-ultra', + }); + }).not.toThrow(); + + expect(consoleSpy).toHaveBeenCalledWith( + 'Error parsing settings file:', + expect.any(Error), + ); + expect(consoleSpy).toHaveBeenCalledWith( + 'Settings file may be corrupted. Please check the JSON syntax.', + ); + + const unchangedContent = fs.readFileSync(testFilePath, 'utf-8'); + expect(unchangedContent).toBe(corruptedContent); + + consoleSpy.mockRestore(); + }); + }); +}); diff --git a/packages/cli/src/utils/commentJson.ts b/packages/cli/src/utils/commentJson.ts new file mode 100644 index 0000000000..9ea4d3f802 --- /dev/null +++ b/packages/cli/src/utils/commentJson.ts @@ -0,0 +1,67 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import * as fs from 'node:fs'; +import { parse, stringify } from 'comment-json'; + +/** + * Updates a JSON file while preserving comments and formatting. + */ +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 { + parsed = parse(originalContent) as Record; + } catch (error) { + console.error('Error parsing settings file:', error); + console.error( + 'Settings file may be corrupted. Please check the JSON syntax.', + ); + return; + } + + const updatedStructure = applyUpdates(parsed, updates); + const updatedContent = stringify(updatedStructure, null, 2); + + fs.writeFileSync(filePath, updatedContent, 'utf-8'); +} + +function applyUpdates( + current: Record, + updates: Record, +): Record { + const result = current; + + for (const key of Object.getOwnPropertyNames(updates)) { + const value = updates[key]; + if ( + typeof value === 'object' && + value !== null && + !Array.isArray(value) && + typeof result[key] === 'object' && + result[key] !== null && + !Array.isArray(result[key]) + ) { + result[key] = applyUpdates( + result[key] as Record, + value as Record, + ); + } else { + result[key] = value; + } + } + + return result; +}