Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions packages/cli/src/config/settings-validation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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', () => {
Expand Down
27 changes: 21 additions & 6 deletions packages/cli/src/config/settings-validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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();
}
Expand All @@ -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();
}
}

Expand Down
131 changes: 131 additions & 0 deletions packages/cli/src/config/settings.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down
61 changes: 44 additions & 17 deletions packages/cli/src/config/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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,
Expand All @@ -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,
Comment thread
cocosheng-g marked this conversation as resolved.
rawSettings: settingsObject as Settings,
rawJson: content,
};
}
} catch (error: unknown) {
settingsErrors.push({
Expand All @@ -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') {
Expand Down
Loading