MediaMetz/gemini-cli
1
0
Fork 0
mirror of https://github.com/google-gemini/gemini-cli.git synced 2026-04-27 13:34:15 -07:00
Code Issues Packages Projects Releases Wiki Activity

refactor(setting): Improve settings migration and tool loading (#7445)

Browse Source 
Co-authored-by: psinha40898 <pyushsinha20@gmail.com>
This commit is contained in:
Gal Zahavi
2025-09-03 19:23:25 -07:00
committed by GitHub
parent e7a4142b2a
commit 3885f7b6ae
7 changed files with 552 additions and 311 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/cli/src/config/settings.test.ts
+94 -158
View File
@@ -44,6 +44,7 @@ import {
afterEach,
type Mocked,
type Mock,
fail,
} from 'vitest';
import * as fs from 'node:fs'; // fs will be mocked separately
import stripJsonComments from 'strip-json-comments'; // Will be mocked separately
@@ -57,6 +58,7 @@ import {
getSystemDefaultsPath,
SETTINGS_DIRECTORY_NAME, // This is from the original module, but used by the mock.
migrateSettingsToV1,
needsMigration,
type Settings,
loadEnvironment,
} from './settings.js';
@@ -124,28 +126,7 @@ describe('Settings Loading and Merging', () => {
expect(settings.system.settings).toEqual({});
expect(settings.user.settings).toEqual({});
expect(settings.workspace.settings).toEqual({});
expect(settings.merged).toEqual({
general: {},
ui: {
customThemes: {},
},
mcp: {},
mcpServers: {},
context: {
includeDirectories: [],
},
model: {
chatCompression: {},
},
advanced: {
excludedEnvVars: [],
},
extensions: {
disabled: [],
workspacesWithMigrationNudge: [],
},
security: {},
});
expect(settings.merged).toEqual({});
});
it('should load system settings if only system file exists', () => {
@@ -179,27 +160,6 @@ describe('Settings Loading and Merging', () => {
expect(settings.workspace.settings).toEqual({});
expect(settings.merged).toEqual({
...systemSettingsContent,
general: {},
ui: {
...systemSettingsContent.ui,
customThemes: {},
},
mcp: {},
mcpServers: {},
context: {
includeDirectories: [],
},
model: {
chatCompression: {},
},
advanced: {
excludedEnvVars: [],
},
extensions: {
disabled: [],
workspacesWithMigrationNudge: [],
},
security: {},
});
});
@@ -235,28 +195,6 @@ describe('Settings Loading and Merging', () => {
expect(settings.workspace.settings).toEqual({});
expect(settings.merged).toEqual({
...userSettingsContent,
general: {},
ui: {
...userSettingsContent.ui,
customThemes: {},
},
mcp: {},
mcpServers: {},
context: {
...userSettingsContent.context,
includeDirectories: [],
},
model: {
chatCompression: {},
},
advanced: {
excludedEnvVars: [],
},
extensions: {
disabled: [],
workspacesWithMigrationNudge: [],
},
security: {},
});
});
@@ -289,30 +227,7 @@ describe('Settings Loading and Merging', () => {
expect(settings.user.settings).toEqual({});
expect(settings.workspace.settings).toEqual(workspaceSettingsContent);
expect(settings.merged).toEqual({
tools: {
sandbox: true,
},
context: {
fileName: 'WORKSPACE_CONTEXT.md',
includeDirectories: [],
},
general: {},
ui: {
customThemes: {},
},
mcp: {},
mcpServers: {},
model: {
chatCompression: {},
},
advanced: {
excludedEnvVars: [],
},
extensions: {
disabled: [],
workspacesWithMigrationNudge: [],
},
security: {},
...workspaceSettingsContent,
});
});
@@ -377,34 +292,20 @@ describe('Settings Loading and Merging', () => {
expect(settings.user.settings).toEqual(userSettingsContent);
expect(settings.workspace.settings).toEqual(workspaceSettingsContent);
expect(settings.merged).toEqual({
general: {},
ui: {
theme: 'system-theme',
customThemes: {},
},
tools: {
sandbox: false,
core: ['tool1'],
},
telemetry: { enabled: false },
context: {
fileName: 'WORKSPACE_CONTEXT.md',
includeDirectories: [],
},
mcp: {
allowed: ['server1', 'server2'],
},
advanced: {
excludedEnvVars: [],
},
extensions: {
disabled: [],
workspacesWithMigrationNudge: [],
},
mcpServers: {},
model: {
chatCompression: {},
},
security: {},
});
});
@@ -446,18 +347,15 @@ describe('Settings Loading and Merging', () => {
expect(settings.merged).toEqual({
ui: {
theme: 'legacy-dark',
customThemes: {},
},
general: {
vimMode: true,
},
context: {
fileName: 'LEGACY_CONTEXT.md',
includeDirectories: [],
},
model: {
name: 'gemini-pro',
chatCompression: {},
},
mcpServers: {
'legacy-server-1': {
@@ -475,14 +373,6 @@ describe('Settings Loading and Merging', () => {
allowed: ['legacy-server-1'],
},
someUnrecognizedSetting: 'should-be-preserved',
advanced: {
excludedEnvVars: [],
},
extensions: {
disabled: [],
workspacesWithMigrationNudge: [],
},
security: {},
});
});
@@ -611,9 +501,6 @@ describe('Settings Loading and Merging', () => {
expect(settings.user.settings).toEqual(userSettingsContent);
expect(settings.workspace.settings).toEqual(workspaceSettingsContent);
expect(settings.merged).toEqual({
advanced: {
excludedEnvVars: [],
},
context: {
fileName: 'WORKSPACE_CONTEXT.md',
includeDirectories: [
@@ -624,23 +511,11 @@ describe('Settings Loading and Merging', () => {
'/system/dir',
],
},
extensions: {
disabled: [],
workspacesWithMigrationNudge: [],
},
mcp: {},
mcpServers: {},
model: {
chatCompression: {},
},
security: {},
telemetry: false,
tools: {
sandbox: false,
},
general: {},
ui: {
customThemes: {},
theme: 'system-theme',
},
});
@@ -924,8 +799,8 @@ describe('Settings Loading and Merging', () => {
(fs.readFileSync as Mock).mockReturnValue('{}');
const settings = loadSettings(MOCK_WORKSPACE_DIR);
expect(settings.merged.telemetry).toBeUndefined();
expect(settings.merged.ui?.customThemes).toEqual({});
expect(settings.merged.mcpServers).toEqual({});
expect(settings.merged.ui).toBeUndefined();
expect(settings.merged.mcpServers).toBeUndefined();
});
it('should merge MCP servers correctly, with workspace taking precedence', () => {
@@ -1050,11 +925,11 @@ describe('Settings Loading and Merging', () => {
});
});
it('should have mcpServers as empty object if not in any settings file', () => {
it('should have mcpServers as undefined if not in any settings file', () => {
(mockFsExistsSync as Mock).mockReturnValue(false); // No settings files exist
(fs.readFileSync as Mock).mockReturnValue('{}');
const settings = loadSettings(MOCK_WORKSPACE_DIR);
expect(settings.merged.mcpServers).toEqual({});
expect(settings.merged.mcpServers).toBeUndefined();
});
it('should merge MCP servers from system, user, and workspace with system taking precedence', () => {
@@ -1222,11 +1097,11 @@ describe('Settings Loading and Merging', () => {
});
});
it('should have chatCompression as an empty object if not in any settings file', () => {
it('should have model as undefined if not in any settings file', () => {
(mockFsExistsSync as Mock).mockReturnValue(false); // No settings files exist
(fs.readFileSync as Mock).mockReturnValue('{}');
const settings = loadSettings(MOCK_WORKSPACE_DIR);
expect(settings.merged.model?.chatCompression).toEqual({});
expect(settings.merged.model).toBeUndefined();
});
it('should ignore chatCompression if contextPercentageThreshold is invalid', () => {
@@ -1351,7 +1226,22 @@ describe('Settings Loading and Merging', () => {
},
);
expect(() => loadSettings(MOCK_WORKSPACE_DIR)).toThrow(FatalConfigError);
try {
loadSettings(MOCK_WORKSPACE_DIR);
fail('loadSettings should have thrown a FatalConfigError');
} catch (e) {
expect(e).toBeInstanceOf(FatalConfigError);
const error = e as FatalConfigError;
expect(error.message).toContain(
`Error in ${USER_SETTINGS_PATH}: ${userReadError.message}`,
);
expect(error.message).toContain(
`Error in ${MOCK_WORKSPACE_SETTINGS_PATH}: ${workspaceReadError.message}`,
);
expect(error.message).toContain(
'Please fix the configuration file(s) and try again.',
);
}
// Restore JSON.parse mock if it was spied on specifically for this test
vi.restoreAllMocks(); // Or more targeted restore if needed
@@ -1742,27 +1632,6 @@ describe('Settings Loading and Merging', () => {
expect(settings.system.settings).toEqual(systemSettingsContent);
expect(settings.merged).toEqual({
...systemSettingsContent,
general: {},
ui: {
...systemSettingsContent.ui,
customThemes: {},
},
mcp: {},
mcpServers: {},
context: {
includeDirectories: [],
},
model: {
chatCompression: {},
},
advanced: {
excludedEnvVars: [],
},
extensions: {
disabled: [],
workspacesWithMigrationNudge: [],
},
security: {},
});
});
});
@@ -2348,4 +2217,71 @@ describe('Settings Loading and Merging', () => {
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 = {
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);
});
});
});