refactor(config): remove legacy V1 settings migration logic (#16252)

This commit is contained in:
Gal Zahavi
2026-01-09 14:34:23 -08:00
committed by GitHub
parent 1fb55dcb2e
commit 356f76e545
10 changed files with 22 additions and 1160 deletions

View File

@@ -44,7 +44,7 @@ vi.mock('./settingsSchema.js', async (importOriginal) => {
});
// NOW import everything else, including the (now effectively re-exported) settings.js
import path, * as pathActual from 'node:path'; // Restored for MOCK_WORKSPACE_SETTINGS_PATH
import * as pathActual from 'node:path'; // Restored for MOCK_WORKSPACE_SETTINGS_PATH
import {
describe,
it,
@@ -65,18 +65,12 @@ import {
USER_SETTINGS_PATH, // This IS the mocked path.
getSystemSettingsPath,
getSystemDefaultsPath,
migrateSettingsToV1,
needsMigration,
type Settings,
loadEnvironment,
migrateDeprecatedSettings,
SettingScope,
saveSettings,
type SettingsFile,
getDefaultsFromSchema,
} from './settings.js';
import { FatalConfigError, GEMINI_DIR } from '@google/gemini-cli-core';
import { ExtensionManager } from './extension-manager.js';
import { updateSettingsFilePreservingFormat } from '../utils/commentJson.js';
import {
getSettingsSchema,
@@ -290,169 +284,6 @@ describe('Settings Loading and Merging', () => {
});
});
it('should correctly migrate a complex legacy (v1) settings file', () => {
(mockFsExistsSync as Mock).mockImplementation(
(p: fs.PathLike) => p === USER_SETTINGS_PATH,
);
const legacySettingsContent = {
theme: 'legacy-dark',
vimMode: true,
contextFileName: 'LEGACY_CONTEXT.md',
model: 'gemini-2.5-pro',
mcpServers: {
'legacy-server-1': {
command: 'npm',
args: ['run', 'start:server1'],
description: 'Legacy Server 1',
},
'legacy-server-2': {
command: 'node',
args: ['server2.js'],
description: 'Legacy Server 2',
},
},
allowMCPServers: ['legacy-server-1'],
someUnrecognizedSetting: 'should-be-preserved',
};
(fs.readFileSync as Mock).mockImplementation(
(p: fs.PathOrFileDescriptor) => {
if (p === USER_SETTINGS_PATH)
return JSON.stringify(legacySettingsContent);
return '{}';
},
);
const settings = loadSettings(MOCK_WORKSPACE_DIR);
expect(settings.merged).toMatchObject({
ui: {
theme: 'legacy-dark',
},
general: {
vimMode: true,
},
context: {
fileName: 'LEGACY_CONTEXT.md',
},
model: {
name: 'gemini-2.5-pro',
},
mcpServers: {
'legacy-server-1': {
command: 'npm',
args: ['run', 'start:server1'],
description: 'Legacy Server 1',
},
'legacy-server-2': {
command: 'node',
args: ['server2.js'],
description: 'Legacy Server 2',
},
},
mcp: {
allowed: ['legacy-server-1'],
},
someUnrecognizedSetting: 'should-be-preserved',
});
});
it('should rewrite allowedTools to tools.allowed during migration', () => {
(mockFsExistsSync as Mock).mockImplementation(
(p: fs.PathLike) => p === USER_SETTINGS_PATH,
);
const legacySettingsContent = {
allowedTools: ['fs', 'shell'],
};
(fs.readFileSync as Mock).mockImplementation(
(p: fs.PathOrFileDescriptor) => {
if (p === USER_SETTINGS_PATH)
return JSON.stringify(legacySettingsContent);
return '{}';
},
);
const settings = loadSettings(MOCK_WORKSPACE_DIR);
expect(settings.merged.tools?.allowed).toEqual(['fs', 'shell']);
expect((settings.merged as TestSettings)['allowedTools']).toBeUndefined();
});
it('should allow V2 settings to override V1 settings when both are present (zombie setting fix)', () => {
(mockFsExistsSync as Mock).mockImplementation(
(p: fs.PathLike) => p === USER_SETTINGS_PATH,
);
const mixedSettingsContent = {
// V1 setting (migrates to ui.accessibility.screenReader = true)
accessibility: {
screenReader: true,
},
// V2 setting (explicitly set to false)
ui: {
accessibility: {
screenReader: false,
},
},
};
(fs.readFileSync as Mock).mockImplementation(
(p: fs.PathOrFileDescriptor) => {
if (p === USER_SETTINGS_PATH)
return JSON.stringify(mixedSettingsContent);
return '{}';
},
);
const settings = loadSettings(MOCK_WORKSPACE_DIR);
// We expect the V2 setting (false) to win, NOT the migrated V1 setting (true)
expect(settings.merged.ui?.accessibility?.screenReader).toBe(false);
});
it('should correctly merge and migrate legacy array properties from multiple scopes', () => {
(mockFsExistsSync as Mock).mockReturnValue(true);
const legacyUserSettings = {
includeDirectories: ['/user/dir'],
excludeTools: ['user-tool'],
excludedProjectEnvVars: ['USER_VAR'],
};
const legacyWorkspaceSettings = {
includeDirectories: ['/workspace/dir'],
excludeTools: ['workspace-tool'],
excludedProjectEnvVars: ['WORKSPACE_VAR', 'USER_VAR'],
};
(fs.readFileSync as Mock).mockImplementation(
(p: fs.PathOrFileDescriptor) => {
if (p === USER_SETTINGS_PATH)
return JSON.stringify(legacyUserSettings);
if (p === MOCK_WORKSPACE_SETTINGS_PATH)
return JSON.stringify(legacyWorkspaceSettings);
return '{}';
},
);
const settings = loadSettings(MOCK_WORKSPACE_DIR);
// Verify includeDirectories are concatenated
expect(settings.merged.context?.includeDirectories).toEqual([
'/user/dir',
'/workspace/dir',
]);
// Verify excludeTools are concatenated and de-duped
expect(settings.merged.tools?.exclude).toEqual([
'user-tool',
'workspace-tool',
]);
// Verify excludedProjectEnvVars are concatenated and de-duped
expect(settings.merged.advanced?.excludedEnvVars).toEqual(
expect.arrayContaining(['USER_VAR', 'WORKSPACE_VAR']),
);
expect(settings.merged.advanced?.excludedEnvVars).toHaveLength(4);
});
it('should merge all settings files with the correct precedence', () => {
// Mock schema to test defaults application
const mockSchema = {
@@ -1771,653 +1602,6 @@ describe('Settings Loading and Merging', () => {
});
});
describe('with workspace trust', () => {
it('should merge workspace settings when workspace is trusted', () => {
(mockFsExistsSync as Mock).mockReturnValue(true);
const userSettingsContent = {
ui: { theme: 'dark' },
tools: { sandbox: false },
};
const workspaceSettingsContent = {
tools: { sandbox: true },
context: { fileName: 'WORKSPACE.md' },
};
(fs.readFileSync as Mock).mockImplementation(
(p: fs.PathOrFileDescriptor) => {
if (p === USER_SETTINGS_PATH)
return JSON.stringify(userSettingsContent);
if (p === MOCK_WORKSPACE_SETTINGS_PATH)
return JSON.stringify(workspaceSettingsContent);
return '{}';
},
);
const settings = loadSettings(MOCK_WORKSPACE_DIR);
expect(settings.merged.tools?.sandbox).toBe(true);
expect(settings.merged.context?.fileName).toBe('WORKSPACE.md');
expect(settings.merged.ui?.theme).toBe('dark');
});
it('should NOT merge workspace settings when workspace is not trusted', () => {
vi.mocked(isWorkspaceTrusted).mockReturnValue({
isTrusted: false,
source: 'file',
});
(mockFsExistsSync as Mock).mockReturnValue(true);
const userSettingsContent = {
ui: { theme: 'dark' },
tools: { sandbox: false },
context: { fileName: 'USER.md' },
};
const workspaceSettingsContent = {
tools: { sandbox: true },
context: { fileName: 'WORKSPACE.md' },
};
(fs.readFileSync as Mock).mockImplementation(
(p: fs.PathOrFileDescriptor) => {
if (p === USER_SETTINGS_PATH)
return JSON.stringify(userSettingsContent);
if (p === MOCK_WORKSPACE_SETTINGS_PATH)
return JSON.stringify(workspaceSettingsContent);
return '{}';
},
);
const settings = loadSettings(MOCK_WORKSPACE_DIR);
expect(settings.merged.tools?.sandbox).toBe(false); // User setting
expect(settings.merged.context?.fileName).toBe('USER.md'); // User setting
expect(settings.merged.ui?.theme).toBe('dark'); // User setting
});
});
describe('migrateSettingsToV1', () => {
it('should handle an empty object', () => {
const v2Settings = {};
const v1Settings = migrateSettingsToV1(v2Settings);
expect(v1Settings).toEqual({});
});
it('should migrate a simple v2 settings object to v1', () => {
const v2Settings = {
general: {
preferredEditor: 'vscode',
vimMode: true,
},
ui: {
theme: 'dark',
},
};
const v1Settings = migrateSettingsToV1(v2Settings);
expect(v1Settings).toEqual({
preferredEditor: 'vscode',
vimMode: true,
theme: 'dark',
});
});
it('should handle nested properties correctly', () => {
const v2Settings = {
security: {
folderTrust: {
enabled: true,
},
auth: {
selectedType: 'oauth',
},
},
advanced: {
autoConfigureMemory: true,
},
};
const v1Settings = migrateSettingsToV1(v2Settings);
expect(v1Settings).toEqual({
folderTrust: true,
selectedAuthType: 'oauth',
autoConfigureMaxOldSpaceSize: true,
});
});
it('should preserve mcpServers at the top level', () => {
const v2Settings = {
general: {
preferredEditor: 'vscode',
},
mcpServers: {
'my-server': {
command: 'npm start',
},
},
};
const v1Settings = migrateSettingsToV1(v2Settings);
expect(v1Settings).toEqual({
preferredEditor: 'vscode',
mcpServers: {
'my-server': {
command: 'npm start',
},
},
});
});
it('should carry over unrecognized top-level properties', () => {
const v2Settings = {
general: {
vimMode: false,
},
unrecognized: 'value',
another: {
nested: true,
},
};
const v1Settings = migrateSettingsToV1(v2Settings);
expect(v1Settings).toEqual({
vimMode: false,
unrecognized: 'value',
another: {
nested: true,
},
});
});
it('should handle a complex object with mixed properties', () => {
const v2Settings = {
general: {
disableAutoUpdate: true,
},
ui: {
hideBanner: true,
customThemes: {
myTheme: {},
},
},
model: {
name: 'gemini-pro',
},
mcpServers: {
'server-1': {
command: 'node server.js',
},
},
unrecognized: {
should: 'be-preserved',
},
};
const v1Settings = migrateSettingsToV1(v2Settings);
expect(v1Settings).toEqual({
disableAutoUpdate: true,
hideBanner: true,
customThemes: {
myTheme: {},
},
model: 'gemini-pro',
mcpServers: {
'server-1': {
command: 'node server.js',
},
},
unrecognized: {
should: 'be-preserved',
},
});
});
it('should not migrate a v1 settings object', () => {
const v1Settings = {
preferredEditor: 'vscode',
vimMode: true,
theme: 'dark',
};
const migratedSettings = migrateSettingsToV1(v1Settings);
expect(migratedSettings).toEqual({
preferredEditor: 'vscode',
vimMode: true,
theme: 'dark',
});
});
it('should migrate a full v2 settings object to v1', () => {
const v2Settings: TestSettings = {
general: {
preferredEditor: 'code',
vimMode: true,
},
ui: {
theme: 'dark',
},
privacy: {
usageStatisticsEnabled: false,
},
model: {
name: 'gemini-2.5-pro',
},
context: {
fileName: 'CONTEXT.md',
includeDirectories: ['/src'],
},
tools: {
sandbox: true,
exclude: ['toolA'],
},
mcp: {
allowed: ['server1'],
},
security: {
folderTrust: {
enabled: true,
},
},
advanced: {
dnsResolutionOrder: 'ipv4first',
excludedEnvVars: ['SECRET'],
},
mcpServers: {
'my-server': {
command: 'npm start',
},
},
unrecognizedTopLevel: {
value: 'should be preserved',
},
};
const v1Settings = migrateSettingsToV1(v2Settings);
expect(v1Settings).toEqual({
preferredEditor: 'code',
vimMode: true,
theme: 'dark',
usageStatisticsEnabled: false,
model: 'gemini-2.5-pro',
contextFileName: 'CONTEXT.md',
includeDirectories: ['/src'],
sandbox: true,
excludeTools: ['toolA'],
allowMCPServers: ['server1'],
folderTrust: true,
dnsResolutionOrder: 'ipv4first',
excludedProjectEnvVars: ['SECRET'],
mcpServers: {
'my-server': {
command: 'npm start',
},
},
unrecognizedTopLevel: {
value: 'should be preserved',
},
});
});
it('should handle partial v2 settings', () => {
const v2Settings: TestSettings = {
general: {
vimMode: false,
},
ui: {},
model: {
name: 'gemini-2.5-pro',
},
unrecognized: 'value',
};
const v1Settings = migrateSettingsToV1(v2Settings);
expect(v1Settings).toEqual({
vimMode: false,
model: 'gemini-2.5-pro',
unrecognized: 'value',
});
});
it('should handle settings with different data types', () => {
const v2Settings: TestSettings = {
general: {
vimMode: false,
},
model: {
maxSessionTurns: -1,
},
context: {
includeDirectories: [],
},
security: {
folderTrust: {
enabled: undefined,
},
},
};
const v1Settings = migrateSettingsToV1(v2Settings);
expect(v1Settings).toEqual({
vimMode: false,
maxSessionTurns: -1,
includeDirectories: [],
security: {
folderTrust: {
enabled: undefined,
},
},
});
});
it('should preserve unrecognized top-level keys', () => {
const v2Settings: TestSettings = {
general: {
vimMode: true,
},
customTopLevel: {
a: 1,
b: [2],
},
anotherOne: 'hello',
};
const v1Settings = migrateSettingsToV1(v2Settings);
expect(v1Settings).toEqual({
vimMode: true,
customTopLevel: {
a: 1,
b: [2],
},
anotherOne: 'hello',
});
});
it('should handle an empty v2 settings object', () => {
const v2Settings = {};
const v1Settings = migrateSettingsToV1(v2Settings);
expect(v1Settings).toEqual({});
});
it('should correctly handle mcpServers at the top level', () => {
const v2Settings: TestSettings = {
mcpServers: {
serverA: { command: 'a' },
},
mcp: {
allowed: ['serverA'],
},
};
const v1Settings = migrateSettingsToV1(v2Settings);
expect(v1Settings).toEqual({
mcpServers: {
serverA: { command: 'a' },
},
allowMCPServers: ['serverA'],
});
});
it('should correctly migrate customWittyPhrases', () => {
const v2Settings: Partial<Settings> = {
ui: {
customWittyPhrases: ['test phrase'],
},
};
const v1Settings = migrateSettingsToV1(v2Settings as Settings);
expect(v1Settings).toEqual({
customWittyPhrases: ['test phrase'],
});
});
});
describe('loadEnvironment', () => {
function setup({
isFolderTrustEnabled = true,
isWorkspaceTrustedValue = true,
}) {
delete process.env['TESTTEST']; // reset
const geminiEnvPath = path.resolve(path.join(GEMINI_DIR, '.env'));
vi.mocked(isWorkspaceTrusted).mockReturnValue({
isTrusted: isWorkspaceTrustedValue,
source: 'file',
});
(mockFsExistsSync as Mock).mockImplementation((p: fs.PathLike) =>
[USER_SETTINGS_PATH, geminiEnvPath].includes(p.toString()),
);
const userSettingsContent: Settings = {
ui: {
theme: 'dark',
},
security: {
folderTrust: {
enabled: isFolderTrustEnabled,
},
},
context: {
fileName: 'USER_CONTEXT.md',
},
};
(fs.readFileSync as Mock).mockImplementation(
(p: fs.PathOrFileDescriptor) => {
if (p === USER_SETTINGS_PATH)
return JSON.stringify(userSettingsContent);
if (p === geminiEnvPath) return 'TESTTEST=1234';
return '{}';
},
);
}
it('sets environment variables from .env files', () => {
setup({ isFolderTrustEnabled: false, isWorkspaceTrustedValue: true });
loadEnvironment(loadSettings(MOCK_WORKSPACE_DIR).merged);
expect(process.env['TESTTEST']).toEqual('1234');
});
it('does not load env files from untrusted spaces', () => {
setup({ isFolderTrustEnabled: true, isWorkspaceTrustedValue: false });
loadEnvironment(loadSettings(MOCK_WORKSPACE_DIR).merged);
expect(process.env['TESTTEST']).not.toEqual('1234');
});
});
describe('needsMigration', () => {
it('should return false for an empty object', () => {
expect(needsMigration({})).toBe(false);
});
it('should return false for settings that are already in V2 format', () => {
const v2Settings: Partial<Settings> = {
ui: {
theme: 'dark',
},
tools: {
sandbox: true,
},
};
expect(needsMigration(v2Settings)).toBe(false);
});
it('should return true for settings with a V1 key that needs to be moved', () => {
const v1Settings = {
theme: 'dark', // v1 key
};
expect(needsMigration(v1Settings)).toBe(true);
});
it('should return true for settings with a mix of V1 and V2 keys', () => {
const mixedSettings = {
theme: 'dark', // v1 key
tools: {
sandbox: true, // v2 key
},
};
expect(needsMigration(mixedSettings)).toBe(true);
});
it('should return false for settings with only V1 keys that are the same in V2', () => {
const v1Settings = {
mcpServers: {},
telemetry: {},
extensions: [],
};
expect(needsMigration(v1Settings)).toBe(false);
});
it('should return true for settings with a mix of V1 keys that are the same in V2 and V1 keys that need moving', () => {
const v1Settings = {
mcpServers: {}, // same in v2
theme: 'dark', // needs moving
};
expect(needsMigration(v1Settings)).toBe(true);
});
it('should return false for settings with unrecognized keys', () => {
const settings = {
someUnrecognizedKey: 'value',
};
expect(needsMigration(settings)).toBe(false);
});
it('should return false for settings with v2 keys and unrecognized keys', () => {
const settings = {
ui: { theme: 'dark' },
someUnrecognizedKey: 'value',
};
expect(needsMigration(settings)).toBe(false);
});
});
describe('migrateDeprecatedSettings', () => {
let mockFsExistsSync: Mock;
let mockFsReadFileSync: Mock;
beforeEach(() => {
vi.resetAllMocks();
mockFsExistsSync = vi.mocked(fs.existsSync);
mockFsExistsSync.mockReturnValue(true);
mockFsReadFileSync = vi.mocked(fs.readFileSync);
mockFsReadFileSync.mockReturnValue('{}');
vi.mocked(isWorkspaceTrusted).mockReturnValue({
isTrusted: true,
source: undefined,
});
});
afterEach(() => {
vi.restoreAllMocks();
});
it('should migrate disabled extensions from user and workspace settings', () => {
const userSettingsContent = {
extensions: {
disabled: ['user-ext-1', 'shared-ext'],
},
};
const workspaceSettingsContent = {
extensions: {
disabled: ['workspace-ext-1', 'shared-ext'],
},
};
mockFsReadFileSync.mockImplementation((p: fs.PathOrFileDescriptor) => {
if (p === USER_SETTINGS_PATH)
return JSON.stringify(userSettingsContent);
if (p === MOCK_WORKSPACE_SETTINGS_PATH)
return JSON.stringify(workspaceSettingsContent);
return '{}';
});
const loadedSettings = loadSettings(MOCK_WORKSPACE_DIR);
const setValueSpy = vi.spyOn(loadedSettings, 'setValue');
const extensionManager = new ExtensionManager({
settings: loadedSettings.merged,
workspaceDir: MOCK_WORKSPACE_DIR,
requestConsent: vi.fn(),
requestSetting: vi.fn(),
});
const mockDisableExtension = vi.spyOn(
extensionManager,
'disableExtension',
);
mockDisableExtension.mockImplementation(async () => {});
migrateDeprecatedSettings(loadedSettings, extensionManager);
// Check user settings migration
expect(mockDisableExtension).toHaveBeenCalledWith(
'user-ext-1',
SettingScope.User,
);
expect(mockDisableExtension).toHaveBeenCalledWith(
'shared-ext',
SettingScope.User,
);
// Check workspace settings migration
expect(mockDisableExtension).toHaveBeenCalledWith(
'workspace-ext-1',
SettingScope.Workspace,
);
expect(mockDisableExtension).toHaveBeenCalledWith(
'shared-ext',
SettingScope.Workspace,
);
// Check that setValue was called to remove the deprecated setting
expect(setValueSpy).toHaveBeenCalledWith(
SettingScope.User,
'extensions',
{
disabled: undefined,
},
);
expect(setValueSpy).toHaveBeenCalledWith(
SettingScope.Workspace,
'extensions',
{
disabled: undefined,
},
);
});
it('should not do anything if there are no deprecated settings', () => {
const userSettingsContent = {
extensions: {
enabled: ['user-ext-1'],
},
};
const workspaceSettingsContent = {
someOtherSetting: 'value',
};
mockFsReadFileSync.mockImplementation((p: fs.PathOrFileDescriptor) => {
if (p === USER_SETTINGS_PATH)
return JSON.stringify(userSettingsContent);
if (p === MOCK_WORKSPACE_SETTINGS_PATH)
return JSON.stringify(workspaceSettingsContent);
return '{}';
});
const loadedSettings = loadSettings(MOCK_WORKSPACE_DIR);
const setValueSpy = vi.spyOn(loadedSettings, 'setValue');
const extensionManager = new ExtensionManager({
settings: loadedSettings.merged,
workspaceDir: MOCK_WORKSPACE_DIR,
requestConsent: vi.fn(),
requestSetting: vi.fn(),
});
const mockDisableExtension = vi.spyOn(
extensionManager,
'disableExtension',
);
mockDisableExtension.mockImplementation(async () => {});
migrateDeprecatedSettings(loadedSettings, extensionManager);
expect(mockDisableExtension).not.toHaveBeenCalled();
expect(setValueSpy).not.toHaveBeenCalled();
});
});
describe('saveSettings', () => {
it('should save settings using updateSettingsFilePreservingFormat', () => {
const mockUpdateSettings = vi.mocked(updateSettingsFilePreservingFormat);