feat(mcp): add stdio transport and MCP tool registration#192
Merged
Conversation
📝 WalkthroughWalkthroughAdds a transport abstraction for MCP (HTTP and stdio), refactors McpClient to be transport-agnostic, extends discovery/config to detect both transport types, wires MCP tool registration during CLI/agent startup, and adds lifecycle shutdown management for MCP clients. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Init
participant Discovery as MCP Discovery
participant Config as Config/Workspace
participant TransportFactory as Transport Factory
participant Client as McpClient
participant Server as MCP Server (HTTP/Stdio)
participant Registry as Tool Registry
CLI->>Discovery: discover_mcp_servers()
Discovery->>Config: load_mcp_config()
Config-->>Discovery: DiscoveredMcpServer[]{transport}
loop For each MCP server
Discovery->>TransportFactory: determine transport
alt HTTP
TransportFactory->>Client: new_http(url, timeout)
else Stdio
TransportFactory->>Client: new_stdio(command,args,env,timeout)
Client->>Server: spawn process (stdio)
end
Client->>Server: initialize()
Server-->>Client: init_response
Client->>Server: list_tools()
Server-->>Client: [tool_defs]
loop For each enabled tool
Client->>Registry: register tool via McpToolWrapper
Registry-->>CLI: tool_registered
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 24: The quick-reference test-count line ("# lib: 2635, main: 97,
cli_smoke: 23, e2e: 13, integration: 70, doc: 121 passed (27 ignored)") and the
later "Testing" section totals in CLAUDE.md are inconsistent; update the totals
in the "Testing" section to exactly match the quick-reference numbers (or
vice-versa) by copying the authoritative counts from the same test-run source,
and ensure both locations use the same phrasing and totals so they remain
synchronized; look for the exact quick-reference string and the "Testing"
heading in CLAUDE.md to find and update the mismatched counts.
In `@src/cli/common.rs`:
- Around line 1097-1122: The loop currently appends config-specified MCP servers
into all_servers (populated by discover_mcp_servers) causing duplicate server
names; change the logic in this block so config entries override discovered ones
by name: for each server_cfg from config.mcp.servers, check all_servers for an
existing DiscoveredMcpServer with the same name (from discover_mcp_servers), and
if found replace that entry with the new transport/source ("config"), otherwise
push the new DiscoveredMcpServer as before; use the existing symbols
discover_mcp_servers, all_servers, DiscoveredMcpServer, and server_cfg.name to
locate where to perform the lookup-and-replace.
In `@src/tools/mcp/discovery.rs`:
- Around line 92-108: When constructing DiscoveredMcpServer items in
discovery.rs, skip entries whose entry.url or entry.command are empty strings
(e.g., url.is_empty() or command.is_empty() after trimming) instead of accepting
them; modify the branch that builds McpTransportType::Http to only push when
entry.url.as_ref().map(|s| !s.trim().is_empty()).unwrap_or(false) (or
equivalent) and likewise only build McpTransportType::Stdio when entry.command
is non-empty, otherwise continue (and optionally log a warning mentioning the
entry name/source) so blank values are rejected at discovery time.
In `@src/tools/mcp/transport.rs`:
- Around line 99-104: The code pipes child stderr (created with let mut cmd =
tokio::process::Command::new(...) and .stderr(std::process::Stdio::piped())) but
never reads it, which can block if stderr fills; fix by either not piping stderr
(use .stderr(std::process::Stdio::null()) or
.stderr(std::process::Stdio::inherit())) or, if you need stderr, after spawning
the child take child.stderr, spawn a tokio task to continuously read/drain it
(e.g., read_to_end or copy into /dev/null or log it) so the pipe never fills and
the child cannot block; modify the code around the Command creation and the
child spawn/await logic that handles cmd to implement the chosen approach.
- Around line 133-163: The send() method currently locks stdin and stdout
separately, allowing concurrent callers to interleave and corrupt
request/response pairs; fix by serializing the entire request/response exchange
with a single in-flight guard (e.g., add and use a single async Mutex or similar
token on the Transport struct) so each call to send() acquires that guard before
serializing the request, writing/flushing to self.stdin, and reading a full
response_line from self.stdout (respect existing timeout_secs), then release the
guard after parsing the response to ensure no cross-request corruption.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
AGENTS.mdCLAUDE.mdsrc/cli/common.rssrc/config/types.rssrc/tools/mcp/client.rssrc/tools/mcp/discovery.rssrc/tools/mcp/mod.rssrc/tools/mcp/transport.rs
|
|
||
| # Test counts (cargo test) | ||
| # lib: 2612, main: 97, cli_smoke: 23, e2e: 13, integration: 70, doc: 122 passed (33 ignored) | ||
| # lib: 2635, main: 97, cli_smoke: 23, e2e: 13, integration: 70, doc: 121 passed (27 ignored) |
There was a problem hiding this comment.
Test-count documentation is internally inconsistent.
You updated the quick-reference count here, but the later Testing section still shows older totals. Please sync both count locations from the same source to avoid stale instructions.
As per coding guidelines: *.md: "Keep README and documentation claims aligned with executable behavior" and CLAUDE.md: "Update CLAUDE.md ... test counts ... after completing any feature".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` at line 24, The quick-reference test-count line ("# lib: 2635,
main: 97, cli_smoke: 23, e2e: 13, integration: 70, doc: 121 passed (27
ignored)") and the later "Testing" section totals in CLAUDE.md are inconsistent;
update the totals in the "Testing" section to exactly match the quick-reference
numbers (or vice-versa) by copying the authoritative counts from the same
test-run source, and ensure both locations use the same phrasing and totals so
they remain synchronized; look for the exact quick-reference string and the
"Testing" heading in CLAUDE.md to find and update the mismatched counts.
Comment on lines
+1097
to
+1122
| let workspace = config.workspace_path(); | ||
| let mut all_servers = discover_mcp_servers(Some(&workspace)); | ||
|
|
||
| for server_cfg in &config.mcp.servers { | ||
| let transport = if let Some(url) = server_cfg.url.clone() { | ||
| McpTransportType::Http { url } | ||
| } else if let Some(command) = server_cfg.command.clone() { | ||
| McpTransportType::Stdio { | ||
| command, | ||
| args: server_cfg.args.clone().unwrap_or_default(), | ||
| env: server_cfg.env.clone().unwrap_or_default(), | ||
| } | ||
| } else { | ||
| warn!( | ||
| server = %server_cfg.name, | ||
| "MCP server config has neither url nor command, skipping" | ||
| ); | ||
| continue; | ||
| }; | ||
|
|
||
| all_servers.push(DiscoveredMcpServer { | ||
| name: server_cfg.name.clone(), | ||
| transport, | ||
| source: "config".to_string(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Deduplicate MCP servers by name before registration.
When the same server name appears in discovery and config.mcp.servers, this block registers both, which can create duplicate/conflicting {server}_{tool} entries.
💡 Suggested fix (config entry overrides discovered entry)
let workspace = config.workspace_path();
let mut all_servers = discover_mcp_servers(Some(&workspace));
+ let mut seen_names = HashSet::new();
+ all_servers.retain(|s| seen_names.insert(s.name.to_ascii_lowercase()));
for server_cfg in &config.mcp.servers {
+ // Config-defined server should override discovered server with same name.
+ all_servers.retain(|s| !s.name.eq_ignore_ascii_case(&server_cfg.name));
let transport = if let Some(url) = server_cfg.url.clone() {
McpTransportType::Http { url }
} else if let Some(command) = server_cfg.command.clone() {
McpTransportType::Stdio {
command,
args: server_cfg.args.clone().unwrap_or_default(),
env: server_cfg.env.clone().unwrap_or_default(),
}
} else {
warn!(
server = %server_cfg.name,
"MCP server config has neither url nor command, skipping"
);
continue;
};
all_servers.push(DiscoveredMcpServer {
name: server_cfg.name.clone(),
transport,
source: "config".to_string(),
});
}📝 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
| let workspace = config.workspace_path(); | |
| let mut all_servers = discover_mcp_servers(Some(&workspace)); | |
| for server_cfg in &config.mcp.servers { | |
| let transport = if let Some(url) = server_cfg.url.clone() { | |
| McpTransportType::Http { url } | |
| } else if let Some(command) = server_cfg.command.clone() { | |
| McpTransportType::Stdio { | |
| command, | |
| args: server_cfg.args.clone().unwrap_or_default(), | |
| env: server_cfg.env.clone().unwrap_or_default(), | |
| } | |
| } else { | |
| warn!( | |
| server = %server_cfg.name, | |
| "MCP server config has neither url nor command, skipping" | |
| ); | |
| continue; | |
| }; | |
| all_servers.push(DiscoveredMcpServer { | |
| name: server_cfg.name.clone(), | |
| transport, | |
| source: "config".to_string(), | |
| }); | |
| } | |
| let workspace = config.workspace_path(); | |
| let mut all_servers = discover_mcp_servers(Some(&workspace)); | |
| let mut seen_names = HashSet::new(); | |
| all_servers.retain(|s| seen_names.insert(s.name.to_ascii_lowercase())); | |
| for server_cfg in &config.mcp.servers { | |
| // Config-defined server should override discovered server with same name. | |
| all_servers.retain(|s| !s.name.eq_ignore_ascii_case(&server_cfg.name)); | |
| let transport = if let Some(url) = server_cfg.url.clone() { | |
| McpTransportType::Http { url } | |
| } else if let Some(command) = server_cfg.command.clone() { | |
| McpTransportType::Stdio { | |
| command, | |
| args: server_cfg.args.clone().unwrap_or_default(), | |
| env: server_cfg.env.clone().unwrap_or_default(), | |
| } | |
| } else { | |
| warn!( | |
| server = %server_cfg.name, | |
| "MCP server config has neither url nor command, skipping" | |
| ); | |
| continue; | |
| }; | |
| all_servers.push(DiscoveredMcpServer { | |
| name: server_cfg.name.clone(), | |
| transport, | |
| source: "config".to_string(), | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/common.rs` around lines 1097 - 1122, The loop currently appends
config-specified MCP servers into all_servers (populated by
discover_mcp_servers) causing duplicate server names; change the logic in this
block so config entries override discovered ones by name: for each server_cfg
from config.mcp.servers, check all_servers for an existing DiscoveredMcpServer
with the same name (from discover_mcp_servers), and if found replace that entry
with the new transport/source ("config"), otherwise push the new
DiscoveredMcpServer as before; use the existing symbols discover_mcp_servers,
all_servers, DiscoveredMcpServer, and server_cfg.name to locate where to perform
the lookup-and-replace.
Comment on lines
+92
to
+108
| if let Some(url) = entry.url { | ||
| result.push(DiscoveredMcpServer { | ||
| name, | ||
| transport: McpTransportType::Http { url }, | ||
| source: source.to_string(), | ||
| }); | ||
| } else if let Some(command) = entry.command { | ||
| result.push(DiscoveredMcpServer { | ||
| name, | ||
| transport: McpTransportType::Stdio { | ||
| command, | ||
| args: entry.args.unwrap_or_default(), | ||
| env: entry.env.unwrap_or_default(), | ||
| }, | ||
| source: source.to_string(), | ||
| }); | ||
| } else { |
There was a problem hiding this comment.
Skip blank url/command values during discovery.
Entries with url: "" or command: "" currently pass discovery and fail later. Rejecting them here gives cleaner behavior and logs.
💡 Suggested fix
- if let Some(url) = entry.url {
+ if let Some(url) = entry
+ .url
+ .as_deref()
+ .map(str::trim)
+ .filter(|u| !u.is_empty())
+ .map(str::to_string)
+ {
result.push(DiscoveredMcpServer {
name,
transport: McpTransportType::Http { url },
source: source.to_string(),
});
- } else if let Some(command) = entry.command {
+ } else if let Some(command) = entry
+ .command
+ .as_deref()
+ .map(str::trim)
+ .filter(|c| !c.is_empty())
+ .map(str::to_string)
+ {
result.push(DiscoveredMcpServer {
name,
transport: McpTransportType::Stdio {
command,
args: entry.args.unwrap_or_default(),
env: entry.env.unwrap_or_default(),
},
source: source.to_string(),
});
} else {📝 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
| if let Some(url) = entry.url { | |
| result.push(DiscoveredMcpServer { | |
| name, | |
| transport: McpTransportType::Http { url }, | |
| source: source.to_string(), | |
| }); | |
| } else if let Some(command) = entry.command { | |
| result.push(DiscoveredMcpServer { | |
| name, | |
| transport: McpTransportType::Stdio { | |
| command, | |
| args: entry.args.unwrap_or_default(), | |
| env: entry.env.unwrap_or_default(), | |
| }, | |
| source: source.to_string(), | |
| }); | |
| } else { | |
| if let Some(url) = entry | |
| .url | |
| .as_deref() | |
| .map(str::trim) | |
| .filter(|u| !u.is_empty()) | |
| .map(str::to_string) | |
| { | |
| result.push(DiscoveredMcpServer { | |
| name, | |
| transport: McpTransportType::Http { url }, | |
| source: source.to_string(), | |
| }); | |
| } else if let Some(command) = entry | |
| .command | |
| .as_deref() | |
| .map(str::trim) | |
| .filter(|c| !c.is_empty()) | |
| .map(str::to_string) | |
| { | |
| result.push(DiscoveredMcpServer { | |
| name, | |
| transport: McpTransportType::Stdio { | |
| command, | |
| args: entry.args.unwrap_or_default(), | |
| env: entry.env.unwrap_or_default(), | |
| }, | |
| source: source.to_string(), | |
| }); | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/mcp/discovery.rs` around lines 92 - 108, When constructing
DiscoveredMcpServer items in discovery.rs, skip entries whose entry.url or
entry.command are empty strings (e.g., url.is_empty() or command.is_empty()
after trimming) instead of accepting them; modify the branch that builds
McpTransportType::Http to only push when entry.url.as_ref().map(|s|
!s.trim().is_empty()).unwrap_or(false) (or equivalent) and likewise only build
McpTransportType::Stdio when entry.command is non-empty, otherwise continue (and
optionally log a warning mentioning the entry name/source) so blank values are
rejected at discovery time.
C1: Track MCP clients on AgentLoop and shut them down gracefully in stop(). Previously shutdown() was never called, relying only on a best-effort Drop impl that could leave zombie child processes. C2: Combine separate stdin/stdout mutexes into a single Mutex<StdioIo> so the entire write→read cycle is serialized. Separate locks allowed concurrent send() calls to interleave requests and misroute responses. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/cli/common.rs (1)
1100-1122:⚠️ Potential issue | 🟠 MajorDeduplicate MCP servers by name before registration.
Line 1100 appends config-defined servers on top of discovered ones without removing same-name entries first. This can register duplicate/conflicting
{server}_{tool}names and create duplicate clients for one logical server.💡 Proposed fix (config entries override discovered entries)
let workspace = config.workspace_path(); let mut all_servers = discover_mcp_servers(Some(&workspace)); + let mut seen_names = HashSet::new(); + all_servers.retain(|s| seen_names.insert(s.name.to_ascii_lowercase())); for server_cfg in &config.mcp.servers { + // Config-defined entry overrides discovered entry with same name. + all_servers.retain(|s| !s.name.eq_ignore_ascii_case(&server_cfg.name)); let transport = if let Some(url) = server_cfg.url.clone() { McpTransportType::Http { url } } else if let Some(command) = server_cfg.command.clone() { McpTransportType::Stdio { command, args: server_cfg.args.clone().unwrap_or_default(), env: server_cfg.env.clone().unwrap_or_default(), } } else { warn!( server = %server_cfg.name, "MCP server config has neither url nor command, skipping" ); continue; }; all_servers.push(DiscoveredMcpServer { name: server_cfg.name.clone(), transport, source: "config".to_string(), }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/common.rs` around lines 1100 - 1122, The loop that pushes config-defined servers into all_servers may produce duplicates because discovered servers with the same name are not removed; update the logic that iterates config.mcp.servers (using server_cfg and creating DiscoveredMcpServer instances) to ensure config entries override discovered ones—either deduplicate by converting all_servers into a map keyed by name before adding config items and replace any existing entry with the config variant, or explicitly remove any existing all_servers entry with the same name prior to pushing the new DiscoveredMcpServer; ensure the resulting behavior keeps config-defined server_cfg entries and prevents duplicate {server}_{tool} registrations.src/tools/mcp/transport.rs (1)
109-114:⚠️ Potential issue | 🟠 MajorAvoid piping child stderr when it is never consumed.
Line 113 pipes stderr but nothing reads it. If the MCP server writes enough logs, stderr backpressure can block the child and stall responses.
💡 Proposed fix
cmd.args(args) .stdin(std::process::Stdio::piped()) .stdout(std::process::Stdio::piped()) - .stderr(std::process::Stdio::piped()); + .stderr(std::process::Stdio::inherit());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/mcp/transport.rs` around lines 109 - 114, The child process currently configures stderr as piped on the Command builder (the `cmd` created with `tokio::process::Command::new(command)` and modified with `.args(args)`), but nothing reads from that pipe which can cause backpressure and stall the child; change the stderr setup to not pipe it (e.g., use `std::process::Stdio::null()` or inherit it) by replacing the `.stderr(std::process::Stdio::piped())` call with a non-piped alternative so the child cannot block on unwritten stderr.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools/mcp/transport.rs`:
- Around line 182-184: The match on
tokio::time::timeout(std::time::Duration::from_secs(3), child.wait()).await
currently treats any Ok(_) as success and therefore swallows errors from
child.wait(); change the arms to inspect the nested Result from timeout so that
Ok(Ok(status)) is treated as success (and you can optionally check
status.success()), Ok(Err(e)) returns or logs/propagates that error from
child.wait(), and Err(_) still handles the timeout case—update the match in
transport.rs around the child.wait() call to explicitly handle Ok(Ok(...)),
Ok(Err(e)) and Err(_) instead of collapsing all Ok variants.
---
Duplicate comments:
In `@src/cli/common.rs`:
- Around line 1100-1122: The loop that pushes config-defined servers into
all_servers may produce duplicates because discovered servers with the same name
are not removed; update the logic that iterates config.mcp.servers (using
server_cfg and creating DiscoveredMcpServer instances) to ensure config entries
override discovered ones—either deduplicate by converting all_servers into a map
keyed by name before adding config items and replace any existing entry with the
config variant, or explicitly remove any existing all_servers entry with the
same name prior to pushing the new DiscoveredMcpServer; ensure the resulting
behavior keeps config-defined server_cfg entries and prevents duplicate
{server}_{tool} registrations.
In `@src/tools/mcp/transport.rs`:
- Around line 109-114: The child process currently configures stderr as piped on
the Command builder (the `cmd` created with
`tokio::process::Command::new(command)` and modified with `.args(args)`), but
nothing reads from that pipe which can cause backpressure and stall the child;
change the stderr setup to not pipe it (e.g., use `std::process::Stdio::null()`
or inherit it) by replacing the `.stderr(std::process::Stdio::piped())` call
with a non-piped alternative so the child cannot block on unwritten stderr.
Comment on lines
+182
to
+184
| match tokio::time::timeout(std::time::Duration::from_secs(3), child.wait()).await { | ||
| Ok(_) => Ok(()), | ||
| Err(_) => { |
There was a problem hiding this comment.
Do not swallow wait() errors during shutdown.
Line 183 treats any Ok(_) as success, including Ok(Err(e)), which hides real process wait failures.
💡 Proposed fix
- match tokio::time::timeout(std::time::Duration::from_secs(3), child.wait()).await {
- Ok(_) => Ok(()),
+ match tokio::time::timeout(std::time::Duration::from_secs(3), child.wait()).await {
+ Ok(Ok(_)) => Ok(()),
+ Ok(Err(e)) => Err(format!("Failed waiting for MCP server exit: {}", e)),
Err(_) => {
child
.kill()
.await
.map_err(|e| format!("Failed to kill MCP server: {}", e))?;
Ok(())
}
}📝 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
| match tokio::time::timeout(std::time::Duration::from_secs(3), child.wait()).await { | |
| Ok(_) => Ok(()), | |
| Err(_) => { | |
| match tokio::time::timeout(std::time::Duration::from_secs(3), child.wait()).await { | |
| Ok(Ok(_)) => Ok(()), | |
| Ok(Err(e)) => Err(format!("Failed waiting for MCP server exit: {}", e)), | |
| Err(_) => { | |
| child | |
| .kill() | |
| .await | |
| .map_err(|e| format!("Failed to kill MCP server: {}", e))?; | |
| Ok(()) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/mcp/transport.rs` around lines 182 - 184, The match on
tokio::time::timeout(std::time::Duration::from_secs(3), child.wait()).await
currently treats any Ok(_) as success and therefore swallows errors from
child.wait(); change the arms to inspect the nested Result from timeout so that
Ok(Ok(status)) is treated as success (and you can optionally check
status.success()), Ok(Err(e)) returns or logs/propagates that error from
child.wait(), and Err(_) still handles the timeout case—update the match in
transport.rs around the child.wait() call to explicitly handle Ok(Ok(...)),
Ok(Err(e)) and Err(_) instead of collapsing all Ok variants.
…adlock 1. stop() now checks Handle::try_current() before spawning MCP shutdown. Falls back to Drop-based cleanup when called outside a Tokio runtime (library embedding, shutdown hooks). 2. StdioTransport stderr changed from Stdio::piped() to Stdio::null(). Piped stderr was never consumed, so noisy MCP servers could fill the pipe buffer, block, and hang requests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/tools/mcp/transport.rs (1)
179-192:⚠️ Potential issue | 🟡 Minor
child.wait()errors are still swallowed.Line 183 matches
Ok(_)which includes bothOk(Ok(exit_status))andOk(Err(io_error)). Ifchild.wait()returns an I/O error, it's silently treated as success.🔧 Proposed fix to propagate wait errors
match tokio::time::timeout(std::time::Duration::from_secs(3), child.wait()).await { - Ok(_) => Ok(()), + Ok(Ok(_)) => Ok(()), + Ok(Err(e)) => Err(format!("Failed waiting for MCP server exit: {}", e)), Err(_) => { child .kill()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/mcp/transport.rs` around lines 179 - 192, The shutdown method currently treats any Ok from tokio::time::timeout as success, which hides I/O errors from child.wait(); update the match on tokio::time::timeout(std::time::Duration::from_secs(3), child.wait()).await in async fn shutdown(&self) to explicitly handle the nested Result: on Ok(Ok(_)) return Ok(()), on Ok(Err(e)) return Err(format!("Failed waiting for MCP server: {}", e)), and on Err(_) perform the existing child.kill().await error mapping; reference child.wait(), child.kill(), and async fn shutdown to locate the code to change.
🧹 Nitpick comments (1)
src/tools/mcp/transport.rs (1)
240-255: Test usescatas echo server — fragile assertion.The test at line 249-252 allows both success and parse error, which is correct since
catechoes the JSON back verbatim (valid MCP response structure but not semantically valid). However, consider adding a comment explaining why this behavior is expected.📝 Suggested clarifying comment
let req = McpRequest::new(1, "initialize", None); let resp = t.send(&req).await; + // `cat` echoes our request back verbatim. Since McpRequest and McpResponse + // have different structures, parsing may succeed (if fields overlap) or + // fail with a parse error — either outcome proves the I/O path works. assert!( resp.is_ok() || resp.unwrap_err().contains("parse"), "Should get I/O success or parse error, not a crash" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/mcp/transport.rs` around lines 240 - 255, The test test_stdio_transport_echo_server uses StdioTransport::spawn("cat", ...) and then sends an MCP request via McpRequest::new and t.send(&req).await; update the test to include a short clarifying comment above the assertion explaining that using "cat" simply echoes bytes back (so we may get a successful I/O result or a JSON parse error because the echoed payload is not a semantically valid MCP response), and that the assertion allowing either resp.is_ok() or a parse error is intentional to avoid flakiness when using a raw echo server; leave the existing assert logic (and calls to transport_type() and t.shutdown()) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/tools/mcp/transport.rs`:
- Around line 179-192: The shutdown method currently treats any Ok from
tokio::time::timeout as success, which hides I/O errors from child.wait();
update the match on tokio::time::timeout(std::time::Duration::from_secs(3),
child.wait()).await in async fn shutdown(&self) to explicitly handle the nested
Result: on Ok(Ok(_)) return Ok(()), on Ok(Err(e)) return Err(format!("Failed
waiting for MCP server: {}", e)), and on Err(_) perform the existing
child.kill().await error mapping; reference child.wait(), child.kill(), and
async fn shutdown to locate the code to change.
---
Nitpick comments:
In `@src/tools/mcp/transport.rs`:
- Around line 240-255: The test test_stdio_transport_echo_server uses
StdioTransport::spawn("cat", ...) and then sends an MCP request via
McpRequest::new and t.send(&req).await; update the test to include a short
clarifying comment above the assertion explaining that using "cat" simply echoes
bytes back (so we may get a successful I/O result or a JSON parse error because
the echoed payload is not a semantically valid MCP response), and that the
assertion allowing either resp.is_ok() or a parse error is intentional to avoid
flakiness when using a raw echo server; leave the existing assert logic (and
calls to transport_type() and t.shutdown()) unchanged.
1. StdioTransport now uses MCP-spec Content-Length header framing instead of newline-delimited JSON. This matches the official MCP stdio transport specification and interoperates with standard SDK servers (TypeScript, Python, etc.). 2. Moved MCP client shutdown out of stop() into a separate public shutdown_mcp_clients() method. stop() must remain restart-safe; killing MCP clients there would leave registered tools pointing at dead transports after restart. Gateway teardown and hot-reload paths now call shutdown_mcp_clients() explicitly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/tools/mcp/transport.rs (1)
185-198:⚠️ Potential issue | 🟡 Minor
child.wait()errors are still being swallowed.The match on line 188-189 treats any
Ok(_)as success, butchild.wait()returnsResult<ExitStatus, io::Error>. When the timeout succeeds butwait()itself fails (i.e.,Ok(Err(e))), the error is discarded.,
🔧 Proposed fix to propagate wait errors
async fn shutdown(&self) -> Result<(), String> { let mut child = self.child.lock().await; match tokio::time::timeout(std::time::Duration::from_secs(3), child.wait()).await { - Ok(_) => Ok(()), + Ok(Ok(_)) => Ok(()), + Ok(Err(e)) => Err(format!("Failed waiting for MCP server exit: {}", e)), Err(_) => { child .kill() .await .map_err(|e| format!("Failed to kill MCP server: {}", e))?; Ok(()) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/mcp/transport.rs` around lines 185 - 198, The shutdown function is swallowing errors from child.wait(): when awaiting tokio::time::timeout you must handle the nested Result<Result<ExitStatus, io::Error>, Elapsed>; change the match in shutdown (referencing shutdown, child.wait(), and tokio::time::timeout) to explicitly pattern-match Ok(Ok(_)) => Ok(()), Ok(Err(e)) => return Err(format!("Failed waiting for MCP server: {}", e)) (or attempt kill then return the wait error), and Err(_) => perform the kill as currently implemented; this ensures child.wait() errors are propagated instead of discarded.
🧹 Nitpick comments (1)
src/tools/mcp/transport.rs (1)
32-49: Consider handling client builder failure more explicitly.Line 38 uses
unwrap_or_default()which silently falls back to a default client if the builder fails (e.g., due to invalid timeout configuration). While this won't crash, it could mask configuration issues.💡 Optional: Log a warning on builder failure
pub fn new(url: &str, timeout_secs: u64) -> Self { let http = reqwest::Client::builder() .timeout(std::time::Duration::from_secs(timeout_secs)) .build() - .unwrap_or_default(); + .unwrap_or_else(|e| { + tracing::warn!("Failed to build HTTP client with timeout, using default: {}", e); + reqwest::Client::default() + }); Self { url: url.to_string(), http, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/mcp/transport.rs` around lines 32 - 49, The code in HttpTransport::new currently calls reqwest::Client::builder().timeout(...).build().unwrap_or_default(), which silently masks builder failures; change HttpTransport::new to handle build()'s Result explicitly by either returning Result<Self, reqwest::Error> or logging the build error and failing fast instead of falling back silently. Concretely, replace the unwrap_or_default() with a match or ? propagation on reqwest::Client::builder().build() inside HttpTransport::new (or use .map_err(...) to convert the error) so that failures are reported (via process logger or error return) and only construct Self { url: ..., http } when build() succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/tools/mcp/transport.rs`:
- Around line 185-198: The shutdown function is swallowing errors from
child.wait(): when awaiting tokio::time::timeout you must handle the nested
Result<Result<ExitStatus, io::Error>, Elapsed>; change the match in shutdown
(referencing shutdown, child.wait(), and tokio::time::timeout) to explicitly
pattern-match Ok(Ok(_)) => Ok(()), Ok(Err(e)) => return Err(format!("Failed
waiting for MCP server: {}", e)) (or attempt kill then return the wait error),
and Err(_) => perform the kill as currently implemented; this ensures
child.wait() errors are propagated instead of discarded.
---
Nitpick comments:
In `@src/tools/mcp/transport.rs`:
- Around line 32-49: The code in HttpTransport::new currently calls
reqwest::Client::builder().timeout(...).build().unwrap_or_default(), which
silently masks builder failures; change HttpTransport::new to handle build()'s
Result explicitly by either returning Result<Self, reqwest::Error> or logging
the build error and failing fast instead of falling back silently. Concretely,
replace the unwrap_or_default() with a match or ? propagation on
reqwest::Client::builder().build() inside HttpTransport::new (or use
.map_err(...) to convert the error) so that failures are reported (via process
logger or error return) and only construct Self { url: ..., http } when build()
succeeds.
This was referenced Feb 28, 2026
This was referenced Mar 3, 2026
This was referenced Mar 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary\n- add transport abstraction for MCP with McpTransport, HttpTransport, and StdioTransport\n- refactor McpClient to be transport-agnostic and add new_stdio() + shutdown()\n- include stdio MCP entries in discovery with explicit McpTransportType\n- wire MCP server discovery/config registration into create_agent()\n- extend MCP config schema (url optional + command/args/env) and update docs\n\n## Verification\n- cargo fmt -- --check\n- cargo clippy -- -D warnings\n- cargo test\n\nCloses #191
Summary by CodeRabbit
New Features
Documentation