diff --git a/packages/cli/src/config/settings-validation.test.ts b/packages/cli/src/config/settings-validation.test.ts index e6f920e44b..01cd545cde 100644 --- a/packages/cli/src/config/settings-validation.test.ts +++ b/packages/cli/src/config/settings-validation.test.ts @@ -13,6 +13,7 @@ import { settingsZodSchema, } from './settings-validation.js'; import { z } from 'zod'; +import { type Settings } from './settingsSchema.js'; describe('settings-validation', () => { describe('validateSettings', () => { @@ -325,6 +326,90 @@ describe('settings-validation', () => { const result = validateSettings(validSettings); expect(result.success).toBe(true); }); + + describe('type casting', () => { + it('should cast "true" and "false" strings to booleans', () => { + const settings = { + ui: { + autoThemeSwitching: 'true', + hideWindowTitle: 'false', + }, + }; + + const result = validateSettings(settings); + expect(result.success).toBe(true); + const data = result.data as Settings; + expect(data.ui?.autoThemeSwitching).toBe(true); + expect(data.ui?.hideWindowTitle).toBe(false); + }); + + it('should cast boolean strings case-insensitively', () => { + const settings = { + ui: { + autoThemeSwitching: 'TRUE', + hideWindowTitle: 'fAlSe', + }, + }; + + const result = validateSettings(settings); + expect(result.success).toBe(true); + const data = result.data as Settings; + expect(data.ui?.autoThemeSwitching).toBe(true); + expect(data.ui?.hideWindowTitle).toBe(false); + }); + + it('should cast numeric strings to numbers', () => { + const settings = { + model: { + maxSessionTurns: '42', + compressionThreshold: '0.5', + }, + }; + + const result = validateSettings(settings); + expect(result.success).toBe(true); + const data = result.data as Settings; + expect(data.model?.maxSessionTurns).toBe(42); + expect(data.model?.compressionThreshold).toBe(0.5); + }); + + it('should reject invalid castable strings', () => { + const settings = { + ui: { + autoThemeSwitching: 'not-a-boolean', + }, + model: { + maxSessionTurns: 'not-a-number', + }, + }; + + const result = validateSettings(settings); + expect(result.success).toBe(false); + expect(result.error?.issues).toHaveLength(2); + expect(result.error?.issues[0].message).toContain( + 'Expected boolean, received string', + ); + expect(result.error?.issues[1].message).toContain( + 'Expected number, received string', + ); + }); + + it('should cast strings to booleans/numbers in shared definitions (refs)', () => { + const settings = { + mcpServers: { + 'test-server': { + command: 'node', + trust: 'true', // from boolean ref + }, + }, + }; + + const result = validateSettings(settings); + expect(result.success).toBe(true); + const data = result.data as Settings; + expect(data.mcpServers?.['test-server'].trust).toBe(true); + }); + }); }); describe('formatValidationError', () => { diff --git a/packages/cli/src/config/settings-validation.ts b/packages/cli/src/config/settings-validation.ts index 9921f6c6cc..829cec506e 100644 --- a/packages/cli/src/config/settings-validation.ts +++ b/packages/cli/src/config/settings-validation.ts @@ -25,10 +25,10 @@ function buildZodSchemaFromJsonSchema(def: any): z.ZodTypeAny { if (def.type === 'string') { // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion if (def.enum) return z.enum(def.enum as [string, ...string[]]); - return z.string(); + return buildPrimitiveSchema('string'); } - if (def.type === 'number') return z.number(); - if (def.type === 'boolean') return z.boolean(); + if (def.type === 'number') return buildPrimitiveSchema('number'); + if (def.type === 'boolean') return buildPrimitiveSchema('boolean'); if (def.type === 'array') { if (def.items) { @@ -133,9 +133,22 @@ function buildPrimitiveSchema( case 'string': return z.string(); case 'number': - return z.number(); + return z.preprocess((val) => { + if (typeof val === 'string' && val.trim() !== '') { + const num = Number(val); + if (!isNaN(num)) return num; + } + return val; + }, z.number()); case 'boolean': - return z.boolean(); + return z.preprocess((val) => { + if (typeof val === 'string') { + const lower = val.toLowerCase(); + if (lower === 'true') return true; + if (lower === 'false') return false; + } + return val; + }, z.boolean()); default: return z.unknown(); } @@ -160,7 +173,9 @@ function buildZodSchemaFromDefinition( if (definition.ref === 'TelemetrySettings') { const objectSchema = REF_SCHEMAS['TelemetrySettings']; if (objectSchema) { - return z.union([z.boolean(), objectSchema]).optional(); + return z + .union([buildPrimitiveSchema('boolean'), objectSchema]) + .optional(); } } diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index af0e47b99f..3896a7f5de 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -479,6 +479,137 @@ describe('Settings Loading and Merging', () => { expect(settings.merged.security?.folderTrust?.enabled).toBe(false); // Workspace setting should be used }); + it('should resolve environment variables and cast them to correct types before validation', () => { + vi.stubEnv('TEST_AUTO_THEME', 'false'); + vi.stubEnv('TEST_MAX_TURNS', '15'); + + (mockFsExistsSync as Mock).mockImplementation( + (p: fs.PathLike) => + path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH), + ); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if ( + path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH) + ) { + return JSON.stringify({ + ui: { autoThemeSwitching: '$TEST_AUTO_THEME' }, + model: { maxSessionTurns: '$TEST_MAX_TURNS' }, + }); + } + return '{}'; + }, + ); + + const settings = loadSettings(MOCK_WORKSPACE_DIR); + + expect(settings.merged.ui.autoThemeSwitching).toBe(false); + expect(settings.merged.model.maxSessionTurns).toBe(15); + expect(settings.errors).toHaveLength(0); + }); + + it('should use default values from environment variable placeholders', () => { + vi.stubEnv('TEST_AUTO_THEME', ''); // Should trigger default + delete process.env['TEST_AUTO_THEME']; + + (mockFsExistsSync as Mock).mockImplementation( + (p: fs.PathLike) => + path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH), + ); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if ( + path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH) + ) { + return JSON.stringify({ + ui: { autoThemeSwitching: '${TEST_AUTO_THEME:-true}' }, + }); + } + return '{}'; + }, + ); + + const settings = loadSettings(MOCK_WORKSPACE_DIR); + + expect(settings.merged.ui.autoThemeSwitching).toBe(true); + expect(settings.errors).toHaveLength(0); + }); + + it('should record validation errors if expansion result is invalid', () => { + vi.stubEnv('TEST_MAX_TURNS', 'not-a-number'); + + (mockFsExistsSync as Mock).mockImplementation( + (p: fs.PathLike) => + path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH), + ); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if ( + path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH) + ) { + return JSON.stringify({ + model: { maxSessionTurns: '$TEST_MAX_TURNS' }, + }); + } + return '{}'; + }, + ); + + const settings = loadSettings(MOCK_WORKSPACE_DIR); + + expect(settings.errors.length).toBeGreaterThan(0); + expect(settings.errors[0].message).toContain( + 'Expected number, received string', + ); + // Should fall back to the expanded string value + expect(settings.merged.model.maxSessionTurns).toBe('not-a-number'); + }); + + it('should preserve environment variable placeholders on save', () => { + vi.stubEnv('TEST_AUTO_THEME', 'true'); + const placeholder = '${TEST_AUTO_THEME:-false}'; + + (mockFsExistsSync as Mock).mockImplementation( + (p: fs.PathLike) => + path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH), + ); + (fs.readFileSync as Mock).mockImplementation( + (p: fs.PathOrFileDescriptor) => { + if ( + path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH) + ) { + return JSON.stringify({ + ui: { autoThemeSwitching: placeholder }, + }); + } + return '{}'; + }, + ); + + // Load settings - this will expand the placeholder for runtime use + const loaded = loadSettings(MOCK_WORKSPACE_DIR); + expect(loaded.merged.ui.autoThemeSwitching).toBe(true); + + // Verify that the original settings for the user scope still have the placeholder + const userFile = loaded.forScope(SettingScope.User); + expect(userFile.originalSettings.ui?.autoThemeSwitching).toBe( + placeholder, + ); + + // Save settings - this should use the originalSettings (with placeholders) + const mockUpdate = vi.mocked(updateSettingsFilePreservingFormat); + saveSettings(userFile); + + expect(mockUpdate).toHaveBeenCalledWith( + USER_SETTINGS_PATH, + expect.objectContaining({ + ui: expect.objectContaining({ + autoThemeSwitching: placeholder, + }), + }), + ); + }); + it('should use system folderTrust over user setting', () => { (mockFsExistsSync as Mock).mockReturnValue(true); const userSettingsContent = { diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index b84c1bda40..aace550a25 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -673,7 +673,9 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { const storage = new Storage(workspaceDir); const workspaceSettingsPath = storage.getWorkspaceSettingsPath(); - const load = (filePath: string): { settings: Settings; rawJson?: string } => { + const load = ( + filePath: string, + ): { settings: Settings; rawSettings: Settings; rawJson?: string } => { try { if (fs.existsSync(filePath)) { const content = fs.readFileSync(filePath, 'utf-8'); @@ -689,14 +691,19 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { path: filePath, severity: 'error', }); - return { settings: {} }; + return { settings: {}, rawSettings: {} }; } // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion const settingsObject = rawSettings as Record; - // Validate settings structure with Zod - const validationResult = validateSettings(settingsObject); + // Expand environment variables + const expandedSettings = resolveEnvVarsInObject( + settingsObject as Settings, + ); + + // Validate settings structure with Zod after environment variable expansion + const validationResult = validateSettings(expandedSettings); if (!validationResult.success && validationResult.error) { const errorMessage = formatValidationError( validationResult.error, @@ -707,9 +714,22 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { path: filePath, severity: 'warning', }); + return { + settings: expandedSettings, + rawSettings: settingsObject as Settings, + rawJson: content, + }; } - return { settings: settingsObject as Settings, rawJson: content }; + // Return the successfully cast and validated data + return { + // Since we've successfully validated expandedSettings against settingsZodSchema, + // it's safe to cast the resulting data to the Settings type. + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + settings: (validationResult.data as Settings) ?? expandedSettings, + rawSettings: settingsObject as Settings, + rawJson: content, + }; } } catch (error: unknown) { settingsErrors.push({ @@ -718,33 +738,40 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { severity: 'error', }); } - return { settings: {} }; + return { settings: {}, rawSettings: {} }; }; const systemResult = load(systemSettingsPath); const systemDefaultsResult = load(systemDefaultsPath); const userResult = load(USER_SETTINGS_PATH); - let workspaceResult: { settings: Settings; rawJson?: string } = { + let workspaceResult: { + settings: Settings; + rawSettings: Settings; + rawJson?: string; + } = { settings: {} as Settings, + rawSettings: {} as Settings, rawJson: undefined, }; if (!storage.isWorkspaceHomeDir()) { workspaceResult = load(workspaceSettingsPath); } - const systemOriginalSettings = structuredClone(systemResult.settings); + const systemOriginalSettings = structuredClone(systemResult.rawSettings); const systemDefaultsOriginalSettings = structuredClone( - systemDefaultsResult.settings, + systemDefaultsResult.rawSettings, + ); + const userOriginalSettings = structuredClone(userResult.rawSettings); + const workspaceOriginalSettings = structuredClone( + workspaceResult.rawSettings, ); - const userOriginalSettings = structuredClone(userResult.settings); - const workspaceOriginalSettings = structuredClone(workspaceResult.settings); - // Environment variables for runtime use - systemSettings = resolveEnvVarsInObject(systemResult.settings); - systemDefaultSettings = resolveEnvVarsInObject(systemDefaultsResult.settings); - userSettings = resolveEnvVarsInObject(userResult.settings); - workspaceSettings = resolveEnvVarsInObject(workspaceResult.settings); + // Environment variables for runtime use are already resolved and validated in load() + systemSettings = systemResult.settings; + systemDefaultSettings = systemDefaultsResult.settings; + userSettings = userResult.settings; + workspaceSettings = workspaceResult.settings; // Support legacy theme names if (userSettings.ui?.theme === 'VS') {