fix: prevent disabled MCP servers from starting processes and show correct status

.roo/temp/pr-6037/architecture-review.md

@@ -0,0 +1,119 @@
+## Architecture Review for PR #6037
+
+### Module Boundaries
+
+1. **Separation of Concerns**
+
+    - ✅ Backend logic (McpHub.ts) handles connection state management
+    - ✅ UI logic (McpView.tsx) handles visual representation
+    - ✅ Clear separation between server state and UI state
+    - Good adherence to module boundaries
+
+2. **State Management**
+    - The disabled state is properly stored in the server configuration
+    - State changes flow correctly: Config → Backend → UI
+    - No violations of data flow patterns
+
+### Dependency Analysis
+
+1. **No New Dependencies**
+
+    - The PR doesn't introduce any new external dependencies
+    - Uses existing patterns and methods
+    - No risk of dependency conflicts
+
+2. **Internal Dependencies**
+    - Relies on existing methods: `deleteConnection()`, `connectToServer()`
+    - No circular dependencies introduced
+    - Clean dependency chain maintained
+
+### Architectural Concerns
+
+1. **State Synchronization Complexity**
+
+    - The disabled state is checked in multiple places:
+        - `updateServerConnections()` - during initialization/updates
+        - `toggleServerDisabled()` - during state changes
+        - `getStatusColor()` - for UI display
+    - This distributed checking could lead to inconsistencies
+
+2. **Missing Centralized Validation**
+
+    - The `connectToServer()` method doesn't check disabled state
+    - This means other code paths could potentially connect disabled servers
+    - Architectural weakness: validation should be centralized
+
+3. **Race Condition Potential**
+    - No locking mechanism during toggle operations
+    - Rapid toggling could lead to race conditions
+    - The PR doesn't address concurrent state changes
+
+### Impact on System Architecture
+
+1. **Performance Impact**
+
+    - ✅ Positive: Disabled servers won't consume system resources
+    - ✅ Reduces unnecessary process spawning
+    - ✅ Improves overall system efficiency
+
+2. **Scalability Considerations**
+
+    - The solution scales well with multiple servers
+    - No performance degradation with many disabled servers
+    - Clean resource management
+
+3. **Maintainability**
+    - ⚠️ The distributed disabled checks could make maintenance harder
+    - Future developers might miss updating all check locations
+    - Would benefit from centralization
+
+### Architectural Patterns
+
+1. **Observer Pattern**
+
+    - The PR maintains the existing observer pattern
+    - State changes properly notify observers via `notifyWebviewOfServerChanges()`
+    - No pattern violations
+
+2. **Command Pattern**
+    - Toggle operations follow command-like pattern
+    - Clear action → state change → notification flow
+    - Consistent with existing architecture
+
+### Recommendations
+
+1. **Centralize Disabled Check**
+
+    - Move the disabled check into `connectToServer()` method
+    - This ensures no code path can bypass the check
+    - Reduces maintenance burden
+
+2. **Add State Validation**
+
+    - Before toggling, validate current state
+    - Prevent unnecessary operations (e.g., disabling already disabled server)
+    - Add guards against invalid state transitions
+
+3. **Consider State Machine**
+
+    - Server states could benefit from formal state machine
+    - States: disconnected, connecting, connected, disabled
+    - Would prevent invalid state transitions
+
+4. **Add Concurrency Protection**
+    - Consider adding a flag to prevent concurrent toggle operations
+    - Or queue state change operations
+    - Prevents race conditions
+
+### Overall Assessment
+
+The PR follows most architectural patterns correctly but has some concerns:
+
+- ✅ Maintains module boundaries
+- ✅ No dependency issues
+- ✅ Improves resource management
+- ⚠️ Distributed validation logic
+- ⚠️ Potential race conditions
+- ⚠️ Could benefit from centralization
+
+The architecture remains sound but could be improved with centralized validation and better state management.

.roo/temp/pr-6037/comments.json

@@ -0,0 +1 @@
+[]

.roo/temp/pr-6037/final-review.md

@@ -0,0 +1,91 @@
+# PR Review: Fix disabled MCP servers issue (#6037)
+
+## Executive Summary
+
+This PR addresses issue #6036 where disabled MCP servers were incorrectly starting processes and showing incorrect status indicators. The implementation is functional and fixes the reported issue, but there are several areas for improvement regarding code organization, redundancy, and architectural consistency.
+
+## Critical Issues (Must Fix)
+
+### 1. Centralize Disabled State Validation
+
+The disabled state check is distributed across multiple locations rather than being centralized:
+
+- [`src/services/mcp/McpHub.ts:977-984`](src/services/mcp/McpHub.ts:977) - Check for new servers
+- [`src/services/mcp/McpHub.ts:991-993`](src/services/mcp/McpHub.ts:991) - Check for config changes
+- [`src/services/mcp/McpHub.ts:1266-1272`](src/services/mcp/McpHub.ts:1266) - Toggle logic
+
+**Recommendation**: Move the disabled check into the [`connectToServer()`](src/services/mcp/McpHub.ts:555) method itself to ensure no code path can bypass this validation.
+
+### 2. Race Condition Risk
+
+The [`toggleServerDisabled()`](src/services/mcp/McpHub.ts:1244) method doesn't protect against concurrent operations. Rapid toggling could lead to race conditions where multiple connect/disconnect operations overlap.
+
+**Recommendation**: Add a state flag or queue mechanism to prevent concurrent toggle operations on the same server.
+
+## Pattern Inconsistencies
+
+### 1. Redundant Visual Indicators
+
+The UI implementation adds a grey color for disabled servers in [`getStatusColor()`](webview-ui/src/components/mcp/McpView.tsx:220), but the parent div already handles opacity changes for disabled servers (line 281).
+
+**Recommendation**: Choose one visual indicator approach for consistency. The opacity change is likely sufficient.
+
+### 2. Unnecessary Status Check
+
+In [`toggleServerDisabled()`](src/services/mcp/McpHub.ts:1266), the code checks if the server status is "connected" before disconnecting, but [`deleteConnection()`](src/services/mcp/McpHub.ts:915) already handles non-existent connections gracefully.
+
+**Recommendation**: Remove the redundant status check.
+
+## Test Coverage Issues
+
+### 1. Missing Edge Cases
+
+The tests don't cover:
+
+- Toggling a server that's already in the target state
+- Error handling during toggle operations
+- Concurrent toggle operations
+
+### 2. No UI Tests
+
+The [`getStatusColor()`](webview-ui/src/components/mcp/McpView.tsx:220) changes are not covered by tests.
+
+**Recommendation**: Add tests for these scenarios to ensure robustness.
+
+## Architecture Concerns
+
+### 1. State Management Complexity
+
+The disabled state validation is spread across multiple methods, making it harder to maintain and potentially leading to inconsistencies.
+
+### 2. Missing State Validation
+
+The implementation doesn't validate whether a state change is necessary before performing it (e.g., disabling an already disabled server).
+
+## Minor Suggestions
+
+### 1. Improve Error Messages
+
+Consider adding more specific error messages when operations fail due to disabled state.
+
+### 2. Add Logging
+
+Add debug logging for state transitions to help with troubleshooting.
+
+### 3. Consider State Machine Pattern
+
+For better state management, consider implementing a formal state machine for server states (disconnected, connecting, connected, disabled).
+
+## Positive Aspects
+
+✅ The fix correctly addresses the reported issue  
+✅ Tests are well-organized and follow existing patterns  
+✅ The implementation maintains module boundaries  
+✅ Resource management is improved by not starting disabled servers  
+✅ State changes properly trigger UI updates
+
+## Conclusion
+
+While this PR successfully fixes the immediate issue, it would benefit from centralizing the disabled state validation and addressing the potential race conditions. The test coverage should be expanded to include edge cases and UI changes. With these improvements, the implementation would be more robust and maintainable.
+
+**Recommendation**: Request changes to address the critical issues before merging.

.roo/temp/pr-6037/linked-issue.json

@@ -0,0 +1,7 @@
+{
+	"author": { "id": "MDQ6VXNlcjQ5MTAzMjQ3", "is_bot": false, "login": "hannesrudolph", "name": "Hannes Rudolph" },
+	"body": "## App Version\nLatest (based on current codebase analysis)\n\n## API Provider\nNot Applicable / Other\n\n## Model Used\nN/A\n\n## 🔁 Steps to Reproduce\n\n1. Open VSCode with Roo Code extension\n2. Navigate to the MCP Servers view (click MCP icon in sidebar)\n3. Toggle any MCP server to disabled state using the toggle switch\n4. Observe the server row - note the status indicator color\n5. Check system processes (Activity Monitor on macOS, Task Manager on Windows)\n6. Look for the MCP server process still running\n7. Restart VSCode\n8. Check processes again - disabled MCP servers are still launched\n\nInclude:\n- The disabled server still shows a green status indicator (connected state)\n- The server process continues running in the background\n- After VSCode restart, disabled servers are started again despite being disabled\n\n## 💥 Outcome Summary\n\nExpected: Disabled MCP servers should not start processes, should show appropriate disabled status (grey/red indicator), and should not consume system resources\nActual: Disabled servers start processes, show green \"connected\" status, and continue consuming resources even when disabled\n\n## 📄 Relevant Logs or Errors\n\nNo specific error messages - the issue is silent resource consumption and incorrect UI state.\n\n---\n\n## 🛠️ Contributing & Technical Analysis\n\n✅ **I'm interested in providing technical analysis**\n✅ **I understand this needs approval before implementation begins**\n\n### Root Cause / Implementation Target\n\nThe issue stems from incomplete handling of disabled MCP servers in multiple areas:\n\n1. **Process Management**: In `src/services/mcp/McpHub.ts` (lines 975-992), the `updateServerConnections()` method doesn't check if a server is disabled before calling `connectToServer()`\n2. **UI Status Display**: In `webview-ui/src/components/mcp/McpView.tsx` (lines 220-229), the `getStatusColor()` function only considers connection status, not disabled state\n3. **Server Filtering**: While `getServers()` filters out disabled servers (line 410), the UI still receives all servers including disabled ones\n\n### Affected Components\n\n- **Primary Files:**\n  - `src/services/mcp/McpHub.ts` (lines 975-992): Server connection logic\n  - `webview-ui/src/components/mcp/McpView.tsx` (lines 220-229, 275): Status display logic\n\n- **Secondary Impact:**\n  - All components that display MCP server status\n  - System resource management\n  - User experience when managing multiple MCP servers\n\n### Current Implementation Analysis\n\nThe current code properly stores the disabled state but fails to act on it in several critical places:\n\n1. PR #5922 added checks in `updateServerConnections()` but these checks were incomplete\n2. The UI receives disabled servers but doesn't differentiate their visual state\n3. Disabled servers are filtered from the agent's view but still consume resources\n\n### Proposed Implementation\n\n#### Step 1: Fix process spawning for disabled servers\n- File: `src/services/mcp/McpHub.ts`\n- Changes: Add disabled check before `connectToServer()` calls in `updateServerConnections()`\n- Rationale: Prevents disabled servers from starting processes\n\n#### Step 2: Update UI status indicators\n- File: `webview-ui/src/components/mcp/McpView.tsx`\n- Changes: Modify `getStatusColor()` to check disabled state first\n- Rationale: Provides clear visual feedback about server state\n\n#### Step 3: Handle server state transitions\n- File: `src/services/mcp/McpHub.ts`\n- Changes: When toggling disabled state, properly disconnect running servers\n- Rationale: Ensures immediate resource cleanup\n\n### Code Architecture Considerations\n- Follow existing patterns for server state management\n- Maintain backward compatibility with existing configurations\n- Ensure consistent state between backend and UI\n\n### Testing Requirements\n- Unit Tests:\n  - [ ] Test case 1: Disabled servers don't start processes during initialization\n  - [ ] Test case 2: Toggling disabled state properly disconnects servers\n  - [ ] Test case 3: UI correctly displays disabled state\n- Integration Tests:\n  - [ ] Test scenario 1: Multiple servers with mixed enabled/disabled states\n  - [ ] Test scenario 2: State persistence across VSCode restarts\n- Edge Cases:\n  - [ ] Edge case 1: Server disabled while actively processing a request\n  - [ ] Edge case 2: Rapid toggling of disabled state\n\n### Performance Impact\n- Expected performance change: Positive - reduced resource consumption\n- Benchmarking needed: No\n- Optimization opportunities: Lazy loading of disabled server configurations\n\n### Security Considerations\n- No security implications - this is a resource management fix\n\n### Migration Strategy\nNot applicable - this is a bug fix that maintains existing configuration format\n\n### Rollback Plan\nIf issues arise, the fix can be reverted without data loss as it doesn't change configuration schema\n\n### Dependencies and Breaking Changes\n- External dependencies affected: None\n- API contract changes: None\n- Breaking changes for users: None\n\n### Implementation Complexity\n- Estimated effort: Small\n- Risk level: Low\n- Prerequisites: None\n\n## Acceptance Criteria\n\nGiven I have MCP servers configured in my settings\nWhen I disable a server using the toggle switch\nThen the server process should immediately stop\nAnd the status indicator should show grey/disabled state\nAnd no system resources should be consumed by that server\nBut the server configuration should remain in settings\n\nGiven I have disabled MCP servers\nWhen I restart VSCode\nThen disabled servers should not start processes\nAnd they should show as disabled in the UI\nAnd they should not appear in the agent's available servers list\nBut I should be able to re-enable them at any time\n\nGiven I toggle a server from enabled to disabled\nWhen the server is currently processing a request\nThen the current request should complete or timeout gracefully\nAnd the server should then disconnect\nAnd no new requests should be accepted\nBut no data corruption should occur\n\n---\n\n**Note:** This issue supersedes and expands upon #2797, which focused only on the process starting aspect. This comprehensive issue addresses all aspects of disabled MCP server behavior including UI state and resource management.",
+	"number": 6036,
+	"state": "OPEN",
+	"title": "Disabled MCP servers still start processes and show incorrect status indicators"
+}

.roo/temp/pr-6037/pattern-analysis.md

@@ -0,0 +1,75 @@
+## Pattern Analysis for PR #6037
+
+### Similar Existing Implementations
+
+1. **Disabled State Handling Pattern**
+
+    - Found similar disabled state handling in [`webview-ui/src/components/ui/toggle-switch.tsx`](webview-ui/src/components/ui/toggle-switch.tsx:32-51)
+    - The toggle switch component already implements disabled state with opacity changes
+    - Pattern: Use `disabled` prop to control visual state and interaction
+
+2. **Server Connection Management**
+
+    - The `updateServerConnections()` method already handles server lifecycle
+    - Pattern: Check configuration before connecting, handle errors gracefully
+
+3. **Toggle State Management**
+    - Found similar toggle patterns in [`McpToolRow.tsx`](webview-ui/src/components/mcp/McpToolRow.tsx:87-95) for tool enabling/disabling
+    - Pattern: Toggle state updates both UI and backend configuration
+
+### Established Patterns
+
+1. **Configuration Validation Before Connection**
+
+    - The codebase validates server configurations using `validateServerConfig()` before attempting connections
+    - This PR correctly follows this pattern by checking `disabled` state
+
+2. **Disconnection Before Reconnection**
+
+    - Pattern seen in `restartConnection()` and config changes: always disconnect before reconnecting
+    - This PR follows this pattern in `toggleServerDisabled()`
+
+3. **UI State Synchronization**
+    - Pattern: Backend state changes trigger `notifyWebviewOfServerChanges()`
+    - This PR correctly notifies the webview after toggling disabled state
+
+### Pattern Deviations
+
+1. **Inconsistent Disabled Check Placement**
+
+    - The disabled check is added in `updateServerConnections()` but only for new servers and config changes
+    - Missing disabled check in the initial connection logic during server initialization
+    - Should be more centralized in `connectToServer()` method itself
+
+2. **UI Color State Logic**
+    - The UI adds disabled check at the beginning of `getStatusColor()` which is good
+    - However, the opacity change is already handled by the parent div (line 281)
+    - This creates redundant visual indicators
+
+### Redundancy Findings
+
+1. **Disabled State Visual Indicators**
+
+    - The PR adds grey color for disabled servers in `getStatusColor()`
+    - But the parent div already sets `opacity: server.disabled ? 0.6 : 1` (line 281)
+    - Both visual indicators might be redundant
+
+2. **Connection State Checks**
+    - The PR checks `connection.server.status === "connected"` before disconnecting
+    - But `deleteConnection()` already handles non-existent connections gracefully
+    - The check might be unnecessary
+
+### Organization Issues
+
+1. **Test Organization**
+
+    - New tests are correctly placed within the existing "server disabled state" describe block
+    - Tests follow the existing pattern of mocking transport and client
+    - Good organization - no issues here
+
+2. **Logic Distribution**
+    - Disabled check logic is spread across multiple places:
+        - `updateServerConnections()` - for new/changed servers
+        - `toggleServerDisabled()` - for toggling state
+        - `getStatusColor()` - for UI display
+    - Could be more centralized