Make default settings apply (#15354)

This commit is contained in:
Dev Randalpura
2026-01-05 15:00:20 -05:00
committed by GitHub
parent ad651fdab0
commit da7162deba
7 changed files with 135 additions and 53 deletions
+6 -7
View File
@@ -666,7 +666,7 @@ export async function loadCliConfig(
screenReader, screenReader,
}, },
telemetry: telemetrySettings, telemetry: telemetrySettings,
usageStatisticsEnabled: settings.privacy?.usageStatisticsEnabled ?? true, usageStatisticsEnabled: settings.privacy?.usageStatisticsEnabled,
fileFiltering, fileFiltering,
checkpointing: settings.general?.checkpointing?.enabled, checkpointing: settings.general?.checkpointing?.enabled,
proxy: proxy:
@@ -678,7 +678,7 @@ export async function loadCliConfig(
fileDiscoveryService: fileService, fileDiscoveryService: fileService,
bugCommand: settings.advanced?.bugCommand, bugCommand: settings.advanced?.bugCommand,
model: resolvedModel, model: resolvedModel,
maxSessionTurns: settings.model?.maxSessionTurns ?? -1, maxSessionTurns: settings.model?.maxSessionTurns,
experimentalZedIntegration: argv.experimentalAcp || false, experimentalZedIntegration: argv.experimentalAcp || false,
listExtensions: argv.listExtensions || false, listExtensions: argv.listExtensions || false,
listSessions: argv.listSessions || false, listSessions: argv.listSessions || false,
@@ -698,13 +698,12 @@ export async function loadCliConfig(
interactive, interactive,
trustedFolder, trustedFolder,
useRipgrep: settings.tools?.useRipgrep, useRipgrep: settings.tools?.useRipgrep,
enableInteractiveShell: enableInteractiveShell: settings.tools?.shell?.enableInteractiveShell,
settings.tools?.shell?.enableInteractiveShell ?? true,
shellToolInactivityTimeout: settings.tools?.shell?.inactivityTimeout, shellToolInactivityTimeout: settings.tools?.shell?.inactivityTimeout,
enableShellOutputEfficiency: enableShellOutputEfficiency:
settings.tools?.shell?.enableShellOutputEfficiency ?? true, settings.tools?.shell?.enableShellOutputEfficiency ?? true,
skipNextSpeakerCheck: settings.model?.skipNextSpeakerCheck, skipNextSpeakerCheck: settings.model?.skipNextSpeakerCheck,
enablePromptCompletion: settings.general?.enablePromptCompletion ?? false, enablePromptCompletion: settings.general?.enablePromptCompletion,
truncateToolOutputThreshold: settings.tools?.truncateToolOutputThreshold, truncateToolOutputThreshold: settings.tools?.truncateToolOutputThreshold,
truncateToolOutputLines: settings.tools?.truncateToolOutputLines, truncateToolOutputLines: settings.tools?.truncateToolOutputLines,
enableToolOutputTruncation: settings.tools?.enableToolOutputTruncation, enableToolOutputTruncation: settings.tools?.enableToolOutputTruncation,
@@ -719,11 +718,11 @@ export async function loadCliConfig(
settings.experimental?.introspectionAgentSettings, settings.experimental?.introspectionAgentSettings,
fakeResponses: argv.fakeResponses, fakeResponses: argv.fakeResponses,
recordResponses: argv.recordResponses, recordResponses: argv.recordResponses,
retryFetchErrors: settings.general?.retryFetchErrors ?? false, retryFetchErrors: settings.general?.retryFetchErrors,
ptyInfo: ptyInfo?.name, ptyInfo: ptyInfo?.name,
modelConfigServiceConfig: settings.modelConfigs, modelConfigServiceConfig: settings.modelConfigs,
// TODO: loading of hooks based on workspace trust // TODO: loading of hooks based on workspace trust
enableHooks: settings.tools?.enableHooks ?? false, enableHooks: settings.tools?.enableHooks,
hooks: settings.hooks || {}, hooks: settings.hooks || {},
projectHooks: projectHooks || {}, projectHooks: projectHooks || {},
onModelChange: (model: string) => saveModelChange(loadedSettings, model), onModelChange: (model: string) => saveModelChange(loadedSettings, model),
+96 -28
View File
@@ -35,6 +35,14 @@ vi.mock('./trustedFolders.js', () => ({
.mockReturnValue({ isTrusted: true, source: 'file' }), .mockReturnValue({ isTrusted: true, source: 'file' }),
})); }));
vi.mock('./settingsSchema.js', async (importOriginal) => {
const actual = await importOriginal<typeof import('./settingsSchema.js')>();
return {
...actual,
getSettingsSchema: vi.fn(actual.getSettingsSchema),
};
});
// NOW import everything else, including the (now effectively re-exported) settings.js // 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 path, * as pathActual from 'node:path'; // Restored for MOCK_WORKSPACE_SETTINGS_PATH
import { import {
@@ -65,10 +73,16 @@ import {
SettingScope, SettingScope,
saveSettings, saveSettings,
type SettingsFile, type SettingsFile,
getDefaultsFromSchema,
} from './settings.js'; } from './settings.js';
import { FatalConfigError, GEMINI_DIR } from '@google/gemini-cli-core'; import { FatalConfigError, GEMINI_DIR } from '@google/gemini-cli-core';
import { ExtensionManager } from './extension-manager.js'; import { ExtensionManager } from './extension-manager.js';
import { updateSettingsFilePreservingFormat } from '../utils/commentJson.js'; import { updateSettingsFilePreservingFormat } from '../utils/commentJson.js';
import {
getSettingsSchema,
MergeStrategy,
type SettingsSchema,
} from './settingsSchema.js';
const MOCK_WORKSPACE_DIR = '/mock/workspace'; const MOCK_WORKSPACE_DIR = '/mock/workspace';
// Use the (mocked) GEMINI_DIR for consistency // Use the (mocked) GEMINI_DIR for consistency
@@ -149,14 +163,6 @@ describe('Settings Loading and Merging', () => {
}); });
describe('loadSettings', () => { describe('loadSettings', () => {
it('should load empty settings if no files exist', () => {
const settings = loadSettings(MOCK_WORKSPACE_DIR);
expect(settings.system.settings).toEqual({});
expect(settings.user.settings).toEqual({});
expect(settings.workspace.settings).toEqual({});
expect(settings.merged).toEqual({});
});
it.each([ it.each([
{ {
scope: 'system', scope: 'system',
@@ -201,7 +207,7 @@ describe('Settings Loading and Merging', () => {
expect( expect(
settings[scope as 'system' | 'user' | 'workspace'].settings, settings[scope as 'system' | 'user' | 'workspace'].settings,
).toEqual(content); ).toEqual(content);
expect(settings.merged).toEqual(content); expect(settings.merged).toMatchObject(content);
}, },
); );
@@ -265,7 +271,7 @@ describe('Settings Loading and Merging', () => {
expect(settings.system.settings).toEqual(systemSettingsContent); expect(settings.system.settings).toEqual(systemSettingsContent);
expect(settings.user.settings).toEqual(userSettingsContent); expect(settings.user.settings).toEqual(userSettingsContent);
expect(settings.workspace.settings).toEqual(workspaceSettingsContent); expect(settings.workspace.settings).toEqual(workspaceSettingsContent);
expect(settings.merged).toEqual({ expect(settings.merged).toMatchObject({
ui: { ui: {
theme: 'system-theme', theme: 'system-theme',
}, },
@@ -318,7 +324,7 @@ describe('Settings Loading and Merging', () => {
const settings = loadSettings(MOCK_WORKSPACE_DIR); const settings = loadSettings(MOCK_WORKSPACE_DIR);
expect(settings.merged).toEqual({ expect(settings.merged).toMatchObject({
ui: { ui: {
theme: 'legacy-dark', theme: 'legacy-dark',
}, },
@@ -443,10 +449,33 @@ describe('Settings Loading and Merging', () => {
expect(settings.merged.advanced?.excludedEnvVars).toEqual( expect(settings.merged.advanced?.excludedEnvVars).toEqual(
expect.arrayContaining(['USER_VAR', 'WORKSPACE_VAR']), expect.arrayContaining(['USER_VAR', 'WORKSPACE_VAR']),
); );
expect(settings.merged.advanced?.excludedEnvVars).toHaveLength(2); expect(settings.merged.advanced?.excludedEnvVars).toHaveLength(4);
}); });
it('should merge all settings files with the correct precedence', () => { it('should merge all settings files with the correct precedence', () => {
// Mock schema to test defaults application
const mockSchema = {
ui: { type: 'object', default: {}, properties: {} },
tools: { type: 'object', default: {}, properties: {} },
context: {
type: 'object',
default: {},
properties: {
discoveryMaxDirs: { type: 'number', default: 200 },
includeDirectories: {
type: 'array',
default: [],
mergeStrategy: MergeStrategy.CONCAT,
},
},
},
mcpServers: { type: 'object', default: {} },
};
(getSettingsSchema as Mock).mockReturnValue(
mockSchema as unknown as SettingsSchema,
);
(mockFsExistsSync as Mock).mockReturnValue(true); (mockFsExistsSync as Mock).mockReturnValue(true);
const systemDefaultsContent = { const systemDefaultsContent = {
ui: { ui: {
@@ -510,7 +539,7 @@ describe('Settings Loading and Merging', () => {
expect(settings.workspace.settings).toEqual(workspaceSettingsContent); expect(settings.workspace.settings).toEqual(workspaceSettingsContent);
expect(settings.merged).toEqual({ expect(settings.merged).toEqual({
context: { context: {
fileName: 'WORKSPACE_CONTEXT.md', discoveryMaxDirs: 200,
includeDirectories: [ includeDirectories: [
'/system/defaults/dir', '/system/defaults/dir',
'/user/dir1', '/user/dir1',
@@ -518,14 +547,12 @@ describe('Settings Loading and Merging', () => {
'/workspace/dir', '/workspace/dir',
'/system/dir', '/system/dir',
], ],
fileName: 'WORKSPACE_CONTEXT.md',
}, },
mcpServers: {},
ui: { theme: 'system-theme' },
tools: { sandbox: false },
telemetry: false, telemetry: false,
tools: {
sandbox: false,
},
ui: {
theme: 'system-theme',
},
}); });
}); });
@@ -660,7 +687,7 @@ describe('Settings Loading and Merging', () => {
}, },
expected: { expected: {
key: 'advanced.excludedEnvVars', key: 'advanced.excludedEnvVars',
value: ['DEBUG', 'NODE_ENV', 'CUSTOM_VAR'], value: ['DEBUG', 'DEBUG_MODE', 'NODE_ENV', 'CUSTOM_VAR'],
}, },
}, },
{ {
@@ -671,7 +698,7 @@ describe('Settings Loading and Merging', () => {
}, },
expected: { expected: {
key: 'advanced.excludedEnvVars', key: 'advanced.excludedEnvVars',
value: ['WORKSPACE_DEBUG', 'WORKSPACE_VAR'], value: ['DEBUG', 'DEBUG_MODE', 'WORKSPACE_DEBUG', 'WORKSPACE_VAR'],
}, },
}, },
])( ])(
@@ -734,6 +761,7 @@ describe('Settings Loading and Merging', () => {
]); ]);
expect(settings.merged.advanced?.excludedEnvVars).toEqual([ expect(settings.merged.advanced?.excludedEnvVars).toEqual([
'DEBUG', 'DEBUG',
'DEBUG_MODE',
'NODE_ENV', 'NODE_ENV',
'USER_VAR', 'USER_VAR',
'WORKSPACE_DEBUG', 'WORKSPACE_DEBUG',
@@ -814,8 +842,8 @@ describe('Settings Loading and Merging', () => {
(fs.readFileSync as Mock).mockReturnValue('{}'); (fs.readFileSync as Mock).mockReturnValue('{}');
const settings = loadSettings(MOCK_WORKSPACE_DIR); const settings = loadSettings(MOCK_WORKSPACE_DIR);
expect(settings.merged.telemetry).toBeUndefined(); expect(settings.merged.telemetry).toBeUndefined();
expect(settings.merged.ui).toBeUndefined(); expect(settings.merged.ui).toBeDefined();
expect(settings.merged.mcpServers).toBeUndefined(); expect(settings.merged.mcpServers).toEqual({});
}); });
it('should merge MCP servers correctly, with workspace taking precedence', () => { it('should merge MCP servers correctly, with workspace taking precedence', () => {
@@ -941,7 +969,7 @@ describe('Settings Loading and Merging', () => {
(mockFsExistsSync as Mock).mockReturnValue(false); // No settings files exist (mockFsExistsSync as Mock).mockReturnValue(false); // No settings files exist
(fs.readFileSync as Mock).mockReturnValue('{}'); (fs.readFileSync as Mock).mockReturnValue('{}');
const settings = loadSettings(MOCK_WORKSPACE_DIR); const settings = loadSettings(MOCK_WORKSPACE_DIR);
expect(settings.merged.mcpServers).toBeUndefined(); expect(settings.merged.mcpServers).toEqual({});
}); });
it('should merge MCP servers from system, user, and workspace with system taking precedence', () => { it('should merge MCP servers from system, user, and workspace with system taking precedence', () => {
@@ -1075,10 +1103,10 @@ describe('Settings Loading and Merging', () => {
expected: 0.8, expected: 0.8,
}, },
{ {
description: 'should be undefined if not in any settings file', description: 'should be default if not in any settings file',
userContent: {}, userContent: {},
workspaceContent: {}, workspaceContent: {},
expected: undefined, expected: 0.5,
}, },
])('$description', ({ userContent, workspaceContent, expected }) => { ])('$description', ({ userContent, workspaceContent, expected }) => {
(mockFsExistsSync as Mock).mockReturnValue(true); (mockFsExistsSync as Mock).mockReturnValue(true);
@@ -1590,7 +1618,7 @@ describe('Settings Loading and Merging', () => {
); );
expect(settings.system.path).toBe(MOCK_ENV_SYSTEM_SETTINGS_PATH); expect(settings.system.path).toBe(MOCK_ENV_SYSTEM_SETTINGS_PATH);
expect(settings.system.settings).toEqual(systemSettingsContent); expect(settings.system.settings).toEqual(systemSettingsContent);
expect(settings.merged).toEqual({ expect(settings.merged).toMatchObject({
...systemSettingsContent, ...systemSettingsContent,
}); });
}); });
@@ -1692,8 +1720,9 @@ describe('Settings Loading and Merging', () => {
'DEBUG', 'DEBUG',
]); ]);
expect(settings.merged.advanced?.excludedEnvVars).toEqual([ expect(settings.merged.advanced?.excludedEnvVars).toEqual([
'NODE_ENV',
'DEBUG', 'DEBUG',
'DEBUG_MODE',
'NODE_ENV',
]); ]);
}); });
@@ -1732,6 +1761,7 @@ describe('Settings Loading and Merging', () => {
]); ]);
expect(settings.merged.advanced?.excludedEnvVars).toEqual([ expect(settings.merged.advanced?.excludedEnvVars).toEqual([
'DEBUG', 'DEBUG',
'DEBUG_MODE',
'NODE_ENV', 'NODE_ENV',
'USER_VAR', 'USER_VAR',
'WORKSPACE_DEBUG', 'WORKSPACE_DEBUG',
@@ -2444,4 +2474,42 @@ describe('Settings Loading and Merging', () => {
); );
}); });
}); });
describe('getDefaultsFromSchema', () => {
it('should extract defaults from a schema', () => {
const mockSchema = {
prop1: {
type: 'string',
default: 'default1',
label: 'Prop 1',
category: 'General',
requiresRestart: false,
},
nested: {
type: 'object',
label: 'Nested',
category: 'General',
requiresRestart: false,
default: {},
properties: {
prop2: {
type: 'number',
default: 42,
label: 'Prop 2',
category: 'General',
requiresRestart: false,
},
},
},
};
const defaults = getDefaultsFromSchema(mockSchema as SettingsSchema);
expect(defaults).toEqual({
prop1: 'default1',
nested: {
prop2: 42,
},
});
});
});
}); });
+25 -5
View File
@@ -424,6 +424,24 @@ export function migrateSettingsToV1(
return v1Settings; return v1Settings;
} }
export function getDefaultsFromSchema(
schema: SettingsSchema = getSettingsSchema(),
): Settings {
const defaults: Record<string, unknown> = {};
for (const key in schema) {
const definition = schema[key];
if (definition.properties) {
const childDefaults = getDefaultsFromSchema(definition.properties);
if (Object.keys(childDefaults).length > 0) {
defaults[key] = childDefaults;
}
} else if (definition.default !== undefined) {
defaults[key] = definition.default;
}
}
return defaults as Settings;
}
function mergeSettings( function mergeSettings(
system: Settings, system: Settings,
systemDefaults: Settings, systemDefaults: Settings,
@@ -432,16 +450,18 @@ function mergeSettings(
isTrusted: boolean, isTrusted: boolean,
): Settings { ): Settings {
const safeWorkspace = isTrusted ? workspace : ({} as Settings); const safeWorkspace = isTrusted ? workspace : ({} as Settings);
const schemaDefaults = getDefaultsFromSchema();
// Settings are merged with the following precedence (last one wins for // Settings are merged with the following precedence (last one wins for
// single values): // single values):
// 1. System Defaults // 1. Schema Defaults (Built-in)
// 2. User Settings // 2. System Defaults
// 3. Workspace Settings // 3. User Settings
// 4. System Settings (as overrides) // 4. Workspace Settings
// 5. System Settings (as overrides)
return customDeepMerge( return customDeepMerge(
getMergeStrategyForPath, getMergeStrategyForPath,
{}, // Start with an empty object schemaDefaults,
systemDefaults, systemDefaults,
user, user,
safeWorkspace, safeWorkspace,
+4 -5
View File
@@ -60,12 +60,11 @@ export const Footer: React.FC = () => {
const showMemoryUsage = const showMemoryUsage =
config.getDebugMode() || settings.merged.ui?.showMemoryUsage || false; config.getDebugMode() || settings.merged.ui?.showMemoryUsage || false;
const hideCWD = settings.merged.ui?.footer?.hideCWD || false; const hideCWD = settings.merged.ui?.footer?.hideCWD;
const hideSandboxStatus = const hideSandboxStatus = settings.merged.ui?.footer?.hideSandboxStatus;
settings.merged.ui?.footer?.hideSandboxStatus || false; const hideModelInfo = settings.merged.ui?.footer?.hideModelInfo;
const hideModelInfo = settings.merged.ui?.footer?.hideModelInfo || false;
const hideContextPercentage = const hideContextPercentage =
settings.merged.ui?.footer?.hideContextPercentage ?? true; settings.merged.ui?.footer?.hideContextPercentage;
const pathLength = Math.max(20, Math.floor(mainAreaWidth * 0.25)); const pathLength = Math.max(20, Math.floor(mainAreaWidth * 0.25));
const displayPath = shortenPath(tildeifyPath(targetDir), pathLength); const displayPath = shortenPath(tildeifyPath(targetDir), pathLength);
+2 -2
View File
@@ -35,8 +35,8 @@ describe('ui-sizing', () => {
[80, true, true, 79], // -1 for alternate buffer [80, true, true, 79], // -1 for alternate buffer
[100, true, true, 99], [100, true, true, 99],
// Default behavior (useFullWidth undefined or true) // Default behavior (useFullWidth true)
[100, undefined, false, 100], [100, true, false, 100],
// useFullWidth: false (Smart sizing) // useFullWidth: false (Smart sizing)
[80, false, false, 78], // 98% of 80 [80, false, false, 78], // 98% of 80
+1 -1
View File
@@ -27,7 +27,7 @@ export const calculateMainAreaWidth = (
terminalWidth: number, terminalWidth: number,
settings: LoadedSettings, settings: LoadedSettings,
): number => { ): number => {
if (settings.merged.ui?.useFullWidth !== false) { if (settings.merged.ui?.useFullWidth) {
if (isAlternateBufferEnabled(settings)) { if (isAlternateBufferEnabled(settings)) {
return terminalWidth - 1; return terminalWidth - 1;
} }
+1 -5
View File
@@ -279,11 +279,7 @@ export function getSettingValue(
if (typeof value === 'boolean') { if (typeof value === 'boolean') {
return value; return value;
} }
// Fall back to default value, ensuring it's a boolean
const defaultValue = definition.default;
if (typeof defaultValue === 'boolean') {
return defaultValue;
}
return false; // Final fallback return false; // Final fallback
} }