Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/mcp/client.rs (1)
40-45: 💤 Low valueSilent
url-over-commandprecedence when both fields are setIf a user accidentally specifies both
urlandcommandin a[mcp_servers.X]block, theurlbranch wins silently. A diagnostic warning or an explicit error would help surface misconfigured entries.💡 Suggested improvement
+ } else if config.url.is_some() && config.command.is_some() { + return Err(Error::ConfigError(format!( + "MCP server '{}' has both 'command' and 'url' set; specify only one", + name + ))); } else if let Some(ref command) = config.command {Or, add it as a
tracing::warn!if you want to keep the existing fallback behaviour.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/client.rs` around lines 40 - 45, The config parsing currently allows both 'url' and 'command' to be set and silently prefers 'url'; update the branch that chooses between url vs command to detect when both fields are present (check the 'url' and 'command' fields for the selected server entry using the existing variables like name, url, command) and either return an explicit Error::ConfigError or emit a tracing::warn! indicating that both were set and 'url' will be used; implement the change adjacent to the existing match/if that constructs the MCP client so the warning/error is produced before returning the HTTP client branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Line 30: The Cargo.toml dependency for rmcp references a non-published version
"1.6.0" which will cause Cargo to fail; update the rmcp dependency entry (the
rmcp package line) to a published crate version such as "1.5.0" or, if you need
the GitHub-only 1.6.0 changes, replace the version with a git dependency
pointing to the rmcp repository and a specific tag/commit (e.g., git =
"https://github.com/..." and rev = "...") so Cargo can resolve it reliably.
In `@src/mcp/client.rs`:
- Around line 48-56: The current list_tools method calls
peer().list_tools(Default::default()) once and thus only returns the first page;
replace that single-page call with the SDK's pagination helper by calling
peer().list_all_tools().await and map errors the same way (preserving the
Error::Other message including self.server_name), then collect and return the
tools as before; update the function body in the list_tools method in
src/mcp/client.rs to use list_all_tools instead of list_tools to ensure full
enumeration.
- Around line 85-95: The current extract_text_content function drops non-text
items and returns an empty String when no "text" fields are present; change it
so that after building the Vec<String> of extracted text, if that vec is empty
then produce a fallback JSON dump of the original content slice (by serializing
each item with serde_json::to_string or to_string_pretty and joining them) so
callers (e.g., call_tool) receive the tool's raw output instead of "". Update
extract_text_content to attempt the normal text extraction first, and if
result.is_empty() then return the serialized JSON fallback (handling
serialization errors by returning an appropriate string like a minimal JSON
error message).
In `@src/mcp/mod.rs`:
- Around line 38-40: The current prefix sanitization only replaces '-' and ' '
and should instead normalize all non-alphanumeric characters to underscores to
avoid invalid identifiers; change the prefix computation (the let prefix =
name.replace([...]) expression) to map any character not in [A-Za-z0-9] to '_'
(e.g., via a regex or iterator/filter) and trim/condense consecutive underscores
if desired, and also add collision detection where prefixes are registered (see
register() and the tracing::warn path) so duplicate prefixes are caught/rejected
(or logged as errors) rather than silently overwriting.
- Around line 15-28: Wrap each call to connect_server(name, config).await in a
tokio::time::timeout(...) and handle the timeout case by logging a warn with the
server name and skipping that server; also add timeouts inside
McpClient::connect (both HTTP and stdio paths) and around client.list_tools() so
those internal awaits return an explicit timeout error instead of blocking
forever. Ensure you import tokio::time::Duration/timeout, choose a sensible
Duration constant, translate a timeout error into an Err returned from
connect_server (or into the same logging path used for other connection errors)
so connect_server, McpClient::connect, and callers uniformly handle and report
timeouts.
---
Nitpick comments:
In `@src/mcp/client.rs`:
- Around line 40-45: The config parsing currently allows both 'url' and
'command' to be set and silently prefers 'url'; update the branch that chooses
between url vs command to detect when both fields are present (check the 'url'
and 'command' fields for the selected server entry using the existing variables
like name, url, command) and either return an explicit Error::ConfigError or
emit a tracing::warn! indicating that both were set and 'url' will be used;
implement the change adjacent to the existing match/if that constructs the MCP
client so the warning/error is produced before returning the HTTP client branch.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ea8e5a4-cd37-4613-b1ae-159a38245bc8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Cargo.tomlsrc/api/mod.rssrc/cli/mod.rssrc/config/client.rssrc/config/config.toml.defaultsrc/config/mod.rssrc/config/resolve.rssrc/config/types.rssrc/core/agent.rssrc/lib.rssrc/mcp/client.rssrc/mcp/mod.rssrc/mcp/tool_handler.rssrc/tools/mod.rs
Comment on lines
+15
to
+28
| for (name, config) in configs { | ||
| match connect_server(name, config).await { | ||
| Ok(server_handlers) => { | ||
| tracing::info!( | ||
| server = %name, | ||
| count = server_handlers.len(), | ||
| "MCP server connected" | ||
| ); | ||
| handlers.extend(server_handlers); | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!(server = %name, error = %e, "MCP server failed to connect"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "mod.rs" -path "*/mcp/*" -type fRepository: weirdstuff-dev/openheim
Length of output: 84
🏁 Script executed:
find . -name "*.rs" | head -20Repository: weirdstuff-dev/openheim
Length of output: 485
🏁 Script executed:
fd -e rs mcp | head -20Repository: weirdstuff-dev/openheim
Length of output: 49
🏁 Script executed:
rg "connect_server" -A 5 -B 5Repository: weirdstuff-dev/openheim
Length of output: 1234
🏁 Script executed:
rg "McpClient" -A 10 -B 2Repository: weirdstuff-dev/openheim
Length of output: 4000
🏁 Script executed:
wc -l src/mcp/mod.rsRepository: weirdstuff-dev/openheim
Length of output: 85
🏁 Script executed:
cat -n src/mcp/mod.rsRepository: weirdstuff-dev/openheim
Length of output: 1954
🏁 Script executed:
wc -l src/mcp/client.rsRepository: weirdstuff-dev/openheim
Length of output: 88
🏁 Script executed:
cat -n src/mcp/client.rsRepository: weirdstuff-dev/openheim
Length of output: 4104
🏁 Script executed:
rg "timeout|Timeout" src/mcp/Repository: weirdstuff-dev/openheim
Length of output: 49
Add a connection timeout to prevent MCP servers from blocking startup indefinitely.
McpClient::connect (both HTTP and stdio paths) and client.list_tools() have no timeout. A misconfigured or unresponsive MCP server will block startup indefinitely with no error output.
Proposed fix: wrap each `connect_server` call with `tokio::time::timeout`
+use std::time::Duration;
+
+const MCP_CONNECT_TIMEOUT: Duration = Duration::from_secs(30);
pub(crate) async fn load_mcp_tools(configs: &BTreeMap<String, McpServerConfig>) -> Vec<Box<dyn ToolHandler>> {
let mut handlers: Vec<Box<dyn ToolHandler>> = Vec::new();
for (name, config) in configs {
- match connect_server(name, config).await {
- Ok(server_handlers) => {
+ match tokio::time::timeout(MCP_CONNECT_TIMEOUT, connect_server(name, config)).await {
+ Ok(Ok(server_handlers)) => {
tracing::info!(
server = %name,
count = server_handlers.len(),
"MCP server connected"
);
handlers.extend(server_handlers);
}
- Err(e) => {
- tracing::warn!(server = %name, error = %e, "MCP server failed to connect");
+ Ok(Err(e)) => {
+ tracing::warn!(server = %name, error = %e, "MCP server failed to connect");
+ }
+ Err(_) => {
+ tracing::warn!(server = %name, timeout_secs = MCP_CONNECT_TIMEOUT.as_secs(), "MCP server connection timed out");
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (name, config) in configs { | |
| match connect_server(name, config).await { | |
| Ok(server_handlers) => { | |
| tracing::info!( | |
| server = %name, | |
| count = server_handlers.len(), | |
| "MCP server connected" | |
| ); | |
| handlers.extend(server_handlers); | |
| } | |
| Err(e) => { | |
| tracing::warn!(server = %name, error = %e, "MCP server failed to connect"); | |
| } | |
| } | |
| use std::time::Duration; | |
| const MCP_CONNECT_TIMEOUT: Duration = Duration::from_secs(30); | |
| pub(crate) async fn load_mcp_tools(configs: &BTreeMap<String, McpServerConfig>) -> Vec<Box<dyn ToolHandler>> { | |
| let mut handlers: Vec<Box<dyn ToolHandler>> = Vec::new(); | |
| for (name, config) in configs { | |
| match tokio::time::timeout(MCP_CONNECT_TIMEOUT, connect_server(name, config)).await { | |
| Ok(Ok(server_handlers)) => { | |
| tracing::info!( | |
| server = %name, | |
| count = server_handlers.len(), | |
| "MCP server connected" | |
| ); | |
| handlers.extend(server_handlers); | |
| } | |
| Ok(Err(e)) => { | |
| tracing::warn!(server = %name, error = %e, "MCP server failed to connect"); | |
| } | |
| Err(_) => { | |
| tracing::warn!(server = %name, timeout_secs = MCP_CONNECT_TIMEOUT.as_secs(), "MCP server connection timed out"); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp/mod.rs` around lines 15 - 28, Wrap each call to connect_server(name,
config).await in a tokio::time::timeout(...) and handle the timeout case by
logging a warn with the server name and skipping that server; also add timeouts
inside McpClient::connect (both HTTP and stdio paths) and around
client.list_tools() so those internal awaits return an explicit timeout error
instead of blocking forever. Ensure you import tokio::time::Duration/timeout,
choose a sensible Duration constant, translate a timeout error into an Err
returned from connect_server (or into the same logging path used for other
connection errors) so connect_server, McpClient::connect, and callers uniformly
handle and report timeouts.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR integrates MCP (Model Context Protocol) server support by adding the ChangesMCP Server Integration
Sequence DiagramsequenceDiagram
participant App as Application
participant SE as SystemToolExecutor
participant MCP as MCP Module
participant MC as MCP Client
participant Server as MCP Server
App->>SE: build(&mcp_servers).await
SE->>MCP: load_mcp_tools(configs)
MCP->>MCP: For each server config
MCP->>MC: connect(name, config).await
MC->>Server: Establish transport (HTTP or stdio)
Server-->>MC: Connected
MC->>Server: list_all_tools()
Server-->>MC: [Tool 1, Tool 2, ...]
MC-->>MCP: McpClient ready
MCP->>MCP: Build McpToolHandler per tool
MCP-->>SE: Vec<Box<dyn ToolHandler>>
SE->>SE: register_builtins()
SE->>SE: register(mcp_handlers)
SE-->>App: ToolExecutor with all tools
App->>SE: list_tools()
SE-->>App: [all tool definitions]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The PR introduces a new protocol integration (MCP) across multiple new modules and refactors the tool executor, requiring review of protocol handling, async patterns, and integration points. The logic is moderately dense (connection handling, tool adaptation, transport selection) and distributed across multiple files, but changes follow a consistent pattern and the modifications to existing code are relatively localized. Test updates are straightforward. Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
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 by CodeRabbit