[codex] Add fixed cargo MCP tools#1
Conversation
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 51 minutes and 41 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds fixed cargo command execution to the rust-analyzer MCP server. The implementation includes a cargo runner with concurrent output reading, timeout enforcement, and platform-specific process cleanup; five MCP tool handlers (cargo_check, cargo_test, cargo_clippy, cargo_fmt_check, cargo_metadata); configurable server-wide toggle for enabling/disabling cargo tools; comprehensive parameter validation and type-safe CLI argument building; and extensive unit and integration tests covering success paths, timeouts, output truncation, and stdin handling. ChangesCargo Tool Execution
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 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 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| let mut command = StdCommand::new(&invocation.command); | ||
| command | ||
| .args(&invocation.args) | ||
| .current_dir(workspace_root) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()); |
There was a problem hiding this comment.
🔴 Cargo process inherits MCP server's stdin instead of using Stdio::null()
The run_cargo function sets stdout(Stdio::piped()) and stderr(Stdio::piped()) but does not set stdin, which defaults to Stdio::inherit(). Since the MCP server's stdin is the JSON-RPC protocol transport, cargo and all its child processes (build scripts, proc macros, test binaries) inherit this file descriptor. If any subprocess reads from stdin, it will consume MCP protocol messages, causing protocol desynchronization and communication failures.
Contrast with existing rust-analyzer client setup
The existing RustAnalyzerClient::spawn at src/lsp/client.rs:66-71 correctly isolates its stdin with .stdin(std::process::Stdio::piped()). The cargo command at src/cargo.rs:128-133 is missing an equivalent isolation. The taskkill.exe helper at line 277 already uses .stdin(Stdio::null()), showing the pattern is known but was missed for the main cargo command.
| let mut command = StdCommand::new(&invocation.command); | |
| command | |
| .args(&invocation.args) | |
| .current_dir(workspace_root) | |
| .stdout(Stdio::piped()) | |
| .stderr(Stdio::piped()); | |
| let mut command = StdCommand::new(&invocation.command); | |
| command | |
| .args(&invocation.args) | |
| .current_dir(workspace_root) | |
| .stdin(Stdio::null()) | |
| .stdout(Stdio::piped()) | |
| .stderr(Stdio::piped()); |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/cargo.rs`:
- Around line 191-195: The TimedOut branch in the match for
OutputCollection::TimedOut should perform the same descendant cleanup as other
terminal branches: call cleanup_process_tree with the run/process identifier
used elsewhere in this function before returning truncated_output_pair(), set
timed_out = true and push the note as already done, and handle/log any error
from cleanup_process_tree similarly to the other branch implementations; locate
the OutputCollection::TimedOut arm and insert the cleanup_process_tree(...) call
(using the same process id/handle variable used by the function) prior to
returning truncated_output_pair() so long-lived child processes are cleaned up
on timeout.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 7914e85c-7c0d-45a2-8bd7-15cf11e3b1f5
📒 Files selected for processing (5)
README.mdsrc/cargo.rssrc/server.rstests/cargo_tests.rstests/integration_basic.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test (windows-latest)
🔇 Additional comments (2)
README.md (2)
241-241: LGTM!
351-352: LGTM!
1 participant
Summary
Test Plan
Summary by CodeRabbit
Release Notes
New Features
cargo_check,cargo_test,cargo_clippy,cargo_fmt_check, andcargo_metadatafor executing cargo commands with customizable parameters (timeouts, output limits, features, targets, test filters, lock/offline flags)--disable-cargo-toolsCLI option to disable cargo tool executionDocumentation