From ca0fb9cde0903b9124ae8e29c9bcdc4c06242d66 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Tue, 28 Apr 2026 10:59:18 -0400 Subject: [PATCH 01/11] feat(cli): support boolean and number casting for env vars in settings.json --- .../cli/src/config/settings-validation.ts | 17 +++++++- packages/cli/src/config/settings.ts | 43 ++++++++++++------- 2 files changed, 43 insertions(+), 17 deletions(-) diff --git a/packages/cli/src/config/settings-validation.ts b/packages/cli/src/config/settings-validation.ts index 9921f6c6cc6..de99e34a031 100644 --- a/packages/cli/src/config/settings-validation.ts +++ b/packages/cli/src/config/settings-validation.ts @@ -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(); } diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index b84c1bda40c..6a3a5750e3f 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -695,20 +695,6 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion const settingsObject = rawSettings as Record; - // Validate settings structure with Zod - const validationResult = validateSettings(settingsObject); - if (!validationResult.success && validationResult.error) { - const errorMessage = formatValidationError( - validationResult.error, - filePath, - ); - settingsErrors.push({ - message: errorMessage, - path: filePath, - severity: 'warning', - }); - } - return { settings: settingsObject as Settings, rawJson: content }; } } catch (error: unknown) { @@ -740,12 +726,39 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { const userOriginalSettings = structuredClone(userResult.settings); const workspaceOriginalSettings = structuredClone(workspaceResult.settings); - // Environment variables for runtime use + // Environment variables for runtime use systemSettings = resolveEnvVarsInObject(systemResult.settings); systemDefaultSettings = resolveEnvVarsInObject(systemDefaultsResult.settings); userSettings = resolveEnvVarsInObject(userResult.settings); workspaceSettings = resolveEnvVarsInObject(workspaceResult.settings); + // Validate settings structure with Zod after environment variable expansion + const validateAndRecordErrors = (settings: Settings, filePath: string): Settings => { + if (Object.keys(settings).length === 0) return settings; + const validationResult = validateSettings(settings); + if (!validationResult.success && validationResult.error) { + const errorMessage = formatValidationError( + validationResult.error, + filePath, + ); + settingsErrors.push({ + message: errorMessage, + path: filePath, + severity: 'warning', + }); + return settings; + } + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + return (validationResult.data as Settings) ?? settings; + }; + + systemSettings = validateAndRecordErrors(systemSettings, systemSettingsPath); + systemDefaultSettings = validateAndRecordErrors(systemDefaultSettings, systemDefaultsPath); + userSettings = validateAndRecordErrors(userSettings, USER_SETTINGS_PATH); + if (!storage.isWorkspaceHomeDir()) { + workspaceSettings = validateAndRecordErrors(workspaceSettings, workspaceSettingsPath); + } + // Support legacy theme names if (userSettings.ui?.theme === 'VS') { userSettings.ui.theme = DefaultLight.name; From b6b802a3d6fa0e460b842ca87772fdeb37e56546 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Tue, 28 Apr 2026 11:01:35 -0400 Subject: [PATCH 02/11] test(cli): add unit tests for boolean/number casting and delayed validation --- .../src/config/settings-validation.test.ts | 62 +++++++++++++++ packages/cli/src/config/settings.test.ts | 78 +++++++++++++++++++ 2 files changed, 140 insertions(+) diff --git a/packages/cli/src/config/settings-validation.test.ts b/packages/cli/src/config/settings-validation.test.ts index e6f920e44ba..7b80c67b2c4 100644 --- a/packages/cli/src/config/settings-validation.test.ts +++ b/packages/cli/src/config/settings-validation.test.ts @@ -325,6 +325,68 @@ 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 any; + 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 any; + 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 any; + 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); + }); + }); }); describe('formatValidationError', () => { diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index af0e47b99fc..42fa79d752f 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -479,6 +479,84 @@ 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 use system folderTrust over user setting', () => { (mockFsExistsSync as Mock).mockReturnValue(true); const userSettingsContent = { From 27f09b4e9ca0efc77a5d226c34cb655f8a25e678 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Tue, 28 Apr 2026 11:20:58 -0400 Subject: [PATCH 03/11] fix(cli): fix lint errors and type errors in tests --- .../cli/src/config/settings-validation.test.ts | 3 +++ packages/cli/src/config/settings.test.ts | 18 +++++++++++++----- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/config/settings-validation.test.ts b/packages/cli/src/config/settings-validation.test.ts index 7b80c67b2c4..995fd3a1650 100644 --- a/packages/cli/src/config/settings-validation.test.ts +++ b/packages/cli/src/config/settings-validation.test.ts @@ -337,6 +337,7 @@ describe('settings-validation', () => { const result = validateSettings(settings); expect(result.success).toBe(true); + // eslint-disable-next-line @typescript-eslint/no-explicit-any const data = result.data as any; expect(data.ui.autoThemeSwitching).toBe(true); expect(data.ui.hideWindowTitle).toBe(false); @@ -352,6 +353,7 @@ describe('settings-validation', () => { const result = validateSettings(settings); expect(result.success).toBe(true); + // eslint-disable-next-line @typescript-eslint/no-explicit-any const data = result.data as any; expect(data.ui.autoThemeSwitching).toBe(true); expect(data.ui.hideWindowTitle).toBe(false); @@ -367,6 +369,7 @@ describe('settings-validation', () => { const result = validateSettings(settings); expect(result.success).toBe(true); + // eslint-disable-next-line @typescript-eslint/no-explicit-any const data = result.data as any; expect(data.model.maxSessionTurns).toBe(42); expect(data.model.compressionThreshold).toBe(0.5); diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 42fa79d752f..32406448ab8 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -489,7 +489,9 @@ describe('Settings Loading and Merging', () => { ); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH)) { + if ( + path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH) + ) { return JSON.stringify({ ui: { autoThemeSwitching: '$TEST_AUTO_THEME' }, model: { maxSessionTurns: '$TEST_MAX_TURNS' }, @@ -508,7 +510,7 @@ describe('Settings Loading and Merging', () => { it('should use default values from environment variable placeholders', () => { vi.stubEnv('TEST_AUTO_THEME', ''); // Should trigger default - delete process.env.TEST_AUTO_THEME; + delete process.env['TEST_AUTO_THEME']; (mockFsExistsSync as Mock).mockImplementation( (p: fs.PathLike) => @@ -516,7 +518,9 @@ describe('Settings Loading and Merging', () => { ); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH)) { + if ( + path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH) + ) { return JSON.stringify({ ui: { autoThemeSwitching: '${TEST_AUTO_THEME:-true}' }, }); @@ -540,7 +544,9 @@ describe('Settings Loading and Merging', () => { ); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH)) { + if ( + path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH) + ) { return JSON.stringify({ model: { maxSessionTurns: '$TEST_MAX_TURNS' }, }); @@ -552,7 +558,9 @@ describe('Settings Loading and Merging', () => { const settings = loadSettings(MOCK_WORKSPACE_DIR); expect(settings.errors.length).toBeGreaterThan(0); - expect(settings.errors[0].message).toContain('Expected number, received string'); + 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'); }); From ef667e05f968f07cfe858c5ffe10a9edc8550c91 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Tue, 28 Apr 2026 11:37:07 -0400 Subject: [PATCH 04/11] refactor(cli): improve security by failing closed on validation error --- packages/cli/src/config/settings.test.ts | 4 +- packages/cli/src/config/settings.ts | 62 ++++++++++++++++++------ 2 files changed, 48 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 32406448ab8..a79791632b8 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -561,8 +561,8 @@ describe('Settings Loading and Merging', () => { 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'); + // Should fall back to the schema default (since we return {} on validation failure for that scope) + expect(settings.merged.model.maxSessionTurns).toBe(-1); }); it('should use system folderTrust over user setting', () => { diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 6a3a5750e3f..6b13788929f 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -726,37 +726,67 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { const userOriginalSettings = structuredClone(userResult.settings); const workspaceOriginalSettings = structuredClone(workspaceResult.settings); - // Environment variables for runtime use + // Environment variables for runtime use systemSettings = resolveEnvVarsInObject(systemResult.settings); systemDefaultSettings = resolveEnvVarsInObject(systemDefaultsResult.settings); userSettings = resolveEnvVarsInObject(userResult.settings); workspaceSettings = resolveEnvVarsInObject(workspaceResult.settings); - // Validate settings structure with Zod after environment variable expansion - const validateAndRecordErrors = (settings: Settings, filePath: string): Settings => { - if (Object.keys(settings).length === 0) return settings; + // Validate settings structure with Zod after environment variable expansion. + // We use a functional approach to avoid mutating outer state, and "fail closed" + // by returning an empty object if validation fails to prevent insecure configurations. + const validate = ( + settings: Settings, + filePath: string, + ): { settings: Settings; errors: SettingsError[] } => { + if (Object.keys(settings).length === 0) return { settings, errors: [] }; const validationResult = validateSettings(settings); if (!validationResult.success && validationResult.error) { const errorMessage = formatValidationError( validationResult.error, filePath, ); - settingsErrors.push({ - message: errorMessage, - path: filePath, - severity: 'warning', - }); - return settings; + return { + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + settings: {} as unknown as Settings, + errors: [ + { + message: errorMessage, + path: filePath, + severity: 'warning', + }, + ], + }; } - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - return (validationResult.data as Settings) ?? settings; + return { + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + settings: (validationResult.data as Settings) ?? settings, + errors: [], + }; }; - systemSettings = validateAndRecordErrors(systemSettings, systemSettingsPath); - systemDefaultSettings = validateAndRecordErrors(systemDefaultSettings, systemDefaultsPath); - userSettings = validateAndRecordErrors(userSettings, USER_SETTINGS_PATH); + const systemValidation = validate(systemSettings, systemSettingsPath); + systemSettings = systemValidation.settings; + settingsErrors.push(...systemValidation.errors); + + const systemDefaultsValidation = validate( + systemDefaultSettings, + systemDefaultsPath, + ); + systemDefaultSettings = systemDefaultsValidation.settings; + settingsErrors.push(...systemDefaultsValidation.errors); + + const userValidation = validate(userSettings, USER_SETTINGS_PATH); + userSettings = userValidation.settings; + settingsErrors.push(...userValidation.errors); + if (!storage.isWorkspaceHomeDir()) { - workspaceSettings = validateAndRecordErrors(workspaceSettings, workspaceSettingsPath); + const workspaceValidation = validate( + workspaceSettings, + workspaceSettingsPath, + ); + workspaceSettings = workspaceValidation.settings; + settingsErrors.push(...workspaceValidation.errors); } // Support legacy theme names From c856609e23e8a78e92adbaa21cec7f82dc57a4c8 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Tue, 28 Apr 2026 11:54:11 -0400 Subject: [PATCH 05/11] refactor(cli): improve security by failing closed on validation error --- packages/cli/src/config/settings.test.ts | 11 +++----- packages/cli/src/config/settings.ts | 6 +++-- .../settings_validation_warning.test.ts | 25 ++++--------------- 3 files changed, 12 insertions(+), 30 deletions(-) diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index a79791632b8..16b4c24c551 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -535,7 +535,7 @@ describe('Settings Loading and Merging', () => { expect(settings.errors).toHaveLength(0); }); - it('should record validation errors if expansion result is invalid', () => { + it('should throw FatalConfigError if expansion result is invalid', () => { vi.stubEnv('TEST_MAX_TURNS', 'not-a-number'); (mockFsExistsSync as Mock).mockImplementation( @@ -555,14 +555,9 @@ describe('Settings Loading and Merging', () => { }, ); - const settings = loadSettings(MOCK_WORKSPACE_DIR); - - expect(settings.errors.length).toBeGreaterThan(0); - expect(settings.errors[0].message).toContain( - 'Expected number, received string', + expect(() => loadSettings(MOCK_WORKSPACE_DIR)).toThrow( + /Expected number, received string/, ); - // Should fall back to the schema default (since we return {} on validation failure for that scope) - expect(settings.merged.model.maxSessionTurns).toBe(-1); }); it('should use system folderTrust over user setting', () => { diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 6b13788929f..b49a453c3f1 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -734,7 +734,7 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { // Validate settings structure with Zod after environment variable expansion. // We use a functional approach to avoid mutating outer state, and "fail closed" - // by returning an empty object if validation fails to prevent insecure configurations. + // by marking errors as 'error' (fatal) if validation fails to prevent insecure configurations. const validate = ( settings: Settings, filePath: string, @@ -747,13 +747,15 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { filePath, ); return { + // If validation fails, we return an empty object for this layer. + // Since we also return a fatal 'error', the app will stop anyway. // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion settings: {} as unknown as Settings, errors: [ { message: errorMessage, path: filePath, - severity: 'warning', + severity: 'error', }, ], }; diff --git a/packages/cli/src/config/settings_validation_warning.test.ts b/packages/cli/src/config/settings_validation_warning.test.ts index 435c797d81a..62874b187bf 100644 --- a/packages/cli/src/config/settings_validation_warning.test.ts +++ b/packages/cli/src/config/settings_validation_warning.test.ts @@ -80,7 +80,6 @@ vi.mock('fs', async (importOriginal) => { import { loadSettings, USER_SETTINGS_PATH, - type LoadedSettings, resetSettingsCacheForTesting, } from './settings.js'; @@ -94,20 +93,14 @@ describe('Settings Validation Warning', () => { (fs.existsSync as Mock).mockReturnValue(false); }); - it('should emit a warning and NOT throw when settings are invalid', () => { + it('should throw a fatal error when settings have invalid types (fail-closed)', () => { (fs.existsSync as Mock).mockImplementation( (p: string) => p === USER_SETTINGS_PATH, ); const invalidSettingsContent = { ui: { - customThemes: { - terafox: { - name: 'terafox', - type: 'custom', - DiffModified: '#ffffff', // Invalid key - }, - }, + autoThemeSwitching: 'not-a-boolean', }, }; @@ -117,18 +110,10 @@ describe('Settings Validation Warning', () => { return '{}'; }); - // Should NOT throw - let settings: LoadedSettings | undefined; + // Should throw FatalConfigError expect(() => { - settings = loadSettings(MOCK_WORKSPACE_DIR); - }).not.toThrow(); - - // Should have recorded a warning in the settings object - expect( - settings?.errors.some((e) => - e.message.includes("Unrecognized key(s) in object: 'DiffModified'"), - ), - ).toBe(true); + loadSettings(MOCK_WORKSPACE_DIR); + }).toThrow(/Expected boolean, received string/); }); it('should throw a fatal error when settings file is not a valid JSON object', () => { From e3b88689e967e444ff81b391733e9afd1fa7a7d8 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Tue, 28 Apr 2026 11:56:25 -0400 Subject: [PATCH 06/11] refactor(cli): ensure fail-closed security for settings validation --- packages/cli/src/config/settings.test.ts | 5 +++++ packages/cli/src/config/settings.ts | 5 ++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 16b4c24c551..2bf9a7282dd 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -555,9 +555,14 @@ describe('Settings Loading and Merging', () => { }, ); + // Should throw FatalConfigError expect(() => loadSettings(MOCK_WORKSPACE_DIR)).toThrow( /Expected number, received string/, ); + + // Verify that if we bypass the throw (internal check), we still have the original values + // This is a bit tricky since loadSettings is cached and throws. + // But we can clear cache and use a non-throwing mock for coreEvents if needed. }); it('should use system folderTrust over user setting', () => { diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index b49a453c3f1..38b1058446e 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -747,10 +747,9 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { filePath, ); return { - // If validation fails, we return an empty object for this layer. + // If validation fails, we return the original settings. // Since we also return a fatal 'error', the app will stop anyway. - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - settings: {} as unknown as Settings, + settings, errors: [ { message: errorMessage, From 2f5e8f330f426d7b4f20186baf4d25a9cc89d15c Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Tue, 28 Apr 2026 12:00:24 -0400 Subject: [PATCH 07/11] refactor(cli): restore warning-only validation while ensuring security --- packages/cli/src/config/settings.test.ts | 16 ++++++------ packages/cli/src/config/settings.ts | 11 ++++---- .../settings_validation_warning.test.ts | 26 +++++++++++++++---- 3 files changed, 35 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 2bf9a7282dd..32406448ab8 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -535,7 +535,7 @@ describe('Settings Loading and Merging', () => { expect(settings.errors).toHaveLength(0); }); - it('should throw FatalConfigError if expansion result is invalid', () => { + it('should record validation errors if expansion result is invalid', () => { vi.stubEnv('TEST_MAX_TURNS', 'not-a-number'); (mockFsExistsSync as Mock).mockImplementation( @@ -555,14 +555,14 @@ describe('Settings Loading and Merging', () => { }, ); - // Should throw FatalConfigError - expect(() => loadSettings(MOCK_WORKSPACE_DIR)).toThrow( - /Expected number, received string/, - ); + const settings = loadSettings(MOCK_WORKSPACE_DIR); - // Verify that if we bypass the throw (internal check), we still have the original values - // This is a bit tricky since loadSettings is cached and throws. - // But we can clear cache and use a non-throwing mock for coreEvents if needed. + 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 use system folderTrust over user setting', () => { diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 38b1058446e..ce9e25388a2 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -733,8 +733,9 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { workspaceSettings = resolveEnvVarsInObject(workspaceResult.settings); // Validate settings structure with Zod after environment variable expansion. - // We use a functional approach to avoid mutating outer state, and "fail closed" - // by marking errors as 'error' (fatal) if validation fails to prevent insecure configurations. + // We use a functional approach to avoid mutating outer state. + // If validation fails, we mark it as a 'warning' and return the original settings + // (instead of an empty object) to avoid "ignoring" the file (fail-open). const validate = ( settings: Settings, filePath: string, @@ -747,14 +748,14 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { filePath, ); return { - // If validation fails, we return the original settings. - // Since we also return a fatal 'error', the app will stop anyway. + // If validation fails, we return the original settings (unvalidated/uncast). + // This ensures the file is NOT ignored during merging, while still reporting the error. settings, errors: [ { message: errorMessage, path: filePath, - severity: 'error', + severity: 'warning', }, ], }; diff --git a/packages/cli/src/config/settings_validation_warning.test.ts b/packages/cli/src/config/settings_validation_warning.test.ts index 62874b187bf..b7000675889 100644 --- a/packages/cli/src/config/settings_validation_warning.test.ts +++ b/packages/cli/src/config/settings_validation_warning.test.ts @@ -80,6 +80,7 @@ vi.mock('fs', async (importOriginal) => { import { loadSettings, USER_SETTINGS_PATH, + type LoadedSettings, resetSettingsCacheForTesting, } from './settings.js'; @@ -93,14 +94,20 @@ describe('Settings Validation Warning', () => { (fs.existsSync as Mock).mockReturnValue(false); }); - it('should throw a fatal error when settings have invalid types (fail-closed)', () => { + it('should emit a warning and NOT throw when settings are invalid', () => { (fs.existsSync as Mock).mockImplementation( (p: string) => p === USER_SETTINGS_PATH, ); const invalidSettingsContent = { ui: { - autoThemeSwitching: 'not-a-boolean', + customThemes: { + terafox: { + name: 'terafox', + type: 'custom', + DiffModified: '#ffffff', // Invalid key + }, + }, }, }; @@ -110,10 +117,19 @@ describe('Settings Validation Warning', () => { return '{}'; }); - // Should throw FatalConfigError + // Should NOT throw + let settings: LoadedSettings | undefined; expect(() => { - loadSettings(MOCK_WORKSPACE_DIR); - }).toThrow(/Expected boolean, received string/); + settings = loadSettings(MOCK_WORKSPACE_DIR); + }).not.toThrow(); + + // Should have recorded a warning in the settings object + expect( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + settings?.errors.some((e: any) => + e.message.includes("Unrecognized key(s) in object: 'DiffModified'"), + ), + ).toBe(true); }); it('should throw a fatal error when settings file is not a valid JSON object', () => { From b12ec7da579792132bbd0183f6fa70cefd23e8e2 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Tue, 28 Apr 2026 12:03:54 -0400 Subject: [PATCH 08/11] refactor(cli): simplify settings loading and validation logic --- packages/cli/src/config/settings.ts | 96 ++++++++++------------------- 1 file changed, 31 insertions(+), 65 deletions(-) diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index ce9e25388a2..7c4b9e3bfaf 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -695,7 +695,32 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion const settingsObject = rawSettings as Record; - return { settings: settingsObject as Settings, rawJson: content }; + // 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, + filePath, + ); + settingsErrors.push({ + message: errorMessage, + path: filePath, + severity: 'warning', + }); + return { settings: expandedSettings, rawJson: content }; + } + + // Return the successfully cast and validated data + return { + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + settings: (validationResult.data as Settings) ?? expandedSettings, + rawJson: content, + }; } } catch (error: unknown) { settingsErrors.push({ @@ -726,70 +751,11 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { 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); - - // Validate settings structure with Zod after environment variable expansion. - // We use a functional approach to avoid mutating outer state. - // If validation fails, we mark it as a 'warning' and return the original settings - // (instead of an empty object) to avoid "ignoring" the file (fail-open). - const validate = ( - settings: Settings, - filePath: string, - ): { settings: Settings; errors: SettingsError[] } => { - if (Object.keys(settings).length === 0) return { settings, errors: [] }; - const validationResult = validateSettings(settings); - if (!validationResult.success && validationResult.error) { - const errorMessage = formatValidationError( - validationResult.error, - filePath, - ); - return { - // If validation fails, we return the original settings (unvalidated/uncast). - // This ensures the file is NOT ignored during merging, while still reporting the error. - settings, - errors: [ - { - message: errorMessage, - path: filePath, - severity: 'warning', - }, - ], - }; - } - return { - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - settings: (validationResult.data as Settings) ?? settings, - errors: [], - }; - }; - - const systemValidation = validate(systemSettings, systemSettingsPath); - systemSettings = systemValidation.settings; - settingsErrors.push(...systemValidation.errors); - - const systemDefaultsValidation = validate( - systemDefaultSettings, - systemDefaultsPath, - ); - systemDefaultSettings = systemDefaultsValidation.settings; - settingsErrors.push(...systemDefaultsValidation.errors); - - const userValidation = validate(userSettings, USER_SETTINGS_PATH); - userSettings = userValidation.settings; - settingsErrors.push(...userValidation.errors); - - if (!storage.isWorkspaceHomeDir()) { - const workspaceValidation = validate( - workspaceSettings, - workspaceSettingsPath, - ); - workspaceSettings = workspaceValidation.settings; - settingsErrors.push(...workspaceValidation.errors); - } + // 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') { From c28c2295181ccb968c8a0a9f73fef9f694a34bfa Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Tue, 28 Apr 2026 12:06:58 -0400 Subject: [PATCH 09/11] chore(cli): revert unnecessary changes to settings_validation_warning.test.ts --- packages/cli/src/config/settings_validation_warning.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/cli/src/config/settings_validation_warning.test.ts b/packages/cli/src/config/settings_validation_warning.test.ts index b7000675889..435c797d81a 100644 --- a/packages/cli/src/config/settings_validation_warning.test.ts +++ b/packages/cli/src/config/settings_validation_warning.test.ts @@ -125,8 +125,7 @@ describe('Settings Validation Warning', () => { // Should have recorded a warning in the settings object expect( - // eslint-disable-next-line @typescript-eslint/no-explicit-any - settings?.errors.some((e: any) => + settings?.errors.some((e) => e.message.includes("Unrecognized key(s) in object: 'DiffModified'"), ), ).toBe(true); From abc1315d1b35a51a12eb6f6b2c22918b39af592e Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Tue, 28 Apr 2026 13:10:41 -0400 Subject: [PATCH 10/11] chore(cli): address code review feedback on type casting and any usage --- .../src/config/settings-validation.test.ts | 22 +++++++++---------- packages/cli/src/config/settings.ts | 2 ++ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/cli/src/config/settings-validation.test.ts b/packages/cli/src/config/settings-validation.test.ts index 995fd3a1650..7a4edf7143d 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', () => { @@ -337,10 +338,9 @@ describe('settings-validation', () => { const result = validateSettings(settings); expect(result.success).toBe(true); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const data = result.data as any; - expect(data.ui.autoThemeSwitching).toBe(true); - expect(data.ui.hideWindowTitle).toBe(false); + 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', () => { @@ -353,10 +353,9 @@ describe('settings-validation', () => { const result = validateSettings(settings); expect(result.success).toBe(true); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const data = result.data as any; - expect(data.ui.autoThemeSwitching).toBe(true); - expect(data.ui.hideWindowTitle).toBe(false); + 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', () => { @@ -369,10 +368,9 @@ describe('settings-validation', () => { const result = validateSettings(settings); expect(result.success).toBe(true); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const data = result.data as any; - expect(data.model.maxSessionTurns).toBe(42); - expect(data.model.compressionThreshold).toBe(0.5); + 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', () => { diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 7c4b9e3bfaf..4452b8bce52 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -717,6 +717,8 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { // 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, rawJson: content, From c8dd38f0f8abdff2511123143f64cc01c8a56dc1 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Tue, 28 Apr 2026 13:15:30 -0400 Subject: [PATCH 11/11] fix(cli): address code review feedback on placeholder preservation and consistent casting --- .../src/config/settings-validation.test.ts | 22 +++++++++ .../cli/src/config/settings-validation.ts | 10 +++-- packages/cli/src/config/settings.test.ts | 45 +++++++++++++++++++ packages/cli/src/config/settings.ts | 32 +++++++++---- 4 files changed, 96 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/config/settings-validation.test.ts b/packages/cli/src/config/settings-validation.test.ts index 7a4edf7143d..01cd545cdea 100644 --- a/packages/cli/src/config/settings-validation.test.ts +++ b/packages/cli/src/config/settings-validation.test.ts @@ -386,6 +386,28 @@ describe('settings-validation', () => { 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); }); }); }); diff --git a/packages/cli/src/config/settings-validation.ts b/packages/cli/src/config/settings-validation.ts index de99e34a031..829cec506e7 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) { @@ -173,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 32406448ab8..3896a7f5de5 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -565,6 +565,51 @@ describe('Settings Loading and Merging', () => { 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 4452b8bce52..aace550a25d 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,7 +691,7 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { path: filePath, severity: 'error', }); - return { settings: {} }; + return { settings: {}, rawSettings: {} }; } // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion @@ -712,7 +714,11 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { path: filePath, severity: 'warning', }); - return { settings: expandedSettings, rawJson: content }; + return { + settings: expandedSettings, + rawSettings: settingsObject as Settings, + rawJson: content, + }; } // Return the successfully cast and validated data @@ -721,6 +727,7 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings { // 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, }; } @@ -731,27 +738,34 @@ 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 are already resolved and validated in load() systemSettings = systemResult.settings;