From 468db8730ae12dbe878644f40d5a151ad0758479 Mon Sep 17 00:00:00 2001 From: Wietse Venema <356014+wietsevenema@users.noreply.github.com> Date: Sat, 20 Sep 2025 16:19:11 +0200 Subject: [PATCH] fix(mcp): handle `gemini mcp add` scope correctly in home directory (#7800) Co-authored-by: Jack Wotherspoon Co-authored-by: Arya Gummadi --- packages/cli/src/commands/mcp/add.test.ts | 229 ++++++++++++++++++++++ packages/cli/src/commands/mcp/add.ts | 12 +- 2 files changed, 240 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/commands/mcp/add.test.ts b/packages/cli/src/commands/mcp/add.test.ts index fc1ffb64cd..357235b5fa 100644 --- a/packages/cli/src/commands/mcp/add.test.ts +++ b/packages/cli/src/commands/mcp/add.test.ts @@ -13,6 +13,16 @@ vi.mock('fs/promises', () => ({ writeFile: vi.fn(), })); +vi.mock('os', () => { + const homedir = vi.fn(() => '/home/user'); + return { + default: { + homedir, + }, + homedir, + }; +}); + vi.mock('../../config/settings.js', async () => { const actual = await vi.importActual('../../config/settings.js'); return { @@ -26,15 +36,20 @@ const mockedLoadSettings = loadSettings as vi.Mock; describe('mcp add command', () => { let parser: yargs.Argv; let mockSetValue: vi.Mock; + let mockConsoleError: vi.Mock; beforeEach(() => { vi.resetAllMocks(); const yargsInstance = yargs([]).command(addCommand); parser = yargsInstance; mockSetValue = vi.fn(); + mockConsoleError = vi.fn(); + vi.spyOn(console, 'error').mockImplementation(mockConsoleError); mockedLoadSettings.mockReturnValue({ forScope: () => ({ settings: {} }), setValue: mockSetValue, + workspace: { path: '/path/to/project' }, + user: { path: '/home/user' }, }); }); @@ -119,4 +134,218 @@ describe('mcp add command', () => { }, ); }); + + describe('when handling scope and directory', () => { + const serverName = 'test-server'; + const command = 'echo'; + + const setupMocks = (cwd: string, workspacePath: string) => { + vi.spyOn(process, 'cwd').mockReturnValue(cwd); + mockedLoadSettings.mockReturnValue({ + forScope: () => ({ settings: {} }), + setValue: mockSetValue, + workspace: { path: workspacePath }, + user: { path: '/home/user' }, + }); + }; + + describe('when in a project directory', () => { + beforeEach(() => { + setupMocks('/path/to/project', '/path/to/project'); + }); + + it('should use project scope by default', async () => { + await parser.parseAsync(`add ${serverName} ${command}`); + expect(mockSetValue).toHaveBeenCalledWith( + SettingScope.Workspace, + 'mcpServers', + expect.any(Object), + ); + }); + + it('should use project scope when --scope=project is used', async () => { + await parser.parseAsync(`add --scope project ${serverName} ${command}`); + expect(mockSetValue).toHaveBeenCalledWith( + SettingScope.Workspace, + 'mcpServers', + expect.any(Object), + ); + }); + + it('should use user scope when --scope=user is used', async () => { + await parser.parseAsync(`add --scope user ${serverName} ${command}`); + expect(mockSetValue).toHaveBeenCalledWith( + SettingScope.User, + 'mcpServers', + expect.any(Object), + ); + }); + }); + + describe('when in a subdirectory of a project', () => { + beforeEach(() => { + setupMocks('/path/to/project/subdir', '/path/to/project'); + }); + + it('should use project scope by default', async () => { + await parser.parseAsync(`add ${serverName} ${command}`); + expect(mockSetValue).toHaveBeenCalledWith( + SettingScope.Workspace, + 'mcpServers', + expect.any(Object), + ); + }); + }); + + describe('when in the home directory', () => { + beforeEach(() => { + setupMocks('/home/user', '/home/user'); + }); + + it('should show an error by default', async () => { + const mockProcessExit = vi + .spyOn(process, 'exit') + .mockImplementation((() => { + throw new Error('process.exit called'); + }) as (code?: number) => never); + + await expect( + parser.parseAsync(`add ${serverName} ${command}`), + ).rejects.toThrow('process.exit called'); + + expect(mockConsoleError).toHaveBeenCalledWith( + 'Error: Please use --scope user to edit settings in the home directory.', + ); + expect(mockProcessExit).toHaveBeenCalledWith(1); + expect(mockSetValue).not.toHaveBeenCalled(); + }); + + it('should show an error when --scope=project is used explicitly', async () => { + const mockProcessExit = vi + .spyOn(process, 'exit') + .mockImplementation((() => { + throw new Error('process.exit called'); + }) as (code?: number) => never); + + await expect( + parser.parseAsync(`add --scope project ${serverName} ${command}`), + ).rejects.toThrow('process.exit called'); + + expect(mockConsoleError).toHaveBeenCalledWith( + 'Error: Please use --scope user to edit settings in the home directory.', + ); + expect(mockProcessExit).toHaveBeenCalledWith(1); + expect(mockSetValue).not.toHaveBeenCalled(); + }); + + it('should use user scope when --scope=user is used', async () => { + await parser.parseAsync(`add --scope user ${serverName} ${command}`); + expect(mockSetValue).toHaveBeenCalledWith( + SettingScope.User, + 'mcpServers', + expect.any(Object), + ); + expect(mockConsoleError).not.toHaveBeenCalled(); + }); + }); + + describe('when in a subdirectory of home (not a project)', () => { + beforeEach(() => { + setupMocks('/home/user/some/dir', '/home/user/some/dir'); + }); + + it('should use project scope by default', async () => { + await parser.parseAsync(`add ${serverName} ${command}`); + expect(mockSetValue).toHaveBeenCalledWith( + SettingScope.Workspace, + 'mcpServers', + expect.any(Object), + ); + }); + + it('should write to the WORKSPACE scope, not the USER scope', async () => { + await parser.parseAsync(`add my-new-server echo`); + + // We expect setValue to be called once. + expect(mockSetValue).toHaveBeenCalledTimes(1); + + // We get the scope that setValue was called with. + const calledScope = mockSetValue.mock.calls[0][0]; + + // We assert that the scope was Workspace, not User. + expect(calledScope).toBe(SettingScope.Workspace); + }); + }); + + describe('when outside of home (not a project)', () => { + beforeEach(() => { + setupMocks('/tmp/foo', '/tmp/foo'); + }); + + it('should use project scope by default', async () => { + await parser.parseAsync(`add ${serverName} ${command}`); + expect(mockSetValue).toHaveBeenCalledWith( + SettingScope.Workspace, + 'mcpServers', + expect.any(Object), + ); + }); + }); + }); + + describe('when updating an existing server', () => { + const serverName = 'existing-server'; + const initialCommand = 'echo old'; + const updatedCommand = 'echo'; + const updatedArgs = ['new']; + + beforeEach(() => { + mockedLoadSettings.mockReturnValue({ + forScope: () => ({ + settings: { + mcpServers: { + [serverName]: { + command: initialCommand, + }, + }, + }, + }), + setValue: mockSetValue, + workspace: { path: '/path/to/project' }, + user: { path: '/home/user' }, + }); + }); + + it('should update the existing server in the project scope', async () => { + await parser.parseAsync( + `add ${serverName} ${updatedCommand} ${updatedArgs.join(' ')}`, + ); + expect(mockSetValue).toHaveBeenCalledWith( + SettingScope.Workspace, + 'mcpServers', + expect.objectContaining({ + [serverName]: expect.objectContaining({ + command: updatedCommand, + args: updatedArgs, + }), + }), + ); + }); + + it('should update the existing server in the user scope', async () => { + await parser.parseAsync( + `add --scope user ${serverName} ${updatedCommand} ${updatedArgs.join(' ')}`, + ); + expect(mockSetValue).toHaveBeenCalledWith( + SettingScope.User, + 'mcpServers', + expect.objectContaining({ + [serverName]: expect.objectContaining({ + command: updatedCommand, + args: updatedArgs, + }), + }), + ); + }); + }); }); diff --git a/packages/cli/src/commands/mcp/add.ts b/packages/cli/src/commands/mcp/add.ts index ecfc1b77c1..eb275ba104 100644 --- a/packages/cli/src/commands/mcp/add.ts +++ b/packages/cli/src/commands/mcp/add.ts @@ -36,9 +36,19 @@ async function addMcpServer( includeTools, excludeTools, } = options; + + const settings = loadSettings(process.cwd()); + const inHome = settings.workspace.path === settings.user.path; + + if (scope === 'project' && inHome) { + console.error( + 'Error: Please use --scope user to edit settings in the home directory.', + ); + process.exit(1); + } + const settingsScope = scope === 'user' ? SettingScope.User : SettingScope.Workspace; - const settings = loadSettings(); let newServer: Partial = {};