feat(cli): support boolean and number casting for env vars in settings.json (#26118)

This commit is contained in:
Coco Sheng
2026-04-28 13:32:51 -04:00
committed by GitHub
parent 4b8d5e7624
commit c841070582
4 changed files with 280 additions and 22 deletions
@@ -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', () => {
+21 -6
View File
@@ -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();
}
}
+131
View File
@@ -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 = {
+43 -16
View File
@@ -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<string, unknown>;
// 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') {