feat(mcp-clients): MCP client subsystem with Smithery registry + UI#2409
Merged
Conversation
Adds an `mcp_clients` domain that lets users browse the Smithery.ai MCP
server registry, install servers locally, spawn them over stdio via the
existing JS/Python runtimes, and expose their tools to the agent.
Rust core (src/openhuman/mcp_clients/):
- Smithery registry client with 10-minute SQLite cache
- stdio MCP JSON-RPC client (initialize / tools/list / tools/call)
behind a McpTransport trait so tests can inject a fake
- Per-server SQLite persistence (env values stored separately and
never returned in list/status responses — only key names)
- 10 RPCs under openhuman.mcp_clients_* covering registry browse,
install/uninstall, connect/disconnect, status, tool_call, and an
LLM-backed config_assist for credential walkthroughs
- DomainEvent::Mcp{ServerInstalled,Connected,Disconnected,
ClientToolExecuted} + bus subscriber
- tool_registry surfaces connected MCP-client tools to the agent
with route { protocol: "mcp-client", rpc_method, server_id, tool_name }
- json_rpc_e2e lifecycle test + ~60 unit tests across the domain
React UI (app/src/components/channels/mcp/):
- New "MCP Servers" tab in the Channels page (virtual tab — not a
backend channel definition)
- Catalog browser with debounced Smithery search + pagination
- Install dialog with per-required-env-key inputs (password type,
values never logged or displayed back)
- Installed-server list + detail with Connect/Disconnect/Uninstall
- Inline config assistant chat that calls config_assist and can
surface suggested env values
- Typed RPC wrapper in services/api/mcpClientsApi.ts
- Vitest suites for the wrapper and each non-trivial component
- McpCatalogBrowser.test.tsx: switch back to real timers after
`advanceTimersByTimeAsync` so `waitFor` polling is not deadlocked
by fake timers (4 tests previously timed out at 30s).
- json_rpc_e2e.rs (mcp_clients_lifecycle): unwrap the
`{ result, logs }` envelope that `RpcOutcome::into_cli_compatible_json`
produces when handlers emit log lines — `installed_list` and `status`
now return logs, so the bare `installed` / `servers` arrays sit one
level deeper.
A fast MCP server could reply between `proc.writer.send(&msg)` and the oneshot insertion into `pending` inside `send_request_and_wait`, causing the reader to drop the response and the call to hang until the request timeout fires. Move the write into the future passed to `send_request_and_wait` so the pending entry is always registered first. Addresses CodeRabbit comment on src/openhuman/mcp_clients/client/mod.rs:125 (also covers L146-153 and L176-181 in the same file).
7 tasks
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)
📝 WalkthroughWalkthroughAdds end-to-end MCP clients support: Rust backend (registry HTTP client, SQLite store, stdio JSON-RPC transport, client, connections, ops, schemas), event/catalog integration, typed frontend API, channel wiring for a virtual ChangesMCP Client Feature Implementation
Sequence DiagramsequenceDiagram
participant User
participant UI as Frontend UI
participant API as mcpClientsApi
participant RPC as JSON-RPC Server
participant Ops as mcp_clients::ops
participant Conn as connections
participant Registry as Smithery Registry
participant Store as store
rect rgba(100, 149, 237, 0.5)
User->>UI: Browse catalog / Install
UI->>API: registrySearch / registryGet / install
API->>RPC: openhuman.mcp_clients_registry_search / _registry_get / _install
RPC->>Ops: mcp_clients_registry_search / mcp_clients_registry_get / mcp_clients_install
Ops->>Registry: registry_search / registry_get
Ops->>Store: insert_server + set_env_values
Store-->>Ops: success
Ops-->>RPC: RpcOutcome
RPC-->>API: result
API-->>UI: render results / onSuccess
end
rect rgba(144, 238, 144, 0.5)
User->>UI: Connect / Call tool
UI->>API: connect(server_id)
API->>RPC: openhuman.mcp_clients_connect
RPC->>Ops: mcp_clients_connect
Ops->>Conn: connect (spawn McpStdioClient, initialize)
Conn-->>Ops: tools
Ops-->>RPC: RpcOutcome{status, tools}
UI->>API: toolCall(...)
API->>RPC: openhuman.mcp_clients_tool_call
RPC->>Ops: mcp_clients_tool_call
Ops->>Conn: call_tool
Conn-->>Ops: result
Ops-->>RPC: RpcOutcome{is_error,result}
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
|
![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: 15
🧹 Nitpick comments (2)
src/openhuman/mcp_clients/connections.rs (1)
142-161: 💤 Low valueConsider releasing the read lock before awaiting
tools_snapshot().Holding the
RwLockread guard across the.awaitat line 154 blocks writers for the duration of alltools_snapshot()calls. Since you already collected the server IDs in the first block, you could fetch each client individually outside the lock.♻️ Suggested refactor
pub async fn all_connected_tools() -> Vec<(String, String, McpTool)> { - let installed_ids: Vec<(String, String)> = { + let clients: Vec<(String, Arc<McpStdioClient>)> = { let map = connections().read().await; - map.keys().map(|id| (id.clone(), id.clone())).collect() + map.iter().map(|(id, c)| (id.clone(), Arc::clone(c))).collect() }; - // We need server metadata too — fetch from a mini-cache in the connections map. - // For simplicity, return server_id as qualified_name here; ops.rs enriches it. let mut result = Vec::new(); - let map = connections().read().await; - for (server_id, _) in &installed_ids { - if let Some(client) = map.get(server_id) { - let tools = client.tools_snapshot().await; - for tool in tools { - result.push((server_id.clone(), server_id.clone(), tool)); - } + for (server_id, client) in clients { + let tools = client.tools_snapshot().await; + for tool in tools { + result.push((server_id.clone(), server_id.clone(), tool)); } } result }🤖 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 `@src/openhuman/mcp_clients/connections.rs` around lines 142 - 161, all_connected_tools currently holds the RwLock read guard across an await when calling client.tools_snapshot(), blocking writers; change it to acquire the client reference (clone or Arc clone the McpClient) while holding the read lock, then drop the lock before awaiting tools_snapshot() so each tools_snapshot() call happens without the lock held—use connections().read().await to fetch and clone the client for each server_id, release the guard, then call client.tools_snapshot() and push results.tests/json_rpc_e2e.rs (1)
6310-6451: ⚡ Quick winAdd a true happy-path install lifecycle assertion.
This test currently validates registration/error/no-op behavior, but it doesn’t execute a successful
openhuman.mcp_clients_installfollowed byinstalled_listanduninstallverification. That misses the primary UI path this PR introduces.As per coding guidelines, “Extend
tests/json_rpc_e2e.rsandscripts/test-rust-with-mock.shfor new RPC methods to verify they match what the UI will call before surfacing in the frontend.”🤖 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 `@tests/json_rpc_e2e.rs` around lines 6310 - 6451, Add a true happy-path flow: call post_json_rpc to invoke "openhuman.mcp_clients_install" with a valid install payload, use assert_no_jsonrpc_error to unwrap the response, capture the returned server_id (or inspect result for the installed record), then call "openhuman.mcp_clients_installed_list" and assert the "installed" array contains an entry matching that server_id and expected fields, next call "openhuman.mcp_clients_uninstall" with that server_id and assert the response indicates removal (e.g., "removed": true or result present), and finally call "openhuman.mcp_clients_installed_list" again to assert the list is empty; use the existing helpers post_json_rpc and assert_no_jsonrpc_error and reference the JSON fields "installed", "server_id", and "removed" when making assertions, and also update scripts/test-rust-with-mock.sh to ensure the new RPC method is exercised in CI as per guidelines.
🤖 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 `@app/src/components/channels/mcp/InstallDialog.tsx`:
- Around line 39-57: The fetch for registry details can race when qualifiedName
changes: responses from an earlier mcpClientsApi.registryGet may arrive after a
later selection and incorrectly overwrite state; to fix, add a per-request guard
(e.g., a local requestId counter or AbortController token) inside the effect
that calls mcpClientsApi.registryGet and only call setDetail, setEnvValues,
setDetailError, and setLoadingDetail if the token matches the latest request (or
the abort signal is not aborted); ensure the guard is created at the start of
the effect and cleaned up in the effect cleanup so stale promises are ignored
when qualifiedName or prefillEnv changes.
- Around line 35-38: InstallDialog's useEffect that calls
mcpClientsApi.registryGet can suffer from stale async responses overwriting
state; guard the effect by tracking a local cancellation/sequence token (or an
isMounted flag) inside the effect and, before calling setLoadingDetail,
setDetailError, setDetail/envValues, only apply results when the token matches
(or isMounted is true). Specifically update the effect that calls
mcpClientsApi.registryGet so it generates a unique requestId (or sets let
cancelled=false), capture it in the closure, check it before calling
setLoadingDetail/setDetailError/setEnvValues, and cancel/mark cancelled in the
cleanup function to prevent late responses from updating state after
qualifiedName changes or unmount.
In `@app/src/components/channels/mcp/InstalledServerDetail.tsx`:
- Around line 27-33: The tools array is stored in local state (tools) and can
remain stale after a disconnect; when rendering the tool list or passing it to
child components (e.g., where tools are mapped or used around the block that
currently reads tools and the UI at the bottom of the component), only provide
tools when the current connection status variable (status) equals 'connected' —
e.g., replace usages with a gated value (status === 'connected' ? tools : []) so
the UI shows an empty list when disconnected or errored and similarly apply this
gating to the other tools-rendering spot referenced in the component.
In `@app/src/components/channels/mcp/McpCatalogBrowser.tsx`:
- Around line 70-76: The search input in McpCatalogBrowser relies on placeholder
text only; add an explicit accessible label by assigning an id to the input and
either adding a visible <label> tied to that id or adding an aria-label (e.g.,
"Search Smithery catalog") to the same input (the element that uses
value={query} and onChange={e => setQuery(e.target.value)}), so screen readers
get a clear label.
- Around line 29-50: The fetchPage function can apply out-of-order (stale)
responses and overwrite newer results; to fix, add a local request sequence
token or AbortController that is updated for each call (e.g., latestRequestIdRef
or latestAbortRef) and capture it before awaiting mcpClientsApi.registrySearch,
then when the response returns check the token/abort state and only call
setTotalPages, setPage, and setServers if the token still matches (or if the
request was not aborted); ensure you increment/reset the token or abort previous
controller at the start of fetchPage so slower prior responses are ignored and
do not update state.
In `@app/src/components/channels/mcp/McpServersTab.tsx`:
- Around line 37-44: The loadError state is only set on failure and never
cleared on success; after successfully fetching installed servers with
mcpClientsApi.installedList() and calling setServers(installed) (inside the try
success path), call setLoadError('') (or null depending on your state type) to
clear any previous error and stop the error banner from persisting; update the
success path surrounding setServers and log to also reset load error.
- Around line 58-61: Remove the redundant initial setLoading(true) call inside
the useEffect in McpServersTab: since loading is initialized to true, delete the
setLoading(true) line and leave the Promise.all([loadInstalled(),
fetchStatuses()]).finally(() => setLoading(false)); so the effect only triggers
the async loads; locate the useEffect that references setLoading, loadInstalled,
and fetchStatuses and remove the single setLoading(true) statement.
In `@app/src/store/channelConnectionsSlice.ts`:
- Around line 34-36: The migration adds an initial-state key mcp with
makeEmptyChannelModes(), but completeBreakingMigration doesn’t set
connections.mcp for persisted stores, leaving connections.mcp undefined; update
completeBreakingMigration to ensure it creates or merges a default mcp entry
(using makeEmptyChannelModes()) into the restored connections object so
pre-existing persisted state gets connections.mcp initialized the same way the
new initial state does.
In `@src/openhuman/mcp_clients/client/transport.rs`:
- Around line 68-72: The trace currently logs raw subprocess output via
tracing::trace with server_id and the full (or truncated) line; change it to
avoid emitting raw stdout/stderr by logging only metadata — e.g., server_id,
stream type (stdout/stderr), and line length — and redact or omit the content;
update the same pattern used at the similar tracing::trace call later (lines
around the second occurrence) so both occurrences use the new metadata-only
format and do not include the raw &line substring.
- Around line 64-102: The stdout reader currently exits without notifying
unresolved callers in the pending map; after the while loop finishes (just
before the tracing::debug exit log) acquire the pending mutex
(pending.lock().await), drain or take the map contents, and for each removed tx
call tx.send(Err(...)) with a clear error Value or string (e.g., indicating
EOF/process exit and including server_id) so all waiters receive an immediate
error instead of waiting for their timeouts; ensure you handle send errors by
ignoring the result. This change should be made in the same async task that
contains the lines.next_line() loop and references pending, tx, and server_id.
In `@src/openhuman/mcp_clients/ops.rs`:
- Around line 529-533: The code slices the UTF-8 string `text` by bytes in the
`tracing::warn!` call (`&text[..text.len().min(200)]`), which can panic if the
cut falls inside a multibyte character; change this to a character-safe
truncation (e.g., compute a safe preview with
`text.chars().take(200).collect::<String>()` or equivalent) and log that preview
instead of the byte-sliced `&text[.....]` — update the `tracing::warn!` site
where `status` and `text` are used so it logs the truncated, char-safe string.
- Around line 199-213: The conditional returning identical tuples should be
fixed: either collapse the duplicate branches into a single return that always
yields (CommandKind::Node, "npx".to_string(), vec!["-y".to_string(),
qualified_name.to_string()]) or implement the intended Python path by switching
the else branch to use "uvx" (and the appropriate CommandKind if needed) when
the package appears to be Python; locate the code around qualified_name,
CommandKind::Node, and the "npx"/"uvx" strings and update the else branch
accordingly so behavior matches the comment.
In `@src/openhuman/mcp_clients/store.rs`:
- Around line 227-239: set_env_values_conn currently upserts only the provided
keys so keys removed from the incoming env map remain in mcp_client_env; modify
set_env_values_conn to first delete all rows for the given server_id (e.g.
execute a DELETE FROM mcp_client_env WHERE server_id = ?1) and then insert the
new key/value pairs, and wrap the delete+inserts in a transaction on the
provided Connection to keep the operation atomic (use the existing conn and the
function name set_env_values_conn to locate where to add the deletion and
transaction).
- Around line 193-197: The current parsing of args_json, env_keys_json, and
config_json silently defaults on malformed JSON (using unwrap_or_default and
.ok()), which can mask data corruption; change the parsing for args: Vec<String>
(args_json), env_keys: Vec<String> (env_keys_json), and config: Option<Value>
(config_json) to fail fast by returning/propagating an error when
serde_json::from_str fails instead of defaulting — e.g., replace
unwrap_or_default and the .and_then(...).ok() pattern with proper error handling
that maps serde_json errors into the function's Result (including contextual
info like which column failed and the offending JSON string) so callers can
abort or handle the malformed data rather than proceeding with []/None.
In `@src/openhuman/tool_registry/ops.rs`:
- Around line 65-110: The dynamic MCP-client loop can insert duplicate tool_ids
and trigger assertions in insert_registry_entry; modify the logic that processes
client_tools so that before calling insert_registry_entry(&mut entries, entry,
"mcp_client") you check for an existing entry with the same tool_id in entries
(compare entry.tool_id) and skip or dedupe (e.g., keep the first/last) to avoid
panics; keep the current assert-based behavior for static sources but ensure the
MCP-client branch (the loop over client_tools and the tool_id variable /
ToolRegistryEntry creation) performs the de-duplication check and only calls
insert_registry_entry when the tool_id is not already present.
---
Nitpick comments:
In `@src/openhuman/mcp_clients/connections.rs`:
- Around line 142-161: all_connected_tools currently holds the RwLock read guard
across an await when calling client.tools_snapshot(), blocking writers; change
it to acquire the client reference (clone or Arc clone the McpClient) while
holding the read lock, then drop the lock before awaiting tools_snapshot() so
each tools_snapshot() call happens without the lock held—use
connections().read().await to fetch and clone the client for each server_id,
release the guard, then call client.tools_snapshot() and push results.
In `@tests/json_rpc_e2e.rs`:
- Around line 6310-6451: Add a true happy-path flow: call post_json_rpc to
invoke "openhuman.mcp_clients_install" with a valid install payload, use
assert_no_jsonrpc_error to unwrap the response, capture the returned server_id
(or inspect result for the installed record), then call
"openhuman.mcp_clients_installed_list" and assert the "installed" array contains
an entry matching that server_id and expected fields, next call
"openhuman.mcp_clients_uninstall" with that server_id and assert the response
indicates removal (e.g., "removed": true or result present), and finally call
"openhuman.mcp_clients_installed_list" again to assert the list is empty; use
the existing helpers post_json_rpc and assert_no_jsonrpc_error and reference the
JSON fields "installed", "server_id", and "removed" when making assertions, and
also update scripts/test-rust-with-mock.sh to ensure the new RPC method is
exercised in CI as per guidelines.
🪄 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: cb59a909-0261-4028-ad0a-2b182a03480b
📒 Files selected for processing (37)
app/src/components/channels/ChannelConfigPanel.tsxapp/src/components/channels/ChannelSelector.tsxapp/src/components/channels/mcp/ConfigAssistantPanel.test.tsxapp/src/components/channels/mcp/ConfigAssistantPanel.tsxapp/src/components/channels/mcp/InstallDialog.test.tsxapp/src/components/channels/mcp/InstallDialog.tsxapp/src/components/channels/mcp/InstalledServerDetail.test.tsxapp/src/components/channels/mcp/InstalledServerDetail.tsxapp/src/components/channels/mcp/InstalledServerList.tsxapp/src/components/channels/mcp/McpCatalogBrowser.test.tsxapp/src/components/channels/mcp/McpCatalogBrowser.tsxapp/src/components/channels/mcp/McpServersTab.tsxapp/src/components/channels/mcp/McpStatusBadge.tsxapp/src/components/channels/mcp/McpToolList.tsxapp/src/components/channels/mcp/SmitheryServerCard.tsxapp/src/components/channels/mcp/types.tsapp/src/services/api/mcpClientsApi.test.tsapp/src/services/api/mcpClientsApi.tsapp/src/store/channelConnectionsSlice.tsapp/src/types/channels.tssrc/core/all.rssrc/core/event_bus/events.rssrc/openhuman/about_app/catalog.rssrc/openhuman/mcp_clients/bus.rssrc/openhuman/mcp_clients/client/mod.rssrc/openhuman/mcp_clients/client/protocol.rssrc/openhuman/mcp_clients/client/transport.rssrc/openhuman/mcp_clients/connections.rssrc/openhuman/mcp_clients/mod.rssrc/openhuman/mcp_clients/ops.rssrc/openhuman/mcp_clients/registry.rssrc/openhuman/mcp_clients/schemas.rssrc/openhuman/mcp_clients/store.rssrc/openhuman/mcp_clients/types.rssrc/openhuman/mod.rssrc/openhuman/tool_registry/ops.rstests/json_rpc_e2e.rs
…guards, and tests Rust core: - transport.rs: flush pending waiters with an error on stdout EOF so they don't leak until their own timeout fires; redact raw MCP subprocess output from logs (PII/secret risk); add rustdoc on both reader methods - ops.rs: collapse dead-code branch in resolve_command; fix byte-index slice panic in config_assist error logging (use char boundary via .chars().take(200)) - store.rs: propagate JSON deserialization errors in map_server_row instead of silently swallowing them; fix set_env_values_conn to DELETE-then-INSERT so removed keys don't linger - tool_registry/ops.rs: replace panic-on-duplicate assert with a warn+skip so external MCP servers reusing well-known tool IDs don't crash the process; update test accordingly Frontend: - InstallDialog.tsx: guard stale async responses with a latestQualifiedNameRef so switching servers quickly can't apply an old detail fetch - McpCatalogBrowser.tsx: add monotonic request sequence counter to discard stale search responses; add aria-label to search input - InstalledServerDetail.tsx: clear tools on disconnect; gate tool list render on status === 'connected' to prevent stale tools lingering after disconnect - McpServersTab.tsx: remove redundant setLoading(true) before async work (loading starts true); clear loadError on successful reload - channelConnectionsSlice.ts: add mcp entry to migration so persisted states from before PR tinyhumansai#2276 have a complete ChannelType record Tests (new): - InstalledServerList.test.tsx: 11 synchronous branch-coverage tests - McpToolList.test.tsx: 8 tests covering collapse/expand and display variants - McpServersTab.test.tsx: 12 tests covering load/error/pane/install/poll flows Fix two test failures: InstalledServerList and McpServersTab both have two "Browse catalog" buttons when servers is empty (header link + empty-state CTA); use getAllByRole with correct index in those assertions.
coderabbitai
Bot
added
the
feature
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/src/components/channels/mcp/InstallDialog.tsx (1)
34-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a per-run request token instead of
qualifiedName.This effect also reruns on
prefillEnv, so two in-flight fetches for the samequalifiedNamestill pass the current guard. A slower older response can overwrite newer prefill values or error/loading state, and there is still no cleanup to suppress late setters after unmount.💡 Minimal fix
- const latestQualifiedNameRef = useRef(qualifiedName); + const latestRequestRef = useRef(0); useEffect(() => { - latestQualifiedNameRef.current = qualifiedName; + const requestId = ++latestRequestRef.current; + let cancelled = false; setLoadingDetail(true); setDetailError(null); log('fetching detail for %s', qualifiedName); - const requestedName = qualifiedName; + const requestedName = qualifiedName; mcpClientsApi .registryGet(qualifiedName) .then(d => { - if (latestQualifiedNameRef.current !== requestedName) { + if (cancelled || latestRequestRef.current !== requestId) { log('discarding stale detail response for %s', requestedName); return; } setDetail(d); const initial: Record<string, string> = {}; for (const key of d.required_env_keys ?? []) { initial[key] = prefillEnv?.[key] ?? ''; } setEnvValues(initial); }) .catch(err => { - if (latestQualifiedNameRef.current !== requestedName) return; + if (cancelled || latestRequestRef.current !== requestId) return; const msg = err instanceof Error ? err.message : 'Failed to load server details'; setDetailError(msg); }) .finally(() => { - if (latestQualifiedNameRef.current === requestedName) { + if (!cancelled && latestRequestRef.current === requestId) { setLoadingDetail(false); } }); + + return () => { + cancelled = true; + }; }, [qualifiedName, prefillEnv]);🤖 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/InstallDialog.tsx` around lines 34 - 73, The effect should use a per-run request token instead of relying on qualifiedName: create a run-scoped identifier (e.g., requestId via a useRef like requestIdRef.current = Symbol() or incrementing number) at the start of the useEffect, capture it in requestedId, and replace comparisons against latestQualifiedNameRef.current with checks against requestIdRef.current to discard stale responses; also add a cleanup function that clears or increments the requestId (so any in-flight promises won’t call setState after unmount) and ensure the .finally, .then and .catch handlers compare the saved requestedId to requestIdRef.current before calling setDetail, setEnvValues, setDetailError or setLoadingDetail. Reference: useEffect, latestQualifiedNameRef (replace/augment with requestIdRef), mcpClientsApi.registryGet, and the handlers that call setDetail/setEnvValues/setDetailError/setLoadingDetail.
🧹 Nitpick comments (1)
src/openhuman/mcp_clients/client/transport.rs (1)
64-116: ⚡ Quick winLog
next_line()failures before exiting the reader tasks.Both loops treat I/O errors the same as EOF, so a broken stdout/stderr pipe exits without recording why. Switching to an explicit
matchhere would preserve the useful metadata-only logging you added and make transport failures diagnosable.💡 Minimal fix
- while let Ok(Some(line)) = lines.next_line().await { + loop { + let line = match lines.next_line().await { + Ok(Some(line)) => line, + Ok(None) => break, + Err(err) => { + tracing::warn!( + "[mcp-client] server_id={} stdout read error: {}", + server_id, + err + ); + break; + } + }; if line.trim().is_empty() { continue; } // ... }Apply the same pattern in
spawn_stderr_reader.As per coding guidelines: “Use
log/tracingatdebugortracelevel on RPC entry and exit, error paths, state transitions, and any branch that is hard to infer from tests alone.”Also applies to: 123-140
🤖 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 `@src/openhuman/mcp_clients/client/transport.rs` around lines 64 - 116, The stdout reader treats lines.next_line().await errors as EOF, losing useful I/O error details; change the while let Ok(Some(line)) = lines.next_line().await into an explicit match on lines.next_line().await so you can handle Ok(Some(line)) as before, Ok(None) as EOF, and Err(e) by logging the error (e.g., tracing::warn/debug with server_id and e) before breaking; ensure you still flush pending waiters (pending.lock().await drain) with the same error message when exiting due to an Err, and apply the identical pattern to the spawn_stderr_reader path so stderr I/O errors are also logged and pending txs are flushed consistently.
🤖 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 `@app/src/components/channels/mcp/McpServersTab.test.tsx`:
- Around line 178-210: The test currently never asserts that the transient error
is cleared; after simulating the successful second mockInstalledList/mockInstall
flow in the McpServersTab test, wait for and assert that the error UI is removed
(e.g., use waitFor or waitForElementToBeRemoved to assert
screen.queryByText('Transient error') is not in the document) so the test
actually verifies the setLoadError(null)/loadInstalled success path; locate the
test in McpServersTab.test.tsx and update the final assertion to await the
removal of the 'Transient error' text instead of asserting it is still present.
In `@app/src/components/channels/mcp/McpToolList.test.tsx`:
- Around line 49-61: The test in McpToolList.test.tsx computes `descriptions`
(by filtering listitems for a second paragraph) but then discards it; replace
the unused suppression with an assertion that verifies the expected count (e.g.,
expect(descriptions).toHaveLength(2)) so the test actually checks that only two
items have description paragraphs; update the test around the `descriptions`
variable and remove `void descriptions`.
In `@src/openhuman/tool_registry/ops.rs`:
- Around line 121-129: The current duplicate handling uses
entries.insert(key.clone(), entry) which already overwrites the existing value,
contrary to the log message saying "skipping"; change the logic in the block
around entries.insert / key / entry / source so you detect existence before
inserting (e.g., check entries.contains_key(&key) or use the Entry API) and only
log-and-skip when a duplicate exists, otherwise insert the new entry; ensure the
log remains the same and no replacement occurs for duplicates.
---
Duplicate comments:
In `@app/src/components/channels/mcp/InstallDialog.tsx`:
- Around line 34-73: The effect should use a per-run request token instead of
relying on qualifiedName: create a run-scoped identifier (e.g., requestId via a
useRef like requestIdRef.current = Symbol() or incrementing number) at the start
of the useEffect, capture it in requestedId, and replace comparisons against
latestQualifiedNameRef.current with checks against requestIdRef.current to
discard stale responses; also add a cleanup function that clears or increments
the requestId (so any in-flight promises won’t call setState after unmount) and
ensure the .finally, .then and .catch handlers compare the saved requestedId to
requestIdRef.current before calling setDetail, setEnvValues, setDetailError or
setLoadingDetail. Reference: useEffect, latestQualifiedNameRef (replace/augment
with requestIdRef), mcpClientsApi.registryGet, and the handlers that call
setDetail/setEnvValues/setDetailError/setLoadingDetail.
---
Nitpick comments:
In `@src/openhuman/mcp_clients/client/transport.rs`:
- Around line 64-116: The stdout reader treats lines.next_line().await errors as
EOF, losing useful I/O error details; change the while let Ok(Some(line)) =
lines.next_line().await into an explicit match on lines.next_line().await so you
can handle Ok(Some(line)) as before, Ok(None) as EOF, and Err(e) by logging the
error (e.g., tracing::warn/debug with server_id and e) before breaking; ensure
you still flush pending waiters (pending.lock().await drain) with the same error
message when exiting due to an Err, and apply the identical pattern to the
spawn_stderr_reader path so stderr I/O errors are also logged and pending txs
are flushed consistently.
🪄 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: bb74edba-b00d-45a5-bbce-4bdba41ca5ac
📒 Files selected for processing (12)
app/src/components/channels/mcp/InstallDialog.tsxapp/src/components/channels/mcp/InstalledServerDetail.tsxapp/src/components/channels/mcp/InstalledServerList.test.tsxapp/src/components/channels/mcp/McpCatalogBrowser.tsxapp/src/components/channels/mcp/McpServersTab.test.tsxapp/src/components/channels/mcp/McpServersTab.tsxapp/src/components/channels/mcp/McpToolList.test.tsxapp/src/store/channelConnectionsSlice.tssrc/openhuman/mcp_clients/client/transport.rssrc/openhuman/mcp_clients/ops.rssrc/openhuman/mcp_clients/store.rssrc/openhuman/tool_registry/ops.rs
The previous implementation called `entries.insert(...)` then checked the return value, which silently overwrote the original entry with the duplicate before warning. Test `insert_registry_entry_skips_duplicate_tool_id` (added in 8e03804) caught this — it asserts the first entry is preserved. Fix: check `contains_key` first, log+return on duplicate, only insert when the slot is empty.
- McpServersTab.test "clears load error on successful reload": rewrite to actually drive an install through to completion and assert the error banner is gone afterward (previously asserted the error was still present, which contradicted the test name). - McpToolList.test "no description paragraph": replace `void descriptions` with `expect(descriptions).toHaveLength(2)` so the computed value is actually verified.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/openhuman/tool_registry/ops.rs (1)
121-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep fail-fast duplicate handling for internal registry sources.
insert_registry_entry()is shared bymcp_stdio,controller, andmcp_client. With the new unconditional skip path here, an internaltool_idcollision now just logs and drops one built-in tool instead of surfacing a hard failure, which can hide a real registry regression and make a tool disappear in production. Please keep first-write-wins only for the externalmcp_clientsource and preserve assert/panic behavior for internal sources.Suggested fix
fn insert_registry_entry( entries: &mut BTreeMap<String, ToolRegistryEntry>, entry: ToolRegistryEntry, source: &str, ) { let key = entry.tool_id.clone(); if entries.contains_key(&key) { - // Duplicate tool IDs can arrive from external MCP servers that reuse - // well-known names. First-write-wins: log and skip the duplicate - // rather than panicking or silently overwriting in production. - log::warn!( - "[tool_registry] duplicate tool_id={} from source={}; skipping", - key, - source - ); - return; + if source == "mcp_client" { + log::warn!( + "[tool_registry] duplicate tool_id={} from source={}; skipping", + key, + source + ); + return; + } + + panic!( + "[tool_registry] duplicate tool_id={} from source={}", + key, + source + ); } entries.insert(key, entry); }🤖 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 `@src/openhuman/tool_registry/ops.rs` around lines 121 - 132, The current insert_registry_entry() unconditionally logs and skips duplicate keys; change it so duplicates are tolerated (first-write-wins) only when source == "mcp_client" (or the enum/variant representing external MCP client), but for internal sources (e.g., "mcp_stdio" and "controller" or their enum variants) keep the fail-fast behavior by asserting/panicking or returning an error; locate the duplicate-check using entries.contains_key(&key) and key/source and make the branch conditional on source to either log-and-return for the external mcp_client case or panic/assert for internal sources.
🤖 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.
Duplicate comments:
In `@src/openhuman/tool_registry/ops.rs`:
- Around line 121-132: The current insert_registry_entry() unconditionally logs
and skips duplicate keys; change it so duplicates are tolerated
(first-write-wins) only when source == "mcp_client" (or the enum/variant
representing external MCP client), but for internal sources (e.g., "mcp_stdio"
and "controller" or their enum variants) keep the fail-fast behavior by
asserting/panicking or returning an error; locate the duplicate-check using
entries.contains_key(&key) and key/source and make the branch conditional on
source to either log-and-return for the external mcp_client case or panic/assert
for internal sources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e6b0a4b-7837-44f7-b92c-483bb80fe2fe
📒 Files selected for processing (1)
src/openhuman/tool_registry/ops.rs
CodeGhost21
pushed a commit
to CodeGhost21/openhuman
that referenced
this pull request
May 22, 2026
This was referenced May 22, 2026
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
1 participant
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
Adds an MCP client subsystem: browse the Smithery.ai MCP server registry from the Connections page, install servers locally, spawn them over stdio via the existing JS/Python runtimes, and expose their tools to the agent.
src/openhuman/mcp_clients/): Smithery registry client with SQLite cache, stdio MCP JSON-RPC client (initialize / tools/list / tools/call) behind aMcpTransporttrait so tests can inject a fake, per-server SQLite persistence (env values stored separately and never returned in list/status responses — only key names), 10 RPCs underopenhuman.mcp_clients_*,DomainEvent::Mcp*lifecycle events, and tool-registry surfacing of connected MCP tools to the agent.app/src/components/channels/mcp/): new "MCP Servers" virtual tab in the Channels page — catalog browser (debounced search + pagination), install dialog with required env-var form, installed-server list + detail with Connect/Disconnect/Uninstall, and an inline LLM-driven config assistant (config_assist) that can surface suggested env values.json_rpc_e2elifecycle test; Vitest suites for the typed RPC wrapper and each non-trivial component.RPC surface
All
openhuman.mcp_clients_*:registry_search,registry_get,installed_list,install,uninstall,connect,disconnect,status,tool_call,config_assist.Test plan
cargo check --manifest-path Cargo.toml— cleancargo test --manifest-path Cargo.toml -p openhuman mcp_clients— all unit tests passpnpm test— Vitest suites for new components pass (3027 passed locally on this branch)pnpm typecheck— cleanpnpm lint— clean on new files (2 warnings match an existing repo-widereact-hooks/set-state-in-effectpattern)@modelcontextprotocol/server-filesystem), provide env vars, connect, see tool list, agent can call the toolsuggested_envpopulatedSummary by CodeRabbit
New Features
Tests