Refactor architecture modules for cargo and rust-analyzer tools by qkal · Pull Request #2 · qkal/rustmcp

qkal · 2026-05-23T02:47:09Z

Summary

Split cargo internals into params, args, output, process, and tool orchestration modules behind the existing facade.
Split rust-analyzer result shaping into focused ra helper modules while keeping rmcp tool wrappers on RaMcpServer.
Move response envelope and server state helpers into dedicated server modules, then convert cargo/server to directory module layouts.

Test Plan

cargo fmt --check
cargo test --locked --all
cargo clippy --locked --all-targets --all-features -- -D warnings
cargo build --release --locked
.\target\release\rust-analyzer-mcp.exe --version

Summary by CodeRabbit

Release Notes

Documentation
- Added comprehensive architecture planning and design documents detailing the internal module reorganization strategy for improved code clarity.
Refactor
- Reorganized internal codebase structure into distinct, well-defined module boundaries for cargo operations and rust-analyzer tool functionality.
- Extracted and centralized response envelope handling and supporting helper functions to enhance code maintainability.

coderabbitai · 2026-05-23T02:47:20Z

Warning

Review limit reached

@qkal, we couldn't start this review because you've used your available PR reviews for now.

Your plan currently allows 1 review/hour. Refill in 44 minutes and 50 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: e0021f5d-aa4c-4292-9cc7-54fd496cb90d

📥 Commits

Reviewing files that changed from the base of the PR and between 5555687 and 8fbb641.

📒 Files selected for processing (7)

src/cargo/args.rs
src/cargo/process.rs
src/cargo/tools.rs
src/error.rs
src/server/mod.rs
src/server/response.rs
src/server/state.rs

📝 Walkthrough

Walkthrough

This PR executes a multi-phase architecture cleanup that reorganizes the codebase into focused module subsystems. The server module gains state.rs and response.rs submodules; cargo is split into args, output, params, process, and tools submodules with a facade mod.rs; and a new ra module aggregates rust-analyzer helper functions. Server handlers are refactored to use these new subsystem APIs, and src/tools.rs becomes a re-export facade. The refactor preserves all existing MCP tool behavior and naming while clarifying internal code organization.

Changes

Architecture Cleanup Refactor

Layer / File(s)	Summary
Architecture documentation `docs/superpowers/plans/2026-05-23-architecture-cleanup.md`, `docs/superpowers/specs/2026-05-23-architecture-cleanup-design.md`	Design and implementation plan documents outlining the refactor goals, module structure, staged migration steps, and regression verification targets.
Response envelope and re-export foundation `src/server/response.rs`, `src/tools.rs`	Extracts MCP tool response envelope into `src/server/response.rs` with `ToolEnvelope`, `success()`, `failure()`, and output truncation enforcement; `src/tools.rs` begins re-exporting from this module.
Cargo subsystem parameters and output types `src/cargo/params.rs`, `src/cargo/output.rs`	Extracts cargo configuration constants and parameter structs into `src/cargo/params.rs`, and adds output/truncation types to `src/cargo/output.rs` with UTF-8-safe truncation and metadata JSON parsing.
Cargo process execution and argument validation `src/cargo/args.rs`, `src/cargo/process.rs`	Splits cargo CLI construction into `src/cargo/args.rs` (argument validation), and implements process execution with timeout/cleanup in `src/cargo/process.rs`, including platform-specific process-tree termination and bounded I/O collection.
Cargo tool orchestration and module facade `src/cargo/tools.rs`, `src/cargo/mod.rs`	Adds `src/cargo/tools.rs` to gate and execute cargo commands with semaphore-bounded concurrency, response preparation, and hint-enriched error handling; wires `src/cargo/mod.rs` as the public facade with re-exports.
RA (rust-analyzer) parameter types `src/ra/params.rs`	Defines LSP operation parameter structs and defaults for workspace, hover, definition, references, symbols, completion, formatting, code actions, and diagnostics operations.
RA helper functions for LSP normalization `src/ra/locate.rs`, `src/ra/navigation.rs`, `src/ra/completion.rs`, `src/ra/edits.rs`, `src/ra/symbols.rs`, `src/ra/diagnostics.rs`, `src/ra/mod.rs`, `src/lib.rs`	Extracts LSP response normalization helpers into dedicated `src/ra/*` modules: location/range handling, definition/reference resolution, completion items, code actions, document symbols, and diagnostics summaries; exports via `src/ra/mod.rs`.
Server state and error handling centralization `src/server/state.rs`	Extracts server configuration and stateful helpers into `src/server/state.rs`, including `ServerConfig`, `ServerState` with workspace/client lifecycle, workspace notes, and error hint mapping for user-facing guidance.
Server handler refactoring to use new modules `src/server/mod.rs`	Refactors all LSP tool handlers to use `ServerState` helpers, call new `crate::ra::*` functions for response normalization, and updates cargo tool handlers to call `crate::cargo::tools::run_cargo_tool()` with proper error hinting.
Tools.rs re-export facade and test reorganization `src/tools.rs`, `tests/cargo_tests.rs`	Converts `src/tools.rs` from a monolithic module into a pure re-export facade from `cargo/params`, `ra/params`, and `server/response`; reorganizes `tests/cargo_tests.rs` into submodules and adds UTF-8 truncation boundary test.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

qkal/rustmcp#1: The retrieved PR's new fixed cargo_* MCP tools and their cargo invocation/execution wiring are directly refactored in the main PR, which splits and relocates that cargo-tool implementation across src/cargo/* and updates src/server/mod.rs handlers to call the new crate::cargo::tools::run_cargo_tool(...)/response helpers.

Poem

🐰 A rabbit hops through the code so neat,
Modules split where once tangled beats,
Cargo, server, RA—each in their place,
Refactored with style, with architectural grace!
Tests passing, hints flowing, boundaries clear,
The cleanup complete—hooray, hip hooray! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Refactor architecture modules for cargo and rust-analyzer tools' accurately summarizes the main change: a multi-module refactoring that reorganizes cargo and rust-analyzer logic into clearer, separate module boundaries.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/architecture-cleanup

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

devin-ai-integration

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 4 additional findings.

coderabbitai

Actionable comments posted: 3

🤖 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/process.rs`:
- Around line 51-60: When any early return or error occurs (e.g., the Err branch
of tokio::time::timeout around child.wait() or early returns inside
collect_task_outputs), currently spawned reader tasks are left running; modify
the error paths to always abort or join those background tasks before returning.
Specifically, in the timeout match around child.wait() ensure you abort/await
the spawned stdout/stderr reader JoinHandles (the reader tasks created earlier)
in the Ok(Err(error)) and Err(_) branches so they don't detach; likewise update
collect_task_outputs to cancel the sibling task and await its JoinHandle on any
stream error or early return. Ensure you call handle.abort() and then
handle.await().ok() (or otherwise await/join) for the unique JoinHandle
variables so no background tasks remain running.

In `@src/server/response.rs`:
- Around line 62-79: envelope_text currently only enforces
DEFAULT_MAX_TOTAL_OUTPUT_BYTES before truncation; after replacing
envelope.result it reserializes once and returns without guaranteeing the cap.
Modify envelope_text (function handling ToolEnvelope) so that after the second
serde_json::to_string_pretty(&envelope) you check text.len() again and, if it
still exceeds DEFAULT_MAX_TOTAL_OUTPUT_BYTES, further reduce payloads (e.g.,
replace envelope.result with a minimal sentinel message, clear or truncate
envelope.notes and envelope.input) while keeping envelope.truncated = true, then
reserialize in a loop or retry until text.len() <=
DEFAULT_MAX_TOTAL_OUTPUT_BYTES; ensure you use the same error-safe
unwrap_or_else fallback for serialization.

In `@src/server/state.rs`:
- Around line 22-27: The ensure_client method currently returns &mut
RustAnalyzerClient from ServerState which forces callers to hold the ServerState
mutex for the entire async round-trip; refactor by extracting the client field
into its own lock/handle (e.g., replace the single client
Option<RustAnalyzerClient> inside ServerState with a separate
Arc<Mutex<Option<ClientHandle>>> or an Arc<ClientHandle> that can be cloned) and
change ensure_client to return a cloned, owned handle (not a &mut reference) or
a future-safe proxy that performs async operations without holding the global
ServerState mutex; keep workspace/config fields on the original ServerState and
add new accessor methods to read them separately so callers acquire the smaller
lock only to spawn or swap the client (identify code referencing ensure_client,
ServerState::client, and RustAnalyzerClient::spawn to implement the split).

🪄 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: 13cf0f01-eb51-435a-a885-3f95ead5424b

📥 Commits

Reviewing files that changed from the base of the PR and between bd13199 and 5555687.

📒 Files selected for processing (22)

docs/superpowers/plans/2026-05-23-architecture-cleanup.md
docs/superpowers/specs/2026-05-23-architecture-cleanup-design.md
src/cargo/args.rs
src/cargo/mod.rs
src/cargo/output.rs
src/cargo/params.rs
src/cargo/process.rs
src/cargo/tools.rs
src/lib.rs
src/ra/completion.rs
src/ra/diagnostics.rs
src/ra/edits.rs
src/ra/locate.rs
src/ra/mod.rs
src/ra/navigation.rs
src/ra/params.rs
src/ra/symbols.rs
src/server/mod.rs
src/server/response.rs
src/server/state.rs
src/tools.rs
tests/cargo_tests.rs

📜 Review details

🧰 Additional context used

🪛 LanguageTool

docs/superpowers/specs/2026-05-23-architecture-cleanup-design.md

[style] ~19-~19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... and metadata parsing. - src/tools.rs mixes tool parameter structs, defaults, and r...