fix(mcp): switch tool names from underscore to hyphen separator#237
fix(mcp): switch tool names from underscore to hyphen separator#237
Conversation
🦋 Changeset detectedLatest commit: 25d9652 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the naming convention for MCP tools, transitioning from underscore-separated names to hyphen-separated names. This modification enhances the robustness of tool name parsing by eliminating conflicts with Google API identifiers that often contain underscores. Additionally, it corrects an issue where tool names were incorrectly prefixed with Discovery document names instead of their configured service aliases, leading to a more consistent and predictable naming scheme. The update ensures better maintainability and clarity for tool identification within the system. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
googleworkspace-bot
added
the
cla: yes
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly switches the MCP tool name separator from underscores to hyphens to resolve parsing ambiguities. It also fixes an issue where discovery document names were used instead of configured service aliases for tool name prefixes. The changes are consistent across tool definitions, parsing logic, and tests. However, I've found a critical issue in the new tool name parsing logic that fails to handle service aliases containing hyphens (e.g., admin-reports), which undermines one of the goals of this change. Please see my detailed comment.
googleworkspace-bot
added
the
gemini: reviewed
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #237 +/- ##
==========================================
- Coverage 57.69% 57.67% -0.03%
==========================================
Files 38 38
Lines 14328 14336 +8
==========================================
+ Hits 8267 8268 +1
- Misses 6061 6068 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Addresses review comment: naive split on '-' broke hyphenated aliases like admin-reports. Now finds the enabled service that is a prefix of the tool name, then splits only the remainder for resource/method.
googleworkspace-bot
removed
the
gemini: reviewed
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly refactors MCP tool names to use hyphens instead of underscores, which resolves ambiguity with service and resource names. The change to use the service alias instead of the discovery document name for the tool prefix is also a good improvement.
I've found one high-severity issue in the tool name parsing logic. When multiple service aliases are configured and one is a prefix of another (e.g., admin and admin-reports), the current logic may pick the wrong service. My review includes a suggestion to fix this by finding the longest matching prefix.
| let (svc_alias, rest) = config | ||
| .services | ||
| .iter() | ||
| .find_map(|s| { | ||
| tool_name | ||
| ))); | ||
| } | ||
|
|
||
| let svc_alias = parts[0]; | ||
| .strip_prefix(s.as_str()) | ||
| .and_then(|r| r.strip_prefix('-')) | ||
| .map(|remainder| (s.as_str(), remainder)) | ||
| }) | ||
| .ok_or_else(|| { | ||
| GwsError::Validation(format!( | ||
| "Could not determine service from tool name '{}'. No enabled service is a prefix.", | ||
| tool_name | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
The current implementation uses find_map to identify the service from the tool name. This can lead to incorrect parsing if one enabled service alias is a prefix of another (e.g., admin and admin-reports). find_map will return the first match, which may not be the longest (most specific) one.
For example, with services admin and admin-reports enabled, a tool call for admin-reports-activities-list would be incorrectly attributed to the admin service.
To fix this, you should find the longest matching prefix instead of the first one. Using filter_map followed by max_by_key(|(s, _)| s.len()) will ensure the most specific service alias is always chosen.
| let (svc_alias, rest) = config | |
| .services | |
| .iter() | |
| .find_map(|s| { | |
| tool_name | |
| ))); | |
| } | |
| let svc_alias = parts[0]; | |
| .strip_prefix(s.as_str()) | |
| .and_then(|r| r.strip_prefix('-')) | |
| .map(|remainder| (s.as_str(), remainder)) | |
| }) | |
| .ok_or_else(|| { | |
| GwsError::Validation(format!( | |
| "Could not determine service from tool name '{}'. No enabled service is a prefix.", | |
| tool_name | |
| )) | |
| })?; | |
| let (svc_alias, rest) = config | |
| .services | |
| .iter() | |
| .filter_map(|s| { | |
| tool_name | |
| .strip_prefix(s.as_str()) | |
| .and_then(|r| r.strip_prefix('-')) | |
| .map(|remainder| (s.as_str(), remainder)) | |
| }) | |
| .max_by_key(|(s, _)| s.len()) | |
| .ok_or_else(|| { | |
| GwsError::Validation(format!( | |
| "Could not determine service from tool name '{}'. No enabled service is a prefix.", | |
| tool_name | |
| )) | |
| })?; |
googleworkspace-bot
added
the
gemini: reviewed
Addresses review: find_map returns first match, but admin could match before admin-reports. Use filter_map + max_by_key to always pick the longest (most specific) service alias prefix.
googleworkspace-bot
removed
the
gemini: reviewed
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the ambiguity of using underscores in MCP tool names by switching to hyphens, which is a solid improvement for parsing reliability. The change to use service aliases instead of discovery document names is also a great fix. The implementation is largely well-executed. I've identified one high-severity issue concerning insufficient validation in the new tool name parsing logic, which could result in misleading error messages for users. A code suggestion is provided to make the validation more robust.
src/mcp_server.rs
Outdated
| if parts.is_empty() { | ||
| return Err(GwsError::Validation(format!( | ||
| "Service '{}' is not enabled in this MCP session", | ||
| svc_alias | ||
| "Invalid API tool name: {}", | ||
| tool_name | ||
| ))); | ||
| } |
There was a problem hiding this comment.
The validation for the tool name parts is insufficient. The previous logic checked for a minimum number of parts, but the new logic only checks if parts is empty. This check is ineffective as split on a string never produces an empty iterator, making it dead code. This can lead to misleading "Resource not found" errors for malformed tool names like drive-files (missing method) or drive- (missing resource and method), instead of a clearer "Invalid API tool name" error.
A more robust validation should ensure there's at least one resource and one method part, and that no part is empty (which would indicate double hyphens like drive--files-list).
| if parts.is_empty() { | |
| return Err(GwsError::Validation(format!( | |
| "Service '{}' is not enabled in this MCP session", | |
| svc_alias | |
| "Invalid API tool name: {}", | |
| tool_name | |
| ))); | |
| } | |
| if parts.len() < 2 || parts.iter().any(|p| p.is_empty()) { | |
| return Err(GwsError::Validation(format!( | |
| "Invalid API tool name: '{}'. Expected format: <service>-<resource...>-<method>", | |
| tool_name | |
| ))); | |
| } |
googleworkspace-bot
added
the
gemini: reviewed
Addresses review: parts.is_empty() was dead code since split never returns empty. Now checks parts.len() >= 2 (resource + method) and rejects empty segments from double hyphens.
googleworkspace-bot
removed
the
gemini: reviewed
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the MCP tool naming convention by replacing underscores with hyphens as separators. This change addresses a parsing ambiguity that occurred when service or resource names contained underscores. The new implementation for parsing tool names is more robust, correctly identifying the service by finding the longest matching enabled service alias as a prefix. This also handles service aliases that contain hyphens, such as admin-reports. Additionally, the code is updated to use the service alias for tool name generation instead of the discovery document's service name, which fixes a separate issue. The changes are applied consistently across tool name generation, parsing, and tests. The implementation appears correct and addresses the stated goals of the pull request.
googleworkspace-bot
added
the
gemini: reviewed
3 participants
Summary
Fixes #235, fixes #162
Switch MCP tool names from underscore (
_) to hyphen (-) separator:drive_files_listdrive-files-listgmail_users_messages_sendgmail-users-messages-sendadmin_reports_activities_listadmin-reports-activities-listgws_discovergws-discoverworkflow_standup_reportworkflow-standup-reportWhy hyphens?
Underscores were ambiguous because Google API service/resource names contain underscores (e.g.,
admin_reports). This madesplit("_")parsing unreliable for multi-word resources (#170).Hyphens are safe because Google API identifiers never contain hyphens, making
split("-")unambiguous. Hyphens are also MCP spec-compliant (the spec allows lowercase letters and hyphens).Also fixes
Uses the configured service alias (e.g.,
events) instead of the Discovery doc name (e.g.,workspaceevents) as the tool name prefix, fixing #162.Breaking change
MCP tool names have changed format. Well-behaved clients that discover tools via
tools/listpick up new names automatically.Test plan
cargo clippy -- -D warningsclean