Merge pull request #277 from alex-feel/alex-feel-dev

Ensure profile MCP servers removed from all scopes before configuration

Author: alex-feel

scripts/setup_environment.py

@@ -3864,12 +3864,6 @@ def configure_mcp_server(server: dict[str, Any]) -> bool:
     """Configure a single MCP server."""
     name = server.get('name')
     scope = server.get('scope', 'user')
-
-    # Skip profile-scoped servers - they are configured via create_mcp_config_file()
-    if scope == 'profile':
-        info(f'MCP server {name} has scope: profile (will be configured via --strict-mcp-config)')
-        return True  # Not an error, just handled elsewhere
-
     transport = server.get('transport')
     url = server.get('url')
     command = server.get('command')
@@ -3925,6 +3919,11 @@ def configure_mcp_server(server: dict[str, Any]) -> bool:
         else:
             info(f'MCP server {name} was not found in any scope')
 
+        # Profile-scoped servers are configured via create_mcp_config_file(), not claude mcp add
+        if scope == 'profile':
+            info(f'MCP server {name} has scope: profile (will be configured via --strict-mcp-config)')
+            return True
+
         # Build the base command
         base_cmd = [str(claude_cmd), 'mcp', 'add']
 
@@ -4127,6 +4126,14 @@ def configure_all_mcp_servers(
     if profile_servers and profile_mcp_config_path:
         info(f'Creating profile MCP config with {len(profile_servers)} server(s)...')
         create_mcp_config_file(profile_servers, profile_mcp_config_path)
+    elif profile_mcp_config_path and profile_mcp_config_path.exists():
+        # Remove stale profile MCP config file when no profile servers are configured
+        info(f'Removing stale profile MCP config: {profile_mcp_config_path.name}')
+        try:
+            profile_mcp_config_path.unlink()
+            success(f'Removed stale profile MCP config: {profile_mcp_config_path.name}')
+        except OSError as e:
+            warning(f'Failed to remove stale profile MCP config: {e}')
 
     return True, profile_servers
 

tests/test_setup_environment_additional.py

@@ -2655,12 +2655,12 @@ def test_create_mcp_config_file_permission_error(self) -> None:
 
     @patch('platform.system', return_value='Linux')
     @patch('setup_environment.find_command_robust', return_value='claude')
-    def test_configure_mcp_server_profile_scope_skipped(
+    def test_configure_mcp_server_profile_scope_removes_from_all_scopes(
         self,
         mock_find: MagicMock,
         _mock_system: MagicMock,
     ) -> None:
-        """Test that profile-scoped servers are skipped by configure_mcp_server."""
+        """Test that profile-scoped servers are removed from all scopes but not added via claude mcp add."""
         del mock_find  # Unused but required for patch
         del _mock_system  # Unused but required for patch
 
@@ -2670,13 +2670,29 @@ def test_configure_mcp_server_profile_scope_skipped(
             'scope': 'profile',
         }
 
-        # Should return True (success) but not actually configure
         with patch('setup_environment.run_command') as mock_run:
+            mock_run.return_value = subprocess.CompletedProcess([], 0, '', '')
             result = setup_environment.configure_mcp_server(server)
 
             assert result is True
-            # run_command should NOT be called for profile-scoped servers
-            mock_run.assert_not_called()
+            # run_command should be called 3 times (once per scope) for removal
+            assert mock_run.call_count == 3
+
+            # Verify removal was attempted from all scopes
+            calls = mock_run.call_args_list
+            scopes_removed = set()
+            for call in calls:
+                args = call[0][0]  # First positional argument is the command list
+                # Verify it's a removal command
+                assert 'mcp' in args
+                assert 'remove' in args
+                assert 'profile-server' in args
+                # Extract scope
+                scope_idx = args.index('--scope') + 1
+                scopes_removed.add(args[scope_idx])
+
+            # Verify all three scopes were attempted
+            assert scopes_removed == {'user', 'local', 'project'}
 
     @patch('platform.system', return_value='Linux')
     @patch('setup_environment.find_command_robust', return_value='claude')
@@ -2840,3 +2856,68 @@ def test_configure_all_mcp_servers_mixed_scopes(
             assert 'profile-2' in config['mcpServers']
             assert 'default-scope' not in config['mcpServers']
             assert 'explicit-user' not in config['mcpServers']
+
+    def test_stale_mcp_config_file_removed_when_no_profile_servers(self) -> None:
+        """Test that stale MCP config file is removed when no profile servers exist."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            config_path = Path(tmpdir) / 'test-mcp.json'
+            # Create a stale MCP config file
+            config_path.write_text('{"mcpServers": {"stale-server": {}}}')
+            assert config_path.exists()
+
+            # Configure with NO profile servers (only user-scoped)
+            servers = [
+                {'name': 'global-server', 'transport': 'http', 'url': 'http://a.com', 'scope': 'user'},
+            ]
+
+            with patch('setup_environment.configure_mcp_server', return_value=True):
+                success, profile_servers = setup_environment.configure_all_mcp_servers(
+                    servers, config_path,
+                )
+
+            # Verify stale file was removed
+            assert success is True
+            assert len(profile_servers) == 0
+            assert not config_path.exists()
+
+    def test_mcp_config_file_not_touched_when_path_none(self) -> None:
+        """Test that no file operations occur when profile_mcp_config_path is None."""
+        servers = [
+            {'name': 'server', 'transport': 'http', 'url': 'http://a.com', 'scope': 'user'},
+        ]
+
+        with patch('setup_environment.configure_mcp_server', return_value=True):
+            success, profile_servers = setup_environment.configure_all_mcp_servers(
+                servers, None,  # No profile config path
+            )
+
+        assert success is True
+        assert len(profile_servers) == 0
+        # No assertion about file - just verify no crash
+
+    def test_stale_mcp_config_removal_failure_logged_as_warning(self) -> None:
+        """Test that stale MCP config removal failure is logged as warning, not error."""
+        with tempfile.TemporaryDirectory() as tmpdir:
+            config_path = Path(tmpdir) / 'test-mcp.json'
+            # Create a stale MCP config file
+            config_path.write_text('{"mcpServers": {"stale-server": {}}}')
+
+            servers = [
+                {'name': 'server', 'transport': 'http', 'url': 'http://a.com', 'scope': 'user'},
+            ]
+
+            with (
+                patch('setup_environment.configure_mcp_server', return_value=True),
+                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(
+                    servers, config_path,
+                )
+
+            # Should still return success (non-fatal error)
+            assert success is True
+            assert len(profile_servers) == 0
+            # Warning should have been logged
+            mock_warning.assert_called_once()
+            assert 'Permission denied' in str(mock_warning.call_args)