From a976c227c21e6753e72311d86f726b0e0148e662 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Mon, 4 May 2026 16:34:47 -0400 Subject: [PATCH] fix(config): ensure configuration persistence and fix in-memory regressions This commit addresses issue #25428 by: 1. Aligning the settings loader to use the same lenient parser (comment-json) as the saver, preventing data loss on minor JSON syntax errors. 2. Refactoring migrations to be granular instead of category-wide, protecting sibling keys from accidental deletion. 3. Expanding the hot-reload payload to ensure all configuration changes are immediately propagated to the core engine without a restart. 4. Fixing UI search result collisions for settings with duplicate labels. --- packages/cli/src/config/config.ts | 17 +- .../src/config/persistence_regression.test.ts | 151 ++++++++++++ packages/cli/src/config/settings.ts | 228 +++++++++++------- packages/cli/src/gemini.tsx | 6 + .../cli/src/ui/components/SettingsDialog.tsx | 16 +- packages/cli/src/utils/commentJson.ts | 5 +- packages/core/src/config/config.ts | 51 ++++ 7 files changed, 379 insertions(+), 95 deletions(-) create mode 100644 packages/cli/src/config/persistence_regression.test.ts diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 389fc4d2a7..28f0646385 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -1124,9 +1124,22 @@ export async function loadCliConfig( onModelChange: (model: string) => saveModelChange(loadSettings(cwd), model), onReload: async () => { const refreshedSettings = loadSettings(cwd); + const merged = refreshedSettings.merged; return { - disabledSkills: refreshedSettings.merged.skills.disabled, - agents: refreshedSettings.merged.agents, + disabledSkills: merged.skills.disabled, + adminSkillsEnabled: merged.skills.enabled, + agents: merged.agents, + settings: { + model: merged.model.name, + compressionThreshold: merged.model.compressionThreshold, + ideMode: merged.ide.enabled, + contextManagement: { + enabled: merged.experimental.contextManagement, + }, + topicUpdateNarration: merged.general.topicUpdateNarration, + experimentalAutoMemory: merged.experimental.autoMemory, + experimentalMemoryV2: merged.experimental.memoryV2, + }, }; }, enableConseca: settings.security?.enableConseca, diff --git a/packages/cli/src/config/persistence_regression.test.ts b/packages/cli/src/config/persistence_regression.test.ts new file mode 100644 index 0000000000..3c8fb8aed6 --- /dev/null +++ b/packages/cli/src/config/persistence_regression.test.ts @@ -0,0 +1,151 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import * as fs from 'node:fs'; +import * as osActual from 'node:os'; + +// Mock 'os' +vi.mock('os', async (importOriginal) => { + const actualOs = await importOriginal(); + return { + ...actualOs, + homedir: vi.fn(() => '/mock/home/user'), + platform: vi.fn(() => 'linux'), + }; +}); + +// Mock trustedFolders +vi.mock('./trustedFolders.js', () => ({ + isWorkspaceTrusted: vi + .fn() + .mockReturnValue({ isTrusted: true, source: 'file' }), +})); + +import { isWorkspaceTrusted } from './trustedFolders.js'; +import { loadSettings, SettingScope } from './settings.js'; + +vi.mock('fs', async (importOriginal) => { + const actualFs = await importOriginal(); + return { + ...actualFs, + existsSync: vi.fn(), + readFileSync: vi.fn(), + writeFileSync: vi.fn(), + mkdirSync: vi.fn(), + realpathSync: (p: string) => p, + }; +}); + +const mockCoreEvents = vi.hoisted(() => ({ + emitFeedback: vi.fn(), + on: vi.fn(), + off: vi.fn(), + emit: vi.fn(), + emitSettingsChanged: vi.fn(), +})); + +vi.mock('@google/gemini-cli-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + coreEvents: mockCoreEvents, + CoreEvent: actual.CoreEvent, + }; +}); + +import { updateSettingsFilePreservingFormat } from '../utils/commentJson.js'; +vi.mock('../utils/commentJson.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + updateSettingsFilePreservingFormat: vi.fn(), + }; +}); + +describe('Issue 25428 Regression', () => { + beforeEach(() => { + vi.resetAllMocks(); + vi.mocked(isWorkspaceTrusted).mockReturnValue({ + isTrusted: true, + source: 'file', + }); + vi.mocked(osActual.homedir).mockReturnValue('/mock/home/user'); + vi.mocked(fs.existsSync).mockReturnValue(false); + vi.mocked(fs.readFileSync).mockReturnValue('{}'); + vi.mocked(fs.mkdirSync).mockImplementation(() => undefined); + }); + + it('should load settings from a file with trailing commas', () => { + const contentWithComma = '{ "ui": { "compactToolOutput": true, } }'; + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.readFileSync).mockReturnValue(contentWithComma); + + const settings = loadSettings('/mock/workspace-' + Math.random()); + + expect(settings.user.settings.ui?.compactToolOutput).toBe(true); + }); + + it('should migrate settings granularly without nuking sibling keys', () => { + const initialContent = { + experimental: { + plan: true, + keepMe: 'important', + }, + }; + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.readFileSync).mockReturnValue(JSON.stringify(initialContent)); + + const settings = loadSettings('/mock/workspace-' + Math.random()); + + expect(settings.user.settings.general?.plan?.enabled).toBe(true); + expect(settings.user.settings.experimental?.plan).toBeUndefined(); + expect(settings.user.settings.experimental?.keepMe).toBe('important'); + + expect(updateSettingsFilePreservingFormat).toHaveBeenCalled(); + const lastCall = vi + .mocked(updateSettingsFilePreservingFormat) + .mock.calls.at(-1); + const savedSettings = lastCall![1]; + + const experimental = savedSettings['experimental'] as Record< + string, + unknown + >; + expect(experimental['keepMe']).toBe('important'); + expect(experimental['plan']).toBeUndefined(); + }); + + it('should update specific settings without affecting raw siblings', () => { + const initialContent = { + ui: { + compactToolOutput: true, + footer: { + hideSandboxStatus: false, + }, + }, + }; + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.readFileSync).mockReturnValue(JSON.stringify(initialContent)); + + const settings = loadSettings('/mock/workspace-' + Math.random()); + + settings.setValue(SettingScope.User, 'ui.footer.hideSandboxStatus', true); + + expect(updateSettingsFilePreservingFormat).toHaveBeenCalled(); + const lastCall = vi + .mocked(updateSettingsFilePreservingFormat) + .mock.calls.at(-1); + const savedSettings = lastCall![1]; + + const ui = savedSettings['ui'] as Record; + expect(ui['compactToolOutput']).toBe(true); + const footer = ui['footer'] as Record; + expect(footer['hideSandboxStatus']).toBe(true); + }); +}); diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 5a52e5af3c..b94be5fe50 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -22,7 +22,6 @@ import { type AdminControlsSettings, createCache, } from '@google/gemini-cli-core'; -import stripJsonComments from 'strip-json-comments'; import { DefaultLight } from '../ui/themes/builtin/light/default-light.js'; import { DefaultDark } from '../ui/themes/builtin/dark/default-dark.js'; import { isWorkspaceTrusted } from './trustedFolders.js'; @@ -48,7 +47,10 @@ export { import { resolveEnvVarsInObject } from '../utils/envVarResolver.js'; import { customDeepMerge } from '../utils/deepMerge.js'; -import { updateSettingsFilePreservingFormat } from '../utils/commentJson.js'; +import { + updateSettingsFilePreservingFormat, + parse as parseCommentJson, +} from '../utils/commentJson.js'; import { validateSettings, formatValidationError, @@ -209,6 +211,46 @@ export interface SettingsFile { readOnly?: boolean; } +function getNestedProperty( + obj: Record, + path: string, +): unknown { + const keys = path.split('.'); + let current: unknown = obj; + for (const key of keys) { + if ( + typeof current !== 'object' || + current === null || + Array.isArray(current) + ) { + return undefined; + } + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + const record = current as Record; + current = record[key]; + } + return current; +} + +function deleteNestedProperty(obj: Record, path: string) { + const keys = path.split('.'); + const lastKey = keys.pop(); + if (!lastKey) return; + + let current: Record = obj; + for (const key of keys) { + const next = current[key]; + if (typeof next === 'object' && next !== null) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + current = next as Record; + } else { + // Path doesn't exist + return; + } + } + delete current[lastKey]; +} + function setNestedProperty( obj: Record, path: string, @@ -471,6 +513,21 @@ export class LoadedSettings { coreEvents.emitSettingsChanged(); } + deleteValue(scope: LoadableSettingScope, key: string): void { + const settingsFile = this.forScope(scope); + + deleteNestedProperty(settingsFile.settings, key); + + if (this.isPersistable(settingsFile)) { + deleteNestedProperty(settingsFile.originalSettings, key); + saveSettings(settingsFile); + } + + this._merged = this.computeMergedSettings(); + this._snapshot = this.computeSnapshot(); + coreEvents.emitSettingsChanged(); + } + setRemoteAdminSettings(remoteSettings: AdminControlsSettings): void { const admin: Settings['admin'] = {}; const { strictModeDisabled, mcpSetting, cliFeatureSetting } = @@ -715,7 +772,7 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { try { if (fs.existsSync(filePath)) { const content = fs.readFileSync(filePath, 'utf-8'); - const rawSettings: unknown = JSON.parse(stripJsonComments(content)); + const rawSettings: unknown = parseCommentJson(content); if ( typeof rawSettings !== 'object' || @@ -914,15 +971,32 @@ export function migrateDeprecatedSettings( * Helper to migrate a boolean setting and track it if it's deprecated. */ const migrateBoolean = ( - settings: Record, + scope: LoadableSettingScope, + categoryKey: string, oldKey: string, newKey: string, prefix: string, foundDeprecated?: string[], ): boolean => { let modified = false; - const oldValue = settings[oldKey]; - const newValue = settings[newKey]; + const settingsFile = loadedSettings.forScope(scope); + const property = getNestedProperty( + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + settingsFile.settings as unknown as Record, + categoryKey, + ); + if ( + typeof property !== 'object' || + property === null || + Array.isArray(property) + ) { + return false; + } + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + const category = property as Record; + + const oldValue = category[oldKey]; + const newValue = category[newKey]; if (typeof oldValue === 'boolean') { if (foundDeprecated) { @@ -931,14 +1005,14 @@ export function migrateDeprecatedSettings( if (typeof newValue === 'boolean') { // Both exist, trust the new one if (removeDeprecated) { - delete settings[oldKey]; + loadedSettings.deleteValue(scope, `${categoryKey}.${oldKey}`); modified = true; } } else { // Only old exists, migrate to new (inverted) - settings[newKey] = !oldValue; + loadedSettings.setValue(scope, `${categoryKey}.${newKey}`, !oldValue); if (removeDeprecated) { - delete settings[oldKey]; + loadedSettings.deleteValue(scope, `${categoryKey}.${oldKey}`); } modified = true; } @@ -950,76 +1024,64 @@ export function migrateDeprecatedSettings( const settingsFile = loadedSettings.forScope(scope); const settings = settingsFile.settings; const foundDeprecated: string[] = []; + let modified = false; // Migrate general settings - const generalSettings = settings.general as - | Record - | undefined; - if (generalSettings) { - const newGeneral = { ...generalSettings }; - let modified = false; + modified = + migrateBoolean( + scope, + 'general', + 'disableAutoUpdate', + 'enableAutoUpdate', + 'general', + foundDeprecated, + ) || modified; + modified = + migrateBoolean( + scope, + 'general', + 'disableUpdateNag', + 'enableAutoUpdateNotification', + 'general', + foundDeprecated, + ) || modified; - modified = - migrateBoolean( - newGeneral, - 'disableAutoUpdate', - 'enableAutoUpdate', - 'general', - foundDeprecated, - ) || modified; - modified = - migrateBoolean( - newGeneral, - 'disableUpdateNag', - 'enableAutoUpdateNotification', - 'general', - foundDeprecated, - ) || modified; - - if (modified) { - loadedSettings.setValue(scope, 'general', newGeneral); - if (!settingsFile.readOnly) { - anyModified = true; - } - } + if (modified && !settingsFile.readOnly) { + anyModified = true; } // Migrate ui settings const uiSettings = settings.ui as Record | undefined; if (uiSettings) { - const newUi = { ...uiSettings }; // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - const accessibilitySettings = newUi['accessibility'] as + const accessibilitySettings = uiSettings['accessibility'] as | Record | undefined; if (accessibilitySettings) { - const newAccessibility = { ...accessibilitySettings }; if ( migrateBoolean( - newAccessibility, + scope, + 'ui.accessibility', 'disableLoadingPhrases', 'enableLoadingPhrases', 'ui.accessibility', foundDeprecated, ) ) { - newUi['accessibility'] = newAccessibility; - loadedSettings.setValue(scope, 'ui', newUi); if (!settingsFile.readOnly) { anyModified = true; } } // Migrate enableLoadingPhrases: false → loadingPhrases: 'off' - const enableLP = newAccessibility['enableLoadingPhrases']; + const enableLP = accessibilitySettings['enableLoadingPhrases']; if ( typeof enableLP === 'boolean' && - newUi['loadingPhrases'] === undefined + uiSettings['loadingPhrases'] === undefined ) { if (!enableLP) { - newUi['loadingPhrases'] = 'off'; - loadedSettings.setValue(scope, 'ui', newUi); + loadedSettings.setValue(scope, 'ui.loadingPhrases', 'off'); if (!settingsFile.readOnly) { anyModified = true; } @@ -1034,25 +1096,22 @@ export function migrateDeprecatedSettings( | Record | undefined; if (contextSettings) { - const newContext = { ...contextSettings }; // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - const fileFilteringSettings = newContext['fileFiltering'] as + const fileFilteringSettings = contextSettings['fileFiltering'] as | Record | undefined; if (fileFilteringSettings) { - const newFileFiltering = { ...fileFilteringSettings }; if ( migrateBoolean( - newFileFiltering, + scope, + 'context.fileFiltering', 'disableFuzzySearch', 'enableFuzzySearch', 'context.fileFiltering', foundDeprecated, ) ) { - newContext['fileFiltering'] = newFileFiltering; - loadedSettings.setValue(scope, 'context', newContext); if (!settingsFile.readOnly) { anyModified = true; } @@ -1068,21 +1127,21 @@ export function migrateDeprecatedSettings( const generalSettings = (settings.general as Record | undefined) || {}; - const newGeneral = { ...generalSettings }; // Only set defaultApprovalMode if it's not already set - if (newGeneral['defaultApprovalMode'] === undefined) { - newGeneral['defaultApprovalMode'] = toolsSettings['approvalMode']; - loadedSettings.setValue(scope, 'general', newGeneral); + if (generalSettings['defaultApprovalMode'] === undefined) { + loadedSettings.setValue( + scope, + 'general.defaultApprovalMode', + toolsSettings['approvalMode'], + ); if (!settingsFile.readOnly) { anyModified = true; } } if (removeDeprecated) { - const newTools = { ...toolsSettings }; - delete newTools['approvalMode']; - loadedSettings.setValue(scope, 'tools', newTools); + loadedSettings.deleteValue(scope, 'tools.approvalMode'); if (!settingsFile.readOnly) { anyModified = true; } @@ -1187,13 +1246,11 @@ function migrateExperimentalSettings( | undefined; if (experimentalSettings) { - const agentsSettings = { - ...(settings.agents as Record | undefined), - }; - const agentsOverrides = { + const agentsSettings = + (settings.agents as Record | undefined) || {}; + const agentsOverrides = // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - ...((agentsSettings['overrides'] as Record) || {}), - }; + (agentsSettings['overrides'] as Record) || {}; let modified = false; const migrateExperimental = >( @@ -1271,32 +1328,31 @@ function migrateExperimentalSettings( // Migrate experimental.plan -> general.plan.enabled migrateExperimental('plan', (planValue) => { - const generalSettings = - (settings.general as Record | undefined) || {}; - const newGeneral = { ...generalSettings }; - const planSettings = - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - (newGeneral['plan'] as Record | undefined) || {}; - const newPlan = { ...planSettings }; + // Check if general.plan.enabled is explicitly set in THIS scope + const originalScopeSettings = + loadedSettings.forScope(scope).originalSettings; + const isPlanSetInRaw = + getNestedProperty( + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + originalScopeSettings as unknown as Record, + 'general.plan.enabled', + ) !== undefined; - if (newPlan['enabled'] === undefined) { - newPlan['enabled'] = planValue; - newGeneral['plan'] = newPlan; - loadedSettings.setValue(scope, 'general', newGeneral); - modified = true; + if (!isPlanSetInRaw) { + loadedSettings.setValue(scope, 'general.plan.enabled', planValue); } }); if (modified) { - agentsSettings['overrides'] = agentsOverrides; - loadedSettings.setValue(scope, 'agents', agentsSettings); + loadedSettings.setValue(scope, 'agents.overrides', agentsOverrides); if (removeDeprecated) { - const newExperimental = { ...experimentalSettings }; - delete newExperimental['codebaseInvestigatorSettings']; - delete newExperimental['cliHelpAgentSettings']; - delete newExperimental['plan']; - loadedSettings.setValue(scope, 'experimental', newExperimental); + loadedSettings.deleteValue( + scope, + 'experimental.codebaseInvestigatorSettings', + ); + loadedSettings.deleteValue(scope, 'experimental.cliHelpAgentSettings'); + loadedSettings.deleteValue(scope, 'experimental.plan'); } return true; } diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index 892ee9862a..c5fb7666a2 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -544,6 +544,12 @@ export async function main() { }); loadConfigHandle?.end(); + coreEvents.on(CoreEvent.SettingsChanged, () => { + config?.reloadConfig().catch((e) => { + debugLogger.error('Failed to reload config:', e); + }); + }); + // Initialize storage immediately after loading config to ensure that // storage-related operations (like listing or resuming sessions) have // access to the project identifier. diff --git a/packages/cli/src/ui/components/SettingsDialog.tsx b/packages/cli/src/ui/components/SettingsDialog.tsx index 994bde6ed3..9548963e40 100644 --- a/packages/cli/src/ui/components/SettingsDialog.tsx +++ b/packages/cli/src/ui/components/SettingsDialog.tsx @@ -118,14 +118,18 @@ export function SettingsDialog({ ); const { fzfInstance, searchMap } = useMemo(() => { const keys = getDialogSettingKeys(); - const map = new Map(); + const map = new Map(); const searchItems: string[] = []; keys.forEach((key) => { const def = getSettingDefinition(key); if (def?.label) { - searchItems.push(def.label); - map.set(def.label.toLowerCase(), key); + const labelLower = def.label.toLowerCase(); + if (!map.has(labelLower)) { + searchItems.push(def.label); + map.set(labelLower, []); + } + map.get(labelLower)!.push(key); } }); @@ -152,8 +156,10 @@ export function SettingsDialog({ const matchedKeys = new Set(); results.forEach((res: FzfResult) => { - const key = searchMap.get(res.item.toLowerCase()); - if (key) matchedKeys.add(key); + const keysForLabel = searchMap.get(res.item.toLowerCase()); + if (keysForLabel) { + keysForLabel.forEach((key) => matchedKeys.add(key)); + } }); setFilteredKeys(Array.from(matchedKeys)); }; diff --git a/packages/cli/src/utils/commentJson.ts b/packages/cli/src/utils/commentJson.ts index c60011b81f..fae0716df4 100644 --- a/packages/cli/src/utils/commentJson.ts +++ b/packages/cli/src/utils/commentJson.ts @@ -5,7 +5,8 @@ */ import * as fs from 'node:fs'; -import { parse, stringify } from 'comment-json'; +import { parse as commentJsonParse, stringify } from 'comment-json'; +export { commentJsonParse as parse }; import { coreEvents } from '@google/gemini-cli-core'; /** @@ -30,7 +31,7 @@ export function updateSettingsFilePreservingFormat( let parsed: Record; try { // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - parsed = parse(originalContent) as Record; + parsed = commentJsonParse(originalContent) as Record; } catch (error) { coreEvents.emitFeedback( 'error', diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 985915e6ff..69d1b59c92 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -738,6 +738,7 @@ export interface ConfigParameters { disabledSkills?: string[]; adminSkillsEnabled?: boolean; agents?: AgentSettings; + settings?: Partial; }>; enableConseca?: boolean; billing?: { @@ -943,6 +944,7 @@ export class Config implements McpContext, AgentLoopContext { disabledSkills?: string[]; adminSkillsEnabled?: boolean; agents?: AgentSettings; + settings?: Partial; }>) | undefined; @@ -3528,6 +3530,55 @@ export class Config implements McpContext, AgentLoopContext { } } + /** + * Reloads core configuration settings. + */ + async reloadConfig(): Promise { + if (this.onReload) { + const refreshed = await this.onReload(); + if (refreshed.settings) { + // Hydrate key fields that affect core behavior + const s = refreshed.settings; + if (s.model) this.setModel(s.model); + if (s.compressionThreshold !== undefined) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + (this as unknown as Record).compressionThreshold = + s.compressionThreshold; + } + if (s.ideMode !== undefined) this.ideMode = s.ideMode; + if (s.contextManagement) { + Object.assign(this.contextManagement, s.contextManagement); + } + if (s.topicUpdateNarration !== undefined) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + (this as unknown as Record).topicUpdateNarration = + s.topicUpdateNarration; + } + if (s.experimentalAutoMemory !== undefined) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + (this as unknown as Record).experimentalAutoMemory = + s.experimentalAutoMemory; + } + if (s.experimentalMemoryV2 !== undefined) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + (this as unknown as Record).experimentalMemoryV2 = + s.experimentalMemoryV2; + } + } + if (refreshed.agents) { + this.agents = refreshed.agents; + } + if (refreshed.disabledSkills) { + this.disabledSkills = refreshed.disabledSkills; + } + if (refreshed.adminSkillsEnabled !== undefined) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + (this as unknown as Record).adminSkillsEnabled = + refreshed.adminSkillsEnabled; + } + } + } + isInteractive(): boolean { return this.interactive; }