fix(mcp): fix MCP server removal not persisting to settings (#10098)

Co-authored-by: Jacob Richman <jacob314@gmail.com>

packages/cli/src/commands/mcp/remove.test.ts

@@ -4,66 +4,214 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import { vi, describe, it, expect, beforeEach } from 'vitest';
+import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest';
 import yargs from 'yargs';
-import { loadSettings, SettingScope } from '../../config/settings.js';
+import { SettingScope, type LoadedSettings } from '../../config/settings.js';
 import { removeCommand } from './remove.js';
+import * as fs from 'node:fs';
+import * as path from 'node:path';
+import * as os from 'node:os';
 
 vi.mock('fs/promises', () => ({
   readFile: vi.fn(),
   writeFile: vi.fn(),
 }));
 
-vi.mock('../../config/settings.js', async () => {
-  const actual = await vi.importActual('../../config/settings.js');
-  return {
-    ...actual,
-    loadSettings: vi.fn(),
-  };
-});
-
-const mockedLoadSettings = loadSettings as vi.Mock;
-
 describe('mcp remove command', () => {
-  let parser: yargs.Argv;
-  let mockSetValue: vi.Mock;
-  let mockSettings: Record<string, unknown>;
+  describe('unit tests with mocks', () => {
+    let parser: yargs.Argv;
+    let mockSetValue: vi.Mock;
+    let mockSettings: Record<string, unknown>;
 
-  beforeEach(() => {
-    vi.resetAllMocks();
-    const yargsInstance = yargs([]).command(removeCommand);
-    parser = yargsInstance;
-    mockSetValue = vi.fn();
-    mockSettings = {
-      mcpServers: {
-        'test-server': {
-          command: 'echo "hello"',
+    beforeEach(async () => {
+      vi.resetAllMocks();
+
+      mockSetValue = vi.fn();
+      mockSettings = {
+        mcpServers: {
+          'test-server': {
+            command: 'echo "hello"',
+          },
         },
-      },
-    };
-    mockedLoadSettings.mockReturnValue({
-      forScope: () => ({ settings: mockSettings }),
-      setValue: mockSetValue,
+      };
+
+      vi.spyOn(
+        await import('../../config/settings.js'),
+        'loadSettings',
+      ).mockReturnValue({
+        forScope: () => ({ settings: mockSettings }),
+        setValue: mockSetValue,
+      } as Partial<LoadedSettings> as LoadedSettings);
+
+      const yargsInstance = yargs([]).command(removeCommand);
+      parser = yargsInstance;
+    });
+
+    it('should remove a server from project settings', async () => {
+      await parser.parseAsync('remove test-server');
+
+      expect(mockSetValue).toHaveBeenCalledWith(
+        SettingScope.Workspace,
+        'mcpServers',
+        {},
+      );
+    });
+
+    it('should show a message if server not found', async () => {
+      const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
+      await parser.parseAsync('remove non-existent-server');
+
+      expect(mockSetValue).not.toHaveBeenCalled();
+      expect(consoleSpy).toHaveBeenCalledWith(
+        'Server "non-existent-server" not found in project settings.',
+      );
     });
   });
 
-  it('should remove a server from project settings', async () => {
-    await parser.parseAsync('remove test-server');
+  describe('integration tests with real file I/O', () => {
+    let tempDir: string;
+    let settingsDir: string;
+    let settingsPath: string;
+    let parser: yargs.Argv;
+    let cwdSpy: ReturnType<typeof vi.spyOn>;
 
-    expect(mockSetValue).toHaveBeenCalledWith(
-      SettingScope.Workspace,
-      'mcpServers',
-      {},
-    );
-  });
+    beforeEach(() => {
+      vi.resetAllMocks();
+      vi.restoreAllMocks();
 
-  it('should show a message if server not found', async () => {
-    const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
-    await parser.parseAsync('remove non-existent-server');
+      tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'mcp-remove-test-'));
+      settingsDir = path.join(tempDir, '.gemini');
+      settingsPath = path.join(settingsDir, 'settings.json');
+      fs.mkdirSync(settingsDir, { recursive: true });
 
-    expect(mockSetValue).not.toHaveBeenCalled();
-    expect(consoleSpy).toHaveBeenCalledWith(
-      'Server "non-existent-server" not found in project settings.',
-    );
+      cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(tempDir);
+
+      parser = yargs([]).command(removeCommand);
+    });
+
+    afterEach(() => {
+      cwdSpy.mockRestore();
+
+      if (fs.existsSync(tempDir)) {
+        fs.rmSync(tempDir, { recursive: true, force: true });
+      }
+    });
+
+    it('should actually remove a server from the settings file', async () => {
+      const originalContent = `{
+        "mcpServers": {
+          "server-to-keep": {
+            "command": "node",
+            "args": ["keep.js"]
+          },
+          "server-to-remove": {
+            "command": "node",
+            "args": ["remove.js"]
+          }
+        }
+      }`;
+      fs.writeFileSync(settingsPath, originalContent, 'utf-8');
+
+      const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
+      await parser.parseAsync('remove server-to-remove');
+
+      const updatedContent = fs.readFileSync(settingsPath, 'utf-8');
+      expect(updatedContent).toContain('"server-to-keep"');
+      expect(updatedContent).not.toContain('"server-to-remove"');
+
+      expect(consoleSpy).toHaveBeenCalledWith(
+        'Server "server-to-remove" removed from project settings.',
+      );
+
+      consoleSpy.mockRestore();
+    });
+
+    it('should preserve comments when removing a server', async () => {
+      const originalContent = `{
+        "mcpServers": {
+          // Server to keep
+          "context7": {
+            "command": "node",
+            "args": ["server.js"]
+          },
+          // Server to remove
+          "oldServer": {
+            "command": "old",
+            "args": ["old.js"]
+          }
+        }
+      }`;
+      fs.writeFileSync(settingsPath, originalContent, 'utf-8');
+
+      const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
+      await parser.parseAsync('remove oldServer');
+
+      const updatedContent = fs.readFileSync(settingsPath, 'utf-8');
+      expect(updatedContent).toContain('// Server to keep');
+      expect(updatedContent).toContain('"context7"');
+      expect(updatedContent).not.toContain('"oldServer"');
+      expect(updatedContent).toContain('// Server to remove');
+
+      consoleSpy.mockRestore();
+    });
+
+    it('should handle removing the only server', async () => {
+      const originalContent = `{
+        "mcpServers": {
+          "only-server": {
+            "command": "node",
+            "args": ["server.js"]
+          }
+        }
+      }`;
+      fs.writeFileSync(settingsPath, originalContent, 'utf-8');
+
+      const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
+      await parser.parseAsync('remove only-server');
+
+      const updatedContent = fs.readFileSync(settingsPath, 'utf-8');
+      expect(updatedContent).toContain('"mcpServers"');
+      expect(updatedContent).not.toContain('"only-server"');
+      expect(updatedContent).toMatch(/"mcpServers"\s*:\s*\{\s*\}/);
+
+      consoleSpy.mockRestore();
+    });
+
+    it('should preserve other settings when removing a server', async () => {
+      // Create settings file with other settings
+      // Note: "model" will be migrated to "model": { "name": ... } format
+      const originalContent = `{
+        "model": {
+          "name": "gemini-2.5-pro"
+        },
+        "mcpServers": {
+          "server1": {
+            "command": "node",
+            "args": ["s1.js"]
+          },
+          "server2": {
+            "command": "node",
+            "args": ["s2.js"]
+          }
+        },
+        "ui": {
+          "theme": "dark"
+        }
+      }`;
+      fs.writeFileSync(settingsPath, originalContent, 'utf-8');
+
+      const consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
+      await parser.parseAsync('remove server1');
+
+      const updatedContent = fs.readFileSync(settingsPath, 'utf-8');
+      expect(updatedContent).toContain('"model"');
+      expect(updatedContent).toContain('"gemini-2.5-pro"');
+      expect(updatedContent).toContain('"server2"');
+      expect(updatedContent).toContain('"ui"');
+      expect(updatedContent).toContain('"theme": "dark"');
+      expect(updatedContent).not.toContain('"server1"');
+
+      consoleSpy.mockRestore();
+    });
   });
 });

packages/cli/src/utils/commentJson.test.ts

@@ -42,6 +42,9 @@ describe('commentJson', () => {
 
       updateSettingsFilePreservingFormat(testFilePath, {
         model: 'gemini-2.5-flash',
+        ui: {
+          theme: 'dark',
+        },
       });
 
       const updatedContent = fs.readFileSync(testFilePath, 'utf-8');
@@ -178,5 +181,191 @@ describe('commentJson', () => {
 
       consoleSpy.mockRestore();
     });
+
+    it('should handle array updates while preserving comments', () => {
+      const originalContent = `{
+        // Server configurations
+        "servers": [
+          // First server
+          "server1",
+          "server2" // Second server
+        ]
+      }`;
+
+      fs.writeFileSync(testFilePath, originalContent, 'utf-8');
+
+      updateSettingsFilePreservingFormat(testFilePath, {
+        servers: ['server1', 'server3'],
+      });
+
+      const updatedContent = fs.readFileSync(testFilePath, 'utf-8');
+      expect(updatedContent).toContain('// Server configurations');
+      expect(updatedContent).toContain('"server1"');
+      expect(updatedContent).toContain('"server3"');
+      expect(updatedContent).not.toContain('"server2"');
+    });
+
+    it('should sync nested objects, removing omitted fields', () => {
+      const originalContent = `{
+        // Configuration
+        "model": "gemini-2.5-pro",
+        "ui": {
+          "theme": "dark",
+          "existingSetting": "value"
+        },
+        "preservedField": "keep me"
+      }`;
+
+      fs.writeFileSync(testFilePath, originalContent, 'utf-8');
+
+      updateSettingsFilePreservingFormat(testFilePath, {
+        model: 'gemini-2.5-flash',
+        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"');
+      expect(updatedContent).toContain('"preservedField": "keep me"');
+    });
+
+    it('should handle mcpServers field deletion properly', () => {
+      const originalContent = `{
+        "model": "gemini-2.5-pro",
+        "mcpServers": {
+          // Server to keep
+          "context7": {
+            "command": "node",
+            "args": ["server.js"]
+          },
+          // Server to remove
+          "oldServer": {
+            "command": "old",
+            "args": ["old.js"]
+          }
+        }
+      }`;
+
+      fs.writeFileSync(testFilePath, originalContent, 'utf-8');
+
+      updateSettingsFilePreservingFormat(testFilePath, {
+        model: 'gemini-2.5-pro',
+        mcpServers: {
+          context7: {
+            command: 'node',
+            args: ['server.js'],
+          },
+        },
+      });
+
+      const updatedContent = fs.readFileSync(testFilePath, 'utf-8');
+      expect(updatedContent).toContain('// Server to keep');
+      expect(updatedContent).toContain('"context7"');
+      expect(updatedContent).not.toContain('"oldServer"');
+      // The comment for the removed server should still be preserved
+      expect(updatedContent).toContain('// Server to remove');
+    });
+
+    it('preserves sibling-level commented-out blocks when removing another key', () => {
+      const originalContent = `{
+        "mcpServers": {
+          // "sleep": {
+          //   "command": "node",
+          //   "args": [
+          //     "/Users/testUser/test-mcp-server/sleep-mcp/build/index.js"
+          //   ],
+          //   "timeout": 300000
+          // },
+          "playwright": {
+            "command": "npx",
+            "args": [
+              "@playwright/mcp@latest",
+              "--headless",
+              "--isolated"
+            ]
+          }
+        }
+      }`;
+
+      fs.writeFileSync(testFilePath, originalContent, 'utf-8');
+
+      updateSettingsFilePreservingFormat(testFilePath, {
+        mcpServers: {},
+      });
+
+      const updatedContent = fs.readFileSync(testFilePath, 'utf-8');
+      expect(updatedContent).toContain('// "sleep": {');
+      expect(updatedContent).toContain('"mcpServers"');
+      expect(updatedContent).not.toContain('"playwright"');
+    });
+
+    it('should handle type conversion from object to array', () => {
+      const originalContent = `{
+        "data": {
+          "key": "value"
+        }
+      }`;
+
+      fs.writeFileSync(testFilePath, originalContent, 'utf-8');
+
+      updateSettingsFilePreservingFormat(testFilePath, {
+        data: ['item1', 'item2'],
+      });
+
+      const updatedContent = fs.readFileSync(testFilePath, 'utf-8');
+      expect(updatedContent).toContain('"data": [');
+      expect(updatedContent).toContain('"item1"');
+      expect(updatedContent).toContain('"item2"');
+    });
+
+    it('should remove both nested and non-nested objects when omitted', () => {
+      const originalContent = `{
+        // Top-level config
+        "topLevelObject": {
+          "field1": "value1",
+          "field2": "value2"
+        },
+        // Parent object
+        "parent": {
+          "nestedObject": {
+            "nestedField1": "value1",
+            "nestedField2": "value2"
+          },
+          "keepThis": "value"
+        },
+        // This should be preserved
+        "preservedObject": {
+          "data": "keep"
+        }
+      }`;
+
+      fs.writeFileSync(testFilePath, originalContent, 'utf-8');
+
+      updateSettingsFilePreservingFormat(testFilePath, {
+        parent: {
+          keepThis: 'value',
+        },
+        preservedObject: {
+          data: 'keep',
+        },
+      });
+
+      const updatedContent = fs.readFileSync(testFilePath, 'utf-8');
+
+      expect(updatedContent).not.toContain('"topLevelObject"');
+
+      expect(updatedContent).not.toContain('"nestedObject"');
+
+      expect(updatedContent).toContain('"keepThis": "value"');
+      expect(updatedContent).toContain('"preservedObject"');
+      expect(updatedContent).toContain('"data": "keep"');
+
+      expect(updatedContent).toContain('// This should be preserved');
+    });
   });
 });

packages/cli/src/utils/commentJson.ts

@@ -7,6 +7,11 @@
 import * as fs from 'node:fs';
 import { parse, stringify } from 'comment-json';
 
+/**
+ * Type representing an object that may contain Symbol keys for comments.
+ */
+type CommentedRecord = Record<string | symbol, unknown>;
+
 /**
  * Updates a JSON file while preserving comments and formatting.
  */
@@ -38,30 +43,120 @@ export function updateSettingsFilePreservingFormat(
   fs.writeFileSync(filePath, updatedContent, 'utf-8');
 }
 
+/**
+ * When deleting a property from a comment-json parsed object, relocate any
+ * leading/trailing comments that were attached to that property so they are not lost.
+ *
+ * This function re-attaches comments to the next sibling's leading comments if
+ * available, otherwise to the previous sibling's trailing comments, otherwise
+ * to the container's leading/trailing comments.
+ */
+function preserveCommentsOnPropertyDeletion(
+  container: Record<string, unknown>,
+  propName: string,
+): void {
+  const target = container as CommentedRecord;
+  const beforeSym = Symbol.for(`before:${propName}`);
+  const afterSym = Symbol.for(`after:${propName}`);
+
+  const beforeComments = target[beforeSym] as unknown[] | undefined;
+  const afterComments = target[afterSym] as unknown[] | undefined;
+
+  if (!beforeComments && !afterComments) return;
+
+  const keys = Object.getOwnPropertyNames(container);
+  const idx = keys.indexOf(propName);
+  const nextKey = idx >= 0 && idx + 1 < keys.length ? keys[idx + 1] : undefined;
+  const prevKey = idx > 0 ? keys[idx - 1] : undefined;
+
+  function appendToSymbol(destSym: symbol, comments: unknown[]) {
+    if (!comments || comments.length === 0) return;
+    const existing = target[destSym];
+    target[destSym] = Array.isArray(existing)
+      ? existing.concat(comments)
+      : comments;
+  }
+
+  if (beforeComments && beforeComments.length > 0) {
+    if (nextKey) {
+      appendToSymbol(Symbol.for(`before:${nextKey}`), beforeComments);
+    } else if (prevKey) {
+      appendToSymbol(Symbol.for(`after:${prevKey}`), beforeComments);
+    } else {
+      appendToSymbol(Symbol.for('before'), beforeComments);
+    }
+    delete target[beforeSym];
+  }
+
+  if (afterComments && afterComments.length > 0) {
+    if (nextKey) {
+      appendToSymbol(Symbol.for(`before:${nextKey}`), afterComments);
+    } else if (prevKey) {
+      appendToSymbol(Symbol.for(`after:${prevKey}`), afterComments);
+    } else {
+      appendToSymbol(Symbol.for('after'), afterComments);
+    }
+    delete target[afterSym];
+  }
+}
+
+/**
+ * Applies sync-by-omission semantics: synchronizes base to match desired.
+ * - Adds/updates keys from desired
+ * - Removes keys from base that are not in desired
+ * - Recursively applies to nested objects
+ * - Preserves comments when deleting keys
+ */
+function applyKeyDiff(
+  base: Record<string, unknown>,
+  desired: Record<string, unknown>,
+): 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];
+    const baseVal = base[nextKey];
+
+    const isObj =
+      typeof nextVal === 'object' &&
+      nextVal !== null &&
+      !Array.isArray(nextVal);
+    const isBaseObj =
+      typeof baseVal === 'object' &&
+      baseVal !== null &&
+      !Array.isArray(baseVal);
+    const isArr = Array.isArray(nextVal);
+    const isBaseArr = Array.isArray(baseVal);
+
+    if (isObj && isBaseObj) {
+      applyKeyDiff(
+        baseVal as Record<string, unknown>,
+        nextVal as Record<string, unknown>,
+      );
+    } else if (isArr && isBaseArr) {
+      // In-place mutate arrays to preserve array-level comments on CommentArray
+      const baseArr = baseVal as unknown[];
+      const desiredArr = nextVal as unknown[];
+      baseArr.length = 0;
+      for (const el of desiredArr) {
+        baseArr.push(el);
+      }
+    } else {
+      base[nextKey] = nextVal;
+    }
+  }
+}
+
 function applyUpdates(
   current: Record<string, unknown>,
   updates: Record<string, unknown>,
 ): Record<string, unknown> {
-  const result = current;
-
-  for (const key of Object.getOwnPropertyNames(updates)) {
-    const value = updates[key];
-    if (
-      typeof value === 'object' &&
-      value !== null &&
-      !Array.isArray(value) &&
-      typeof result[key] === 'object' &&
-      result[key] !== null &&
-      !Array.isArray(result[key])
-    ) {
-      result[key] = applyUpdates(
-        result[key] as Record<string, unknown>,
-        value as Record<string, unknown>,
-      );
-    } else {
-      result[key] = value;
-    }
-  }
-
-  return result;
+  // Apply sync-by-omission semantics consistently at all levels
+  applyKeyDiff(current, updates);
+  return current;
 }