Skip to content

Commit c841070

Browse files
cocosheng-gcocosheng-g
authored
feat(cli): support boolean and number casting for env vars in settings.json (#26118)
1 parent 4b8d5e7 commit c841070

4 files changed

Lines changed: 281 additions & 23 deletions

File tree

‎packages/cli/src/config/settings-validation.test.ts‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
settingsZodSchema,
1414
} from './settings-validation.js';
1515
import { z } from 'zod';
16+
import { type Settings } from './settingsSchema.js';
1617

1718
describe('settings-validation', () => {
1819
describe('validateSettings', () => {
@@ -325,6 +326,90 @@ describe('settings-validation', () => {
325326
const result = validateSettings(validSettings);
326327
expect(result.success).toBe(true);
327328
});
329+
330+
describe('type casting', () => {
331+
it('should cast "true" and "false" strings to booleans', () => {
332+
const settings = {
333+
ui: {
334+
autoThemeSwitching: 'true',
335+
hideWindowTitle: 'false',
336+
},
337+
};
338+
339+
const result = validateSettings(settings);
340+
expect(result.success).toBe(true);
341+
const data = result.data as Settings;
342+
expect(data.ui?.autoThemeSwitching).toBe(true);
343+
expect(data.ui?.hideWindowTitle).toBe(false);
344+
});
345+
346+
it('should cast boolean strings case-insensitively', () => {
347+
const settings = {
348+
ui: {
349+
autoThemeSwitching: 'TRUE',
350+
hideWindowTitle: 'fAlSe',
351+
},
352+
};
353+
354+
const result = validateSettings(settings);
355+
expect(result.success).toBe(true);
356+
const data = result.data as Settings;
357+
expect(data.ui?.autoThemeSwitching).toBe(true);
358+
expect(data.ui?.hideWindowTitle).toBe(false);
359+
});
360+
361+
it('should cast numeric strings to numbers', () => {
362+
const settings = {
363+
model: {
364+
maxSessionTurns: '42',
365+
compressionThreshold: '0.5',
366+
},
367+
};
368+
369+
const result = validateSettings(settings);
370+
expect(result.success).toBe(true);
371+
const data = result.data as Settings;
372+
expect(data.model?.maxSessionTurns).toBe(42);
373+
expect(data.model?.compressionThreshold).toBe(0.5);
374+
});
375+
376+
it('should reject invalid castable strings', () => {
377+
const settings = {
378+
ui: {
379+
autoThemeSwitching: 'not-a-boolean',
380+
},
381+
model: {
382+
maxSessionTurns: 'not-a-number',
383+
},
384+
};
385+
386+
const result = validateSettings(settings);
387+
expect(result.success).toBe(false);
388+
expect(result.error?.issues).toHaveLength(2);
389+
expect(result.error?.issues[0].message).toContain(
390+
'Expected boolean, received string',
391+
);
392+
expect(result.error?.issues[1].message).toContain(
393+
'Expected number, received string',
394+
);
395+
});
396+
397+
it('should cast strings to booleans/numbers in shared definitions (refs)', () => {
398+
const settings = {
399+
mcpServers: {
400+
'test-server': {
401+
command: 'node',
402+
trust: 'true', // from boolean ref
403+
},
404+
},
405+
};
406+
407+
const result = validateSettings(settings);
408+
expect(result.success).toBe(true);
409+
const data = result.data as Settings;
410+
expect(data.mcpServers?.['test-server'].trust).toBe(true);
411+
});
412+
});
328413
});
329414

330415
describe('formatValidationError', () => {

‎packages/cli/src/config/settings-validation.ts‎

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,10 @@ function buildZodSchemaFromJsonSchema(def: any): z.ZodTypeAny {
2525
if (def.type === 'string') {
2626
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
2727
if (def.enum) return z.enum(def.enum as [string, ...string[]]);
28-
return z.string();
28+
return buildPrimitiveSchema('string');
2929
}
30-
if (def.type === 'number') return z.number();
31-
if (def.type === 'boolean') return z.boolean();
30+
if (def.type === 'number') return buildPrimitiveSchema('number');
31+
if (def.type === 'boolean') return buildPrimitiveSchema('boolean');
3232

3333
if (def.type === 'array') {
3434
if (def.items) {
@@ -133,9 +133,22 @@ function buildPrimitiveSchema(
133133
case 'string':
134134
return z.string();
135135
case 'number':
136-
return z.number();
136+
return z.preprocess((val) => {
137+
if (typeof val === 'string' && val.trim() !== '') {
138+
const num = Number(val);
139+
if (!isNaN(num)) return num;
140+
}
141+
return val;
142+
}, z.number());
137143
case 'boolean':
138-
return z.boolean();
144+
return z.preprocess((val) => {
145+
if (typeof val === 'string') {
146+
const lower = val.toLowerCase();
147+
if (lower === 'true') return true;
148+
if (lower === 'false') return false;
149+
}
150+
return val;
151+
}, z.boolean());
139152
default:
140153
return z.unknown();
141154
}
@@ -160,7 +173,9 @@ function buildZodSchemaFromDefinition(
160173
if (definition.ref === 'TelemetrySettings') {
161174
const objectSchema = REF_SCHEMAS['TelemetrySettings'];
162175
if (objectSchema) {
163-
return z.union([z.boolean(), objectSchema]).optional();
176+
return z
177+
.union([buildPrimitiveSchema('boolean'), objectSchema])
178+
.optional();
164179
}
165180
}
166181

‎packages/cli/src/config/settings.test.ts‎

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -479,6 +479,137 @@ describe('Settings Loading and Merging', () => {
479479
expect(settings.merged.security?.folderTrust?.enabled).toBe(false); // Workspace setting should be used
480480
});
481481

482+
it('should resolve environment variables and cast them to correct types before validation', () => {
483+
vi.stubEnv('TEST_AUTO_THEME', 'false');
484+
vi.stubEnv('TEST_MAX_TURNS', '15');
485+
486+
(mockFsExistsSync as Mock).mockImplementation(
487+
(p: fs.PathLike) =>
488+
path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH),
489+
);
490+
(fs.readFileSync as Mock).mockImplementation(
491+
(p: fs.PathOrFileDescriptor) => {
492+
if (
493+
path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH)
494+
) {
495+
return JSON.stringify({
496+
ui: { autoThemeSwitching: '$TEST_AUTO_THEME' },
497+
model: { maxSessionTurns: '$TEST_MAX_TURNS' },
498+
});
499+
}
500+
return '{}';
501+
},
502+
);
503+
504+
const settings = loadSettings(MOCK_WORKSPACE_DIR);
505+
506+
expect(settings.merged.ui.autoThemeSwitching).toBe(false);
507+
expect(settings.merged.model.maxSessionTurns).toBe(15);
508+
expect(settings.errors).toHaveLength(0);
509+
});
510+
511+
it('should use default values from environment variable placeholders', () => {
512+
vi.stubEnv('TEST_AUTO_THEME', ''); // Should trigger default
513+
delete process.env['TEST_AUTO_THEME'];
514+
515+
(mockFsExistsSync as Mock).mockImplementation(
516+
(p: fs.PathLike) =>
517+
path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH),
518+
);
519+
(fs.readFileSync as Mock).mockImplementation(
520+
(p: fs.PathOrFileDescriptor) => {
521+
if (
522+
path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH)
523+
) {
524+
return JSON.stringify({
525+
ui: { autoThemeSwitching: '${TEST_AUTO_THEME:-true}' },
526+
});
527+
}
528+
return '{}';
529+
},
530+
);
531+
532+
const settings = loadSettings(MOCK_WORKSPACE_DIR);
533+
534+
expect(settings.merged.ui.autoThemeSwitching).toBe(true);
535+
expect(settings.errors).toHaveLength(0);
536+
});
537+
538+
it('should record validation errors if expansion result is invalid', () => {
539+
vi.stubEnv('TEST_MAX_TURNS', 'not-a-number');
540+
541+
(mockFsExistsSync as Mock).mockImplementation(
542+
(p: fs.PathLike) =>
543+
path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH),
544+
);
545+
(fs.readFileSync as Mock).mockImplementation(
546+
(p: fs.PathOrFileDescriptor) => {
547+
if (
548+
path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH)
549+
) {
550+
return JSON.stringify({
551+
model: { maxSessionTurns: '$TEST_MAX_TURNS' },
552+
});
553+
}
554+
return '{}';
555+
},
556+
);
557+
558+
const settings = loadSettings(MOCK_WORKSPACE_DIR);
559+
560+
expect(settings.errors.length).toBeGreaterThan(0);
561+
expect(settings.errors[0].message).toContain(
562+
'Expected number, received string',
563+
);
564+
// Should fall back to the expanded string value
565+
expect(settings.merged.model.maxSessionTurns).toBe('not-a-number');
566+
});
567+
568+
it('should preserve environment variable placeholders on save', () => {
569+
vi.stubEnv('TEST_AUTO_THEME', 'true');
570+
const placeholder = '${TEST_AUTO_THEME:-false}';
571+
572+
(mockFsExistsSync as Mock).mockImplementation(
573+
(p: fs.PathLike) =>
574+
path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH),
575+
);
576+
(fs.readFileSync as Mock).mockImplementation(
577+
(p: fs.PathOrFileDescriptor) => {
578+
if (
579+
path.normalize(p.toString()) === path.normalize(USER_SETTINGS_PATH)
580+
) {
581+
return JSON.stringify({
582+
ui: { autoThemeSwitching: placeholder },
583+
});
584+
}
585+
return '{}';
586+
},
587+
);
588+
589+
// Load settings - this will expand the placeholder for runtime use
590+
const loaded = loadSettings(MOCK_WORKSPACE_DIR);
591+
expect(loaded.merged.ui.autoThemeSwitching).toBe(true);
592+
593+
// Verify that the original settings for the user scope still have the placeholder
594+
const userFile = loaded.forScope(SettingScope.User);
595+
expect(userFile.originalSettings.ui?.autoThemeSwitching).toBe(
596+
placeholder,
597+
);
598+
599+
// Save settings - this should use the originalSettings (with placeholders)
600+
const mockUpdate = vi.mocked(updateSettingsFilePreservingFormat);
601+
saveSettings(userFile);
602+
603+
expect(mockUpdate).toHaveBeenCalledWith(
604+
USER_SETTINGS_PATH,
605+
expect.objectContaining({
606+
ui: expect.objectContaining({
607+
autoThemeSwitching: placeholder,
608+
}),
609+
}),
610+
);
611+
});
612+
482613
it('should use system folderTrust over user setting', () => {
483614
(mockFsExistsSync as Mock).mockReturnValue(true);
484615
const userSettingsContent = {

‎packages/cli/src/config/settings.ts‎

Lines changed: 44 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -673,7 +673,9 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings {
673673
const storage = new Storage(workspaceDir);
674674
const workspaceSettingsPath = storage.getWorkspaceSettingsPath();
675675

676-
const load = (filePath: string): { settings: Settings; rawJson?: string } => {
676+
const load = (
677+
filePath: string,
678+
): { settings: Settings; rawSettings: Settings; rawJson?: string } => {
677679
try {
678680
if (fs.existsSync(filePath)) {
679681
const content = fs.readFileSync(filePath, 'utf-8');
@@ -689,14 +691,19 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings {
689691
path: filePath,
690692
severity: 'error',
691693
});
692-
return { settings: {} };
694+
return { settings: {}, rawSettings: {} };
693695
}
694696

695697
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
696698
const settingsObject = rawSettings as Record<string, unknown>;
697699

698-
// Validate settings structure with Zod
699-
const validationResult = validateSettings(settingsObject);
700+
// Expand environment variables
701+
const expandedSettings = resolveEnvVarsInObject(
702+
settingsObject as Settings,
703+
);
704+
705+
// Validate settings structure with Zod after environment variable expansion
706+
const validationResult = validateSettings(expandedSettings);
700707
if (!validationResult.success && validationResult.error) {
701708
const errorMessage = formatValidationError(
702709
validationResult.error,
@@ -707,9 +714,22 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings {
707714
path: filePath,
708715
severity: 'warning',
709716
});
717+
return {
718+
settings: expandedSettings,
719+
rawSettings: settingsObject as Settings,
720+
rawJson: content,
721+
};
710722
}
711723

712-
return { settings: settingsObject as Settings, rawJson: content };
724+
// Return the successfully cast and validated data
725+
return {
726+
// Since we've successfully validated expandedSettings against settingsZodSchema,
727+
// it's safe to cast the resulting data to the Settings type.
728+
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
729+
settings: (validationResult.data as Settings) ?? expandedSettings,
730+
rawSettings: settingsObject as Settings,
731+
rawJson: content,
732+
};
713733
}
714734
} catch (error: unknown) {
715735
settingsErrors.push({
@@ -718,33 +738,40 @@ function _doLoadSettings(workspaceDir: string): LoadedSettings {
718738
severity: 'error',
719739
});
720740
}
721-
return { settings: {} };
741+
return { settings: {}, rawSettings: {} };
722742
};
723743

724744
const systemResult = load(systemSettingsPath);
725745
const systemDefaultsResult = load(systemDefaultsPath);
726746
const userResult = load(USER_SETTINGS_PATH);
727747

728-
let workspaceResult: { settings: Settings; rawJson?: string } = {
748+
let workspaceResult: {
749+
settings: Settings;
750+
rawSettings: Settings;
751+
rawJson?: string;
752+
} = {
729753
settings: {} as Settings,
754+
rawSettings: {} as Settings,
730755
rawJson: undefined,
731756
};
732757
if (!storage.isWorkspaceHomeDir()) {
733758
workspaceResult = load(workspaceSettingsPath);
734759
}
735760

736-
const systemOriginalSettings = structuredClone(systemResult.settings);
761+
const systemOriginalSettings = structuredClone(systemResult.rawSettings);
737762
const systemDefaultsOriginalSettings = structuredClone(
738-
systemDefaultsResult.settings,
763+
systemDefaultsResult.rawSettings,
764+
);
765+
const userOriginalSettings = structuredClone(userResult.rawSettings);
766+
const workspaceOriginalSettings = structuredClone(
767+
workspaceResult.rawSettings,
739768
);
740-
const userOriginalSettings = structuredClone(userResult.settings);
741-
const workspaceOriginalSettings = structuredClone(workspaceResult.settings);
742-
743-
// Environment variables for runtime use
744-
systemSettings = resolveEnvVarsInObject(systemResult.settings);
745-
systemDefaultSettings = resolveEnvVarsInObject(systemDefaultsResult.settings);
746-
userSettings = resolveEnvVarsInObject(userResult.settings);
747-
workspaceSettings = resolveEnvVarsInObject(workspaceResult.settings);
769+
770+
// Environment variables for runtime use are already resolved and validated in load()
771+
systemSettings = systemResult.settings;
772+
systemDefaultSettings = systemDefaultsResult.settings;
773+
userSettings = userResult.settings;
774+
workspaceSettings = workspaceResult.settings;
748775

749776
// Support legacy theme names
750777
if (userSettings.ui?.theme === 'VS') {

0 commit comments

Comments
 (0)