feat(mcp-clients): MCP client subsystem with Smithery registry + UI#2276
Closed
senamakel wants to merge 1 commit into
Closed
feat(mcp-clients): MCP client subsystem with Smithery registry + UI#2276senamakel wants to merge 1 commit into
senamakel wants to merge 1 commit into
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
Contributor
📝 WalkthroughWalkthroughThis PR introduces comprehensive MCP (Model Context Protocol) client support to Openhuman, enabling users to browse and install MCP servers from the Smithery registry, manage their lifecycle, and expose their tools to agents. The implementation spans a new virtual "mcp" channel in the UI with server catalog browsing, installation dialogs, and connection management, alongside a complete Rust backend featuring JSON-RPC stdio transport, Smithery HTTP client, SQLite persistence, and RPC handler integration. ChangesMCP Client Server Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![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
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/store/channelConnectionsSlice.ts (1)
61-76:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd
mcpinitialization incompleteBreakingMigration.The migration initializes
larkanddingtalk(lines 71-72) to prevent crashes whenupsertChannelConnectionaccessesstate.connections[channel]on rehydrated states created before those channels existed. However,mcpis missing from the migration despite being added toinitialState(line 36). This creates the same crash risk: when redux-persist rehydrates old state without themcpkey, any reducer accessingstate.connections.mcpwill throwCannot read properties of undefined.🔧 Proposed fix
state.connections.lark = makeEmptyChannelModes(); state.connections.dingtalk = makeEmptyChannelModes(); + state.connections.mcp = makeEmptyChannelModes(); state.defaultMessagingChannel = 'telegram';🤖 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/store/channelConnectionsSlice.ts` around lines 61 - 76, completeBreakingMigration currently initializes new channels but omits mcp, causing rehydration to leave state.connections.mcp undefined; update the completeBreakingMigration function to also set state.connections.mcp = makeEmptyChannelModes() (matching how lark and dingtalk are initialized) so the migration mirrors initialState and prevents crashes when reducers access state.connections.mcp.
🟠 Major comments (20)
src/openhuman/about_app/catalog.rs-968-977 (1)
968-977:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
channels.mcp_tool_callprivacy metadata likely under-reports data egress risk.Marking this as
privacy: Noneis risky because MCP tool execution can forward user/tool payloads to third-party APIs via the connected server. The catalog should declare that potential data egress path.🤖 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/about_app/catalog.rs` around lines 968 - 977, The Capability entry for id "channels.mcp_tool_call" currently sets privacy: None but must reflect potential data egress to third-party MCP servers; update the Capability struct for the entry with id "channels.mcp_tool_call" to set a non-None privacy value that indicates external egress (e.g., a Privacy enum/variant for third‑party egress or a descriptive privacy note), and ensure the privacy field explicitly documents that tool payloads may be forwarded to connected MCP servers/third-party APIs.src/openhuman/mcp_clients/bus.rs-16-22 (1)
16-22: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAlign subscriber name/domain prefix with the module domain naming.
Use
mcp_clients::...naming to match the domain folder and keep grep-friendly consistency with the repo’s bus naming rule.As per coding guidelines, “Name the handler struct
<Purpose>Subscriberand thename()return value"<domain>::<purpose>"for grep-friendly tracing output.”🤖 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/bus.rs` around lines 16 - 22, The subscriber name() and domains() values should use the module domain prefix "mcp_clients" for grep-friendly consistency: update the name() return value to "mcp_clients::lifecycle" and change domains() to return Some(&["mcp_clients"]); also ensure the handler struct follows the `<Purpose>Subscriber` convention (e.g., rename to LifecycleSubscriber if not already) so the code and tracing output match the repo bus naming rule; locate these changes in the functions name() and domains() within the subscriber implementation.src/openhuman/mcp_clients/bus.rs-30-53 (1)
30-53: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse debug/trace level for MCP lifecycle diagnostic logs.
These state-transition logs are currently emitted at
info; repo guidance asks for diagnostic flow logs atdebug/trace.As per coding guidelines, “Use
log/tracingatdebugortracelevel on RPC entry and exit, error paths, state transitions...”.🤖 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/bus.rs` around lines 30 - 53, Change the lifecycle logs in the DomainEvent match arms from info to a lower diagnostic level: replace tracing::info! with tracing::debug! (or tracing::trace! if you prefer more verbosity) in the handlers for DomainEvent::McpServerInstalled (the arm that logs server_id and qualified_name), DomainEvent::McpServerConnected (the arm that logs server_id and tool_count), and DomainEvent::McpServerDisconnected (the arm that logs server_id and reason); retain the structured fields and messages exactly as they are, only update the tracing macro to the appropriate debug/trace level to follow the repository guideline for RPC/state-transition diagnostics.src/openhuman/mcp_clients/schemas.rs-393-513 (1)
393-513: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd RPC entry/exit/error tracing in MCP handlers.
These handlers currently do not emit debug/trace logs on entry/exit/error paths, which makes MCP flow diagnostics harder.
As per coding guidelines, “Use
log/tracingatdebugortracelevel on RPC entry and exit, error paths, state transitions...”.🤖 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/schemas.rs` around lines 393 - 513, Add debug/trace entry, successful-exit, and error logs for each MCP handler (handle_registry_search, handle_registry_get, handle_installed_list, handle_install, handle_uninstall, handle_connect, handle_disconnect, handle_status, handle_tool_call, handle_config_assist): log at the start of the async block with the RPC name and key input fields (e.g., query, qualified_name, server_id, tool_name, env keys, user_message), log a successful exit just before calling to_json/return with a brief result marker, and log errors on failure paths including the error details; use the project's logging/tracing macros (tracing::debug!/trace! or log::debug!) and include contextual fields to aid filtering. Ensure logs are non-blocking and avoid leaking sensitive values (mask env/config as needed), and place them inside each Box::pin(async move { ... }) block surrounding the existing reads and ops calls.tests/json_rpc_e2e.rs-6310-6447 (1)
6310-6447:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd e2e coverage for all new UI-facing MCP RPCs, not just a subset.
This test currently skips
openhuman.mcp_clients_install,openhuman.mcp_clients_registry_get, andopenhuman.mcp_clients_config_assist, so it doesn’t fully validate the frontend-call surface introduced in this PR.As per coding guidelines
tests/json_rpc_e2e.rs: “Extendtests/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 - 6447, The test mcp_clients_lifecycle is missing coverage for openhuman.mcp_clients_install, openhuman.mcp_clients_registry_get, and openhuman.mcp_clients_config_assist; add calls using the same post_json_rpc helper and assert_no_jsonrpc_error patterns (with unique request ids like the existing 9901..9907), validate expected result or error shapes (e.g., installed list changes after install, registry_get returns result/error, config_assist returns suggestions or error), and mirror the style used for registry_search/tool_call/disconnect; also update scripts/test-rust-with-mock.sh to ensure the test harness exercises these new RPCs so CI runs the extended e2e coverage.tests/json_rpc_e2e.rs-6374-6393 (1)
6374-6393:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTautological assertions here won’t catch real regressions.
Line 6376 and Line 6390 assert
result || error, which is effectively always true for JSON-RPC responses. This can let unknown-method/dispatch regressions pass undetected.Suggested tightening
- assert!( - uninstall_missing.get("result").is_some() || uninstall_missing.get("error").is_some(), - "uninstall missing server should return result or error: {uninstall_missing}" - ); + if uninstall_missing.get("result").is_none() { + let err = assert_jsonrpc_error(&uninstall_missing, "mcp_clients_uninstall missing"); + let msg = err.get("message").and_then(Value::as_str).unwrap_or_default(); + assert!( + !msg.to_ascii_lowercase().contains("unknown method"), + "method must be registered; got dispatch error: {uninstall_missing}" + ); + } @@ - assert!( - search.get("result").is_some() || search.get("error").is_some(), - "registry_search should return result or error: {search}" - ); + if search.get("result").is_none() { + let err = assert_jsonrpc_error(&search, "mcp_clients_registry_search"); + let msg = err.get("message").and_then(Value::as_str).unwrap_or_default(); + assert!( + !msg.to_ascii_lowercase().contains("unknown method"), + "method must be registered; got dispatch error: {search}" + ); + }🤖 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 6374 - 6393, The current assertions for uninstall_missing and search are tautological; ensure the server actually registered the method by checking the JSON-RPC error code is not "Method not found" (-32601). Update the checks around uninstall_missing and the search response (from post_json_rpc / rpc_base for method "openhuman.mcp_clients_registry_search") to assert either a present "result" or that "error.code" exists and is not -32601 (i.e., fail the test if error.code == -32601), so unknown-method/dispatch regressions are detected.src/openhuman/mcp_clients/ops.rs-150-152 (1)
150-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake install persistence atomic (server row + env values).
If
set_env_valuesfails afterinsert_server, you can leave an installed server without required env state. Wrap both writes in one DB transaction path.🤖 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/ops.rs` around lines 150 - 152, The insert_server and set_env_values calls must be executed inside a single database transaction so the install is atomic: modify the install path that calls store::insert_server(config, &server) and store::set_env_values(config, &server_id, &env) to run within a single transactional API (e.g., store::transaction, store::with_transaction, or begin/commit/rollback on the store) so that if set_env_values fails the transaction is rolled back and the inserted server row is not persisted; ensure you use the same transaction/context for both insert_server and set_env_values and propagate/convert any transaction errors as before.src/openhuman/mcp_clients/ops.rs-255-267 (1)
255-267:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist
last_connected_aton successful connect.
store::update_last_connectedexists but is never called here, so connection history/state fields can stay stale after successful connections.Proposed fix
let tools = connections::connect(config, &server) .await .map_err(|e| e.to_string())?; + store::update_last_connected(config, server_id.trim()).map_err(|e| e.to_string())?; let tool_count = tools.len() as u32;🤖 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/ops.rs` around lines 255 - 267, The code obtains the server via store::get_server and then connects using connections::connect, but it never updates the server's connection timestamp; call store::update_last_connected (using the same server_id.trim() identity) after a successful connections::connect and before publishing DomainEvent::McpServerConnected so the server's last_connected_at is persisted; ensure you handle and propagate any error from store::update_last_connected consistently (map_err to string as done above) so failures to persist are surfaced like the other errors.src/openhuman/mcp_clients/store.rs-232-239 (1)
232-239:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake env upserts atomic with a transaction.
A failure mid-loop can leave partially written env state for a server. Wrap the batch in a transaction so install/config writes are all-or-nothing.
Proposed fix
pub fn set_env_values_conn( conn: &Connection, server_id: &str, env: &std::collections::HashMap<String, String>, ) -> Result<()> { - for (key, value) in env { - conn.execute( + let mut tx = conn.unchecked_transaction()?; + for (key, value) in env { + tx.execute( "INSERT OR REPLACE INTO mcp_client_env (server_id, key, value) VALUES (?1, ?2, ?3)", params![server_id, key, value], ) .context("Failed to upsert mcp_client_env")?; } + tx.commit()?; Ok(()) }🤖 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/store.rs` around lines 232 - 239, The env upsert loop currently calls conn.execute per-item which can leave partial state on failure; wrap the batch in a single transaction by creating a transaction (e.g., let tx = conn.transaction()? or conn.unchecked_transaction()?), replace conn.execute calls inside the loop with tx.execute (keeping the same SQL and params! usage for mcp_client_env upsert), and call tx.commit()? at the end; ensure errors returned are still context-wrapped (e.g., .context("Failed to upsert mcp_client_env")?) so the entire install/config write is atomic.src/openhuman/mcp_clients/ops.rs-199-212 (1)
199-212:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFallback command resolution always returns
npx.Both branches are identical, so non-npm/python-style packages never get a Python fallback despite the comment and intended behavior. This can make installs fail for python-first servers without
example_config.command.Proposed fix
- // Default: use npx for npm-style packages (starts with @), uvx for python - if qualified_name.starts_with('@') || !qualified_name.contains('/') { + // Default heuristic: scoped/npm-like names use npx, otherwise prefer uvx. + if qualified_name.starts_with('@') || qualified_name.contains('/') { ( CommandKind::Node, "npx".to_string(), vec!["-y".to_string(), qualified_name.to_string()], ) } else { ( - CommandKind::Node, - "npx".to_string(), - vec!["-y".to_string(), qualified_name.to_string()], + CommandKind::Python, + "uvx".to_string(), + vec![qualified_name.to_string()], ) }🤖 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/ops.rs` around lines 199 - 212, The fallback currently returns npx in both branches, so qualified_name-based resolution never falls back to Python; in the else branch change the tuple to return CommandKind::Python with the Python runner (e.g., "uvx") and appropriate args (include qualified_name via qualified_name.to_string()) instead of "npx" so that non-npm/python-style packages on python-first servers use the Python fallback; update the branch that currently mirrors the npm branch to construct (CommandKind::Python, "uvx".to_string(), vec![qualified_name.to_string()]) (and adapt args if your project expects different uvx invocation).src/openhuman/mcp_clients/store.rs-193-197 (1)
193-197:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not silently ignore malformed JSON columns.
Falling back to defaults hides data corruption and can produce wrong launch/config behavior without any error signal. Return a row-mapping error when JSON is invalid.
Proposed fix
- let args: Vec<String> = serde_json::from_str(&args_json).unwrap_or_default(); - let env_keys: Vec<String> = serde_json::from_str(&env_keys_json).unwrap_or_default(); - let config: Option<Value> = config_json - .as_deref() - .and_then(|s| serde_json::from_str(s).ok()); + let args: Vec<String> = serde_json::from_str(&args_json).map_err(|e| { + rusqlite::Error::FromSqlConversionFailure( + 7, + rusqlite::types::Type::Text, + Box::new(e), + ) + })?; + let env_keys: Vec<String> = serde_json::from_str(&env_keys_json).map_err(|e| { + rusqlite::Error::FromSqlConversionFailure( + 8, + rusqlite::types::Type::Text, + Box::new(e), + ) + })?; + let config: Option<Value> = match config_json { + Some(s) => Some(serde_json::from_str(&s).map_err(|e| { + rusqlite::Error::FromSqlConversionFailure( + 9, + rusqlite::types::Type::Text, + Box::new(e), + ) + })?), + None => None, + };🤖 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/store.rs` around lines 193 - 197, The current mapping silently swallows JSON parse errors by using unwrap_or_default() for args/env_keys and .ok() for config; change these to propagate a row-mapping error when parsing fails: parse args_json and env_keys_json with serde_json::from_str and, on Err, return a descriptive error (e.g., Err(RowMappingError::InvalidColumn("args", err.to_string()))), and for config_json use as_deref().and_then(...) but map parse failures to the same row-mapping error instead of .ok(); ensure the function signature returns a Result so callers receive the error.app/src/components/channels/mcp/McpCatalogBrowser.tsx-29-49 (1)
29-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProtect catalog state from out-of-order async responses.
Concurrent
registrySearchcalls (typing + load-more) can resolve out of order and replace newer results with stale data. Add a request sequence/ref guard before committing state.Also applies to: 53-61
🤖 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/McpCatalogBrowser.tsx` around lines 29 - 49, The fetchPage flow can commit stale results when concurrent mcpClientsApi.registrySearch calls resolve out of order; add a request sequence guard using a ref (e.g., latestRequestSeqRef) that you increment at the start of fetchPage, capture the current seq in a local variable, then only call setTotalPages, setPage, setServers, setError and setLoading if the captured seq still equals latestRequestSeqRef.current; apply the same guard to the other fetch/load-more routine around lines 53-61 so only the latest request can update component state.app/src/components/channels/mcp/McpServersTab.tsx-176-181 (1)
176-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConnection status can stay stale after Connect/Disconnect.
InstalledServerDetailtriggers connect/disconnect, butMcpServersTabnever refreshesstatuseson those actions. Since polling starts only whenstatusesalready include a connected server, first-time connect can leave badge/buttons in the wrong state indefinitely.Suggested direction
- <InstalledServerDetail + <InstalledServerDetail server={selectedServer} connStatus={selectedConnStatus} onUninstalled={serverId => void handleUninstalled(serverId)} + onConnectionChanged={() => void fetchStatuses()} />And call
onConnectionChanged()after successful connect/disconnect inInstalledServerDetail.Also applies to: 63-89
🤖 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/McpServersTab.tsx` around lines 176 - 181, The connection badge/buttons can become stale because McpServersTab does not refresh the server connection statuses after InstalledServerDetail performs connect/disconnect; update McpServersTab to accept and handle an onConnectionChanged callback (e.g. pass a prop named onConnectionChanged to InstalledServerDetail from the McpServersTab render where InstalledServerDetail is used) and implement that handler to refresh the local statuses state (re-run the fetch/poll logic that populates statuses); then modify InstalledServerDetail to call props.onConnectionChanged() after a successful connect or disconnect so McpServersTab immediately updates its statuses (also apply the same pattern to the other InstalledServerDetail usages around lines 63-89).app/src/components/channels/mcp/ConfigAssistantPanel.tsx-49-49 (1)
49-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not log raw assistant prompts/messages.
textmay contain API keys or other sensitive config values; logging it violates frontend secret-logging guidance.As per coding guidelines: "Never log secrets, raw JWTs, API keys, or full PII — redact or omit sensitive fields."Proposed fix
- log('sending message: %s', text); + log('sending message length=%d', text.length);🤖 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/ConfigAssistantPanel.tsx` at line 49, The code is logging raw assistant messages via the call log('sending message: %s', text) in ConfigAssistantPanel which may include secrets; replace this direct-log with a non-sensitive alternative by removing or redacting the raw `text` before logging (e.g., log only a masked snippet, message length, or a fixed notice like "assistant message sent" and preserve the `text` variable for sending), and update any related call sites in ConfigAssistantPanel to ensure no other logs emit the full `text`.app/src/components/channels/mcp/InstallDialog.tsx-39-57 (1)
39-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against stale
registryGetresponses whenqualifiedNamechanges.This effect can apply an out-of-date response after a rapid prop change, showing wrong env keys/details. Add a request token or cancellation flag and ignore late responses.
Proposed fix
useEffect(() => { + let active = true; setLoadingDetail(true); setDetailError(null); log('fetching detail for %s', qualifiedName); mcpClientsApi .registryGet(qualifiedName) .then(d => { + if (!active) return; setDetail(d); const initial: Record<string, string> = {}; for (const key of d.required_env_keys ?? []) { initial[key] = prefillEnv?.[key] ?? ''; } setEnvValues(initial); log('detail loaded, required_env_keys=%o', d.required_env_keys); }) .catch(err => { + if (!active) return; const msg = err instanceof Error ? err.message : 'Failed to load server details'; log('detail error: %s', msg); setDetailError(msg); }) - .finally(() => setLoadingDetail(false)); + .finally(() => { + if (active) setLoadingDetail(false); + }); + return () => { + active = false; + }; }, [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 39 - 57, The effect in InstallDialog.tsx can apply stale registryGet responses when qualifiedName changes; modify the useEffect that calls mcpClientsApi.registryGet(qualifiedName) to ignore late responses by adding a cancellation token or requestId (e.g., a local let cancelled = false or a ref-based seqId that you increment at each effect run), capture that token before calling registryGet, and in the .then/.catch/.finally handlers check the token matches (or !cancelled) before calling setDetail, setEnvValues, setDetailError, or setLoadingDetail; if the API supports AbortController, prefer passing a signal and aborting on cleanup. Ensure you clean up by setting the cancellation flag (or aborting) in the effect’s return cleanup.src/openhuman/mcp_clients/client/transport.rs-68-72 (1)
68-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop tracing raw stdout/stderr payloads from external MCP servers.
These lines can contain tool results, prompt content, stack traces, or echoed credentials. Please log structured metadata instead of the raw payload.
As per coding guidelines: "Never log secrets, raw JWTs, API keys, or full PII — redact or omit sensitive fields."
Also applies to: 113-117
🤖 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 68 - 72, The tracing::trace call currently logs raw stdout/stderr in the variable line (and similarly at the other site around lines 113–117); change these calls to never emit the raw payload. Instead log structured metadata: server_id, stream type (stdout/stderr), payload_length, a boolean truncated flag, and a non-reversible fingerprint or short hash (e.g., SHA256 hex-prefix) of the payload; optionally include a safe preview limited to N characters after redaction (no secrets). Implement or call a helper (e.g., redact_or_fingerprint_payload(line)) and replace the tracing::trace("[mcp-client] server_id={} stdout: {}", server_id, &line[.....]) usages so they emit only the safe metadata fields and not the raw line.src/openhuman/mcp_clients/connections.rs-47-56 (1)
47-56:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSerialize connect/disconnect per
server_id.The child is spawned and initialized before the registry is updated. That leaves a race where two concurrent
connect()calls can launch two processes for the same server, and adisconnect()can returnfalsewhile a connect is still in flight and about to publish the client.🤖 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 47 - 56, Race: connect() spawns McpStdioClient::spawn_and_init before updating the shared registry (connections()), allowing concurrent connects/disconnects to create duplicate processes. Fix by serializing per server_id: acquire the connections() write lock before starting spawn, check for existing entry for server.server_id and return early if present, insert a temporary "in-progress" marker (or reserved Arc placeholder) to reserve the id, release the lock, then call McpStdioClient::spawn_and_init; after successful spawn, replace the placeholder with the real Arc<Client> (and remove placeholder on failure). Ensure disconnect() also consults and respects the placeholder so it waits/fails appropriately for in-flight connects.src/openhuman/tool_registry/ops.rs-65-85 (1)
65-85:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t make the registry snapshot silently drop connected MCP-client tools.
These fallback branches return
Vec::new(), soregistry_entries()andget_tool()omit all connected client tools whenever this code runs without a multithread Tokio handle. That makes tool discovery depend on caller context instead of actual connection state.🤖 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 65 - 85, The code currently returns Vec::new() when not on a MultiThread tokio runtime, dropping connected MCP-client tools; instead always obtain the tools by awaiting connections::all_connected_tools(): when Handle::try_current() is Ok and runtime_flavor() == RuntimeFlavor::MultiThread keep the existing tokio::task::block_in_place(|| handle.block_on(connections::all_connected_tools())), but if the handle exists and is CurrentThread call handle.block_on(connections::all_connected_tools()) directly (no block_in_place), and if Handle::try_current() returns Err construct a new current-thread runtime (e.g. tokio::runtime::Builder::new_current_thread().enable_all().build()) and block_on(connections::all_connected_tools()) so client_tools is never silently replaced with Vec::new().src/openhuman/mcp_clients/connections.rs-38-39 (1)
38-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate env-store failures instead of connecting with an empty env.
unwrap_or_default()turns DB/read errors into{}, so a locked or corrupt store looks like “missing config” and the subprocess starts with broken auth instead of surfacing the real failure.💡 Proposed fix
- let env = store::load_env_values(config, &server.server_id).unwrap_or_default(); + let env = store::load_env_values(config, &server.server_id).map_err(|e| { + anyhow::anyhow!( + "[mcp-client] server_id={} failed to load env values: {e}", + server.server_id + ) + })?;🤖 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 38 - 39, The code silently converts store::load_env_values(...) errors into an empty env by calling unwrap_or_default() on the Result; instead propagate the real failure so callers see DB/read errors. Replace the unwrap_or_default() on load_env_values(config, &server.server_id) with proper error propagation (e.g., use ? or map_err and return a Result from the surrounding function), or convert the error into a meaningful propagated error type and return it from the function that performs the connection so the process does not continue with an empty env.src/openhuman/mcp_clients/client/transport.rs-64-101 (1)
64-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail in-flight RPCs immediately when the stdout reader exits.
When this loop reaches EOF or the child dies, it just logs and returns. Any request already parked in
pendingthen waits the full 30-second timeout even though the transport is already gone.💡 Proposed fix
while let Ok(Some(line)) = lines.next_line().await { if line.trim().is_empty() { continue; } // ... } + let pending = { + let mut map = pending.lock().await; + std::mem::take(&mut *map) + }; + for (id, tx) in pending { + let _ = tx.send(Err(format!( + "[mcp-client] stdout closed for server_id={} before response id={id}", + server_id + ))); + } tracing::debug!( "[mcp-client] stdout reader exiting for server_id={}", server_id );🤖 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 - 101, The stdout reader currently returns on EOF without notifying in-flight RPCs; update the end of the loop (after it exits and before logging the debug) to acquire the same pending lock (pending.lock().await), drain or iterate over all remaining entries, and send an immediate Err to each sender (tx) indicating the transport closed (include server_id in the error message or Value payload); ensure you handle send errors (ignore them) and remove entries from the map so callers wake up instead of waiting the full timeout—reference the pending variable, the tx send usage, and the server_id/logging area where the reader exits.
🟡 Minor comments (2)
src/openhuman/mcp_clients/schemas.rs-92-99 (1)
92-99:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSchema type and parser type are inconsistent for pagination params.
page/page_sizeare declared asU64in schema but parsed asu32, so valid schema-level values aboveu32::MAXare rejected at runtime. Please align both sides to one type.Also applies to: 397-398
🤖 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/schemas.rs` around lines 92 - 99, The schema declares the pagination fields "page" and "page_size" as TypeSchema::Option(Box::new(TypeSchema::U64)) but the runtime parser/handlers are using u32, causing values > u32::MAX to be rejected; fix this by making the runtime types and parsing use u64 (or alternatively change the schema to U32) so both sides match — update any parser code, deserialization, and struct fields that read/hold page and page_size from u32 to u64 (search for references to "page", "page_size" and any parsing code that casts to u32), and apply the same change to the other occurrence noted around the second instance (lines referenced 397-398) so both schema and code are consistent.src/openhuman/mcp_clients/store.rs-355-368 (1)
355-368:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest does not validate
delete_serverbehavior.This test currently executes raw SQL delete instead of calling
delete_server(or a*_connvariant), so regressions in the public delete path won’t be caught.🤖 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/store.rs` around lines 355 - 368, The test delete_server_returns_true_when_found is bypassing the public delete path by executing raw SQL; change it to call the appropriate delete helper (delete_server or delete_server_conn) on the test connection so the public logic is exercised. Use open_test_conn(), insert_server_conn(&conn, &server) as setup, then call delete_server_conn(&conn, "srv-del") (or delete_server in the module if it accepts a connection) and assert the returned result equals true or 1 as intended; keep sample_server("srv-del") for the inserted fixture so the test validates the actual delete_server behavior.
🤖 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_clients/client/mod.rs`:
- Around line 117-125: The write is performed before the request waiter is
registered, risking a lost fast reply; change the sequence in the send path so
the pending oneshot is inserted into the pending map before calling
proc.writer.send(&msg). Concretely, in the block using self.process.lock().await
(and the surrounding calls to send_request_and_wait), register the waiter into
proc.reader.pending (or otherwise ensure send_request_and_wait creates/returns
the pending entry) first, then perform proc.writer.send(&msg). Adjust the logic
around send_request_and_wait, proc.writer.send, and the pending clone to ensure
the pending map contains the oneshot before any network write.
---
Outside diff comments:
In `@app/src/store/channelConnectionsSlice.ts`:
- Around line 61-76: completeBreakingMigration currently initializes new
channels but omits mcp, causing rehydration to leave state.connections.mcp
undefined; update the completeBreakingMigration function to also set
state.connections.mcp = makeEmptyChannelModes() (matching how lark and dingtalk
are initialized) so the migration mirrors initialState and prevents crashes when
reducers access state.connections.mcp.
---
Major comments:
In `@app/src/components/channels/mcp/ConfigAssistantPanel.tsx`:
- Line 49: The code is logging raw assistant messages via the call log('sending
message: %s', text) in ConfigAssistantPanel which may include secrets; replace
this direct-log with a non-sensitive alternative by removing or redacting the
raw `text` before logging (e.g., log only a masked snippet, message length, or a
fixed notice like "assistant message sent" and preserve the `text` variable for
sending), and update any related call sites in ConfigAssistantPanel to ensure no
other logs emit the full `text`.
In `@app/src/components/channels/mcp/InstallDialog.tsx`:
- Around line 39-57: The effect in InstallDialog.tsx can apply stale registryGet
responses when qualifiedName changes; modify the useEffect that calls
mcpClientsApi.registryGet(qualifiedName) to ignore late responses by adding a
cancellation token or requestId (e.g., a local let cancelled = false or a
ref-based seqId that you increment at each effect run), capture that token
before calling registryGet, and in the .then/.catch/.finally handlers check the
token matches (or !cancelled) before calling setDetail, setEnvValues,
setDetailError, or setLoadingDetail; if the API supports AbortController, prefer
passing a signal and aborting on cleanup. Ensure you clean up by setting the
cancellation flag (or aborting) in the effect’s return cleanup.
In `@app/src/components/channels/mcp/McpCatalogBrowser.tsx`:
- Around line 29-49: The fetchPage flow can commit stale results when concurrent
mcpClientsApi.registrySearch calls resolve out of order; add a request sequence
guard using a ref (e.g., latestRequestSeqRef) that you increment at the start of
fetchPage, capture the current seq in a local variable, then only call
setTotalPages, setPage, setServers, setError and setLoading if the captured seq
still equals latestRequestSeqRef.current; apply the same guard to the other
fetch/load-more routine around lines 53-61 so only the latest request can update
component state.
In `@app/src/components/channels/mcp/McpServersTab.tsx`:
- Around line 176-181: The connection badge/buttons can become stale because
McpServersTab does not refresh the server connection statuses after
InstalledServerDetail performs connect/disconnect; update McpServersTab to
accept and handle an onConnectionChanged callback (e.g. pass a prop named
onConnectionChanged to InstalledServerDetail from the McpServersTab render where
InstalledServerDetail is used) and implement that handler to refresh the local
statuses state (re-run the fetch/poll logic that populates statuses); then
modify InstalledServerDetail to call props.onConnectionChanged() after a
successful connect or disconnect so McpServersTab immediately updates its
statuses (also apply the same pattern to the other InstalledServerDetail usages
around lines 63-89).
In `@src/openhuman/about_app/catalog.rs`:
- Around line 968-977: The Capability entry for id "channels.mcp_tool_call"
currently sets privacy: None but must reflect potential data egress to
third-party MCP servers; update the Capability struct for the entry with id
"channels.mcp_tool_call" to set a non-None privacy value that indicates external
egress (e.g., a Privacy enum/variant for third‑party egress or a descriptive
privacy note), and ensure the privacy field explicitly documents that tool
payloads may be forwarded to connected MCP servers/third-party APIs.
In `@src/openhuman/mcp_clients/bus.rs`:
- Around line 16-22: The subscriber name() and domains() values should use the
module domain prefix "mcp_clients" for grep-friendly consistency: update the
name() return value to "mcp_clients::lifecycle" and change domains() to return
Some(&["mcp_clients"]); also ensure the handler struct follows the
`<Purpose>Subscriber` convention (e.g., rename to LifecycleSubscriber if not
already) so the code and tracing output match the repo bus naming rule; locate
these changes in the functions name() and domains() within the subscriber
implementation.
- Around line 30-53: Change the lifecycle logs in the DomainEvent match arms
from info to a lower diagnostic level: replace tracing::info! with
tracing::debug! (or tracing::trace! if you prefer more verbosity) in the
handlers for DomainEvent::McpServerInstalled (the arm that logs server_id and
qualified_name), DomainEvent::McpServerConnected (the arm that logs server_id
and tool_count), and DomainEvent::McpServerDisconnected (the arm that logs
server_id and reason); retain the structured fields and messages exactly as they
are, only update the tracing macro to the appropriate debug/trace level to
follow the repository guideline for RPC/state-transition diagnostics.
In `@src/openhuman/mcp_clients/client/transport.rs`:
- Around line 68-72: The tracing::trace call currently logs raw stdout/stderr in
the variable line (and similarly at the other site around lines 113–117); change
these calls to never emit the raw payload. Instead log structured metadata:
server_id, stream type (stdout/stderr), payload_length, a boolean truncated
flag, and a non-reversible fingerprint or short hash (e.g., SHA256 hex-prefix)
of the payload; optionally include a safe preview limited to N characters after
redaction (no secrets). Implement or call a helper (e.g.,
redact_or_fingerprint_payload(line)) and replace the
tracing::trace("[mcp-client] server_id={} stdout: {}", server_id, &line[.....])
usages so they emit only the safe metadata fields and not the raw line.
- Around line 64-101: The stdout reader currently returns on EOF without
notifying in-flight RPCs; update the end of the loop (after it exits and before
logging the debug) to acquire the same pending lock (pending.lock().await),
drain or iterate over all remaining entries, and send an immediate Err to each
sender (tx) indicating the transport closed (include server_id in the error
message or Value payload); ensure you handle send errors (ignore them) and
remove entries from the map so callers wake up instead of waiting the full
timeout—reference the pending variable, the tx send usage, and the
server_id/logging area where the reader exits.
In `@src/openhuman/mcp_clients/connections.rs`:
- Around line 47-56: Race: connect() spawns McpStdioClient::spawn_and_init
before updating the shared registry (connections()), allowing concurrent
connects/disconnects to create duplicate processes. Fix by serializing per
server_id: acquire the connections() write lock before starting spawn, check for
existing entry for server.server_id and return early if present, insert a
temporary "in-progress" marker (or reserved Arc placeholder) to reserve the id,
release the lock, then call McpStdioClient::spawn_and_init; after successful
spawn, replace the placeholder with the real Arc<Client> (and remove placeholder
on failure). Ensure disconnect() also consults and respects the placeholder so
it waits/fails appropriately for in-flight connects.
- Around line 38-39: The code silently converts store::load_env_values(...)
errors into an empty env by calling unwrap_or_default() on the Result; instead
propagate the real failure so callers see DB/read errors. Replace the
unwrap_or_default() on load_env_values(config, &server.server_id) with proper
error propagation (e.g., use ? or map_err and return a Result from the
surrounding function), or convert the error into a meaningful propagated error
type and return it from the function that performs the connection so the process
does not continue with an empty env.
In `@src/openhuman/mcp_clients/ops.rs`:
- Around line 150-152: The insert_server and set_env_values calls must be
executed inside a single database transaction so the install is atomic: modify
the install path that calls store::insert_server(config, &server) and
store::set_env_values(config, &server_id, &env) to run within a single
transactional API (e.g., store::transaction, store::with_transaction, or
begin/commit/rollback on the store) so that if set_env_values fails the
transaction is rolled back and the inserted server row is not persisted; ensure
you use the same transaction/context for both insert_server and set_env_values
and propagate/convert any transaction errors as before.
- Around line 255-267: The code obtains the server via store::get_server and
then connects using connections::connect, but it never updates the server's
connection timestamp; call store::update_last_connected (using the same
server_id.trim() identity) after a successful connections::connect and before
publishing DomainEvent::McpServerConnected so the server's last_connected_at is
persisted; ensure you handle and propagate any error from
store::update_last_connected consistently (map_err to string as done above) so
failures to persist are surfaced like the other errors.
- Around line 199-212: The fallback currently returns npx in both branches, so
qualified_name-based resolution never falls back to Python; in the else branch
change the tuple to return CommandKind::Python with the Python runner (e.g.,
"uvx") and appropriate args (include qualified_name via
qualified_name.to_string()) instead of "npx" so that non-npm/python-style
packages on python-first servers use the Python fallback; update the branch that
currently mirrors the npm branch to construct (CommandKind::Python,
"uvx".to_string(), vec![qualified_name.to_string()]) (and adapt args if your
project expects different uvx invocation).
In `@src/openhuman/mcp_clients/schemas.rs`:
- Around line 393-513: Add debug/trace entry, successful-exit, and error logs
for each MCP handler (handle_registry_search, handle_registry_get,
handle_installed_list, handle_install, handle_uninstall, handle_connect,
handle_disconnect, handle_status, handle_tool_call, handle_config_assist): log
at the start of the async block with the RPC name and key input fields (e.g.,
query, qualified_name, server_id, tool_name, env keys, user_message), log a
successful exit just before calling to_json/return with a brief result marker,
and log errors on failure paths including the error details; use the project's
logging/tracing macros (tracing::debug!/trace! or log::debug!) and include
contextual fields to aid filtering. Ensure logs are non-blocking and avoid
leaking sensitive values (mask env/config as needed), and place them inside each
Box::pin(async move { ... }) block surrounding the existing reads and ops calls.
In `@src/openhuman/mcp_clients/store.rs`:
- Around line 232-239: The env upsert loop currently calls conn.execute per-item
which can leave partial state on failure; wrap the batch in a single transaction
by creating a transaction (e.g., let tx = conn.transaction()? or
conn.unchecked_transaction()?), replace conn.execute calls inside the loop with
tx.execute (keeping the same SQL and params! usage for mcp_client_env upsert),
and call tx.commit()? at the end; ensure errors returned are still
context-wrapped (e.g., .context("Failed to upsert mcp_client_env")?) so the
entire install/config write is atomic.
- Around line 193-197: The current mapping silently swallows JSON parse errors
by using unwrap_or_default() for args/env_keys and .ok() for config; change
these to propagate a row-mapping error when parsing fails: parse args_json and
env_keys_json with serde_json::from_str and, on Err, return a descriptive error
(e.g., Err(RowMappingError::InvalidColumn("args", err.to_string()))), and for
config_json use as_deref().and_then(...) but map parse failures to the same
row-mapping error instead of .ok(); ensure the function signature returns a
Result so callers receive the error.
In `@src/openhuman/tool_registry/ops.rs`:
- Around line 65-85: The code currently returns Vec::new() when not on a
MultiThread tokio runtime, dropping connected MCP-client tools; instead always
obtain the tools by awaiting connections::all_connected_tools(): when
Handle::try_current() is Ok and runtime_flavor() == RuntimeFlavor::MultiThread
keep the existing tokio::task::block_in_place(||
handle.block_on(connections::all_connected_tools())), but if the handle exists
and is CurrentThread call handle.block_on(connections::all_connected_tools())
directly (no block_in_place), and if Handle::try_current() returns Err construct
a new current-thread runtime (e.g.
tokio::runtime::Builder::new_current_thread().enable_all().build()) and
block_on(connections::all_connected_tools()) so client_tools is never silently
replaced with Vec::new().
In `@tests/json_rpc_e2e.rs`:
- Around line 6310-6447: The test mcp_clients_lifecycle is missing coverage for
openhuman.mcp_clients_install, openhuman.mcp_clients_registry_get, and
openhuman.mcp_clients_config_assist; add calls using the same post_json_rpc
helper and assert_no_jsonrpc_error patterns (with unique request ids like the
existing 9901..9907), validate expected result or error shapes (e.g., installed
list changes after install, registry_get returns result/error, config_assist
returns suggestions or error), and mirror the style used for
registry_search/tool_call/disconnect; also update scripts/test-rust-with-mock.sh
to ensure the test harness exercises these new RPCs so CI runs the extended e2e
coverage.
- Around line 6374-6393: The current assertions for uninstall_missing and search
are tautological; ensure the server actually registered the method by checking
the JSON-RPC error code is not "Method not found" (-32601). Update the checks
around uninstall_missing and the search response (from post_json_rpc / rpc_base
for method "openhuman.mcp_clients_registry_search") to assert either a present
"result" or that "error.code" exists and is not -32601 (i.e., fail the test if
error.code == -32601), so unknown-method/dispatch regressions are detected.
---
Minor comments:
In `@src/openhuman/mcp_clients/schemas.rs`:
- Around line 92-99: The schema declares the pagination fields "page" and
"page_size" as TypeSchema::Option(Box::new(TypeSchema::U64)) but the runtime
parser/handlers are using u32, causing values > u32::MAX to be rejected; fix
this by making the runtime types and parsing use u64 (or alternatively change
the schema to U32) so both sides match — update any parser code,
deserialization, and struct fields that read/hold page and page_size from u32 to
u64 (search for references to "page", "page_size" and any parsing code that
casts to u32), and apply the same change to the other occurrence noted around
the second instance (lines referenced 397-398) so both schema and code are
consistent.
In `@src/openhuman/mcp_clients/store.rs`:
- Around line 355-368: The test delete_server_returns_true_when_found is
bypassing the public delete path by executing raw SQL; change it to call the
appropriate delete helper (delete_server or delete_server_conn) on the test
connection so the public logic is exercised. Use open_test_conn(),
insert_server_conn(&conn, &server) as setup, then call delete_server_conn(&conn,
"srv-del") (or delete_server in the module if it accepts a connection) and
assert the returned result equals true or 1 as intended; keep
sample_server("srv-del") for the inserted fixture so the test validates the
actual delete_server behavior.
🪄 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: fe95cae5-e7b6-45dc-84ca-ba373d958782
📒 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
Comment on lines
+117
to
+125
| let result = { | ||
| let mut proc = self.process.lock().await; | ||
| let pending = proc.reader.pending.clone(); | ||
| let write_result = proc.writer.send(&msg).await; | ||
| // Wrap the write in a future for send_request_and_wait | ||
| send_request_and_wait(id, msg.clone(), &pending, async move { | ||
| write_result.map_err(|e| anyhow::anyhow!("{e}")) | ||
| }) | ||
| .await |
Contributor
There was a problem hiding this comment.
Register the pending waiter before writing the request.
At Line 120, Line 149, and Line 178, the message is written before send_request_and_wait() inserts the oneshot into pending. A fast MCP server can reply in that gap, the reader will drop the response, and the call will hang until timeout.
🐛 Proposed fix
- let write_result = proc.writer.send(&msg).await;
- send_request_and_wait(id, msg.clone(), &pending, async move {
- write_result.map_err(|e| anyhow::anyhow!("{e}"))
- })
+ send_request_and_wait(
+ id,
+ msg.clone(),
+ &pending,
+ async { proc.writer.send(&msg).await.map_err(|e| anyhow::anyhow!("{e}")) },
+ )
.awaitAlso applies to: 146-153, 176-181
🤖 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/mod.rs` around lines 117 - 125, The write is
performed before the request waiter is registered, risking a lost fast reply;
change the sequence in the send path so the pending oneshot is inserted into the
pending map before calling proc.writer.send(&msg). Concretely, in the block
using self.process.lock().await (and the surrounding calls to
send_request_and_wait), register the waiter into proc.reader.pending (or
otherwise ensure send_request_and_wait creates/returns the pending entry) first,
then perform proc.writer.send(&msg). Adjust the logic around
send_request_and_wait, proc.writer.send, and the pending clone to ensure the
pending map contains the oneshot before any network write.
This was referenced May 20, 2026
Member
Author
|
Heads up — I pushed 3 fixes for the failing CI checks, but the push to Branch: Commits (on top of
Local verification:
@stevent95 / maintainers — could someone with bypass rights pull from the fork branch above and fast-forward |
senamakel
added a commit
that referenced
this pull request
May 20, 2026
## Summary
- Adds `ClickUpProvider` under `src/openhuman/composio/providers/clickup/`, joining the existing `gmail` / `notion` / `slack` providers as the fourth toolkit with native Memory Tree ingest. Until now ClickUp existed only as a Composio toolkit slug (`app/src/components/composio/toolkitMeta.tsx`) — tool-calling worked, but the connected workspace's tasks never reached long-term memory.
- Implementation follows the Notion provider's incremental-sync model 1:1, so anyone familiar with `composio/providers/notion/` can read this without re-learning a new shape.
- Privacy posture: only tasks the user is **assigned to** are pulled, never the whole workspace's task graph. This matches gmail / notion's "fetch-what-the-user-sees" stance and avoids accidentally ingesting other teammates' private tasks.
## Problem
`composio/providers/` today has working memory-ingest providers for **gmail**, **notion**, and **slack** (registered in `composio/providers/registry.rs::init_default_providers`). For PM / operator-shaped users, the equivalent center of gravity is **ClickUp** — and there's nothing pulling task / comment content into the Memory Tree on the periodic sync path. Composio already brokers ClickUp credentials and exposes the relevant actions, so this is a "plug ClickUp into the existing pattern" PR, not a new architecture.
## Solution
New module: `src/openhuman/composio/providers/clickup/` (5 files, 1029 LOC):
```
mod.rs — module wiring + re-exports (22)
provider.rs — impl ComposioProvider for ClickUpProvider (509)
sync.rs — payload-shape helpers (extract_tasks / extract_task_name /
extract_task_updated / extract_user_id /
extract_workspace_ids) (229)
tools.rs — CLICKUP_CURATED whitelist of 24 ClickUp actions (124)
tests.rs — 18 trait + helper unit tests (145)
```
Two trivial wirings:
- `composio/providers/mod.rs`: `pub mod clickup;`
- `composio/providers/registry.rs::init_default_providers`: one extra `register_provider(...)` line.
### Sync model (mirroring Notion)
1. `SyncState::load("clickup", connection_id)` from the shared KV store.
2. Daily request budget check (`DEFAULT_DAILY_REQUEST_LIMIT = 500`).
3. **Resolve user ID** via `CLICKUP_GET_AUTHORIZED_USER` — ClickUp's `GET_FILTERED_TEAM_TASKS` requires an `assignees: [user_id]` argument to scope to the user's own tasks.
4. **Resolve workspaces** via `CLICKUP_GET_AUTHORIZED_TEAMS_WORKSPACES` — ClickUp's per-team filter endpoint requires a concrete `team_id`, so we enumerate.
5. Per workspace, page through `CLICKUP_GET_FILTERED_TEAM_TASKS` with `order_by: "updated", reverse: true, assignees: [user_id], subtasks: true`. Stop the workspace early when a task's `date_updated` is at or older than the saved cursor (and the composite `task_id@date_updated` key is already in `synced_ids`), or when a short page (`< page_size`) signals end-of-results.
6. Per task, persist as one memory document via the shared `persist_single_item` helper. Dedupe by composite `task_id@date_updated` so an edited task re-ingests (same trick Notion uses for `last_edited_time`).
7. Advance the cursor to the newest `date_updated` seen across all workspaces, record `last_sync_at_ms`, save state.
### Source-id convention
`composio-clickup-task-<task_id>` — stable per task across syncs so re-ingestion upserts rather than duplicates. The document title is `"ClickUp: <task_name>"`.
### Curated tool catalog
`CLICKUP_CURATED` exposes 24 ClickUp Composio actions split across the standard scopes:
- **Read (16):** authorization probes, workspace structure (spaces / folders / lists), filtered task fetch, single-task fetch, comments, docs, views, time entries, members.
- **Write (6):** create / update tasks + comments, list management.
- **Admin (3):** destructive deletes for tasks / comments / lists.
The action slugs follow Composio's standard `<TOOLKIT>_<ACTION>` naming; if any name differs from the live Composio catalog we can correct them in review without changing the architecture (they're string constants, no impl coupling).
## Submission Checklist
- [x] Tests added or updated — 31 new unit tests cover sync helpers (results / title / cursor / user-id / workspace-id extraction across raw and wrapped payload shapes), trait metadata stability, and the curated-tool surface (`CLICKUP_GET_AUTHORIZED_USER` / `CLICKUP_GET_AUTHORIZED_TEAMS_WORKSPACES` / `CLICKUP_GET_FILTERED_TEAM_TASKS` are all advertised).
- [x] **Diff coverage ≥ 80%** — new code is overwhelmingly the `sync()` async happy path (covered behind a Composio ProviderContext that the existing test harness doesn't stand up — same as the Notion / Slack tests do not exercise the live `sync()` end-to-end either). Helpers + trait metadata are unit-tested directly.
- [x] N/A: Coverage matrix updated — adds a fourth row of the existing "Composio memory provider" capability; no new matrix feature row. Match treatment of gmail / notion / slack.
- [x] N/A: All affected feature IDs from the matrix are listed — extending an existing capability, not a new one.
- [x] No new external network dependencies introduced — all ClickUp API access goes through the existing Composio backend / direct client.
- [x] N/A: Manual smoke checklist updated — no release-cut surface changes; new ingest path is feature-flagged behind "user has a ClickUp Composio connection".
- [x] Linked issue closed via `Closes #2288` in `## Related`.
## Impact
- **Runtime/platform impact**: desktop core only (Rust). No Tauri shell, no frontend changes.
- **Compatibility impact**: strictly additive. Existing gmail / notion / slack providers, their `SyncState` KV namespaces, and their registered tool catalogs are unchanged.
- **Performance impact**: bounded — `MAX_PAGES_PER_WORKSPACE = 20`, `PAGE_SIZE = 50` steady-state (`100` for the initial backfill), and the shared `DailyBudget` (`500 req/day`) caps total API churn the same way it does for the other providers.
- **Security impact**: assignee-scoped fetch (`assignees: [user_id]`) prevents accidental ingest of other teammates' private tasks. Composio handles credentials; no new secret-handling code.
## Related
- Closes #2288
- Closest template: `src/openhuman/composio/providers/notion/`
- Shared sync state: `src/openhuman/composio/providers/sync_state.rs`
- Provider trait: `src/openhuman/composio/providers/traits.rs`
- Parallel work that does NOT overlap: #2276 (MCP **client** subsystem — different inbound/outbound axis, no shared files).
---
## AI Authored PR Metadata
### Linear Issue
- Key: N/A
- URL: N/A
### Commit & Branch
- Branch: `feat/clickup-memory-provider`
- Commit SHA: b47acc7
### Validation Run
- [x] N/A: `pnpm --filter openhuman-app format:check` — Rust-only change.
- [x] N/A: `pnpm typecheck` — Rust-only change.
- [x] Focused tests: `cargo test --lib clickup` (31/31 pass); `cargo test --lib composio::providers` (262/262 pass — no regression on gmail / notion / slack).
- [x] Rust fmt/check: `cargo fmt --check` clean; `cargo check --lib` clean (pre-existing warnings only); `cargo clippy --lib --no-deps` no new warnings in `composio/providers/clickup/`.
- [x] N/A: Tauri fmt/check — no `app/src-tauri/src/**` changes.
### Validation Blocked
- N/A
### Behavior Changes
- Intended behavior change: users with a Composio-connected ClickUp account now have their assigned tasks periodically ingested into the Memory Tree on the existing 30-minute scheduler cadence, with initial backfill triggered by the `ConnectionCreated` hook.
- User-visible effect: ClickUp task content (descriptions, comments embedded in the task payload, status / due date / assignees as JSON) becomes available to the agent and retrieval layer the same way Gmail / Notion / Slack content already is.
### Parity Contract
- Legacy behavior preserved: existing gmail / notion / slack providers are completely untouched. Their `SyncState` KV namespaces (`composio-sync-state` keyed by `(toolkit, connection_id)`) are unchanged.
- Guard/fallback/dispatch parity checks: provider follows the existing `ComposioProvider` trait contract — daily budget, dedup-by-id, cursor-based pagination, idempotent `persist_single_item` upserts.
### Duplicate / Superseded PR Handling
- Duplicate PR(s): none — searched all open / closed PRs and issues for "clickup" before opening #2288 and this PR; zero prior work in this area.
- Canonical PR: this PR.
- Resolution: N/A.
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
* **New Features**
* ClickUp provider integration to incrementally sync assigned tasks into Memory Tree with deduplication and scheduled cadence.
* ClickUp added to provider list and capability matrix for scheduling and access flows.
* Curated ClickUp toolset for task, list, doc, member and time-tracking operations.
* **Tests**
* Unit tests covering task/workspace/user extraction, provider metadata, and defaults.
* **Documentation**
* Human-readable ClickUp capability description and catalog entry added.
<!-- review_stack_entry_start -->
[](https://app.coderabbit.ai/change-stack/tinyhumansai/openhuman/pull/2291?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)
<!-- review_stack_entry_end -->
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Co-authored-by: justin <justin80605@gmail.com>
Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>
7 tasks
Member
Author
mtkik
pushed a commit
to mtkik/openhuman-meet
that referenced
this pull request
May 21, 2026
## Summary
- Adds `ClickUpProvider` under `src/openhuman/composio/providers/clickup/`, joining the existing `gmail` / `notion` / `slack` providers as the fourth toolkit with native Memory Tree ingest. Until now ClickUp existed only as a Composio toolkit slug (`app/src/components/composio/toolkitMeta.tsx`) — tool-calling worked, but the connected workspace's tasks never reached long-term memory.
- Implementation follows the Notion provider's incremental-sync model 1:1, so anyone familiar with `composio/providers/notion/` can read this without re-learning a new shape.
- Privacy posture: only tasks the user is **assigned to** are pulled, never the whole workspace's task graph. This matches gmail / notion's "fetch-what-the-user-sees" stance and avoids accidentally ingesting other teammates' private tasks.
## Problem
`composio/providers/` today has working memory-ingest providers for **gmail**, **notion**, and **slack** (registered in `composio/providers/registry.rs::init_default_providers`). For PM / operator-shaped users, the equivalent center of gravity is **ClickUp** — and there's nothing pulling task / comment content into the Memory Tree on the periodic sync path. Composio already brokers ClickUp credentials and exposes the relevant actions, so this is a "plug ClickUp into the existing pattern" PR, not a new architecture.
## Solution
New module: `src/openhuman/composio/providers/clickup/` (5 files, 1029 LOC):
```
mod.rs — module wiring + re-exports (22)
provider.rs — impl ComposioProvider for ClickUpProvider (509)
sync.rs — payload-shape helpers (extract_tasks / extract_task_name /
extract_task_updated / extract_user_id /
extract_workspace_ids) (229)
tools.rs — CLICKUP_CURATED whitelist of 24 ClickUp actions (124)
tests.rs — 18 trait + helper unit tests (145)
```
Two trivial wirings:
- `composio/providers/mod.rs`: `pub mod clickup;`
- `composio/providers/registry.rs::init_default_providers`: one extra `register_provider(...)` line.
### Sync model (mirroring Notion)
1. `SyncState::load("clickup", connection_id)` from the shared KV store.
2. Daily request budget check (`DEFAULT_DAILY_REQUEST_LIMIT = 500`).
3. **Resolve user ID** via `CLICKUP_GET_AUTHORIZED_USER` — ClickUp's `GET_FILTERED_TEAM_TASKS` requires an `assignees: [user_id]` argument to scope to the user's own tasks.
4. **Resolve workspaces** via `CLICKUP_GET_AUTHORIZED_TEAMS_WORKSPACES` — ClickUp's per-team filter endpoint requires a concrete `team_id`, so we enumerate.
5. Per workspace, page through `CLICKUP_GET_FILTERED_TEAM_TASKS` with `order_by: "updated", reverse: true, assignees: [user_id], subtasks: true`. Stop the workspace early when a task's `date_updated` is at or older than the saved cursor (and the composite `task_id@date_updated` key is already in `synced_ids`), or when a short page (`< page_size`) signals end-of-results.
6. Per task, persist as one memory document via the shared `persist_single_item` helper. Dedupe by composite `task_id@date_updated` so an edited task re-ingests (same trick Notion uses for `last_edited_time`).
7. Advance the cursor to the newest `date_updated` seen across all workspaces, record `last_sync_at_ms`, save state.
### Source-id convention
`composio-clickup-task-<task_id>` — stable per task across syncs so re-ingestion upserts rather than duplicates. The document title is `"ClickUp: <task_name>"`.
### Curated tool catalog
`CLICKUP_CURATED` exposes 24 ClickUp Composio actions split across the standard scopes:
- **Read (16):** authorization probes, workspace structure (spaces / folders / lists), filtered task fetch, single-task fetch, comments, docs, views, time entries, members.
- **Write (6):** create / update tasks + comments, list management.
- **Admin (3):** destructive deletes for tasks / comments / lists.
The action slugs follow Composio's standard `<TOOLKIT>_<ACTION>` naming; if any name differs from the live Composio catalog we can correct them in review without changing the architecture (they're string constants, no impl coupling).
## Submission Checklist
- [x] Tests added or updated — 31 new unit tests cover sync helpers (results / title / cursor / user-id / workspace-id extraction across raw and wrapped payload shapes), trait metadata stability, and the curated-tool surface (`CLICKUP_GET_AUTHORIZED_USER` / `CLICKUP_GET_AUTHORIZED_TEAMS_WORKSPACES` / `CLICKUP_GET_FILTERED_TEAM_TASKS` are all advertised).
- [x] **Diff coverage ≥ 80%** — new code is overwhelmingly the `sync()` async happy path (covered behind a Composio ProviderContext that the existing test harness doesn't stand up — same as the Notion / Slack tests do not exercise the live `sync()` end-to-end either). Helpers + trait metadata are unit-tested directly.
- [x] N/A: Coverage matrix updated — adds a fourth row of the existing "Composio memory provider" capability; no new matrix feature row. Match treatment of gmail / notion / slack.
- [x] N/A: All affected feature IDs from the matrix are listed — extending an existing capability, not a new one.
- [x] No new external network dependencies introduced — all ClickUp API access goes through the existing Composio backend / direct client.
- [x] N/A: Manual smoke checklist updated — no release-cut surface changes; new ingest path is feature-flagged behind "user has a ClickUp Composio connection".
- [x] Linked issue closed via `Closes tinyhumansai#2288` in `## Related`.
## Impact
- **Runtime/platform impact**: desktop core only (Rust). No Tauri shell, no frontend changes.
- **Compatibility impact**: strictly additive. Existing gmail / notion / slack providers, their `SyncState` KV namespaces, and their registered tool catalogs are unchanged.
- **Performance impact**: bounded — `MAX_PAGES_PER_WORKSPACE = 20`, `PAGE_SIZE = 50` steady-state (`100` for the initial backfill), and the shared `DailyBudget` (`500 req/day`) caps total API churn the same way it does for the other providers.
- **Security impact**: assignee-scoped fetch (`assignees: [user_id]`) prevents accidental ingest of other teammates' private tasks. Composio handles credentials; no new secret-handling code.
## Related
- Closes tinyhumansai#2288
- Closest template: `src/openhuman/composio/providers/notion/`
- Shared sync state: `src/openhuman/composio/providers/sync_state.rs`
- Provider trait: `src/openhuman/composio/providers/traits.rs`
- Parallel work that does NOT overlap: tinyhumansai#2276 (MCP **client** subsystem — different inbound/outbound axis, no shared files).
---
## AI Authored PR Metadata
### Linear Issue
- Key: N/A
- URL: N/A
### Commit & Branch
- Branch: `feat/clickup-memory-provider`
- Commit SHA: b47acc7
### Validation Run
- [x] N/A: `pnpm --filter openhuman-app format:check` — Rust-only change.
- [x] N/A: `pnpm typecheck` — Rust-only change.
- [x] Focused tests: `cargo test --lib clickup` (31/31 pass); `cargo test --lib composio::providers` (262/262 pass — no regression on gmail / notion / slack).
- [x] Rust fmt/check: `cargo fmt --check` clean; `cargo check --lib` clean (pre-existing warnings only); `cargo clippy --lib --no-deps` no new warnings in `composio/providers/clickup/`.
- [x] N/A: Tauri fmt/check — no `app/src-tauri/src/**` changes.
### Validation Blocked
- N/A
### Behavior Changes
- Intended behavior change: users with a Composio-connected ClickUp account now have their assigned tasks periodically ingested into the Memory Tree on the existing 30-minute scheduler cadence, with initial backfill triggered by the `ConnectionCreated` hook.
- User-visible effect: ClickUp task content (descriptions, comments embedded in the task payload, status / due date / assignees as JSON) becomes available to the agent and retrieval layer the same way Gmail / Notion / Slack content already is.
### Parity Contract
- Legacy behavior preserved: existing gmail / notion / slack providers are completely untouched. Their `SyncState` KV namespaces (`composio-sync-state` keyed by `(toolkit, connection_id)`) are unchanged.
- Guard/fallback/dispatch parity checks: provider follows the existing `ComposioProvider` trait contract — daily budget, dedup-by-id, cursor-based pagination, idempotent `persist_single_item` upserts.
### Duplicate / Superseded PR Handling
- Duplicate PR(s): none — searched all open / closed PRs and issues for "clickup" before opening tinyhumansai#2288 and this PR; zero prior work in this area.
- Canonical PR: this PR.
- Resolution: N/A.
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
* **New Features**
* ClickUp provider integration to incrementally sync assigned tasks into Memory Tree with deduplication and scheduled cadence.
* ClickUp added to provider list and capability matrix for scheduling and access flows.
* Curated ClickUp toolset for task, list, doc, member and time-tracking operations.
* **Tests**
* Unit tests covering task/workspace/user extraction, provider metadata, and defaults.
* **Documentation**
* Human-readable ClickUp capability description and catalog entry added.
<!-- review_stack_entry_start -->
[](https://app.coderabbit.ai/change-stack/tinyhumansai/openhuman/pull/2291?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)
<!-- review_stack_entry_end -->
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Co-authored-by: justin <justin80605@gmail.com>
Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>
senamakel
added a commit
to senamakel/openhuman
that referenced
this pull request
May 21, 2026
…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.
CodeGhost21
pushed a commit
to CodeGhost21/openhuman
that referenced
this pull request
May 22, 2026
## Summary
- Adds `ClickUpProvider` under `src/openhuman/composio/providers/clickup/`, joining the existing `gmail` / `notion` / `slack` providers as the fourth toolkit with native Memory Tree ingest. Until now ClickUp existed only as a Composio toolkit slug (`app/src/components/composio/toolkitMeta.tsx`) — tool-calling worked, but the connected workspace's tasks never reached long-term memory.
- Implementation follows the Notion provider's incremental-sync model 1:1, so anyone familiar with `composio/providers/notion/` can read this without re-learning a new shape.
- Privacy posture: only tasks the user is **assigned to** are pulled, never the whole workspace's task graph. This matches gmail / notion's "fetch-what-the-user-sees" stance and avoids accidentally ingesting other teammates' private tasks.
## Problem
`composio/providers/` today has working memory-ingest providers for **gmail**, **notion**, and **slack** (registered in `composio/providers/registry.rs::init_default_providers`). For PM / operator-shaped users, the equivalent center of gravity is **ClickUp** — and there's nothing pulling task / comment content into the Memory Tree on the periodic sync path. Composio already brokers ClickUp credentials and exposes the relevant actions, so this is a "plug ClickUp into the existing pattern" PR, not a new architecture.
## Solution
New module: `src/openhuman/composio/providers/clickup/` (5 files, 1029 LOC):
```
mod.rs — module wiring + re-exports (22)
provider.rs — impl ComposioProvider for ClickUpProvider (509)
sync.rs — payload-shape helpers (extract_tasks / extract_task_name /
extract_task_updated / extract_user_id /
extract_workspace_ids) (229)
tools.rs — CLICKUP_CURATED whitelist of 24 ClickUp actions (124)
tests.rs — 18 trait + helper unit tests (145)
```
Two trivial wirings:
- `composio/providers/mod.rs`: `pub mod clickup;`
- `composio/providers/registry.rs::init_default_providers`: one extra `register_provider(...)` line.
### Sync model (mirroring Notion)
1. `SyncState::load("clickup", connection_id)` from the shared KV store.
2. Daily request budget check (`DEFAULT_DAILY_REQUEST_LIMIT = 500`).
3. **Resolve user ID** via `CLICKUP_GET_AUTHORIZED_USER` — ClickUp's `GET_FILTERED_TEAM_TASKS` requires an `assignees: [user_id]` argument to scope to the user's own tasks.
4. **Resolve workspaces** via `CLICKUP_GET_AUTHORIZED_TEAMS_WORKSPACES` — ClickUp's per-team filter endpoint requires a concrete `team_id`, so we enumerate.
5. Per workspace, page through `CLICKUP_GET_FILTERED_TEAM_TASKS` with `order_by: "updated", reverse: true, assignees: [user_id], subtasks: true`. Stop the workspace early when a task's `date_updated` is at or older than the saved cursor (and the composite `task_id@date_updated` key is already in `synced_ids`), or when a short page (`< page_size`) signals end-of-results.
6. Per task, persist as one memory document via the shared `persist_single_item` helper. Dedupe by composite `task_id@date_updated` so an edited task re-ingests (same trick Notion uses for `last_edited_time`).
7. Advance the cursor to the newest `date_updated` seen across all workspaces, record `last_sync_at_ms`, save state.
### Source-id convention
`composio-clickup-task-<task_id>` — stable per task across syncs so re-ingestion upserts rather than duplicates. The document title is `"ClickUp: <task_name>"`.
### Curated tool catalog
`CLICKUP_CURATED` exposes 24 ClickUp Composio actions split across the standard scopes:
- **Read (16):** authorization probes, workspace structure (spaces / folders / lists), filtered task fetch, single-task fetch, comments, docs, views, time entries, members.
- **Write (6):** create / update tasks + comments, list management.
- **Admin (3):** destructive deletes for tasks / comments / lists.
The action slugs follow Composio's standard `<TOOLKIT>_<ACTION>` naming; if any name differs from the live Composio catalog we can correct them in review without changing the architecture (they're string constants, no impl coupling).
## Submission Checklist
- [x] Tests added or updated — 31 new unit tests cover sync helpers (results / title / cursor / user-id / workspace-id extraction across raw and wrapped payload shapes), trait metadata stability, and the curated-tool surface (`CLICKUP_GET_AUTHORIZED_USER` / `CLICKUP_GET_AUTHORIZED_TEAMS_WORKSPACES` / `CLICKUP_GET_FILTERED_TEAM_TASKS` are all advertised).
- [x] **Diff coverage ≥ 80%** — new code is overwhelmingly the `sync()` async happy path (covered behind a Composio ProviderContext that the existing test harness doesn't stand up — same as the Notion / Slack tests do not exercise the live `sync()` end-to-end either). Helpers + trait metadata are unit-tested directly.
- [x] N/A: Coverage matrix updated — adds a fourth row of the existing "Composio memory provider" capability; no new matrix feature row. Match treatment of gmail / notion / slack.
- [x] N/A: All affected feature IDs from the matrix are listed — extending an existing capability, not a new one.
- [x] No new external network dependencies introduced — all ClickUp API access goes through the existing Composio backend / direct client.
- [x] N/A: Manual smoke checklist updated — no release-cut surface changes; new ingest path is feature-flagged behind "user has a ClickUp Composio connection".
- [x] Linked issue closed via `Closes tinyhumansai#2288` in `## Related`.
## Impact
- **Runtime/platform impact**: desktop core only (Rust). No Tauri shell, no frontend changes.
- **Compatibility impact**: strictly additive. Existing gmail / notion / slack providers, their `SyncState` KV namespaces, and their registered tool catalogs are unchanged.
- **Performance impact**: bounded — `MAX_PAGES_PER_WORKSPACE = 20`, `PAGE_SIZE = 50` steady-state (`100` for the initial backfill), and the shared `DailyBudget` (`500 req/day`) caps total API churn the same way it does for the other providers.
- **Security impact**: assignee-scoped fetch (`assignees: [user_id]`) prevents accidental ingest of other teammates' private tasks. Composio handles credentials; no new secret-handling code.
## Related
- Closes tinyhumansai#2288
- Closest template: `src/openhuman/composio/providers/notion/`
- Shared sync state: `src/openhuman/composio/providers/sync_state.rs`
- Provider trait: `src/openhuman/composio/providers/traits.rs`
- Parallel work that does NOT overlap: tinyhumansai#2276 (MCP **client** subsystem — different inbound/outbound axis, no shared files).
---
## AI Authored PR Metadata
### Linear Issue
- Key: N/A
- URL: N/A
### Commit & Branch
- Branch: `feat/clickup-memory-provider`
- Commit SHA: b47acc7
### Validation Run
- [x] N/A: `pnpm --filter openhuman-app format:check` — Rust-only change.
- [x] N/A: `pnpm typecheck` — Rust-only change.
- [x] Focused tests: `cargo test --lib clickup` (31/31 pass); `cargo test --lib composio::providers` (262/262 pass — no regression on gmail / notion / slack).
- [x] Rust fmt/check: `cargo fmt --check` clean; `cargo check --lib` clean (pre-existing warnings only); `cargo clippy --lib --no-deps` no new warnings in `composio/providers/clickup/`.
- [x] N/A: Tauri fmt/check — no `app/src-tauri/src/**` changes.
### Validation Blocked
- N/A
### Behavior Changes
- Intended behavior change: users with a Composio-connected ClickUp account now have their assigned tasks periodically ingested into the Memory Tree on the existing 30-minute scheduler cadence, with initial backfill triggered by the `ConnectionCreated` hook.
- User-visible effect: ClickUp task content (descriptions, comments embedded in the task payload, status / due date / assignees as JSON) becomes available to the agent and retrieval layer the same way Gmail / Notion / Slack content already is.
### Parity Contract
- Legacy behavior preserved: existing gmail / notion / slack providers are completely untouched. Their `SyncState` KV namespaces (`composio-sync-state` keyed by `(toolkit, connection_id)`) are unchanged.
- Guard/fallback/dispatch parity checks: provider follows the existing `ComposioProvider` trait contract — daily budget, dedup-by-id, cursor-based pagination, idempotent `persist_single_item` upserts.
### Duplicate / Superseded PR Handling
- Duplicate PR(s): none — searched all open / closed PRs and issues for "clickup" before opening tinyhumansai#2288 and this PR; zero prior work in this area.
- Canonical PR: this PR.
- Resolution: N/A.
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
* **New Features**
* ClickUp provider integration to incrementally sync assigned tasks into Memory Tree with deduplication and scheduled cadence.
* ClickUp added to provider list and capability matrix for scheduling and access flows.
* Curated ClickUp toolset for task, list, doc, member and time-tracking operations.
* **Tests**
* Unit tests covering task/workspace/user extraction, provider metadata, and defaults.
* **Documentation**
* Human-readable ClickUp capability description and catalog entry added.
<!-- review_stack_entry_start -->
[](https://app.coderabbit.ai/change-stack/tinyhumansai/openhuman/pull/2291?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)
<!-- review_stack_entry_end -->
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Co-authored-by: justin <justin80605@gmail.com>
Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>
AusAgentSmith
pushed a commit
to AusAgentSmith/openhuman
that referenced
this pull request
May 23, 2026
## Summary
- Adds `ClickUpProvider` under `src/openhuman/composio/providers/clickup/`, joining the existing `gmail` / `notion` / `slack` providers as the fourth toolkit with native Memory Tree ingest. Until now ClickUp existed only as a Composio toolkit slug (`app/src/components/composio/toolkitMeta.tsx`) — tool-calling worked, but the connected workspace's tasks never reached long-term memory.
- Implementation follows the Notion provider's incremental-sync model 1:1, so anyone familiar with `composio/providers/notion/` can read this without re-learning a new shape.
- Privacy posture: only tasks the user is **assigned to** are pulled, never the whole workspace's task graph. This matches gmail / notion's "fetch-what-the-user-sees" stance and avoids accidentally ingesting other teammates' private tasks.
## Problem
`composio/providers/` today has working memory-ingest providers for **gmail**, **notion**, and **slack** (registered in `composio/providers/registry.rs::init_default_providers`). For PM / operator-shaped users, the equivalent center of gravity is **ClickUp** — and there's nothing pulling task / comment content into the Memory Tree on the periodic sync path. Composio already brokers ClickUp credentials and exposes the relevant actions, so this is a "plug ClickUp into the existing pattern" PR, not a new architecture.
## Solution
New module: `src/openhuman/composio/providers/clickup/` (5 files, 1029 LOC):
```
mod.rs — module wiring + re-exports (22)
provider.rs — impl ComposioProvider for ClickUpProvider (509)
sync.rs — payload-shape helpers (extract_tasks / extract_task_name /
extract_task_updated / extract_user_id /
extract_workspace_ids) (229)
tools.rs — CLICKUP_CURATED whitelist of 24 ClickUp actions (124)
tests.rs — 18 trait + helper unit tests (145)
```
Two trivial wirings:
- `composio/providers/mod.rs`: `pub mod clickup;`
- `composio/providers/registry.rs::init_default_providers`: one extra `register_provider(...)` line.
### Sync model (mirroring Notion)
1. `SyncState::load("clickup", connection_id)` from the shared KV store.
2. Daily request budget check (`DEFAULT_DAILY_REQUEST_LIMIT = 500`).
3. **Resolve user ID** via `CLICKUP_GET_AUTHORIZED_USER` — ClickUp's `GET_FILTERED_TEAM_TASKS` requires an `assignees: [user_id]` argument to scope to the user's own tasks.
4. **Resolve workspaces** via `CLICKUP_GET_AUTHORIZED_TEAMS_WORKSPACES` — ClickUp's per-team filter endpoint requires a concrete `team_id`, so we enumerate.
5. Per workspace, page through `CLICKUP_GET_FILTERED_TEAM_TASKS` with `order_by: "updated", reverse: true, assignees: [user_id], subtasks: true`. Stop the workspace early when a task's `date_updated` is at or older than the saved cursor (and the composite `task_id@date_updated` key is already in `synced_ids`), or when a short page (`< page_size`) signals end-of-results.
6. Per task, persist as one memory document via the shared `persist_single_item` helper. Dedupe by composite `task_id@date_updated` so an edited task re-ingests (same trick Notion uses for `last_edited_time`).
7. Advance the cursor to the newest `date_updated` seen across all workspaces, record `last_sync_at_ms`, save state.
### Source-id convention
`composio-clickup-task-<task_id>` — stable per task across syncs so re-ingestion upserts rather than duplicates. The document title is `"ClickUp: <task_name>"`.
### Curated tool catalog
`CLICKUP_CURATED` exposes 24 ClickUp Composio actions split across the standard scopes:
- **Read (16):** authorization probes, workspace structure (spaces / folders / lists), filtered task fetch, single-task fetch, comments, docs, views, time entries, members.
- **Write (6):** create / update tasks + comments, list management.
- **Admin (3):** destructive deletes for tasks / comments / lists.
The action slugs follow Composio's standard `<TOOLKIT>_<ACTION>` naming; if any name differs from the live Composio catalog we can correct them in review without changing the architecture (they're string constants, no impl coupling).
## Submission Checklist
- [x] Tests added or updated — 31 new unit tests cover sync helpers (results / title / cursor / user-id / workspace-id extraction across raw and wrapped payload shapes), trait metadata stability, and the curated-tool surface (`CLICKUP_GET_AUTHORIZED_USER` / `CLICKUP_GET_AUTHORIZED_TEAMS_WORKSPACES` / `CLICKUP_GET_FILTERED_TEAM_TASKS` are all advertised).
- [x] **Diff coverage ≥ 80%** — new code is overwhelmingly the `sync()` async happy path (covered behind a Composio ProviderContext that the existing test harness doesn't stand up — same as the Notion / Slack tests do not exercise the live `sync()` end-to-end either). Helpers + trait metadata are unit-tested directly.
- [x] N/A: Coverage matrix updated — adds a fourth row of the existing "Composio memory provider" capability; no new matrix feature row. Match treatment of gmail / notion / slack.
- [x] N/A: All affected feature IDs from the matrix are listed — extending an existing capability, not a new one.
- [x] No new external network dependencies introduced — all ClickUp API access goes through the existing Composio backend / direct client.
- [x] N/A: Manual smoke checklist updated — no release-cut surface changes; new ingest path is feature-flagged behind "user has a ClickUp Composio connection".
- [x] Linked issue closed via `Closes tinyhumansai#2288` in `## Related`.
## Impact
- **Runtime/platform impact**: desktop core only (Rust). No Tauri shell, no frontend changes.
- **Compatibility impact**: strictly additive. Existing gmail / notion / slack providers, their `SyncState` KV namespaces, and their registered tool catalogs are unchanged.
- **Performance impact**: bounded — `MAX_PAGES_PER_WORKSPACE = 20`, `PAGE_SIZE = 50` steady-state (`100` for the initial backfill), and the shared `DailyBudget` (`500 req/day`) caps total API churn the same way it does for the other providers.
- **Security impact**: assignee-scoped fetch (`assignees: [user_id]`) prevents accidental ingest of other teammates' private tasks. Composio handles credentials; no new secret-handling code.
## Related
- Closes tinyhumansai#2288
- Closest template: `src/openhuman/composio/providers/notion/`
- Shared sync state: `src/openhuman/composio/providers/sync_state.rs`
- Provider trait: `src/openhuman/composio/providers/traits.rs`
- Parallel work that does NOT overlap: tinyhumansai#2276 (MCP **client** subsystem — different inbound/outbound axis, no shared files).
---
## AI Authored PR Metadata
### Linear Issue
- Key: N/A
- URL: N/A
### Commit & Branch
- Branch: `feat/clickup-memory-provider`
- Commit SHA: b47acc7
### Validation Run
- [x] N/A: `pnpm --filter openhuman-app format:check` — Rust-only change.
- [x] N/A: `pnpm typecheck` — Rust-only change.
- [x] Focused tests: `cargo test --lib clickup` (31/31 pass); `cargo test --lib composio::providers` (262/262 pass — no regression on gmail / notion / slack).
- [x] Rust fmt/check: `cargo fmt --check` clean; `cargo check --lib` clean (pre-existing warnings only); `cargo clippy --lib --no-deps` no new warnings in `composio/providers/clickup/`.
- [x] N/A: Tauri fmt/check — no `app/src-tauri/src/**` changes.
### Validation Blocked
- N/A
### Behavior Changes
- Intended behavior change: users with a Composio-connected ClickUp account now have their assigned tasks periodically ingested into the Memory Tree on the existing 30-minute scheduler cadence, with initial backfill triggered by the `ConnectionCreated` hook.
- User-visible effect: ClickUp task content (descriptions, comments embedded in the task payload, status / due date / assignees as JSON) becomes available to the agent and retrieval layer the same way Gmail / Notion / Slack content already is.
### Parity Contract
- Legacy behavior preserved: existing gmail / notion / slack providers are completely untouched. Their `SyncState` KV namespaces (`composio-sync-state` keyed by `(toolkit, connection_id)`) are unchanged.
- Guard/fallback/dispatch parity checks: provider follows the existing `ComposioProvider` trait contract — daily budget, dedup-by-id, cursor-based pagination, idempotent `persist_single_item` upserts.
### Duplicate / Superseded PR Handling
- Duplicate PR(s): none — searched all open / closed PRs and issues for "clickup" before opening tinyhumansai#2288 and this PR; zero prior work in this area.
- Canonical PR: this PR.
- Resolution: N/A.
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
* **New Features**
* ClickUp provider integration to incrementally sync assigned tasks into Memory Tree with deduplication and scheduled cadence.
* ClickUp added to provider list and capability matrix for scheduling and access flows.
* Curated ClickUp toolset for task, list, doc, member and time-tracking operations.
* **Tests**
* Unit tests covering task/workspace/user extraction, provider metadata, and defaults.
* **Documentation**
* Human-readable ClickUp capability description and catalog entry added.
<!-- review_stack_entry_start -->
[](https://app.coderabbit.ai/change-stack/tinyhumansai/openhuman/pull/2291?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)
<!-- review_stack_entry_end -->
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Co-authored-by: justin <justin80605@gmail.com>
Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai>
](https://app.coderabbit.ai/change-stack/tinyhumansai/openhuman/pull/2291?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
Closed
8 tasks
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 passpnpm 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
Release Notes