From 3c92666ec298b6ddc47268cb9dce5cc12d598dfa Mon Sep 17 00:00:00 2001 From: Dev Randalpura Date: Mon, 5 Jan 2026 15:00:20 -0500 Subject: [PATCH] Make default settings apply (#15354) --- packages/cli/src/config/config.ts | 13 +- packages/cli/src/config/settings.test.ts | 124 +++++++++++++++----- packages/cli/src/config/settings.ts | 30 ++++- packages/cli/src/ui/components/Footer.tsx | 9 +- packages/cli/src/ui/utils/ui-sizing.test.ts | 4 +- packages/cli/src/ui/utils/ui-sizing.ts | 2 +- packages/cli/src/utils/settingsUtils.ts | 6 +- 7 files changed, 135 insertions(+), 53 deletions(-) diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 858a929aff..e08a50893f 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -666,7 +666,7 @@ export async function loadCliConfig( screenReader, }, telemetry: telemetrySettings, - usageStatisticsEnabled: settings.privacy?.usageStatisticsEnabled ?? true, + usageStatisticsEnabled: settings.privacy?.usageStatisticsEnabled, fileFiltering, checkpointing: settings.general?.checkpointing?.enabled, proxy: @@ -678,7 +678,7 @@ export async function loadCliConfig( fileDiscoveryService: fileService, bugCommand: settings.advanced?.bugCommand, model: resolvedModel, - maxSessionTurns: settings.model?.maxSessionTurns ?? -1, + maxSessionTurns: settings.model?.maxSessionTurns, experimentalZedIntegration: argv.experimentalAcp || false, listExtensions: argv.listExtensions || false, listSessions: argv.listSessions || false, @@ -698,13 +698,12 @@ export async function loadCliConfig( interactive, trustedFolder, useRipgrep: settings.tools?.useRipgrep, - enableInteractiveShell: - settings.tools?.shell?.enableInteractiveShell ?? true, + enableInteractiveShell: settings.tools?.shell?.enableInteractiveShell, shellToolInactivityTimeout: settings.tools?.shell?.inactivityTimeout, enableShellOutputEfficiency: settings.tools?.shell?.enableShellOutputEfficiency ?? true, skipNextSpeakerCheck: settings.model?.skipNextSpeakerCheck, - enablePromptCompletion: settings.general?.enablePromptCompletion ?? false, + enablePromptCompletion: settings.general?.enablePromptCompletion, truncateToolOutputThreshold: settings.tools?.truncateToolOutputThreshold, truncateToolOutputLines: settings.tools?.truncateToolOutputLines, enableToolOutputTruncation: settings.tools?.enableToolOutputTruncation, @@ -719,11 +718,11 @@ export async function loadCliConfig( settings.experimental?.introspectionAgentSettings, fakeResponses: argv.fakeResponses, recordResponses: argv.recordResponses, - retryFetchErrors: settings.general?.retryFetchErrors ?? false, + retryFetchErrors: settings.general?.retryFetchErrors, ptyInfo: ptyInfo?.name, modelConfigServiceConfig: settings.modelConfigs, // TODO: loading of hooks based on workspace trust - enableHooks: settings.tools?.enableHooks ?? false, + enableHooks: settings.tools?.enableHooks, hooks: settings.hooks || {}, projectHooks: projectHooks || {}, onModelChange: (model: string) => saveModelChange(loadedSettings, model), diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 1559cbe78c..133b50daae 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -35,6 +35,14 @@ vi.mock('./trustedFolders.js', () => ({ .mockReturnValue({ isTrusted: true, source: 'file' }), })); +vi.mock('./settingsSchema.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + getSettingsSchema: vi.fn(actual.getSettingsSchema), + }; +}); + // 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 { @@ -65,10 +73,16 @@ import { 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, + MergeStrategy, + type SettingsSchema, +} from './settingsSchema.js'; const MOCK_WORKSPACE_DIR = '/mock/workspace'; // Use the (mocked) GEMINI_DIR for consistency @@ -149,14 +163,6 @@ describe('Settings Loading and Merging', () => { }); 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([ { scope: 'system', @@ -201,7 +207,7 @@ describe('Settings Loading and Merging', () => { expect( settings[scope as 'system' | 'user' | 'workspace'].settings, ).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.user.settings).toEqual(userSettingsContent); expect(settings.workspace.settings).toEqual(workspaceSettingsContent); - expect(settings.merged).toEqual({ + expect(settings.merged).toMatchObject({ ui: { theme: 'system-theme', }, @@ -318,7 +324,7 @@ describe('Settings Loading and Merging', () => { const settings = loadSettings(MOCK_WORKSPACE_DIR); - expect(settings.merged).toEqual({ + expect(settings.merged).toMatchObject({ ui: { theme: 'legacy-dark', }, @@ -443,10 +449,33 @@ describe('Settings Loading and Merging', () => { expect(settings.merged.advanced?.excludedEnvVars).toEqual( 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', () => { + // 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); const systemDefaultsContent = { ui: { @@ -510,7 +539,7 @@ describe('Settings Loading and Merging', () => { expect(settings.workspace.settings).toEqual(workspaceSettingsContent); expect(settings.merged).toEqual({ context: { - fileName: 'WORKSPACE_CONTEXT.md', + discoveryMaxDirs: 200, includeDirectories: [ '/system/defaults/dir', '/user/dir1', @@ -518,14 +547,12 @@ describe('Settings Loading and Merging', () => { '/workspace/dir', '/system/dir', ], + fileName: 'WORKSPACE_CONTEXT.md', }, + mcpServers: {}, + ui: { theme: 'system-theme' }, + tools: { sandbox: false }, telemetry: false, - tools: { - sandbox: false, - }, - ui: { - theme: 'system-theme', - }, }); }); @@ -660,7 +687,7 @@ describe('Settings Loading and Merging', () => { }, expected: { 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: { 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([ 'DEBUG', + 'DEBUG_MODE', 'NODE_ENV', 'USER_VAR', 'WORKSPACE_DEBUG', @@ -814,8 +842,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).toBeUndefined(); - expect(settings.merged.mcpServers).toBeUndefined(); + expect(settings.merged.ui).toBeDefined(); + expect(settings.merged.mcpServers).toEqual({}); }); 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 (fs.readFileSync as Mock).mockReturnValue('{}'); 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', () => { @@ -1075,10 +1103,10 @@ describe('Settings Loading and Merging', () => { expected: 0.8, }, { - description: 'should be undefined if not in any settings file', + description: 'should be default if not in any settings file', userContent: {}, workspaceContent: {}, - expected: undefined, + expected: 0.5, }, ])('$description', ({ userContent, workspaceContent, expected }) => { (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.settings).toEqual(systemSettingsContent); - expect(settings.merged).toEqual({ + expect(settings.merged).toMatchObject({ ...systemSettingsContent, }); }); @@ -1692,8 +1720,9 @@ describe('Settings Loading and Merging', () => { 'DEBUG', ]); expect(settings.merged.advanced?.excludedEnvVars).toEqual([ - 'NODE_ENV', 'DEBUG', + 'DEBUG_MODE', + 'NODE_ENV', ]); }); @@ -1732,6 +1761,7 @@ describe('Settings Loading and Merging', () => { ]); expect(settings.merged.advanced?.excludedEnvVars).toEqual([ 'DEBUG', + 'DEBUG_MODE', 'NODE_ENV', 'USER_VAR', '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, + }, + }); + }); + }); }); diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index e0a24cbebf..f347a04be2 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -424,6 +424,24 @@ export function migrateSettingsToV1( return v1Settings; } +export function getDefaultsFromSchema( + schema: SettingsSchema = getSettingsSchema(), +): Settings { + const defaults: Record = {}; + 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( system: Settings, systemDefaults: Settings, @@ -432,16 +450,18 @@ function mergeSettings( isTrusted: boolean, ): Settings { const safeWorkspace = isTrusted ? workspace : ({} as Settings); + const schemaDefaults = getDefaultsFromSchema(); // 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) + // 1. Schema Defaults (Built-in) + // 2. System Defaults + // 3. User Settings + // 4. Workspace Settings + // 5. System Settings (as overrides) return customDeepMerge( getMergeStrategyForPath, - {}, // Start with an empty object + schemaDefaults, systemDefaults, user, safeWorkspace, diff --git a/packages/cli/src/ui/components/Footer.tsx b/packages/cli/src/ui/components/Footer.tsx index 358342faa1..7ab087fd99 100644 --- a/packages/cli/src/ui/components/Footer.tsx +++ b/packages/cli/src/ui/components/Footer.tsx @@ -60,12 +60,11 @@ export const Footer: React.FC = () => { const showMemoryUsage = config.getDebugMode() || settings.merged.ui?.showMemoryUsage || false; - const hideCWD = settings.merged.ui?.footer?.hideCWD || false; - const hideSandboxStatus = - settings.merged.ui?.footer?.hideSandboxStatus || false; - const hideModelInfo = settings.merged.ui?.footer?.hideModelInfo || false; + const hideCWD = settings.merged.ui?.footer?.hideCWD; + const hideSandboxStatus = settings.merged.ui?.footer?.hideSandboxStatus; + const hideModelInfo = settings.merged.ui?.footer?.hideModelInfo; const hideContextPercentage = - settings.merged.ui?.footer?.hideContextPercentage ?? true; + settings.merged.ui?.footer?.hideContextPercentage; const pathLength = Math.max(20, Math.floor(mainAreaWidth * 0.25)); const displayPath = shortenPath(tildeifyPath(targetDir), pathLength); diff --git a/packages/cli/src/ui/utils/ui-sizing.test.ts b/packages/cli/src/ui/utils/ui-sizing.test.ts index 331c416b7a..7611abbaa3 100644 --- a/packages/cli/src/ui/utils/ui-sizing.test.ts +++ b/packages/cli/src/ui/utils/ui-sizing.test.ts @@ -35,8 +35,8 @@ describe('ui-sizing', () => { [80, true, true, 79], // -1 for alternate buffer [100, true, true, 99], - // Default behavior (useFullWidth undefined or true) - [100, undefined, false, 100], + // Default behavior (useFullWidth true) + [100, true, false, 100], // useFullWidth: false (Smart sizing) [80, false, false, 78], // 98% of 80 diff --git a/packages/cli/src/ui/utils/ui-sizing.ts b/packages/cli/src/ui/utils/ui-sizing.ts index be85fe88b9..26d28fefa4 100644 --- a/packages/cli/src/ui/utils/ui-sizing.ts +++ b/packages/cli/src/ui/utils/ui-sizing.ts @@ -27,7 +27,7 @@ export const calculateMainAreaWidth = ( terminalWidth: number, settings: LoadedSettings, ): number => { - if (settings.merged.ui?.useFullWidth !== false) { + if (settings.merged.ui?.useFullWidth) { if (isAlternateBufferEnabled(settings)) { return terminalWidth - 1; } diff --git a/packages/cli/src/utils/settingsUtils.ts b/packages/cli/src/utils/settingsUtils.ts index 7ec5fd5885..69530e6dda 100644 --- a/packages/cli/src/utils/settingsUtils.ts +++ b/packages/cli/src/utils/settingsUtils.ts @@ -279,11 +279,7 @@ export function getSettingValue( if (typeof value === 'boolean') { 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 }