Introduce system defaults (vs system overrides) (#6724)

This commit is contained in:
Billy Biggs
2025-08-24 21:21:22 -07:00
committed by GitHub
parent 1918f4466b
commit 04953d60c1
9 changed files with 241 additions and 26 deletions

View File

@@ -53,6 +53,7 @@ import {
loadSettings,
USER_SETTINGS_PATH, // This IS the mocked path.
getSystemSettingsPath,
getSystemDefaultsPath,
SETTINGS_DIRECTORY_NAME, // This is from the original module, but used by the mock.
SettingScope,
} from './settings.js';
@@ -317,6 +318,68 @@ describe('Settings Loading and Merging', () => {
});
});
it('should merge all settings files with the correct precedence', () => {
(mockFsExistsSync as Mock).mockReturnValue(true);
const systemDefaultsContent = {
theme: 'default-theme',
sandbox: true,
telemetry: true,
includeDirectories: ['/system/defaults/dir'],
};
const userSettingsContent = {
theme: 'user-theme',
contextFileName: 'USER_CONTEXT.md',
includeDirectories: ['/user/dir1', '/user/dir2'],
};
const workspaceSettingsContent = {
sandbox: false,
contextFileName: 'WORKSPACE_CONTEXT.md',
includeDirectories: ['/workspace/dir'],
};
const systemSettingsContent = {
theme: 'system-theme',
telemetry: false,
includeDirectories: ['/system/dir'],
};
(fs.readFileSync as Mock).mockImplementation(
(p: fs.PathOrFileDescriptor) => {
if (p === getSystemDefaultsPath())
return JSON.stringify(systemDefaultsContent);
if (p === getSystemSettingsPath())
return JSON.stringify(systemSettingsContent);
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.systemDefaults.settings).toEqual(systemDefaultsContent);
expect(settings.system.settings).toEqual(systemSettingsContent);
expect(settings.user.settings).toEqual(userSettingsContent);
expect(settings.workspace.settings).toEqual(workspaceSettingsContent);
expect(settings.merged).toEqual({
theme: 'system-theme',
sandbox: false,
telemetry: false,
contextFileName: 'WORKSPACE_CONTEXT.md',
customThemes: {},
mcpServers: {},
includeDirectories: [
'/system/defaults/dir',
'/user/dir1',
'/user/dir2',
'/workspace/dir',
'/system/dir',
],
chatCompression: {},
});
});
it('should ignore folderTrust from workspace settings', () => {
(mockFsExistsSync as Mock).mockReturnValue(true);
const userSettingsContent = {
@@ -806,6 +869,9 @@ describe('Settings Loading and Merging', () => {
const systemSettingsContent = {
includeDirectories: ['/system/dir'],
};
const systemDefaultsContent = {
includeDirectories: ['/system/defaults/dir'],
};
const userSettingsContent = {
includeDirectories: ['/user/dir1', '/user/dir2'],
};
@@ -817,6 +883,8 @@ describe('Settings Loading and Merging', () => {
(p: fs.PathOrFileDescriptor) => {
if (p === getSystemSettingsPath())
return JSON.stringify(systemSettingsContent);
if (p === getSystemDefaultsPath())
return JSON.stringify(systemDefaultsContent);
if (p === USER_SETTINGS_PATH)
return JSON.stringify(userSettingsContent);
if (p === MOCK_WORKSPACE_SETTINGS_PATH)
@@ -828,10 +896,11 @@ describe('Settings Loading and Merging', () => {
const settings = loadSettings(MOCK_WORKSPACE_DIR);
expect(settings.merged.includeDirectories).toEqual([
'/system/dir',
'/system/defaults/dir',
'/user/dir1',
'/user/dir2',
'/workspace/dir',
'/system/dir',
]);
});
@@ -1290,6 +1359,11 @@ describe('Settings Loading and Merging', () => {
expect(loadedSettings.system.settings.theme).toBe('ocean');
expect(loadedSettings.merged.theme).toBe('ocean');
// SystemDefaults theme is overridden by user, workspace, and system themes
loadedSettings.setValue(SettingScope.SystemDefaults, 'theme', 'default');
expect(loadedSettings.systemDefaults.settings.theme).toBe('default');
expect(loadedSettings.merged.theme).toBe('ocean');
});
});

View File

@@ -40,12 +40,23 @@ export function getSystemSettingsPath(): string {
}
}
export function getSystemDefaultsPath(): string {
if (process.env['GEMINI_CLI_SYSTEM_DEFAULTS_PATH']) {
return process.env['GEMINI_CLI_SYSTEM_DEFAULTS_PATH'];
}
return path.join(
path.dirname(getSystemSettingsPath()),
'system-defaults.json',
);
}
export type { DnsResolutionOrder } from './settingsSchema.js';
export enum SettingScope {
User = 'User',
Workspace = 'Workspace',
System = 'System',
SystemDefaults = 'SystemDefaults',
}
export interface CheckpointingSettings {
@@ -73,6 +84,7 @@ export interface SettingsFile {
function mergeSettings(
system: Settings,
systemDefaults: Settings,
user: Settings,
workspace: Settings,
isTrusted: boolean,
@@ -83,29 +95,43 @@ function mergeSettings(
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { folderTrust, ...safeWorkspaceWithoutFolderTrust } = safeWorkspace;
// Settings are merged with the following precedence (last one wins for
// single values):
// 1. System Defaults
// 2. User Settings
// 3. Workspace Settings
// 4. System Settings (as overrides)
//
// For properties that are arrays (e.g., includeDirectories), the arrays
// are concatenated. For objects (e.g., customThemes), they are merged.
return {
...systemDefaults,
...user,
...safeWorkspaceWithoutFolderTrust,
...system,
customThemes: {
...(systemDefaults.customThemes || {}),
...(user.customThemes || {}),
...(safeWorkspace.customThemes || {}),
...(system.customThemes || {}),
},
mcpServers: {
...(systemDefaults.mcpServers || {}),
...(user.mcpServers || {}),
...(safeWorkspace.mcpServers || {}),
...(system.mcpServers || {}),
},
includeDirectories: [
...(system.includeDirectories || []),
...(systemDefaults.includeDirectories || []),
...(user.includeDirectories || []),
...(safeWorkspace.includeDirectories || []),
...(system.includeDirectories || []),
],
chatCompression: {
...(system.chatCompression || {}),
...(systemDefaults.chatCompression || {}),
...(user.chatCompression || {}),
...(safeWorkspace.chatCompression || {}),
...(system.chatCompression || {}),
},
};
}
@@ -113,12 +139,14 @@ function mergeSettings(
export class LoadedSettings {
constructor(
system: SettingsFile,
systemDefaults: SettingsFile,
user: SettingsFile,
workspace: SettingsFile,
errors: SettingsError[],
isTrusted: boolean,
) {
this.system = system;
this.systemDefaults = systemDefaults;
this.user = user;
this.workspace = workspace;
this.errors = errors;
@@ -127,6 +155,7 @@ export class LoadedSettings {
}
readonly system: SettingsFile;
readonly systemDefaults: SettingsFile;
readonly user: SettingsFile;
readonly workspace: SettingsFile;
readonly errors: SettingsError[];
@@ -141,6 +170,7 @@ export class LoadedSettings {
private computeMergedSettings(): Settings {
return mergeSettings(
this.system.settings,
this.systemDefaults.settings,
this.user.settings,
this.workspace.settings,
this.isTrusted,
@@ -155,6 +185,8 @@ export class LoadedSettings {
return this.workspace;
case SettingScope.System:
return this.system;
case SettingScope.SystemDefaults:
return this.systemDefaults;
default:
throw new Error(`Invalid scope: ${scope}`);
}
@@ -331,10 +363,12 @@ export function loadEnvironment(settings?: Settings): void {
*/
export function loadSettings(workspaceDir: string): LoadedSettings {
let systemSettings: Settings = {};
let systemDefaultSettings: Settings = {};
let userSettings: Settings = {};
let workspaceSettings: Settings = {};
const settingsErrors: SettingsError[] = [];
const systemSettingsPath = getSystemSettingsPath();
const systemDefaultsPath = getSystemDefaultsPath();
// Resolve paths to their canonical representation to handle symlinks
const resolvedWorkspaceDir = path.resolve(workspaceDir);
@@ -368,6 +402,25 @@ export function loadSettings(workspaceDir: string): LoadedSettings {
});
}
// Load system defaults
try {
if (fs.existsSync(systemDefaultsPath)) {
const systemDefaultsContent = fs.readFileSync(
systemDefaultsPath,
'utf-8',
);
const parsedSystemDefaults = JSON.parse(
stripJsonComments(systemDefaultsContent),
) as Settings;
systemDefaultSettings = resolveEnvVarsInObject(parsedSystemDefaults);
}
} catch (error: unknown) {
settingsErrors.push({
message: getErrorMessage(error),
path: systemDefaultsPath,
});
}
// Load user settings
try {
if (fs.existsSync(USER_SETTINGS_PATH)) {
@@ -419,6 +472,7 @@ export function loadSettings(workspaceDir: string): LoadedSettings {
// Create a temporary merged settings object to pass to loadEnvironment.
const tempMergedSettings = mergeSettings(
systemSettings,
systemDefaultSettings,
userSettings,
workspaceSettings,
isTrusted,
@@ -439,6 +493,10 @@ export function loadSettings(workspaceDir: string): LoadedSettings {
path: systemSettingsPath,
settings: systemSettings,
},
{
path: systemDefaultsPath,
settings: systemDefaultSettings,
},
{
path: USER_SETTINGS_PATH,
settings: userSettings,

View File

@@ -146,8 +146,13 @@ describe('gemini.tsx main function', () => {
path: '/system/settings.json',
settings: {},
};
const systemDefaultsFile: SettingsFile = {
path: '/system/system-defaults.json',
settings: {},
};
const mockLoadedSettings = new LoadedSettings(
systemSettingsFile,
systemDefaultsFile,
userSettingsFile,
workspaceSettingsFile,
[settingsError],

View File

@@ -287,6 +287,10 @@ describe('App UI', () => {
path: '/system/settings.json',
settings: settings.system || {},
};
const systemDefaultsFile: SettingsFile = {
path: '/system/system-defaults.json',
settings: {},
};
const userSettingsFile: SettingsFile = {
path: '/user/settings.json',
settings: settings.user || {},
@@ -297,6 +301,7 @@ describe('App UI', () => {
};
return new LoadedSettings(
systemSettingsFile,
systemDefaultsFile,
userSettingsFile,
workspaceSettingsFile,
[],

View File

@@ -34,6 +34,10 @@ describe('AuthDialog', () => {
settings: { customThemes: {}, mcpServers: {} },
path: '',
},
{
settings: {},
path: '',
},
{
settings: {
selectedAuthType: AuthType.USE_GEMINI,
@@ -74,6 +78,10 @@ describe('AuthDialog', () => {
},
path: '',
},
{
settings: {},
path: '',
},
{
settings: { customThemes: {}, mcpServers: {} },
path: '',
@@ -108,6 +116,10 @@ describe('AuthDialog', () => {
},
path: '',
},
{
settings: {},
path: '',
},
{
settings: { customThemes: {}, mcpServers: {} },
path: '',
@@ -142,6 +154,10 @@ describe('AuthDialog', () => {
},
path: '',
},
{
settings: {},
path: '',
},
{
settings: { customThemes: {}, mcpServers: {} },
path: '',
@@ -177,6 +193,10 @@ describe('AuthDialog', () => {
},
path: '',
},
{
settings: {},
path: '',
},
{
settings: { customThemes: {}, mcpServers: {} },
path: '',
@@ -207,6 +227,10 @@ describe('AuthDialog', () => {
},
path: '',
},
{
settings: {},
path: '',
},
{
settings: { customThemes: {}, mcpServers: {} },
path: '',
@@ -239,6 +263,10 @@ describe('AuthDialog', () => {
},
path: '',
},
{
settings: {},
path: '',
},
{
settings: { customThemes: {}, mcpServers: {} },
path: '',
@@ -271,6 +299,10 @@ describe('AuthDialog', () => {
settings: { customThemes: {}, mcpServers: {} },
path: '',
},
{
settings: {},
path: '',
},
{
settings: {
selectedAuthType: undefined,
@@ -311,6 +343,10 @@ describe('AuthDialog', () => {
settings: { customThemes: {}, mcpServers: {} },
path: '',
},
{
settings: {},
path: '',
},
{
settings: {
selectedAuthType: undefined,
@@ -354,6 +390,10 @@ describe('AuthDialog', () => {
settings: { customThemes: {}, mcpServers: {} },
path: '',
},
{
settings: {},
path: '',
},
{
settings: {
selectedAuthType: AuthType.USE_GEMINI,

View File

@@ -43,6 +43,10 @@ const createMockSettings = (
settings: { customThemes: {}, mcpServers: {}, ...systemSettings },
path: '/system/settings.json',
},
{
settings: {},
path: '/system/system-defaults.json',
},
{
settings: {
customThemes: {},
@@ -155,6 +159,10 @@ describe('SettingsDialog', () => {
settings: { customThemes: {}, mcpServers: {}, ...systemSettings },
path: '/system/settings.json',
},
{
settings: {},
path: '/system/system-defaults.json',
},
{
settings: {
customThemes: {},

View File

@@ -22,6 +22,7 @@ describe('<MarkdownDisplay />', () => {
{ path: '', settings: {} },
{ path: '', settings: {} },
{ path: '', settings: {} },
{ path: '', settings: {} },
[],
true,
);
@@ -221,6 +222,7 @@ 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: { showLineNumbers: false } },
{ path: '', settings: {} },