feat(mcp): multi-server lifecycle — enable/disable + error visibility (#3196)#3339
Merged
senamakel merged 15 commits intoJun 4, 2026
Conversation
…humansai#3196) Defaults to true so legacy rows keep auto-connecting. New column enabled INTEGER NOT NULL DEFAULT 1 added via the same PRAGMA-driven additive migration used for transport/deployment_url.
…nyhumansai#3196) One-liner adding assert!(s.enabled, ...) in the existing serde-default test so a future refactor that breaks #[serde(default)] is caught here rather than transitively.
…inyhumansai#3196) Wraps connect so each successful run clears the prior error and each failure stores its message keyed by server_id. Exposed via connections::last_error_for so the next status poll can surface err state instead of generic 'disconnected'.
…tinyhumansai#3196) Status priority: Disabled > Connected > Error > Disconnected. Reads LAST_ERRORS so failed-connect servers surface as Error with last_error populated instead of indistinguishable from idle.
A disabled install is left in the store with its env values intact, but its subprocess is never spawned and its tools never enter the agent's tool surface. One failed server still doesn't block others (existing per-iteration error swallow plus LAST_ERRORS recording).
Give server C a bogus command so the post-boot last_error remains None only if boot actually skipped the connect attempt. Without the boot.rs guard the bogus command would record an error, failing the test.
…isabled (tinyhumansai#3196) set_enabled persists the flip and auto-disconnects on false. connect returns a clear error when called on a disabled server. Re-enabling does not auto-connect; the user re-issues connect explicitly so 'enabled' stays a persistent setting and 'connected' stays a live-session state.
Four scenarios against the hermetic test-mcp-stub binary: - Name collision: two servers exposing 'echo' both reach the surface - Routing: call_tool dispatches by server_id even with shared names - Failure isolation: bad subprocess doesn't block a healthy peer - Disabled enforcement: disabled server contributes no tools and refuses explicit connect
…humansai#3196) InstalledServer.enabled threaded through. ServerStatus gains a 'disabled' variant. McpStatusBadge renders it with a quieter italic style and a dedicated i18n key so it reads distinctly from 'disconnected'.
Adds the disabled-status badge translation. mcp.detail.enable and mcp.detail.disable were already added in the toggle commit; this closes the last missing key referenced by McpStatusBadge.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds an enabled flag and disabled status for MCP servers, persists and migrates it, exposes a set_enabled RPC and client, updates connection/boot/status behavior to respect disabled servers, wires an enable/disable toggle in the UI, adds i18n keys, and adds unit/integration/E2E tests. ChangesMCP Server Enable/Disable Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/components/channels/mcp/McpStatusBadge.test.tsx (1)
22-29: ⚡ Quick winAssert the disabled label text (behavior), not only the CSS class.
This test currently validates implementation detail (
className) but not the user-visible disabled label, so it can miss i18n/status regressions.Suggested test adjustment
- it('renders the disabled status badge with the mcp.status.disabled i18n key', () => { + it('renders the disabled status badge label and style', () => { render(<McpStatusBadge status="disabled" />); - // The i18n key 'mcp.status.disabled' will be added in Task 9; until then - // the runtime falls back to the key string itself. const badge = screen.getByRole('status'); - // The badge renders without crashing and carries the italic style. + expect(badge).toHaveTextContent('Disabled'); expect(badge.className).toContain('italic'); });As per coding guidelines, “Prefer behavior over implementation in unit tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/channels/mcp/McpStatusBadge.test.tsx` around lines 22 - 29, The test for McpStatusBadge currently asserts only the CSS implementation (badge.className contains 'italic'); update it to assert the user-visible label as well by checking the rendered badge (screen.getByRole('status') or the same element) contains the expected disabled text (the i18n fallback string 'mcp.status.disabled' until Task 9) in addition to the existing italic class assertion so the test validates behavior not just implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/mcp_registry/ops.rs`:
- Around line 272-277: The connect guard that prevents connecting disabled
servers in mcp_clients_connect is missing from the mcp_clients_update_env
reconnection path: before calling connections::connect inside
mcp_clients_update_env (and other reconnect paths around lines 304-350), check
the server.enabled flag and return an Err with the same message used by
mcp_clients_connect when the server is disabled; this enforces the
disabled-state at the ops level (reference mcp_clients_update_env,
connections::connect, server.enabled, and mcp_clients_set_enabled).
---
Nitpick comments:
In `@app/src/components/channels/mcp/McpStatusBadge.test.tsx`:
- Around line 22-29: The test for McpStatusBadge currently asserts only the CSS
implementation (badge.className contains 'italic'); update it to assert the
user-visible label as well by checking the rendered badge
(screen.getByRole('status') or the same element) contains the expected disabled
text (the i18n fallback string 'mcp.status.disabled' until Task 9) in addition
to the existing italic class assertion so the test validates behavior not just
implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68260bc7-7d03-49f1-a865-e3c57959679d
📒 Files selected for processing (39)
app/src/components/channels/mcp/InstalledServerDetail.enabledToggle.test.tsxapp/src/components/channels/mcp/InstalledServerDetail.test.tsxapp/src/components/channels/mcp/InstalledServerDetail.tsxapp/src/components/channels/mcp/InstalledServerList.test.tsxapp/src/components/channels/mcp/InstalledServerList.tsxapp/src/components/channels/mcp/McpInventoryManifest.test.tsapp/src/components/channels/mcp/McpInventoryPanel.test.tsxapp/src/components/channels/mcp/McpServersTab.tsxapp/src/components/channels/mcp/McpStatusBadge.test.tsxapp/src/components/channels/mcp/McpStatusBadge.tsxapp/src/components/channels/mcp/types.tsapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsapp/src/services/api/mcpClientsApi.test.tsapp/src/services/api/mcpClientsApi.tssrc/openhuman/mcp_registry/boot.rssrc/openhuman/mcp_registry/connections.rssrc/openhuman/mcp_registry/mod.rssrc/openhuman/mcp_registry/ops.rssrc/openhuman/mcp_registry/schemas.rssrc/openhuman/mcp_registry/setup_ops.rssrc/openhuman/mcp_registry/store.rssrc/openhuman/mcp_registry/types.rstests/json_rpc_e2e.rstests/mcp_registry_e2e.rstests/mcp_registry_multi_server.rstests/tool_registry_approval_raw_coverage_e2e.rs
…sh schema counts (tinyhumansai#3196) - update_env on a disabled server now persists the new env values but skips the reconnect, returning status="disabled". Closes the gap CodeRabbit flagged where the existing connect-guard on mcp_clients_connect was not mirrored in the update_env reconnection path. - Bumps the schemas_tests assertions from 19 to 20 (mcp_clients 13 → 14) to account for the new set_enabled controller — was the lib-test failure in CI. - McpStatusBadge test now asserts the visible 'Disabled' label in addition to the italic class, per CodeRabbit nitpick.
…tinyhumansai#3196) Adds two short Vitest cases to push diff-cover above the 80% gate: - McpServersTab: clicks Disable in the detail pane and verifies mcpClientsApi.setEnabled is called and the parent re-fetches the installed list + status (covers handleEnabledChange + the onEnabledChange prop wiring). - InstalledServerDetail: renders with status='connecting' and asserts the Connecting button is shown and disabled (covers the connecting-label branch in the Connect/Disconnect group). Also adds setEnabled + a mock + an enabled:true to the McpServersTab test harness so the toggle render path matches the real component.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
enabledlifecycle to installed MCP servers (additive SQLite migration; defaultstrueso existing rows keep auto-connecting).LAST_ERRORSmap and aServerStatus::Disabledvariant;all_statuspriority isDisabled > Connected > Error > Disconnected.openhuman.mcp_clients_set_enabledRPC + boot-time skip for disabled servers;mcp_clients_connectreturns a clear error when called on a disabled install.disabledbadge style, andsetEnabledonmcpClientsApi. Connect is hidden when a server is disabled.Problem
The MCP server model already supported multiple installs (UUID per
InstalledServer, namespaced tool IDs), but three operational gaps prevented multi-server workflows from being usable:ServerStatus::Errorexisted in the enum but no code path populated it; failed connects rendered as plaindisconnected.Issue #3196 calls these out directly in the acceptance criteria.
Solution
Rust core (
src/openhuman/mcp_registry/)types.rs—InstalledServer.enabled: boolwith#[serde(default = "default_enabled")]. NewServerStatus::Disabledvariant.store.rs— additiveALTER TABLE mcp_servers ADD COLUMN enabled INTEGER NOT NULL DEFAULT 1guarded by the same PRAGMA-driven existence check used fortransport/deployment_url. CRUD helpersupdate_enabled+update_enabled_conn.connections.rs— process-globalLAST_ERRORS: OnceLock<RwLock<HashMap<String, String>>>populated by a wrapper aroundconnect(connect_inneris the renamed original body).all_statusis rewritten with the new priority logic and reads the errors snapshot for theErrorarm.boot.rs—spawn_installed_serversskips!enabledrows with an info log; healthy peers still connect even if a sibling fails.ops.rs—mcp_clients_set_enabledpersists the flip, onfalsecallsdisconnect+clear_last_error+ publishesDomainEvent::McpServerDisconnected { reason: "disabled" }.mcp_clients_connectrejects disabled servers with a clear error.schemas.rs—set_enabledschema + handler wired into the controller registry (nocore/dispatch.rsbranch added).Tests
tests/mcp_registry_e2e.rs— 10 hermetic tests against thetest-mcp-stubbinary covering enabled round-trip,LAST_ERRORSrecord/clear, status priority, boot skip, andset_enabledsemantics.tests/mcp_registry_multi_server.rs— new file with 4 regression scenarios: name-collision safety, routing byserver_id, failure isolation (bad subprocess doesn't block a healthy peer), and disabled enforcement.tests/json_rpc_e2e.rs— smoke check for the newset_enabledRPC.Frontend (
app/src/)components/channels/mcp/types.ts—enabled: boolean+'disabled'status.services/api/mcpClientsApi.ts—setEnabled({ server_id, enabled })method.components/channels/mcp/McpStatusBadge.tsx—disabledstyle (muted gray + italic) usingmcp.status.disabled.components/channels/mcp/InstalledServerDetail.tsx— Enable/Disable button using existingrunBusywrapper, clears local tool list on disable, firesonEnabledChange?callback. Connect/Disconnect hidden when!server.enabled.components/channels/mcp/McpServersTab.tsx— passesonEnabledChangehandler that re-fetches installed list + status, mirroringhandleUninstalled.Record<ServerStatus, …>andInstalledServerliterals would otherwise have failed exhaustiveness checks.i18n —
mcp.detail.enable,mcp.detail.disable,mcp.status.disabledadded with real translations acrossen,ar,bn,de,es,fr,hi,id,it,ko,pl,pt,ru,zh-CN.pnpm i18n:checkreports 0 missing / 0 extra.Submission Checklist
diff-cover) meet the gate enforced by.github/workflows/pr-ci.yml. Runpnpm test:coverageandpnpm test:rustlocally; PRs below 80% on changed lines will not merge.docs/TEST-COVERAGE-MATRIX.mdreflect this change (orN/A: behaviour-only change) — N/A: extending an existing capability (MCP server lifecycle) already represented in the matrix; no new feature row needed.## Related— N/A: extension of existing MCP feature surface.docs/RELEASE-MANUAL-SMOKE.md) — N/A: feature is within existing MCP server settings flow; no new release-cut surface.Closes #NNNin the## RelatedsectionImpact
enabledis a persistent setting,connectedis a live state).ALTER TABLEruns at first boot post-upgrade. Legacy rows back-fill toenabled = 1. Idempotent —init_schemacan be re-entered without error.LAST_ERRORSis a process-globaltokio::sync::RwLock<HashMap>; reads onall_statusare O(n) over installed servers, one snapshot clone per call (matchesCONNECTIONS).mcp_clients_connect— the guard runs afterstore::get_server, returning a clear error.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
issue/3196-add-support-for-multiple-mcp-servers8afb03d0fValidation Run
pnpm --filter openhuman-app format:checkpnpm typecheckcargo test --test mcp_registry_e2e(10 pass),cargo test --test mcp_registry_multi_server(4 pass),pnpm test src/components/channels/mcp/(236 pass),pnpm test src/services/api/mcpClientsApi.test.ts(23 pass)cargo fmt --manifest-path Cargo.toml && cargo check --manifest-path Cargo.toml— cleancargo check --manifest-path app/src-tauri/Cargo.toml— clean (no changes inapp/src-tauri/)Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
enabledflag controlling boot auto-connect and tool exposure. New RPCopenhuman.mcp_clients_set_enabledand matching UI toggle.Parity Contract
enabled = 1so the upgrade is invisible to users with installed servers.connect/disconnect/all_statuskeep their existing return shapes — theConnStatus.last_errorfield was already present and is now actually populated.mcp_clients_connectruns the disabled guard after the existingstore::get_serverlookup, so the "not found" error still wins over the "disabled" error.disconnectclears theLAST_ERRORSentry in addition to its existing connection-map removal.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes
Localization
Tests