diff --git a/packages/cli/src/commands/mcp/remove.test.ts b/packages/cli/src/commands/mcp/remove.test.ts index eb7dedce50..3803a0b77c 100644 --- a/packages/cli/src/commands/mcp/remove.test.ts +++ b/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; + describe('unit tests with mocks', () => { + let parser: yargs.Argv; + let mockSetValue: vi.Mock; + let mockSettings: Record; - 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 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; - 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(); + }); }); }); diff --git a/packages/cli/src/utils/commentJson.test.ts b/packages/cli/src/utils/commentJson.test.ts index 4957b74973..4186a82802 100644 --- a/packages/cli/src/utils/commentJson.test.ts +++ b/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'); + }); }); }); diff --git a/packages/cli/src/utils/commentJson.ts b/packages/cli/src/utils/commentJson.ts index 9ea4d3f802..05b3e8dab0 100644 --- a/packages/cli/src/utils/commentJson.ts +++ b/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; + /** * 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, + 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, + 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]; + 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, + nextVal as Record, + ); + } 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, updates: Record, ): Record { - 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, - value as Record, - ); - } else { - result[key] = value; - } - } - - return result; + // Apply sync-by-omission semantics consistently at all levels + applyKeyDiff(current, updates); + return current; }