feat: add mcp tool provider - #71
Merged
Merged
Conversation
Contributor
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a feature-gated MCP adapter with in-process connections, tool discovery, registration, invocation, schema and annotation preservation, result conversion, and error handling. Adds an example, integration tests, documentation, and Tokio runtime setup for two existing tests. ChangesMCP adapter
Tokio test context
Sequence Diagram(s)sequenceDiagram
participant MCPServer
participant McpClient
participant McpToolProvider
participant ToolRegistry
participant McpTool
MCPServer->>McpClient: Serve tools over duplex transport
McpToolProvider->>McpClient: Discover tools
McpClient-->>McpToolProvider: Return tool definitions
McpToolProvider->>ToolRegistry: Register adapted tools
ToolRegistry->>McpTool: Invoke exposed tool
McpTool->>McpClient: Forward tools/call
McpClient->>MCPServer: Call server tool
MCPServer-->>McpClient: Return MCP content and status
McpClient-->>McpTool: Convert result to ToolOutput
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. 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: 11
🧹 Nitpick comments (5)
src/engine/bare/tests.rs (1)
4461-4468: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the runtime setup with the comments.
tokio::runtime::Runtime::new()enters a multi-thread runtime, not a current-thread runtime. If current-thread scheduling is required, build withRuntime::new_current_thread().enable_all().build()and driveagent.run()withrt.block_on(...). Otherwise, update the comments to describe an entered Tokio runtime.Also applies to: 4555-4562.
🤖 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/engine/bare/tests.rs` around lines 4461 - 4468, Update the runtime setup around agent.run in both affected test blocks to match the stated scheduling model: either construct a current-thread runtime with enable_all and drive the future via rt.block_on, or revise the comments to accurately describe the multi-thread runtime created by Runtime::new. Apply the same correction to both occurrences.src/mcp.rs (4)
388-389: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider manual
Debugimpls for the public types.
McpClient,McpToolProvider, andMcpToolare public and have noDebug. Consumers that deriveDebugon a struct holding one of these fail to compile.RunningServicemay not implementDebug, so write the impls manually and print the adapter-level fields only.♻️ Sketch
impl std::fmt::Debug for McpTool { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("McpTool") .field("server_name", &self.server_name) .field("exposed_name", &self.exposed_name) .field("read_only_hint", &self.read_only_hint) .field("destructive_hint", &self.destructive_hint) .finish_non_exhaustive() } }🤖 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/mcp.rs` around lines 388 - 389, Implement manual std::fmt::Debug implementations for the public McpClient, McpToolProvider, and McpTool types, exposing only their adapter-level fields and avoiding non-Debug internals such as RunningService. Use debug_struct with finish_non_exhaustive where appropriate, preserving existing behavior while allowing consumers to derive Debug for containing types.
123-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the spawned server's initialization failure.
At Line 124 a failed
server.serve(server_end)is discarded with no record. The caller then sees onlyMcpError::Handshakefrom the client side, which reports the EOF and not the real cause. Add atracing::warn!on the error branch. The module already usestracinginbridge_tool_list.♻️ Proposed refactor
tokio::spawn(async move { - if let Ok(running) = server.serve(server_end).await { - let _ = running.waiting().await.ok(); + match server.serve(server_end).await { + Ok(running) => { + let _ = running.waiting().await; + } + Err(e) => { + tracing::warn!(error = %e, "in-process MCP server failed to initialize"); + } } });🤖 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/mcp.rs` around lines 123 - 128, Update the spawned task around server.serve in the MCP initialization flow to handle its error branch explicitly and emit a tracing::warn! containing the initialization error; preserve the existing waiting behavior for successful server startup and the client handshake flow.
170-173: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReject non-object MCP arguments before sending the call.
inputis only attached when it is a JSON object, so strings, numbers, arrays, or booleans are sent as atools/callwithoutarguments. Accept onlyserde_json::Value::Objectandserde_json::Value::Null; returnToolInput::InvalidInput(...)for other values.♻️ Proposed refactor
let mut params = rmcp::model::CallToolRequestParams::new(server_name.to_string()); - if let serde_json::Value::Object(map) = input { - params = params.with_arguments(map); + match input { + serde_json::Value::Object(map) => params = params.with_arguments(map), + serde_json::Value::Null => {} + other => { + return Err(ToolError::InvalidInput(format!( + "MCP tool '{server_name}' expects object arguments, got {other}" + ))); + } }🤖 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/mcp.rs` around lines 170 - 173, Update the MCP tool-call argument handling around CallToolRequestParams::new: accept serde_json::Value::Object by attaching its map and serde_json::Value::Null as no arguments, but return ToolInput::InvalidInput(...) for strings, numbers, arrays, and booleans before sending the request.
769-777: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSurface the readable fields in unsupported-content notes.
ContentBlock::Resource(_)drops embedded text content, andContentBlock::ResourceLink(_)drops theuri. ForResourceContents::TextResourceContents, includeuri/textso the model sees usable payload. For binary resources, include only the resource type rather than replacing it with a fixed unsupported-note 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/mcp.rs` around lines 769 - 777, Update the ContentBlock::Resource and ContentBlock::ResourceLink handling to inspect their payloads instead of discarding fields: include uri and text for TextResourceContents, and for binary resources expose only the resource type. Preserve the existing unsupported-note behavior for other content types and avoid using the fixed generic message for these resource variants.
🤖 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 `@Cargo.toml`:
- Line 36: Update the Tokio dependency features used by the MCP module to
include io-util, ensuring tokio::io::duplex in src/mcp.rs compiles when the mcp
feature is enabled while preserving the existing feature set.
In `@examples/mcp-adapter.rs`:
- Line 61: Update the await handling around running.waiting() in the example to
explicitly consume the Option result, matching the existing let _ pattern used
by mcp.rs, so no unused_must_use warning is emitted.
In `@README.md`:
- Line 161: Update the mcp feature row in the README feature table to state that
the adapter currently supports only in-process MCP servers, and clarify that
stdio and HTTP/SSE transports are not yet available. Replace the broad “any MCP
server” wording while preserving the existing adapter and Tool implementation
references.
In `@src/mcp.rs`:
- Around line 32-43: Update the hidden async run function declaration in the
McpClient documentation example to return a Result compatible with the ?
operators and the final Ok::<(), loopctl::mcp::McpError>(()) expression. Keep
the example’s existing client and registry flow unchanged.
- Around line 174-178: Add a configurable optional per-call timeout field to
McpClient and apply it around the service.call_tool(params) future in the
tool-call method. Preserve unlimited waiting when the timeout is unset, and map
elapsed tokio::time::timeout results to ToolError::Execution with the configured
duration; retain the existing service-error mapping.
- Around line 255-259: Update the documentation for connect and the
corresponding doc text around the earlier connect declaration to remove the
claim that connect runs the MCP initialize handshake. Describe connect as
performing the tools/list snapshot, and retain the statement that handshake
failures occur during McpClient construction via in_process or from_service.
- Around line 608-620: Update the server-tool loop around bridge_tool so the
None/empty-name skip emits a tracing warning before continuing, matching the
documented behavior. Revise the bridge_tool documentation to accurately state
that callers drop empty-name tools with a warning, while preserving the existing
duplicate-name warning and first-tool retention.
- Around line 602-606: Cap the tool collection in the list-all-tools flow before
entries are appended to out, using the existing configured discovery/result
limit if available. Ensure pagination stops once the cap is reached while
preserving normal tools/list cursor handling and the McpError mapping in the
caller.
In `@tests/mcp_tool_provider.rs`:
- Around line 632-663: Correct the documentation comment above AnnotatedServer
to remove the inaccurate claims about #[tool_handler] generating or skipping
list_tools, call_tool, and get_info. Describe only the behavior actually
implemented by the #[tool_router] impl and manual ServerHandler::list_tools
override.
- Around line 44-55: Update tests/mcp_tool_provider.rs lines 44-55 by retaining
connect_in_process for drop_provider_cancels_background_server and adding a
separate test that uses McpClient::in_process to discover tools. Update
examples/mcp-adapter.rs lines 53-67 by replacing the manual duplex, server
spawn, and from_service setup with
McpClient::in_process(GreetServer::new()).await.expect("client initialize").
- Around line 473-507: Update intra_batch_name_collision_keeps_one_no_panic to
use an AnnotatedServer that returns two tool entries with the same name from
separate sources, rather than registering both status_a and status_b in
CollisionServer’s single ToolRouter. Ensure the test reaches bridge_tool_list’s
seen-based deduplication path and still asserts exactly one git__status entry
without panicking.
---
Nitpick comments:
In `@src/engine/bare/tests.rs`:
- Around line 4461-4468: Update the runtime setup around agent.run in both
affected test blocks to match the stated scheduling model: either construct a
current-thread runtime with enable_all and drive the future via rt.block_on, or
revise the comments to accurately describe the multi-thread runtime created by
Runtime::new. Apply the same correction to both occurrences.
In `@src/mcp.rs`:
- Around line 388-389: Implement manual std::fmt::Debug implementations for the
public McpClient, McpToolProvider, and McpTool types, exposing only their
adapter-level fields and avoiding non-Debug internals such as RunningService.
Use debug_struct with finish_non_exhaustive where appropriate, preserving
existing behavior while allowing consumers to derive Debug for containing types.
- Around line 123-128: Update the spawned task around server.serve in the MCP
initialization flow to handle its error branch explicitly and emit a
tracing::warn! containing the initialization error; preserve the existing
waiting behavior for successful server startup and the client handshake flow.
- Around line 170-173: Update the MCP tool-call argument handling around
CallToolRequestParams::new: accept serde_json::Value::Object by attaching its
map and serde_json::Value::Null as no arguments, but return
ToolInput::InvalidInput(...) for strings, numbers, arrays, and booleans before
sending the request.
- Around line 769-777: Update the ContentBlock::Resource and
ContentBlock::ResourceLink handling to inspect their payloads instead of
discarding fields: include uri and text for TextResourceContents, and for binary
resources expose only the resource type. Preserve the existing unsupported-note
behavior for other content types and avoid using the fixed generic message for
these resource variants.
🪄 Autofix
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 Plus
Run ID: be7a6066-b39b-4ba4-83cb-d6d70cd44689
📒 Files selected for processing (8)
CHANGELOG.mdCargo.tomlREADME.mdexamples/mcp-adapter.rssrc/engine/bare/tests.rssrc/lib.rssrc/mcp.rstests/mcp_tool_provider.rs
Comment on lines
+44
to
+55
| async fn connect_in_process<S>(server: S) -> (McpClient, tokio::task::JoinHandle<()>) | ||
| where | ||
| S: ServerHandler + Clone + Send + 'static, | ||
| { | ||
| let (server_end, client_end) = tokio::io::duplex(DUPLEX_BUFFER); | ||
| let server_handle = tokio::spawn(async move { | ||
| let running = server.serve(server_end).await.expect("server serve"); | ||
| let _ = running.waiting().await.ok(); | ||
| }); | ||
| let client = ().serve(client_end).await.map(McpClient::from_service).expect("client serve"); | ||
| (client, server_handle) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
McpClient::in_process is bypassed by both the test suite and the example. Both sites hand-roll the duplex creation, the server spawn, and from_service, which is exactly the sequence src/mcp.rs Lines 118-132 already provides. The public constructor therefore ships with no test coverage and no demonstration, even though src/mcp.rs Lines 12-14 present it as the primary in-process entry point.
tests/mcp_tool_provider.rs#L44-L55: keepconnect_in_processfordrop_provider_cancels_background_server, which needs theJoinHandle, and add one test that builds the client withMcpClient::in_processand discovers tools.examples/mcp-adapter.rs#L53-L67: replace the manual duplex, spawn, andfrom_serviceblock withMcpClient::in_process(GreetServer::new()).await.expect("client initialize").
📍 Affects 2 files
tests/mcp_tool_provider.rs#L44-L55(this comment)examples/mcp-adapter.rs#L53-L67
🤖 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/mcp_tool_provider.rs` around lines 44 - 55, Update
tests/mcp_tool_provider.rs lines 44-55 by retaining connect_in_process for
drop_provider_cancels_background_server and adding a separate test that uses
McpClient::in_process to discover tools. Update examples/mcp-adapter.rs lines
53-67 by replacing the manual duplex, server spawn, and from_service setup with
McpClient::in_process(GreetServer::new()).await.expect("client initialize").
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.
No description provided.