From d70ff150409e94dd3bd1b86b18819a26c94fbe56 Mon Sep 17 00:00:00 2001 From: Cynthia Long Date: Fri, 24 Apr 2026 17:03:55 +0000 Subject: [PATCH] fix(cli): preserve manually added configurations in settings.json --- packages/cli/src/commands/mcp/remove.ts | 2 +- packages/cli/src/config/settings.ts | 17 ++++---- packages/cli/src/utils/commentJson.test.ts | 46 ++++++++++++++++------ packages/cli/src/utils/commentJson.ts | 16 ++++---- 4 files changed, 54 insertions(+), 27 deletions(-) diff --git a/packages/cli/src/commands/mcp/remove.ts b/packages/cli/src/commands/mcp/remove.ts index 8c5bd1efab..2431401220 100644 --- a/packages/cli/src/commands/mcp/remove.ts +++ b/packages/cli/src/commands/mcp/remove.ts @@ -29,7 +29,7 @@ async function removeMcpServer( return; } - delete mcpServers[name]; + (mcpServers as Record)[name] = undefined; settings.setValue(settingsScope, 'mcpServers', mcpServers); diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index b84c1bda40..130737676d 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -868,14 +868,14 @@ export function migrateDeprecatedSettings( if (typeof newValue === 'boolean') { // Both exist, trust the new one if (removeDeprecated) { - delete settings[oldKey]; + settings[oldKey] = undefined; modified = true; } } else { // Only old exists, migrate to new (inverted) settings[newKey] = !oldValue; if (removeDeprecated) { - delete settings[oldKey]; + settings[oldKey] = undefined; } modified = true; } @@ -1017,8 +1017,7 @@ export function migrateDeprecatedSettings( } if (removeDeprecated) { - const newTools = { ...toolsSettings }; - delete newTools['approvalMode']; + const newTools = { ...toolsSettings, approvalMode: undefined }; loadedSettings.setValue(scope, 'tools', newTools); if (!settingsFile.readOnly) { anyModified = true; @@ -1229,10 +1228,12 @@ function migrateExperimentalSettings( loadedSettings.setValue(scope, 'agents', agentsSettings); if (removeDeprecated) { - const newExperimental = { ...experimentalSettings }; - delete newExperimental['codebaseInvestigatorSettings']; - delete newExperimental['cliHelpAgentSettings']; - delete newExperimental['plan']; + const newExperimental = { + ...experimentalSettings, + codebaseInvestigatorSettings: undefined, + cliHelpAgentSettings: undefined, + plan: undefined, + }; loadedSettings.setValue(scope, 'experimental', newExperimental); } return true; diff --git a/packages/cli/src/utils/commentJson.test.ts b/packages/cli/src/utils/commentJson.test.ts index e92b19cb63..d58a6ecf80 100644 --- a/packages/cli/src/utils/commentJson.test.ts +++ b/packages/cli/src/utils/commentJson.test.ts @@ -204,7 +204,7 @@ describe('commentJson', () => { expect(updatedContent).not.toContain('"server2"'); }); - it('should sync nested objects, removing omitted fields', () => { + it('should preserve nested objects when omitted (Bug #23138)', () => { const originalContent = `{ // Configuration "model": "gemini-2.5-pro", @@ -222,18 +222,41 @@ describe('commentJson', () => { ui: { theme: 'light', }, - preservedField: 'keep me', }); const updatedContent = fs.readFileSync(testFilePath, 'utf-8'); expect(updatedContent).toContain('// Configuration'); expect(updatedContent).toContain('"model": "gemini-2.5-flash"'); expect(updatedContent).toContain('"theme": "light"'); - expect(updatedContent).not.toContain('"existingSetting": "value"'); + // These should now be preserved + expect(updatedContent).toContain('"existingSetting": "value"'); expect(updatedContent).toContain('"preservedField": "keep me"'); }); - it('should handle mcpServers field deletion properly', () => { + it('should delete keys when explicitly set to undefined', () => { + const originalContent = `{ + "model": "gemini-2.5-pro", + "ui": { + "theme": "dark", + "deleteMe": "remove" + } + }`; + + fs.writeFileSync(testFilePath, originalContent, 'utf-8'); + + updateSettingsFilePreservingFormat(testFilePath, { + ui: { + theme: 'light', + deleteMe: undefined, + }, + }); + + const updatedContent = fs.readFileSync(testFilePath, 'utf-8'); + expect(updatedContent).toContain('"theme": "light"'); + expect(updatedContent).not.toContain('"deleteMe"'); + }); + + it('should handle mcpServers field deletion using explicit undefined', () => { const originalContent = `{ "model": "gemini-2.5-pro", "mcpServers": { @@ -259,6 +282,7 @@ describe('commentJson', () => { command: 'node', args: ['server.js'], }, + oldServer: undefined, }, }); @@ -270,7 +294,7 @@ describe('commentJson', () => { expect(updatedContent).toContain('// Server to remove'); }); - it('preserves sibling-level commented-out blocks when removing another key', () => { + it('preserves sibling-level commented-out blocks when removing another key with undefined', () => { const originalContent = `{ "mcpServers": { // "sleep": { @@ -294,7 +318,9 @@ describe('commentJson', () => { fs.writeFileSync(testFilePath, originalContent, 'utf-8'); updateSettingsFilePreservingFormat(testFilePath, { - mcpServers: {}, + mcpServers: { + playwright: undefined, + }, }); const updatedContent = fs.readFileSync(testFilePath, 'utf-8'); @@ -322,7 +348,7 @@ describe('commentJson', () => { expect(updatedContent).toContain('"item2"'); }); - it('should remove both nested and non-nested objects when omitted', () => { + it('should preserve both nested and non-nested objects when omitted', () => { const originalContent = `{ // Top-level config "topLevelObject": { @@ -356,10 +382,8 @@ describe('commentJson', () => { const updatedContent = fs.readFileSync(testFilePath, 'utf-8'); - expect(updatedContent).not.toContain('"topLevelObject"'); - - expect(updatedContent).not.toContain('"nestedObject"'); - + expect(updatedContent).toContain('"topLevelObject"'); + expect(updatedContent).toContain('"nestedObject"'); expect(updatedContent).toContain('"keepThis": "value"'); expect(updatedContent).toContain('"preservedObject"'); expect(updatedContent).toContain('"data": "keep"'); diff --git a/packages/cli/src/utils/commentJson.ts b/packages/cli/src/utils/commentJson.ts index c60011b81f..8700eedc7f 100644 --- a/packages/cli/src/utils/commentJson.ts +++ b/packages/cli/src/utils/commentJson.ts @@ -116,15 +116,17 @@ function applyKeyDiff( base: Record, desired: Record, ): void { - for (const existingKey of Object.getOwnPropertyNames(base)) { - if (!Object.prototype.hasOwnProperty.call(desired, existingKey)) { - preserveCommentsOnPropertyDeletion(base, existingKey); - delete base[existingKey]; - } - } - for (const nextKey of Object.getOwnPropertyNames(desired)) { const nextVal = desired[nextKey]; + + if (nextVal === undefined) { + if (Object.prototype.hasOwnProperty.call(base, nextKey)) { + preserveCommentsOnPropertyDeletion(base, nextKey); + delete base[nextKey]; + } + continue; + } + const baseVal = base[nextKey]; const isObj =