refactor: update terminology from "server" to "tool" in MCP components#14
Merged
maximedogawa merged 1 commit intoApr 11, 2026
Merged
Conversation
- Changed references in `AddServerForm`, `McpServerCard`, and `McpToolsPanel` to use "tool" instead of "server" for consistency. - Updated error messages and UI labels to reflect the new terminology. - Enhanced comments and documentation to clarify the purpose of the components related to tools. - Introduced a new module for resolving executable paths for subprocesses, improving command handling in the MCP context.
📝 WalkthroughWalkthroughThis PR introduces a new executable path resolver module to centralize CLI command resolution for subprocess spawning, refactors runtime binary candidate generation to use this shared logic, updates MCP tool registry filtering and configuration resolution with environment variable override support, and rewords user-facing MCP terminology from "servers" to "tools" / "commands" throughout the UI. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as StdioTransport::spawn
participant Resolver as resolve_command_for_spawn
participant Candidates as runtime_binary_candidates
participant PATH as System PATH
participant FileSystem as File System
Client->>Resolver: resolve_command_for_spawn(command)
Resolver->>Resolver: Check if empty/absolute/contains-separator
alt Path resolves directly
Resolver-->>Client: Return original path
else Need to search
Resolver->>Candidates: runtime_binary_candidates(name)
Candidates->>PATH: Scan environment PATH
Candidates->>FileSystem: Check OS-specific dirs<br/>(Homebrew, /usr/local/bin, etc.)
Candidates-->>Resolver: Return candidate paths
Resolver->>FileSystem: Iterate candidates<br/>validate executable bit
FileSystem-->>Resolver: First valid executable
Resolver-->>Client: Return resolved path
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src-tauri/src/modules/mcp/service.rs (1)
275-280: Semantic mismatch: logging provider count as "tools".
mcp_tool_count()returnsproviders.len()(the number of MCP server entries), but the log message says"ready ({n} tool{})". This could mislead users into thinking it's the total number of commands/tools exposed.If the intent is to log provider count, consider rewording the message:
♻️ Suggested log message clarification
let n = state.mcp.read().await.mcp_tool_count(); state .emit_log( "mcp", - &format!("ready ({n} tool{})", if n == 1 { "" } else { "s" }), + &format!("ready ({n} provider{})", if n == 1 { "" } else { "s" }), ) .await;Or, if the intent is to log actual tool/command count:
- let n = state.mcp.read().await.mcp_tool_count(); + let n = state.mcp.read().await.tool_names().len(); state .emit_log( "mcp", - &format!("ready ({n} tool{})", if n == 1 { "" } else { "s" }), + &format!("ready ({n} command{})", if n == 1 { "" } else { "s" }), ) .await;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/mcp/service.rs` around lines 275 - 280, The log message uses mcp_tool_count() (which returns the number of MCP providers) but text says "tool(s)", causing a semantic mismatch; update the emit_log call to reflect providers instead of tools (e.g., "ready ({n} provider{})") or, if you intended to log the total number of commands/tools, replace the call to mcp_tool_count() with the correct function that sums commands across providers (or implement a new method like mcp_command_count()) and use that value in the emit_log call; adjust the formatting grammar logic (if n == 1) accordingly and keep emit_log and state.mcp references intact.src-tauri/src/modules/mcp/registry.rs (1)
146-149: Method namemcp_tool_countis misleading — it counts providers, not tools.The method returns
providers.len(), which is the number of MCP server/provider entries (e.g.,dice,te_*), not the total count of individual tools/commands exposed by those providers. The doc comment clarifies the intent, but the method name suggests counting tools.Consider renaming to
provider_count()orserver_count()for clarity, or change the implementation to return the actual tool count if that's the intended behavior.Option 1: Rename to reflect actual behavior
- /// Number of MCP tools connected (one per `servers` entry in `mcp.json`: `dice`, `te_*`, …). - pub fn mcp_tool_count(&self) -> usize { + /// Number of MCP providers connected (one per `servers` entry in `mcp.json`: `dice`, `te_*`, …). + pub fn provider_count(&self) -> usize { self.providers.len() }Option 2: Return actual tool count
/// Number of MCP tools connected (one per `servers` entry in `mcp.json`: `dice`, `te_*`, …). - pub fn mcp_tool_count(&self) -> usize { - self.providers.len() + pub fn mcp_tool_count(&self) -> usize { + self.cached_tool_names.len() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/mcp/registry.rs` around lines 146 - 149, The method mcp_tool_count currently returns self.providers.len() which counts provider entries, not individual tools; either rename the function to provider_count (update the doc comment and replace all calls to mcp_tool_count with provider_count) to reflect the actual behavior, or if the intended behavior is to count tools, change mcp_tool_count to iterate over self.providers and sum the per-provider tool counts (e.g., sum the length of each provider's tool list), and update the doc comment accordingly; reference the mcp_tool_count function and the providers field when making the change and update all callers/usages.src-tauri/src/infrastructure/executable_resolve.rs (2)
101-114: Test coverage is too narrow for the new resolver module.Only absolute-path passthrough is covered. Consider adding cases for PATH candidate resolution and non-executable candidate rejection to prevent subtle regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/infrastructure/executable_resolve.rs` around lines 101 - 114, Add unit tests for resolve_command_for_spawn to expand coverage: keep the existing resolve_keeps_absolute_paths test, then add a test that simulates a command name found via PATH (e.g., create a temp dir, place an executable file there, prepend that dir to PATH, call resolve_command_for_spawn("mycmd") and assert it returns the temp executable PathBuf) and another test that ensures non-executable candidates are rejected (e.g., place a non-executable file with the command name in a PATH directory and assert resolve_command_for_spawn returns an error or a different result per current API). Use the resolve_command_for_spawn symbol and the tests module to locate where to add these cases and clean up temp PATH changes after each test.
23-31: Usestd::env::var_os+std::env::split_pathsfor PATH parsing.The current manual separator splitting is less robust and duplicates cross-platform logic from the standard library. Using
var_oshandles non-UTF8 environment values correctly, whilesplit_pathsprovides idiomatic, platform-aware PATH parsing.♻️ Suggested refactor
- if let Ok(path_var) = std::env::var("PATH") { - let sep = if cfg!(windows) { ';' } else { ':' }; - for dir in path_var.split(sep) { - if dir.is_empty() { - continue; - } - push_candidate(&mut out, Path::new(dir).join(name)); - } - } + if let Some(path_var) = std::env::var_os("PATH") { + for dir in std::env::split_paths(&path_var) { + if dir.as_os_str().is_empty() { + continue; + } + push_candidate(&mut out, dir.join(name)); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/infrastructure/executable_resolve.rs` around lines 23 - 31, The PATH parsing currently uses std::env::var and manual separator logic; replace this with std::env::var_os and std::env::split_paths to correctly handle non-UTF8 PATHs and platform-specific separators. In the block that builds candidates (where push_candidate(&mut out, Path::new(dir).join(name)) is called), call std::env::var_os("PATH"), and if Some(val) use std::env::split_paths(&val) to iterate PathBufs, skip empty entries as needed, join each PathBuf with name, and call push_candidate for each; remove the manual sep and cfg!(windows) logic. Ensure the rest of the function (including push_candidate) continues to receive Path values as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src-tauri/src/infrastructure/executable_resolve.rs`:
- Around line 101-114: Add unit tests for resolve_command_for_spawn to expand
coverage: keep the existing resolve_keeps_absolute_paths test, then add a test
that simulates a command name found via PATH (e.g., create a temp dir, place an
executable file there, prepend that dir to PATH, call
resolve_command_for_spawn("mycmd") and assert it returns the temp executable
PathBuf) and another test that ensures non-executable candidates are rejected
(e.g., place a non-executable file with the command name in a PATH directory and
assert resolve_command_for_spawn returns an error or a different result per
current API). Use the resolve_command_for_spawn symbol and the tests module to
locate where to add these cases and clean up temp PATH changes after each test.
- Around line 23-31: The PATH parsing currently uses std::env::var and manual
separator logic; replace this with std::env::var_os and std::env::split_paths to
correctly handle non-UTF8 PATHs and platform-specific separators. In the block
that builds candidates (where push_candidate(&mut out,
Path::new(dir).join(name)) is called), call std::env::var_os("PATH"), and if
Some(val) use std::env::split_paths(&val) to iterate PathBufs, skip empty
entries as needed, join each PathBuf with name, and call push_candidate for
each; remove the manual sep and cfg!(windows) logic. Ensure the rest of the
function (including push_candidate) continues to receive Path values as before.
In `@src-tauri/src/modules/mcp/registry.rs`:
- Around line 146-149: The method mcp_tool_count currently returns
self.providers.len() which counts provider entries, not individual tools; either
rename the function to provider_count (update the doc comment and replace all
calls to mcp_tool_count with provider_count) to reflect the actual behavior, or
if the intended behavior is to count tools, change mcp_tool_count to iterate
over self.providers and sum the per-provider tool counts (e.g., sum the length
of each provider's tool list), and update the doc comment accordingly; reference
the mcp_tool_count function and the providers field when making the change and
update all callers/usages.
In `@src-tauri/src/modules/mcp/service.rs`:
- Around line 275-280: The log message uses mcp_tool_count() (which returns the
number of MCP providers) but text says "tool(s)", causing a semantic mismatch;
update the emit_log call to reflect providers instead of tools (e.g., "ready
({n} provider{})") or, if you intended to log the total number of
commands/tools, replace the call to mcp_tool_count() with the correct function
that sums commands across providers (or implement a new method like
mcp_command_count()) and use that value in the emit_log call; adjust the
formatting grammar logic (if n == 1) accordingly and keep emit_log and state.mcp
references intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 805d6bdb-0097-4231-856f-2c894733c70d
📒 Files selected for processing (12)
src-tauri/src/infrastructure/executable_resolve.rssrc-tauri/src/infrastructure/mod.rssrc-tauri/src/modules/mcp/client.rssrc-tauri/src/modules/mcp/registry.rssrc-tauri/src/modules/mcp/service.rssrc-tauri/src/modules/mcp/transport.rssrc-tauri/src/modules/mcp/types.rssrc-tauri/src/modules/tool_engine/runtime.rssrc/modules/mcp/components/AddServerForm.tsxsrc/modules/mcp/components/McpServerCard.tsxsrc/modules/mcp/components/McpToolsPanel.tsxsrc/pages/DashboardPage.tsx
maximedogawa
deleted the
13-bug-mcp-tools-are-not-loaded-on-native-build
branch
This was referenced Apr 17, 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.
AddServerForm,McpServerCard, andMcpToolsPanelto use "tool" instead of "server" for consistency.Summary by CodeRabbit
New Features
Improvements