diff --git a/scripts/setup_environment.py b/scripts/setup_environment.py index be7fbae..6226052 100644 --- a/scripts/setup_environment.py +++ b/scripts/setup_environment.py @@ -4079,12 +4079,38 @@ def configure_mcp_server(server: dict[str, Any]) -> bool: # take precedence, followed by project, then user - so we remove from all scopes info(f'Removing existing MCP server {name} from all scopes if present...') scopes_removed: list[str] = [] - for remove_scope in ['user', 'local', 'project']: - remove_cmd = [str(claude_cmd), 'mcp', 'remove', '--scope', remove_scope, name] - result = run_command(remove_cmd, capture_output=True) - # Check if removal was successful (exit code 0) - if result.returncode == 0: - scopes_removed.append(remove_scope) + + if system == 'Windows': + # Windows: Use bash execution for consistency with add operation + # This ensures removal uses the same environment (PATH, shell, MSYS settings) + # as the add operation, preventing "not found" errors due to asymmetric execution + nodejs_path = r'C:\Program Files\nodejs' + current_path = os.environ.get('PATH', '') + + if Path(nodejs_path).exists() and nodejs_path not in current_path: + windows_explicit_path = f'{nodejs_path};{current_path}' + else: + windows_explicit_path = current_path + + unix_explicit_path = convert_path_env_to_unix(windows_explicit_path) + bash_preferred_cmd = get_bash_preferred_command(str(claude_cmd)) + unix_claude_cmd = convert_to_unix_path(bash_preferred_cmd) + + for remove_scope in ['user', 'local', 'project']: + bash_cmd = ( + f'export PATH="{unix_explicit_path}:$PATH" && ' + f'"{unix_claude_cmd}" mcp remove --scope {remove_scope} {name}' + ) + result = run_bash_command(bash_cmd, capture_output=True, login_shell=True) + if result.returncode == 0: + scopes_removed.append(remove_scope) + else: + # Unix: Direct subprocess execution + for remove_scope in ['user', 'local', 'project']: + remove_cmd = [str(claude_cmd), 'mcp', 'remove', '--scope', remove_scope, name] + result = run_command(remove_cmd, capture_output=True) + if result.returncode == 0: + scopes_removed.append(remove_scope) if scopes_removed: info(f"Removed MCP server {name} from scope(s): {', '.join(scopes_removed)}") @@ -4263,7 +4289,7 @@ def configure_mcp_server(server: dict[str, Any]) -> bool: def configure_all_mcp_servers( servers: list[dict[str, Any]], profile_mcp_config_path: Path | None = None, -) -> tuple[bool, list[dict[str, Any]]]: +) -> tuple[bool, list[dict[str, Any]], dict[str, int]]: """Configure all MCP servers from configuration. Handles combined scope configurations where servers can be added to multiple @@ -4276,17 +4302,28 @@ def configure_all_mcp_servers( profile_mcp_config_path: Path for profile-scoped servers JSON file Returns: - Tuple of (success: bool, profile_servers: list of servers with profile scope) + Tuple of (success: bool, profile_servers: list, stats: dict) + stats contains: + - global_count: Number of servers with any non-profile scope + - profile_count: Number of servers with profile scope + - combined_count: Number of servers with BOTH global AND profile scopes """ if not servers: info('No MCP servers to configure') - return True, [] + return True, [], {'global_count': 0, 'profile_count': 0, 'combined_count': 0} info('Configuring MCP servers...') # Collect servers for profile config profile_servers: list[dict[str, Any]] = [] + # Track statistics for accurate summary display + stats = { + 'global_count': 0, # Servers with any non-profile scope + 'profile_count': 0, # Servers with profile scope + 'combined_count': 0, # Servers with BOTH global AND profile scopes + } + for server in servers: server_name = server.get('name', 'unnamed') scope_value = server.get('scope', 'user') @@ -4299,6 +4336,15 @@ def configure_all_mcp_servers( has_profile = 'profile' in scopes non_profile_scopes = [s for s in scopes if s != 'profile'] + has_global = len(non_profile_scopes) > 0 + + # Update statistics + if has_profile: + stats['profile_count'] += 1 + if has_global: + stats['global_count'] += 1 + if has_profile and has_global: + stats['combined_count'] += 1 # Add to profile config if profile scope present if has_profile: @@ -4323,7 +4369,7 @@ def configure_all_mcp_servers( except OSError as e: warning(f'Failed to remove stale profile MCP config: {e}') - return True, profile_servers + return True, profile_servers, stats def create_mcp_config_file( @@ -5808,7 +5854,7 @@ def main() -> None: if primary_command_name: profile_mcp_config_path = claude_user_dir / f'{primary_command_name}-mcp.json' - _, profile_servers = configure_all_mcp_servers(mcp_servers, profile_mcp_config_path) + _, profile_servers, mcp_stats = configure_all_mcp_servers(mcp_servers, profile_mcp_config_path) has_profile_mcp_servers = len(profile_servers) > 0 # Check if command creation is needed @@ -5893,10 +5939,14 @@ def main() -> None: print(' * System prompt: replacing default') if model: print(f' * Model: {model}') - if profile_servers: - global_count = len(mcp_servers) - len(profile_servers) - profile_count = len(profile_servers) - print(f' * MCP servers: {global_count} global, {profile_count} profile-scoped (isolated)') + if mcp_stats['combined_count'] > 0: + # Servers with BOTH global AND profile scope + print(f" * MCP servers: {mcp_stats['global_count']} global " + f"({mcp_stats['combined_count']} also in profile)") + elif profile_servers: + # Servers with EITHER global OR profile scope (mutually exclusive) + print(f" * MCP servers: {mcp_stats['global_count']} global, " + f"{mcp_stats['profile_count']} profile-scoped (isolated)") else: print(f' * MCP servers: {len(mcp_servers)} configured') if permissions: diff --git a/tests/test_setup_environment.py b/tests/test_setup_environment.py index a65bf0e..f32a9ca 100644 --- a/tests/test_setup_environment.py +++ b/tests/test_setup_environment.py @@ -2492,7 +2492,7 @@ def test_main_success( mock_install.return_value = True mock_deps.return_value = True mock_download.return_value = True - mock_mcp.return_value = (True, []) + mock_mcp.return_value = (True, [], {'global_count': 0, 'profile_count': 0, 'combined_count': 0}) mock_settings.return_value = True mock_launcher.return_value = Path('/tmp/launcher.sh') mock_register.return_value = True @@ -3152,7 +3152,7 @@ def test_command_names_single( mock_install.return_value = True mock_deps.return_value = True mock_download.return_value = True - mock_mcp.return_value = (True, []) + mock_mcp.return_value = (True, [], {'global_count': 0, 'profile_count': 0, 'combined_count': 0}) mock_settings.return_value = True mock_launcher.return_value = Path('/tmp/launcher.sh') mock_register.return_value = True @@ -3211,7 +3211,7 @@ def test_command_names_multiple( mock_install.return_value = True mock_deps.return_value = True mock_download.return_value = True - mock_mcp.return_value = (True, []) + mock_mcp.return_value = (True, [], {'global_count': 0, 'profile_count': 0, 'combined_count': 0}) mock_settings.return_value = True mock_launcher.return_value = Path('/tmp/launcher.sh') mock_register.return_value = True @@ -3271,7 +3271,7 @@ def test_command_name_deprecated_shows_warning( mock_install.return_value = True mock_deps.return_value = True mock_download.return_value = True - mock_mcp.return_value = (True, []) + mock_mcp.return_value = (True, [], {'global_count': 0, 'profile_count': 0, 'combined_count': 0}) mock_settings.return_value = True mock_launcher.return_value = Path('/tmp/launcher.sh') mock_register.return_value = True @@ -3337,7 +3337,7 @@ def test_command_names_takes_precedence_over_deprecated( mock_install.return_value = True mock_deps.return_value = True mock_download.return_value = True - mock_mcp.return_value = (True, []) + mock_mcp.return_value = (True, [], {'global_count': 0, 'profile_count': 0, 'combined_count': 0}) mock_settings.return_value = True mock_launcher.return_value = Path('/tmp/launcher.sh') mock_register.return_value = True diff --git a/tests/test_setup_environment_additional.py b/tests/test_setup_environment_additional.py index ec5dc3f..4a31dbc 100644 --- a/tests/test_setup_environment_additional.py +++ b/tests/test_setup_environment_additional.py @@ -587,9 +587,9 @@ def test_configure_mcp_server_find_in_npm_path(self, mock_find, mock_system): result = setup_environment.configure_mcp_server(server) assert result is True - # run_command: 3 removes; run_bash_command: 1 add (Windows HTTP uses bash) - assert mock_run.call_count == 3 - assert mock_bash.call_count == 1 + # Windows: all operations use bash (3 removes + 1 add) + assert mock_run.call_count == 0 + assert mock_bash.call_count == 4 @patch('platform.system', return_value='Linux') @patch('setup_environment.find_command_robust', return_value='claude') @@ -687,27 +687,29 @@ def test_configure_mcp_server_windows_npx_command(self, mock_run, mock_find, moc result = setup_environment.configure_mcp_server(server) assert result is True - # Should call run_command 3 times for removing from all scopes - assert mock_run.call_count == 3 - # Should call run_bash_command once for add (Windows STDIO uses bash) - assert mock_bash.call_count == 1 - # Check that cmd /c was used for npx in the bash command + # Windows: all operations use bash (3 removes + 1 add) + assert mock_run.call_count == 0 + assert mock_bash.call_count == 4 + # Check that cmd /c was used for npx in the add command (last bash call) bash_cmd = mock_bash.call_args[0][0] assert 'cmd /c npx @modelcontextprotocol/server-memory' in bash_cmd assert 'export PATH=' in bash_cmd + @patch('platform.system', return_value='Linux') @patch('setup_environment.find_command_robust', return_value='claude') - def test_configure_mcp_server_exception(self, mock_find): + def test_configure_mcp_server_exception(self, mock_find, _mock_system): """Test MCP configuration with exception.""" del mock_find # Unused but required for patch + del _mock_system # Unused but required for patch server = {'name': 'test', 'command': 'test'} - # Exception happens on the add command (second call) + # Exception happens during processing (after some remove calls) with patch( 'setup_environment.run_command', side_effect=[ - subprocess.CompletedProcess([], 0, '', ''), # remove succeeds - Exception('Unexpected'), # add throws exception + subprocess.CompletedProcess([], 0, '', ''), # remove user succeeds + subprocess.CompletedProcess([], 0, '', ''), # remove local succeeds + Exception('Unexpected'), # remove project throws exception ], ): result = setup_environment.configure_mcp_server(server) @@ -839,11 +841,11 @@ def test_configure_mcp_server_http_with_env_list(self, mock_run, mock_bash, mock result = setup_environment.configure_mcp_server(server) assert result is True - # run_command: 3 removes; run_bash_command: 1 add (Windows HTTP uses bash) - assert mock_run.call_count == 3 - assert mock_bash.call_count == 1 + # Windows: all operations use bash (3 removes + 1 add) + assert mock_run.call_count == 0 + assert mock_bash.call_count == 4 - # Check bash command contains env flags + # Check bash command contains env flags (last bash call is add) bash_cmd = mock_bash.call_args[0][0] assert '--env "AUTH_TOKEN=token123"' in bash_cmd assert '--env "REGION=us-west"' in bash_cmd @@ -1339,7 +1341,10 @@ def test_main_skip_install_claude_not_found(self, mock_find, mock_load): @patch('setup_environment.process_resources', return_value=True) @patch('setup_environment.handle_resource', return_value=True) @patch('setup_environment.is_admin', return_value=True) - @patch('setup_environment.configure_all_mcp_servers', return_value=(True, [])) + @patch( + 'setup_environment.configure_all_mcp_servers', + return_value=(True, [], {'global_count': 0, 'profile_count': 0, 'combined_count': 0}), + ) @patch('setup_environment.create_additional_settings', return_value=True) @patch('setup_environment.create_launcher_script', return_value=None) @patch('pathlib.Path.mkdir') @@ -1388,7 +1393,10 @@ def test_main_no_launcher_created( @patch('setup_environment.install_claude', return_value=True) @patch('setup_environment.install_dependencies', return_value=True) @patch('setup_environment.process_resources', return_value=True) - @patch('setup_environment.configure_all_mcp_servers', return_value=(True, [])) + @patch( + 'setup_environment.configure_all_mcp_servers', + return_value=(True, [], {'global_count': 0, 'profile_count': 0, 'combined_count': 0}), + ) @patch('setup_environment.create_additional_settings', return_value=True) @patch('setup_environment.create_launcher_script') @patch('setup_environment.register_global_command', return_value=True) @@ -1433,7 +1441,10 @@ def test_main_from_env_variable( @patch('setup_environment.install_dependencies', return_value=True) @patch('setup_environment.process_resources', return_value=True) @patch('setup_environment.handle_resource', return_value=True) - @patch('setup_environment.configure_all_mcp_servers', return_value=(True, [])) + @patch( + 'setup_environment.configure_all_mcp_servers', + return_value=(True, [], {'global_count': 0, 'profile_count': 0, 'combined_count': 0}), + ) @patch('setup_environment.create_additional_settings', return_value=True) @patch('setup_environment.create_launcher_script') @patch('setup_environment.register_global_command', return_value=True) @@ -2731,7 +2742,7 @@ def test_configure_all_mcp_servers_separates_profile( with tempfile.TemporaryDirectory() as tmpdir: profile_config = Path(tmpdir) / 'test-mcp.json' - success, profile_servers = setup_environment.configure_all_mcp_servers( + success, profile_servers, _ = setup_environment.configure_all_mcp_servers( servers, profile_mcp_config_path=profile_config, ) @@ -2827,7 +2838,7 @@ def test_configure_all_mcp_servers_no_profile_path(self) -> None: # Without profile_mcp_config_path, profile servers should still be returned # but no config file created with patch('setup_environment.configure_mcp_server', return_value=True): - success, profile_servers = setup_environment.configure_all_mcp_servers(servers) + success, profile_servers, _ = setup_environment.configure_all_mcp_servers(servers) assert success is True assert len(profile_servers) == 1 @@ -2856,7 +2867,7 @@ def test_configure_all_mcp_servers_mixed_scopes( with tempfile.TemporaryDirectory() as tmpdir: profile_config = Path(tmpdir) / 'test-mcp.json' - success, profile_servers = setup_environment.configure_all_mcp_servers( + success, profile_servers, _ = setup_environment.configure_all_mcp_servers( servers, profile_mcp_config_path=profile_config, ) @@ -2885,7 +2896,7 @@ def test_stale_mcp_config_file_removed_when_no_profile_servers(self) -> None: ] with patch('setup_environment.configure_mcp_server', return_value=True): - success, profile_servers = setup_environment.configure_all_mcp_servers( + success, profile_servers, _ = setup_environment.configure_all_mcp_servers( servers, config_path, ) @@ -2901,7 +2912,7 @@ def test_mcp_config_file_not_touched_when_path_none(self) -> None: ] with patch('setup_environment.configure_mcp_server', return_value=True): - success, profile_servers = setup_environment.configure_all_mcp_servers( + success, profile_servers, _ = setup_environment.configure_all_mcp_servers( servers, None, # No profile config path ) @@ -2925,7 +2936,7 @@ def test_stale_mcp_config_removal_failure_logged_as_warning(self) -> None: patch.object(Path, 'unlink', side_effect=OSError('Permission denied')), patch('setup_environment.warning') as mock_warning, ): - success, profile_servers = setup_environment.configure_all_mcp_servers( + success, profile_servers, _ = setup_environment.configure_all_mcp_servers( servers, config_path, ) @@ -3162,7 +3173,7 @@ def test_combined_scope_adds_to_both_locations( with tempfile.TemporaryDirectory() as tmpdir: profile_config = Path(tmpdir) / 'test-mcp.json' - success_flag, profile_servers = setup_environment.configure_all_mcp_servers( + success_flag, profile_servers, _ = setup_environment.configure_all_mcp_servers( servers, profile_mcp_config_path=profile_config, ) @@ -3204,7 +3215,7 @@ def test_combined_scope_project_profile( with tempfile.TemporaryDirectory() as tmpdir: profile_config = Path(tmpdir) / 'test-mcp.json' - success_flag, profile_servers = setup_environment.configure_all_mcp_servers( + success_flag, profile_servers, _ = setup_environment.configure_all_mcp_servers( servers, profile_mcp_config_path=profile_config, ) @@ -3236,7 +3247,7 @@ def test_combined_scope_with_multiple_servers( with tempfile.TemporaryDirectory() as tmpdir: profile_config = Path(tmpdir) / 'test-mcp.json' - success_flag, profile_servers = setup_environment.configure_all_mcp_servers( + success_flag, profile_servers, _ = setup_environment.configure_all_mcp_servers( servers, profile_mcp_config_path=profile_config, ) @@ -3303,7 +3314,7 @@ def test_invalid_scope_combination_error_logged( with tempfile.TemporaryDirectory() as tmpdir: profile_config = Path(tmpdir) / 'test-mcp.json' - success_flag, profile_servers = setup_environment.configure_all_mcp_servers( + success_flag, profile_servers, _ = setup_environment.configure_all_mcp_servers( servers, profile_config, ) @@ -3330,7 +3341,7 @@ def test_backward_compatibility_string_scope(self) -> None: with tempfile.TemporaryDirectory() as tmpdir: profile_config = Path(tmpdir) / 'test-mcp.json' - success_flag, profile_servers = setup_environment.configure_all_mcp_servers( + success_flag, profile_servers, _ = setup_environment.configure_all_mcp_servers( servers, profile_config, ) @@ -3352,7 +3363,7 @@ def test_backward_compatibility_no_scope(self) -> None: with tempfile.TemporaryDirectory() as tmpdir: profile_config = Path(tmpdir) / 'test-mcp.json' - success_flag, profile_servers = setup_environment.configure_all_mcp_servers( + success_flag, profile_servers, _ = setup_environment.configure_all_mcp_servers( servers, profile_config, ) @@ -3374,7 +3385,7 @@ def test_backward_compatibility_profile_string(self) -> None: with tempfile.TemporaryDirectory() as tmpdir: profile_config = Path(tmpdir) / 'test-mcp.json' - success_flag, profile_servers = setup_environment.configure_all_mcp_servers( + success_flag, profile_servers, _ = setup_environment.configure_all_mcp_servers( servers, profile_config, ) @@ -3403,7 +3414,7 @@ def test_comma_separated_string_scope(self) -> None: with tempfile.TemporaryDirectory() as tmpdir: profile_config = Path(tmpdir) / 'test-mcp.json' - success_flag, profile_servers = setup_environment.configure_all_mcp_servers( + success_flag, profile_servers, _ = setup_environment.configure_all_mcp_servers( servers, profile_config, ) @@ -3430,7 +3441,7 @@ def test_scope_case_insensitivity(self) -> None: with tempfile.TemporaryDirectory() as tmpdir: profile_config = Path(tmpdir) / 'test-mcp.json' - success_flag, profile_servers = setup_environment.configure_all_mcp_servers( + success_flag, profile_servers, _ = setup_environment.configure_all_mcp_servers( servers, profile_config, ) @@ -3457,7 +3468,7 @@ def test_universal_server_all_scopes(self) -> None: with tempfile.TemporaryDirectory() as tmpdir: profile_config = Path(tmpdir) / 'test-mcp.json' - success_flag, profile_servers = setup_environment.configure_all_mcp_servers( + success_flag, profile_servers, _ = setup_environment.configure_all_mcp_servers( servers, profile_config, ) @@ -3485,7 +3496,7 @@ def test_profile_only_scope_no_claude_mcp_add(self) -> None: with tempfile.TemporaryDirectory() as tmpdir: profile_config = Path(tmpdir) / 'test-mcp.json' - success_flag, profile_servers = setup_environment.configure_all_mcp_servers( + success_flag, profile_servers, _ = setup_environment.configure_all_mcp_servers( servers, profile_config, ) @@ -3508,7 +3519,7 @@ def test_stale_profile_config_removed_when_combined_scope_removed(self) -> None: {'name': 'user-only-server', 'command': 'uvx user-only', 'scope': 'user'}, ] - success_flag, profile_servers = setup_environment.configure_all_mcp_servers( + success_flag, profile_servers, _ = setup_environment.configure_all_mcp_servers( servers, config_path, ) @@ -3679,3 +3690,265 @@ def test_non_path_arg_unchanged(self) -> None: assert '8080' in result['args'] assert '--host' in result['args'] assert 'localhost' in result['args'] + + +class TestConfigureAllMcpServersStats: + """Test configure_all_mcp_servers stats tracking (Bug 2 fix).""" + + @patch('platform.system', return_value='Linux') + @patch('setup_environment.find_command_robust', return_value='claude') + @patch('setup_environment.run_command') + def test_stats_combined_scope_server( + self, + mock_run: MagicMock, + mock_find: MagicMock, + _mock_system: MagicMock, + ) -> None: + """Test stats correctly track combined scope servers.""" + del mock_find, _mock_system + mock_run.return_value = subprocess.CompletedProcess([], 0, '', '') + + servers = [ + { + 'name': 'combined-server', + 'command': 'uvx combined-server', + 'scope': ['user', 'profile'], + }, + ] + + with tempfile.TemporaryDirectory() as tmpdir: + profile_config = Path(tmpdir) / 'test-mcp.json' + + success, profile_servers, stats = setup_environment.configure_all_mcp_servers( + servers, + profile_mcp_config_path=profile_config, + ) + + assert success is True + assert len(profile_servers) == 1 + # Combined scope: counted in both global and profile, plus combined + assert stats['global_count'] == 1 + assert stats['profile_count'] == 1 + assert stats['combined_count'] == 1 + + @patch('platform.system', return_value='Linux') + @patch('setup_environment.find_command_robust', return_value='claude') + @patch('setup_environment.run_command') + def test_stats_profile_only_server( + self, + mock_run: MagicMock, + mock_find: MagicMock, + _mock_system: MagicMock, + ) -> None: + """Test stats correctly track profile-only servers.""" + del mock_find, _mock_system + mock_run.return_value = subprocess.CompletedProcess([], 0, '', '') + + servers = [ + { + 'name': 'profile-server', + 'command': 'uvx profile-server', + 'scope': 'profile', + }, + ] + + with tempfile.TemporaryDirectory() as tmpdir: + profile_config = Path(tmpdir) / 'test-mcp.json' + + success, profile_servers, stats = setup_environment.configure_all_mcp_servers( + servers, + profile_mcp_config_path=profile_config, + ) + + assert success is True + assert len(profile_servers) == 1 + # Profile-only: counted only in profile + assert stats['global_count'] == 0 + assert stats['profile_count'] == 1 + assert stats['combined_count'] == 0 + + @patch('platform.system', return_value='Linux') + @patch('setup_environment.find_command_robust', return_value='claude') + @patch('setup_environment.run_command') + def test_stats_user_only_server( + self, + mock_run: MagicMock, + mock_find: MagicMock, + _mock_system: MagicMock, + ) -> None: + """Test stats correctly track user-only (global) servers.""" + del mock_find, _mock_system + mock_run.return_value = subprocess.CompletedProcess([], 0, '', '') + + servers = [ + { + 'name': 'user-server', + 'command': 'uvx user-server', + 'scope': 'user', + }, + ] + + with tempfile.TemporaryDirectory() as tmpdir: + profile_config = Path(tmpdir) / 'test-mcp.json' + + success, profile_servers, stats = setup_environment.configure_all_mcp_servers( + servers, + profile_mcp_config_path=profile_config, + ) + + assert success is True + assert len(profile_servers) == 0 + # User-only: counted only in global + assert stats['global_count'] == 1 + assert stats['profile_count'] == 0 + assert stats['combined_count'] == 0 + + @patch('platform.system', return_value='Linux') + @patch('setup_environment.find_command_robust', return_value='claude') + @patch('setup_environment.run_command') + def test_stats_mixed_servers( + self, + mock_run: MagicMock, + mock_find: MagicMock, + _mock_system: MagicMock, + ) -> None: + """Test stats correctly track mixed scope servers.""" + del mock_find, _mock_system + mock_run.return_value = subprocess.CompletedProcess([], 0, '', '') + + servers = [ + {'name': 'user-only', 'command': 'uvx user-only', 'scope': 'user'}, + {'name': 'profile-only', 'command': 'uvx profile-only', 'scope': 'profile'}, + {'name': 'combined-1', 'command': 'uvx combined-1', 'scope': ['user', 'profile']}, + {'name': 'combined-2', 'command': 'uvx combined-2', 'scope': ['local', 'profile']}, + ] + + with tempfile.TemporaryDirectory() as tmpdir: + profile_config = Path(tmpdir) / 'test-mcp.json' + + success, profile_servers, stats = setup_environment.configure_all_mcp_servers( + servers, + profile_mcp_config_path=profile_config, + ) + + assert success is True + # profile_servers: profile-only, combined-1, combined-2 = 3 + assert len(profile_servers) == 3 + # global_count: user-only, combined-1, combined-2 = 3 + assert stats['global_count'] == 3 + # profile_count: profile-only, combined-1, combined-2 = 3 + assert stats['profile_count'] == 3 + # combined_count: combined-1, combined-2 = 2 + assert stats['combined_count'] == 2 + + +class TestConfigureMcpServerWindowsRemoval: + """Test configure_mcp_server Windows vs Unix removal branching (Bug 1 fix).""" + + @patch('platform.system', return_value='Windows') + @patch('setup_environment.find_command_robust', return_value='claude') + @patch('setup_environment.run_bash_command') + @patch('setup_environment.run_command') + def test_windows_removal_uses_bash( + self, + mock_run: MagicMock, + mock_bash: MagicMock, + mock_find: MagicMock, + _mock_system: MagicMock, + ) -> None: + """Test Windows MCP removal uses run_bash_command for consistency.""" + del mock_find, _mock_system + mock_run.return_value = subprocess.CompletedProcess([], 0, '', '') + mock_bash.return_value = subprocess.CompletedProcess([], 0, '', '') + + server = { + 'name': 'test-server', + 'command': 'uvx test-server', + } + + result = setup_environment.configure_mcp_server(server) + + assert result is True + # On Windows, both removal and add should use bash + # Removal: 3 calls to run_bash_command (one per scope) + # Add: 1 call to run_bash_command + assert mock_bash.call_count >= 3 # At least 3 removal calls + + # Verify removal calls contain 'mcp remove' + removal_calls = [ + call for call in mock_bash.call_args_list + if 'mcp remove' in str(call) + ] + assert len(removal_calls) >= 3 + + @patch('platform.system', return_value='Linux') + @patch('setup_environment.find_command_robust', return_value='claude') + @patch('setup_environment.run_command') + def test_unix_removal_uses_run_command( + self, + mock_run: MagicMock, + mock_find: MagicMock, + _mock_system: MagicMock, + ) -> None: + """Test Unix MCP removal uses run_command directly.""" + del mock_find, _mock_system + mock_run.return_value = subprocess.CompletedProcess([], 0, '', '') + + server = { + 'name': 'test-server', + 'command': 'uvx test-server', + } + + result = setup_environment.configure_mcp_server(server) + + assert result is True + # On Unix, run_command is used for both removal and add + # Removal: 3 calls (one per scope) + # Add: 1 call + assert mock_run.call_count == 4 + + # Verify removal calls contain 'mcp' and 'remove' + removal_calls = [ + call for call in mock_run.call_args_list + if 'remove' in str(call[0][0]) + ] + assert len(removal_calls) == 3 + + @patch('platform.system', return_value='Windows') + @patch('setup_environment.find_command_robust', return_value='claude') + @patch('setup_environment.run_bash_command') + @patch('setup_environment.run_command') + def test_windows_removal_all_scopes( + self, + mock_run: MagicMock, + mock_bash: MagicMock, + mock_find: MagicMock, + _mock_system: MagicMock, + ) -> None: + """Test Windows removal attempts all three scopes via bash.""" + del mock_find, _mock_system, mock_run + mock_bash.return_value = subprocess.CompletedProcess([], 0, '', '') + + server = { + 'name': 'multi-scope-server', + 'command': 'uvx multi-scope-server', + } + + result = setup_environment.configure_mcp_server(server) + + assert result is True + + # Extract scope values from bash commands + scopes_removed = set() + for call in mock_bash.call_args_list: + cmd = call[0][0] + if 'mcp remove' in cmd: + if '--scope user' in cmd: + scopes_removed.add('user') + elif '--scope local' in cmd: + scopes_removed.add('local') + elif '--scope project' in cmd: + scopes_removed.add('project') + + # All three scopes should be attempted + assert scopes_removed == {'user', 'local', 'project'}