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.
This commit is contained in:
Coco Sheng
2026-05-04 16:34:47 -04:00
parent 75a8de83fc
commit a976c227c2
7 changed files with 379 additions and 95 deletions
+15 -2
View File
@@ -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,
@@ -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<typeof osActual>();
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<typeof fs>();
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<typeof import('@google/gemini-cli-core')>();
return {
...actual,
coreEvents: mockCoreEvents,
CoreEvent: actual.CoreEvent,
};
});
import { updateSettingsFilePreservingFormat } from '../utils/commentJson.js';
vi.mock('../utils/commentJson.js', async (importOriginal) => {
const actual =
await importOriginal<typeof import('../utils/commentJson.js')>();
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<string, unknown>;
expect(ui['compactToolOutput']).toBe(true);
const footer = ui['footer'] as Record<string, unknown>;
expect(footer['hideSandboxStatus']).toBe(true);
});
});
+142 -86
View File
@@ -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<string, unknown>,
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<string, unknown>;
current = record[key];
}
return current;
}
function deleteNestedProperty(obj: Record<string, unknown>, path: string) {
const keys = path.split('.');
const lastKey = keys.pop();
if (!lastKey) return;
let current: Record<string, unknown> = 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<string, unknown>;
} else {
// Path doesn't exist
return;
}
}
delete current[lastKey];
}
function setNestedProperty(
obj: Record<string, unknown>,
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<string, unknown>,
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<string, unknown>,
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<string, unknown>;
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<string, unknown>
| 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<string, unknown> | 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<string, unknown>
| 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<string, unknown>
| 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<string, unknown>
| 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<string, unknown> | 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<string, unknown> | undefined),
};
const agentsOverrides = {
const agentsSettings =
(settings.agents as Record<string, unknown> | undefined) || {};
const agentsOverrides =
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
...((agentsSettings['overrides'] as Record<string, unknown>) || {}),
};
(agentsSettings['overrides'] as Record<string, unknown>) || {};
let modified = false;
const migrateExperimental = <T = Record<string, unknown>>(
@@ -1271,32 +1328,31 @@ function migrateExperimentalSettings(
// Migrate experimental.plan -> general.plan.enabled
migrateExperimental<boolean>('plan', (planValue) => {
const generalSettings =
(settings.general as Record<string, unknown> | undefined) || {};
const newGeneral = { ...generalSettings };
const planSettings =
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
(newGeneral['plan'] as Record<string, unknown> | 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<string, unknown>,
'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;
}
+6
View File
@@ -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.
@@ -118,14 +118,18 @@ export function SettingsDialog({
);
const { fzfInstance, searchMap } = useMemo(() => {
const keys = getDialogSettingKeys();
const map = new Map<string, string>();
const map = new Map<string, string[]>();
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<string>();
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));
};
+3 -2
View File
@@ -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<string, unknown>;
try {
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
parsed = parse(originalContent) as Record<string, unknown>;
parsed = commentJsonParse(originalContent) as Record<string, unknown>;
} catch (error) {
coreEvents.emitFeedback(
'error',
+51
View File
@@ -738,6 +738,7 @@ export interface ConfigParameters {
disabledSkills?: string[];
adminSkillsEnabled?: boolean;
agents?: AgentSettings;
settings?: Partial<ConfigParameters>;
}>;
enableConseca?: boolean;
billing?: {
@@ -943,6 +944,7 @@ export class Config implements McpContext, AgentLoopContext {
disabledSkills?: string[];
adminSkillsEnabled?: boolean;
agents?: AgentSettings;
settings?: Partial<ConfigParameters>;
}>)
| undefined;
@@ -3528,6 +3530,55 @@ export class Config implements McpContext, AgentLoopContext {
}
}
/**
* Reloads core configuration settings.
*/
async reloadConfig(): Promise<void> {
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<string, unknown>).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<string, unknown>).topicUpdateNarration =
s.topicUpdateNarration;
}
if (s.experimentalAutoMemory !== undefined) {
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
(this as unknown as Record<string, unknown>).experimentalAutoMemory =
s.experimentalAutoMemory;
}
if (s.experimentalMemoryV2 !== undefined) {
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
(this as unknown as Record<string, unknown>).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<string, unknown>).adminSkillsEnabled =
refreshed.adminSkillsEnabled;
}
}
}
isInteractive(): boolean {
return this.interactive;
}