diff --git a/integration-tests/settings-migration.test.ts b/integration-tests/settings-migration.test.ts index 2964ba0c8..fa5446c17 100644 --- a/integration-tests/settings-migration.test.ts +++ b/integration-tests/settings-migration.test.ts @@ -335,11 +335,12 @@ describe('settings-migration', () => { expect(migratedSettings['customOnlyKey']).toBe('value'); }); - it('should preserve non-boolean disable* values while bumping V2 to V3', async () => { + it('should coerce valid string booleans and remove invalid deprecated keys while bumping V2 to V3', async () => { rig.setup('v2-non-boolean-disable-values-migration'); - // Cover both string variants and non-boolean invalid types: - // only real booleans are migrated, non-boolean values are preserved. + // Cover both coercible string booleans and invalid non-boolean values: + // - "TRUE"/"false" should be coerced and migrated + // - invalid values should have deprecated disable* keys removed const mixedNonBooleanDisableSettings = { ...v2BooleanStringSettings, ui: { @@ -370,11 +371,10 @@ describe('settings-migration', () => { // Read migrated settings const migratedSettings = readSettingsFile(rig); - // Non-boolean disable* values should be preserved + // Coercible strings are migrated; invalid disable* values are removed. expect(migratedSettings['$version']).toBe(3); expect(migratedSettings['general']).toEqual({ - disableAutoUpdate: 'TRUE', - disableUpdateNag: 'false', + enableAutoUpdate: false, }); expect( ( @@ -382,21 +382,42 @@ describe('settings-migration', () => { 'accessibility' ] as Record )?.['disableLoadingPhrases'], - ).toBe('yes'); + ).toBeUndefined(); + expect( + ( + (migratedSettings['ui'] as Record)?.[ + 'accessibility' + ] as Record + )?.['enableLoadingPhrases'], + ).toBeUndefined(); expect( ( (migratedSettings['context'] as Record)?.[ 'fileFiltering' ] as Record )?.['disableFuzzySearch'], - ).toBeNull(); + ).toBeUndefined(); + expect( + ( + (migratedSettings['context'] as Record)?.[ + 'fileFiltering' + ] as Record + )?.['enableFuzzySearch'], + ).toBeUndefined(); expect( ( (migratedSettings['model'] as Record)?.[ 'generationConfig' ] as Record )?.['disableCacheControl'], - ).toEqual([1]); + ).toBeUndefined(); + expect( + ( + (migratedSettings['model'] as Record)?.[ + 'generationConfig' + ] as Record + )?.['enableCacheControl'], + ).toBeUndefined(); }); it('should handle V2 settings with preexisting enable* keys', async () => { diff --git a/packages/cli/src/config/migration/versions/v1-to-v2.ts b/packages/cli/src/config/migration/versions/v1-to-v2.ts index d2e920edc..4dceffe44 100644 --- a/packages/cli/src/config/migration/versions/v1-to-v2.ts +++ b/packages/cli/src/config/migration/versions/v1-to-v2.ts @@ -12,6 +12,7 @@ import { V1_TO_V2_PRESERVE_DISABLE_MAP, V2_CONTAINER_KEYS, } from './v1-to-v2-shared.js'; +import { setNestedPropertySafe } from '../../../utils/settingsUtils.js'; /** * Heuristic indicators for deciding whether an object is "V1-like". @@ -29,43 +30,6 @@ import { * - Primitive/array/null values on indicator keys are treated as legacy V1 signals. */ -/** - * Best-effort nested setter with collision protection. - * - * Strategy: - * - Create missing intermediate objects while traversing the path. - * - If traversal hits a non-object parent, abort this write. - * - * Rationale: - * - Migration should never coerce scalars into objects implicitly because that can - * destroy user data shape. Skipping the write is safer; later carry-over logic - * preserves original values. - */ -function setNestedProperty( - obj: Record, - path: string, - value: unknown, -): void { - const keys = path.split('.'); - const lastKey = keys.pop(); - if (!lastKey) return; - - let current: Record = obj; - for (const key of keys) { - if (current[key] === undefined) { - current[key] = {}; - } - const next = current[key]; - if (typeof next === 'object' && next !== null) { - current = next as Record; - } else { - // Path collision with non-object, stop here - return; - } - } - current[lastKey] = value; -} - /** * V1 -> V2 migration (structural normalization stage). * @@ -200,10 +164,14 @@ export class V1ToV2Migration implements SettingsMigration { ) { // Merge nested properties from this partial V2 structure for (const [nestedKey, nestedValue] of Object.entries(value)) { - setNestedProperty(result, `${v2Path}.${nestedKey}`, nestedValue); + setNestedPropertySafe( + result, + `${v2Path}.${nestedKey}`, + nestedValue, + ); } } else { - setNestedProperty(result, v2Path, value); + setNestedPropertySafe(result, v2Path, value); } processedKeys.add(v1Key); } @@ -218,10 +186,10 @@ export class V1ToV2Migration implements SettingsMigration { if (CONSOLIDATED_DISABLE_KEYS.has(v1Key)) { // Preserve stable behavior: consolidated keys use presence semantics. // Only literal true remains true; all other present values become false. - setNestedProperty(result, v2Path, value === true); + setNestedPropertySafe(result, v2Path, value === true); } else if (typeof value === 'boolean') { // Non-consolidated disable* keys only migrate when explicitly boolean. - setNestedProperty(result, v2Path, value); + setNestedPropertySafe(result, v2Path, value); } processedKeys.add(v1Key); } @@ -274,7 +242,7 @@ export class V1ToV2Migration implements SettingsMigration { }, ); if (!wasProcessed) { - setNestedProperty(result, fullNestedPath, nestedValue); + setNestedPropertySafe(result, fullNestedPath, nestedValue); } } } else { diff --git a/packages/cli/src/config/migration/versions/v2-to-v3.test.ts b/packages/cli/src/config/migration/versions/v2-to-v3.test.ts index dc4ed97c4..a1ba9b46d 100644 --- a/packages/cli/src/config/migration/versions/v2-to-v3.test.ts +++ b/packages/cli/src/config/migration/versions/v2-to-v3.test.ts @@ -272,173 +272,198 @@ describe('V2ToV3Migration', () => { }); }); - it('should preserve string "true" without creating enable key', () => { + it('should coerce string "true" and remove deprecated key', () => { const v2Settings = { $version: 2, general: { disableAutoUpdate: 'true' }, }; - const { settings: result } = migration.migrate(v2Settings, 'user') as { + const { settings: result, warnings } = migration.migrate( + v2Settings, + 'user', + ) as { settings: Record; - warnings: unknown[]; + warnings: string[]; }; expect(result['$version']).toBe(3); expect( (result['general'] as Record)['disableAutoUpdate'], - ).toBe('true'); + ).toBeUndefined(); expect( (result['general'] as Record)['enableAutoUpdate'], - ).toBeUndefined(); + ).toBe(false); + expect(warnings).toHaveLength(0); }); - it('should preserve string "false" without creating enable key', () => { + it('should coerce string "false" and remove deprecated key', () => { const v2Settings = { $version: 2, general: { disableAutoUpdate: 'false' }, }; - const { settings: result } = migration.migrate(v2Settings, 'user') as { + const { settings: result, warnings } = migration.migrate( + v2Settings, + 'user', + ) as { settings: Record; - warnings: unknown[]; + warnings: string[]; }; expect(result['$version']).toBe(3); expect( (result['general'] as Record)['disableAutoUpdate'], - ).toBe('false'); + ).toBeUndefined(); expect( (result['general'] as Record)['enableAutoUpdate'], + ).toBe(true); + expect(warnings).toHaveLength(0); + }); + + it('should coerce case-insensitive strings for consolidated keys', () => { + const v2Settings = { + $version: 2, + general: { + disableAutoUpdate: 'TRUE', + disableUpdateNag: 'FALSE', + }, + }; + + const { settings: result, warnings } = migration.migrate( + v2Settings, + 'user', + ) as { + settings: Record; + warnings: string[]; + }; + + expect(result['$version']).toBe(3); + expect( + (result['general'] as Record)['disableAutoUpdate'], ).toBeUndefined(); - }); - - it('should preserve string "TRUE" without creating enable key', () => { - const v2Settings = { - $version: 2, - general: { disableAutoUpdate: 'TRUE' }, - }; - - const { settings: result } = migration.migrate(v2Settings, 'user') as { - settings: Record; - warnings: unknown[]; - }; - - expect(result['$version']).toBe(3); expect( - (result['general'] as Record)['disableAutoUpdate'], - ).toBe('TRUE'); - }); - - it('should preserve string "FALSE" without creating enable key', () => { - const v2Settings = { - $version: 2, - general: { disableAutoUpdate: 'FALSE' }, - }; - - const { settings: result } = migration.migrate(v2Settings, 'user') as { - settings: Record; - warnings: unknown[]; - }; - - expect(result['$version']).toBe(3); + (result['general'] as Record)['disableUpdateNag'], + ).toBeUndefined(); expect( - (result['general'] as Record)['disableAutoUpdate'], - ).toBe('FALSE'); + (result['general'] as Record)['enableAutoUpdate'], + ).toBe(false); + expect(warnings).toHaveLength(0); }); - it('should preserve number value and not create enable key', () => { + it('should remove number value and emit warning', () => { const v2Settings = { $version: 2, general: { disableAutoUpdate: 123 }, }; - const { settings: result } = migration.migrate(v2Settings, 'user') as { + const { settings: result, warnings } = migration.migrate( + v2Settings, + 'user', + ) as { settings: Record; - warnings: unknown[]; + warnings: string[]; }; expect(result['$version']).toBe(3); expect( (result['general'] as Record)['disableAutoUpdate'], - ).toBe(123); + ).toBeUndefined(); expect( (result['general'] as Record)['enableAutoUpdate'], ).toBeUndefined(); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('general.disableAutoUpdate'); }); - it('should preserve invalid string value and not create enable key', () => { + it('should remove invalid string value and emit warning', () => { const v2Settings = { $version: 2, general: { disableAutoUpdate: 'invalid-string' }, }; - const { settings: result } = migration.migrate(v2Settings, 'user') as { + const { settings: result, warnings } = migration.migrate( + v2Settings, + 'user', + ) as { settings: Record; - warnings: unknown[]; + warnings: string[]; }; expect(result['$version']).toBe(3); expect( (result['general'] as Record)['disableAutoUpdate'], - ).toBe('invalid-string'); + ).toBeUndefined(); expect( (result['general'] as Record)['enableAutoUpdate'], ).toBeUndefined(); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('general.disableAutoUpdate'); }); - it('should preserve disableCacheControl string "true"', () => { + it('should coerce disableCacheControl string "true"', () => { const v2Settings = { $version: 2, model: { generationConfig: { disableCacheControl: 'true' } }, }; - const { settings: result } = migration.migrate(v2Settings, 'user') as { + const { settings: result, warnings } = migration.migrate( + v2Settings, + 'user', + ) as { settings: Record; - warnings: unknown[]; + warnings: string[]; }; expect(result['$version']).toBe(3); expect( (result['model'] as Record)['generationConfig'], ).toEqual({ - disableCacheControl: 'true', + enableCacheControl: false, }); + expect(warnings).toHaveLength(0); }); - it('should preserve disableCacheControl string "false"', () => { + it('should coerce disableCacheControl string "false"', () => { const v2Settings = { $version: 2, model: { generationConfig: { disableCacheControl: 'false' } }, }; - const { settings: result } = migration.migrate(v2Settings, 'user') as { + const { settings: result, warnings } = migration.migrate( + v2Settings, + 'user', + ) as { settings: Record; - warnings: unknown[]; + warnings: string[]; }; expect(result['$version']).toBe(3); expect( (result['model'] as Record)['generationConfig'], ).toEqual({ - disableCacheControl: 'false', + enableCacheControl: true, }); + expect(warnings).toHaveLength(0); }); - it('should preserve disableCacheControl number value and not create enable key', () => { + it('should remove disableCacheControl number value and emit warning', () => { const v2Settings = { $version: 2, model: { generationConfig: { disableCacheControl: 456 } }, }; - const { settings: result } = migration.migrate(v2Settings, 'user') as { + const { settings: result, warnings } = migration.migrate( + v2Settings, + 'user', + ) as { settings: Record; - warnings: unknown[]; + warnings: string[]; }; expect(result['$version']).toBe(3); expect( (result['model'] as Record)['generationConfig'], - ).toEqual({ disableCacheControl: 456 }); + ).toEqual({}); expect( ( (result['model'] as Record)[ @@ -446,6 +471,10 @@ describe('V2ToV3Migration', () => { ] as Record )['enableCacheControl'], ).toBeUndefined(); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain( + 'model.generationConfig.disableCacheControl', + ); }); it('should handle mixed valid and invalid disableAutoUpdate and disableUpdateNag', () => { @@ -457,9 +486,12 @@ describe('V2ToV3Migration', () => { }, }; - const { settings: result } = migration.migrate(v2Settings, 'user') as { + const { settings: result, warnings } = migration.migrate( + v2Settings, + 'user', + ) as { settings: Record; - warnings: unknown[]; + warnings: string[]; }; expect(result['$version']).toBe(3); @@ -473,67 +505,84 @@ describe('V2ToV3Migration', () => { ).toBeUndefined(); expect( (result['general'] as Record)['disableUpdateNag'], - ).toBe('invalid'); + ).toBeUndefined(); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('general.disableUpdateNag'); }); - it('should preserve object value for disable key', () => { + it('should remove object value for disable key and emit warning', () => { const v2Settings = { $version: 2, general: { disableAutoUpdate: { nested: 'value' } }, }; - const { settings: result } = migration.migrate(v2Settings, 'user') as { + const { settings: result, warnings } = migration.migrate( + v2Settings, + 'user', + ) as { settings: Record; - warnings: unknown[]; + warnings: string[]; }; expect(result['$version']).toBe(3); expect( (result['general'] as Record)['disableAutoUpdate'], - ).toEqual({ nested: 'value' }); + ).toBeUndefined(); expect( (result['general'] as Record)['enableAutoUpdate'], ).toBeUndefined(); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('general.disableAutoUpdate'); }); - it('should preserve array value for disable key', () => { + it('should remove array value for disable key and emit warning', () => { const v2Settings = { $version: 2, general: { disableAutoUpdate: [1, 2, 3] }, }; - const { settings: result } = migration.migrate(v2Settings, 'user') as { + const { settings: result, warnings } = migration.migrate( + v2Settings, + 'user', + ) as { settings: Record; - warnings: unknown[]; + warnings: string[]; }; expect(result['$version']).toBe(3); expect( (result['general'] as Record)['disableAutoUpdate'], - ).toEqual([1, 2, 3]); + ).toBeUndefined(); expect( (result['general'] as Record)['enableAutoUpdate'], ).toBeUndefined(); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('general.disableAutoUpdate'); }); - it('should preserve null value for disable key', () => { + it('should remove null value for disable key and emit warning', () => { const v2Settings = { $version: 2, general: { disableAutoUpdate: null }, }; - const { settings: result } = migration.migrate(v2Settings, 'user') as { + const { settings: result, warnings } = migration.migrate( + v2Settings, + 'user', + ) as { settings: Record; - warnings: unknown[]; + warnings: string[]; }; expect(result['$version']).toBe(3); expect( (result['general'] as Record)['disableAutoUpdate'], - ).toBeNull(); + ).toBeUndefined(); expect( (result['general'] as Record)['enableAutoUpdate'], ).toBeUndefined(); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('general.disableAutoUpdate'); }); }); diff --git a/packages/cli/src/config/migration/versions/v2-to-v3.ts b/packages/cli/src/config/migration/versions/v2-to-v3.ts index 0761af4c9..6c0133443 100644 --- a/packages/cli/src/config/migration/versions/v2-to-v3.ts +++ b/packages/cli/src/config/migration/versions/v2-to-v3.ts @@ -5,14 +5,23 @@ */ import type { SettingsMigration } from '../types.js'; +import { + deleteNestedPropertySafe, + getNestedProperty, + setNestedPropertySafe, +} from '../../../utils/settingsUtils.js'; /** * Path mapping for boolean polarity migration (V2 disable* -> V3 enable*). * * Strategy: - * - For each mapped path, only boolean inputs are transformed. + * - For each mapped path, values are normalized before migration: + * - boolean values are accepted directly + * - string values "true"/"false" (case-insensitive, trim-aware) are coerced + * - all other present values are treated as invalid * - Transformation is inversion-based: disable=true -> enable=false, disable=false -> enable=true. - * - Non-boolean values are intentionally ignored here to preserve stable compatibility. + * - Deprecated disable* keys are removed whenever present (valid or invalid). + * - Invalid values do not create enable* keys and produce warnings. */ const V2_TO_V3_BOOLEAN_MAP: Record = { 'general.disableAutoUpdate': 'general.enableAutoUpdate', @@ -31,8 +40,9 @@ const V2_TO_V3_BOOLEAN_MAP: Record = { * Current policy: * - `general.disableAutoUpdate` and `general.disableUpdateNag` both drive * `general.enableAutoUpdate`. - * - If any observed boolean source is true, target becomes false. - * - If no boolean source exists, consolidation does not emit a target value. + * - If any valid normalized source is true, target becomes false. + * - If at least one valid normalized source exists, consolidated target is emitted. + * - Invalid present values are removed and warned, and do not contribute to target calculation. */ const CONSOLIDATED_V2_PATHS: Record = { 'general.enableAutoUpdate': [ @@ -42,86 +52,34 @@ const CONSOLIDATED_V2_PATHS: Record = { }; /** - * Safe nested getter used during migration path inspection. + * Normalizes deprecated disable* values for migration. * - * Returns `undefined` when traversal cannot continue (missing key or non-object parent). + * Returns: + * - `isPresent=false` when the path does not exist + * - `isPresent=true, isValid=true` when value is boolean or coercible string + * - `isPresent=true, isValid=false` for invalid values (number/object/array/null/other strings) */ -function getNestedProperty( - obj: Record, - path: string, -): unknown { - const keys = path.split('.'); - let current: unknown = obj; - for (const key of keys) { - if (typeof current !== 'object' || current === null || !(key in current)) { - return undefined; - } - current = (current as Record)[key]; +function normalizeDisableValue(value: unknown): { + isPresent: boolean; + isValid: boolean; + booleanValue?: boolean; +} { + if (value === undefined) { + return { isPresent: false, isValid: false }; } - return current; -} - -/** - * Safe nested setter used for emitting migrated paths. - * - * Behavior: - * - Creates intermediate objects when absent. - * - Aborts write if a parent segment is a non-object (collision protection). - */ -function setNestedProperty( - obj: Record, - path: string, - value: unknown, -): void { - const keys = path.split('.'); - const lastKey = keys.pop(); - if (!lastKey) return; - - let current: Record = obj; - for (const key of keys) { - if (current[key] === undefined) { - current[key] = {}; + if (typeof value === 'boolean') { + return { isPresent: true, isValid: true, booleanValue: value }; + } + if (typeof value === 'string') { + const normalized = value.trim().toLowerCase(); + if (normalized === 'true') { + return { isPresent: true, isValid: true, booleanValue: true }; } - const next = current[key]; - if (typeof next === 'object' && next !== null) { - current = next as Record; - } else { - // Path collision with non-object, stop here - return; + if (normalized === 'false') { + return { isPresent: true, isValid: true, booleanValue: false }; } } - current[lastKey] = value; -} - -/** - * Best-effort nested delete for removing deprecated disable* keys. - * - * If traversal hits a non-object parent, deletion is skipped. - */ -function deleteNestedProperty( - obj: Record, - path: string, -): void { - const keys = path.split('.'); - const lastKey = keys.pop(); - if (!lastKey) return; - - let current: Record = obj; - for (const key of keys) { - const next = current[key]; - if (typeof next !== 'object' || next === null) { - return; - } - current = next as Record; - } - delete current[lastKey]; -} - -/** - * JSON-based deep clone used to keep `migrate()` input immutable. - */ -function deepClone(obj: T): T { - return JSON.parse(JSON.stringify(obj)); + return { isPresent: true, isValid: false }; } /** @@ -129,14 +87,14 @@ function deepClone(obj: T): T { * * Migration contract: * - Input: V2 settings object (`$version: 2`). - * - Output: `$version: 3` with boolean disable* fields migrated to enable* equivalents. + * - Output: `$version: 3` with deprecated disable* fields removed and + * valid values migrated to enable* equivalents. * * Compatibility strategy: - * - Transform only boolean-valued deprecated fields. - * - Preserve non-boolean deprecated values untouched. + * - Accept boolean values and coercible strings "true"/"false". + * - Remove invalid deprecated values (rather than preserving them). + * - Emit warnings for each removed invalid deprecated key. * - Always bump version to 3 so future loads are idempotent and skip repeated checks. - * - * This implementation mirrors stable behavior and prioritizes parity over aggressive cleanup. */ export class V2ToV3Migration implements SettingsMigration { readonly fromVersion = 2; @@ -161,34 +119,38 @@ export class V2ToV3Migration implements SettingsMigration { } /** - * Applies V2 -> V3 transformation with stable-compatible rules. + * Applies V2 -> V3 transformation with deterministic deprecated-key cleanup. * * Detailed strategy: * 1) Clone input. * 2) Process consolidated paths first: * - Inspect each source path. - * - If value is boolean, consume it (delete old key) and contribute to aggregate. - * - Emit consolidated target when at least one boolean source was consumed. + * - Normalize each present value (boolean / coercible string / invalid). + * - Always delete present deprecated source key. + * - Valid normalized values contribute to aggregate. + * - Invalid values emit warnings. + * - Emit consolidated target when at least one valid source was consumed. * 3) Process remaining one-to-one mappings: - * - For each unmapped source, if boolean -> delete old key and write inverted target. - * - If non-boolean -> keep old value unchanged. + * - For each unmapped source, normalize value. + * - If valid -> delete old key and write inverted target. + * - If invalid -> delete old key and emit warning. * 4) Set `$version = 3`. * * Guarantees: * - Input object is not mutated. - * - Boolean migration is deterministic. - * - Non-boolean legacy values are preserved for compatibility. + * - Valid migration and invalid cleanup are deterministic. + * - Deprecated disable* keys are not retained after migration. */ migrate( settings: unknown, - _scope: string, + scope: string, ): { settings: unknown; warnings: string[] } { if (typeof settings !== 'object' || settings === null) { throw new Error('Settings must be an object'); } // Deep clone to avoid mutating input - const result = deepClone(settings) as Record; + const result = structuredClone(settings) as Record; const processedPaths = new Set(); const warnings: string[] = []; @@ -200,19 +162,29 @@ export class V2ToV3Migration implements SettingsMigration { for (const oldPath of oldPaths) { const oldValue = getNestedProperty(result, oldPath); - if (typeof oldValue === 'boolean') { + const normalized = normalizeDisableValue(oldValue); + if (!normalized.isPresent) { + continue; + } + + deleteNestedPropertySafe(result, oldPath); + processedPaths.add(oldPath); + + if (normalized.isValid) { hasAnyBooleanValue = true; - if (oldValue === true) { + if (normalized.booleanValue === true) { hasAnyDisable = true; } - deleteNestedProperty(result, oldPath); - processedPaths.add(oldPath); + } else { + warnings.push( + `Removed deprecated setting '${oldPath}' from ${scope} settings because the value is invalid. Expected boolean.`, + ); } } if (hasAnyBooleanValue) { // enableAutoUpdate = !hasAnyDisable (if any disable* was true, enable should be false) - setNestedProperty(result, newPath, !hasAnyDisable); + setNestedPropertySafe(result, newPath, !hasAnyDisable); } } @@ -223,10 +195,19 @@ export class V2ToV3Migration implements SettingsMigration { } const oldValue = getNestedProperty(result, oldPath); - if (typeof oldValue === 'boolean') { - deleteNestedProperty(result, oldPath); + const normalized = normalizeDisableValue(oldValue); + if (!normalized.isPresent) { + continue; + } + + deleteNestedPropertySafe(result, oldPath); + if (normalized.isValid) { // Set new property with inverted value - setNestedProperty(result, newPath, !oldValue); + setNestedPropertySafe(result, newPath, !normalized.booleanValue); + } else { + warnings.push( + `Removed deprecated setting '${oldPath}' from ${scope} settings because the value is invalid. Expected boolean or string "true"/"false".`, + ); } } diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 072ef7743..bfc670b60 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -31,6 +31,7 @@ import { getSettingsSchema, } from './settingsSchema.js'; import { resolveEnvVarsInObject } from '../utils/envVarResolver.js'; +import { setNestedPropertySafe } from '../utils/settingsUtils.js'; import { customDeepMerge } from '../utils/deepMerge.js'; import { updateSettingsFilePreservingFormat } from '../utils/commentJson.js'; const debugLogger = createDebugLogger('SETTINGS'); @@ -124,31 +125,6 @@ export interface SettingsFile { rawJson?: string; } -function setNestedProperty( - obj: Record, - path: string, - value: unknown, -) { - const keys = path.split('.'); - const lastKey = keys.pop(); - if (!lastKey) return; - - let current: Record = obj; - for (const key of keys) { - if (current[key] === undefined) { - current[key] = {}; - } - const next = current[key]; - if (typeof next === 'object' && next !== null) { - current = next as Record; - } else { - // This path is invalid, so we stop. - return; - } - } - current[lastKey] = value; -} - function getSettingsFileKeyWarnings( settings: Record, settingsFilePath: string, @@ -333,8 +309,8 @@ export class LoadedSettings { setValue(scope: SettingScope, key: string, value: unknown): void { const settingsFile = this.forScope(scope); - setNestedProperty(settingsFile.settings, key, value); - setNestedProperty(settingsFile.originalSettings, key, value); + setNestedPropertySafe(settingsFile.settings, key, value); + setNestedPropertySafe(settingsFile.originalSettings, key, value); this._merged = this.computeMergedSettings(); saveSettings(settingsFile); } diff --git a/packages/cli/src/utils/settingsUtils.ts b/packages/cli/src/utils/settingsUtils.ts index 1bd5988eb..0effeb738 100644 --- a/packages/cli/src/utils/settingsUtils.ts +++ b/packages/cli/src/utils/settingsUtils.ts @@ -129,6 +129,13 @@ export function getNestedValue( return undefined; } +export function getNestedProperty( + obj: Record, + path: string, +): unknown { + return getNestedValue(obj, path.split('.')); +} + /** * Get the effective value for a setting, considering inheritance from higher scopes * Always returns a value (never undefined) - falls back to default if not set anywhere @@ -382,30 +389,69 @@ export function settingExistsInScope( return value !== undefined; } -/** - * Recursively sets a value in a nested object using a key path array. - */ -function setNestedValue( +export function setNestedPropertyForce( obj: Record, - path: string[], + path: string, value: unknown, -): Record { - const [first, ...rest] = path; - if (!first) { - return obj; +): void { + const keys = path.split('.'); + const lastKey = keys.pop(); + if (!lastKey) return; + + let current: Record = obj; + for (const key of keys) { + if (!current[key] || typeof current[key] !== 'object') { + current[key] = {}; + } + current = current[key] as Record; } - if (rest.length === 0) { - obj[first] = value; - return obj; + current[lastKey] = value; +} + +export function setNestedPropertySafe( + obj: Record, + path: string, + value: unknown, +): void { + const keys = path.split('.'); + const lastKey = keys.pop(); + if (!lastKey) return; + + let current: Record = obj; + for (const key of keys) { + if (current[key] === undefined) { + current[key] = {}; + } + const next = current[key]; + if (typeof next === 'object' && next !== null) { + current = next as Record; + } else { + return; + } } - if (!obj[first] || typeof obj[first] !== 'object') { - obj[first] = {}; + current[lastKey] = value; +} + +export function deleteNestedPropertySafe( + obj: Record, + path: string, +): void { + const keys = path.split('.'); + const lastKey = keys.pop(); + if (!lastKey) return; + + let current: Record = obj; + for (const key of keys) { + const next = current[key]; + if (typeof next !== 'object' || next === null) { + return; + } + current = next as Record; } - setNestedValue(obj[first] as Record, rest, value); - return obj; + delete current[lastKey]; } /** @@ -416,9 +462,8 @@ export function setPendingSettingValue( value: boolean, pendingSettings: Settings, ): Settings { - const path = key.split('.'); const newSettings = JSON.parse(JSON.stringify(pendingSettings)); - setNestedValue(newSettings, path, value); + setNestedPropertyForce(newSettings, key, value); return newSettings; } @@ -430,9 +475,8 @@ export function setPendingSettingValueAny( value: SettingsValue, pendingSettings: Settings, ): Settings { - const path = key.split('.'); const newSettings = structuredClone(pendingSettings); - setNestedValue(newSettings, path, value); + setNestedPropertyForce(newSettings, key, value); return newSettings; }