fix: strip MCP namespace prefix from tool names in batch_execute#810
Open
dsarno wants to merge 2 commits intoCoplayDev:betafrom
Open
fix: strip MCP namespace prefix from tool names in batch_execute#810dsarno wants to merge 2 commits intoCoplayDev:betafrom
dsarno wants to merge 2 commits intoCoplayDev:betafrom
Conversation
Clients may send prefixed tool names inside batch_execute commands (e.g. "UnityMCP:manage_gameobject" or "mcp__UnityMCP__manage_gameobject") which caused "Unknown or unsupported command type" errors since CommandRegistry registers handlers under short names only. Strips prefixes in both Python (server-side, early) and C# (Unity-side, defense-in-depth) to handle both colon and double-underscore formats. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideNormalizes MCP tool names before dispatch in both the Unity C# batch executor and the Python server batch execution path by stripping configurable MCP namespace prefixes, and adds unit tests for the new behavior. Sequence diagram for MCP tool name normalization in batch_executesequenceDiagram
actor Client
participant PythonServer as Python_batch_execute
participant UnityEditor as CSharp_BatchExecute
participant CommandRegistry
Client->>PythonServer: batch_execute(commands)
loop For each command
PythonServer->>PythonServer: strip_mcp_prefix(tool_name)
PythonServer->>UnityEditor: HandleCommand(params with tool_name)
UnityEditor->>UnityEditor: StripMcpPrefix(tool_name)
UnityEditor->>CommandRegistry: Dispatch(tool_name, params)
CommandRegistry-->>UnityEditor: result
UnityEditor-->>PythonServer: result
end
PythonServer-->>Client: aggregated results
Class diagram for updated BatchExecute prefix stripping logicclassDiagram
class BatchExecute {
+Task~object~ HandleCommand(JObject params)
-static bool DetermineCallSucceeded(object result)
-static JObject NormalizeParameterKeys(JObject source)
-static string StripMcpPrefix(string toolName)
}
class BatchExecutePy {
+strip_mcp_prefix(str tool_name) str
+batch_execute(Context ctx, list commands) dict
}
BatchExecutePy ..> BatchExecute : sends commands to
BatchExecute ..> BatchExecute : uses StripMcpPrefix
BatchExecutePy ..> BatchExecutePy : uses strip_mcp_prefix
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughAdds a helper to normalize MCP-prefixed tool names (supports "mcp__Server__tool" and "Server:tool") and integrates it into batch execution flows in both C# and Python; tests added in both projects to validate various formats and edge cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The C# and Python
StripMcpPrefix/strip_mcp_prefiximplementations differ for edge cases like a bare colon (":"): C# intentionally passes it through unchanged while Python returns an empty string, so it would be good to align the semantics (and add a corresponding Python test) to avoid subtle inconsistencies between the two paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The C# and Python `StripMcpPrefix`/`strip_mcp_prefix` implementations differ for edge cases like a bare colon (`":"`): C# intentionally passes it through unchanged while Python returns an empty string, so it would be good to align the semantics (and add a corresponding Python test) to avoid subtle inconsistencies between the two paths.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Tools/BatchExecute.cs:227-230` </location>
<code_context>
+ return toolName;
+
+ // Double-underscore format: "mcp__ServerName__tool_name"
+ if (toolName.StartsWith("mcp__"))
+ {
+ int secondSep = toolName.IndexOf("__", 5, StringComparison.Ordinal);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Specify StringComparison for StartsWith to match the IndexOf usage and avoid culture-dependent behavior.
Since the subsequent `IndexOf` uses `StringComparison.Ordinal`, use the same on `StartsWith` for consistency and to avoid culture-dependent behavior, e.g. `toolName.StartsWith("mcp__", StringComparison.Ordinal)`.
```suggestion
// Double-underscore format: "mcp__ServerName__tool_name"
if (toolName.StartsWith("mcp__", StringComparison.Ordinal))
{
int secondSep = toolName.IndexOf("__", 5, StringComparison.Ordinal);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Server/tests/test_batch_strip_prefix.py (1)
7-25: Consider adding edge-case tests to match C# suite coverage.The C#
BatchExecuteStripPrefixTestscoversnull/empty, colon-only (":"), and multi-colon inputs. Python's suite omits these. While all three pass through as invalid tool names in practice, covering them would document the intended behaviour (and would have caught the":"divergence noted above).➕ Suggested additional test cases
# Tool name itself contains underscores — should be preserved ("mcp__UnityMCP__manage_scriptable_object", "manage_scriptable_object"), + # Edge cases + ("", ""), + # Colon-only: no valid suffix — passes through unchanged (will fail at CommandRegistry) + (":", ":"), + # Trailing colon: no valid suffix — passes through unchanged + ("Server:", "Server:"), + # Multiple colons: last segment used + ("a:b:manage_gameobject", "manage_gameobject"), ], )Note: the expected values for
":"and"Server:"assume the guard fix proposed inbatch_execute.pyis applied; without it the current implementation returns""for both.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/tests/test_batch_strip_prefix.py` around lines 7 - 25, Add edge-case params to test_strip_mcp_prefix to cover null/None, empty, colon-only, and multi-colon inputs: update the param list for strip_mcp_prefix to include (None, None), ("", ""), (":", ":"), ("Server:", "Server:"), and a multi-colon case like ("a::b", "a::b"); update the test signature to accept Optional[str] (e.g., input_name: Optional[str]) so pytest can pass None and ensure strip_mcp_prefix preserves these inputs as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Server/src/services/tools/batch_execute.py`:
- Around line 36-41: strip_mcp_prefix can return an empty string for inputs with
trailing separators (e.g. "mcp__Server__" or "Server:"), unlike the C#
StripMcpPrefix which returns the original input when no characters follow the
separator; update strip_mcp_prefix to validate that the second "__" found (when
handling "mcp__") has at least one character after it and that the ":" when
using rsplit has at least one character after it, and if not return the original
tool_name unchanged (i.e., before the split/rsplit), so the Python behavior
matches the C# guards and the empty result is not propagated.
---
Nitpick comments:
In `@Server/tests/test_batch_strip_prefix.py`:
- Around line 7-25: Add edge-case params to test_strip_mcp_prefix to cover
null/None, empty, colon-only, and multi-colon inputs: update the param list for
strip_mcp_prefix to include (None, None), ("", ""), (":", ":"), ("Server:",
"Server:"), and a multi-colon case like ("a::b", "a::b"); update the test
signature to accept Optional[str] (e.g., input_name: Optional[str]) so pytest
can pass None and ensure strip_mcp_prefix preserves these inputs as-is.
- Add StringComparison.Ordinal to StartsWith in C# (Sourcery) - Guard against empty suffix from trailing separators in Python to match C# semantics (CodeRabbit) - Add edge-case tests for ":", "Server:", "mcp__Server__", "" inputs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Server/tests/test_batch_strip_prefix.py (1)
7-33: Good coverage including edge cases; consider one more scenario.The parametrized cases are thorough, especially the trailing-separator and underscore-preservation tests. One potentially useful addition: a name with
mcp__prefix but no second__separator (e.g.,"mcp__toolname") — the function should return it unchanged since the"__" in tool_name[5:]guard fails.Suggested addition
# Multiple colons: last segment used ("a:b:manage_gameobject", "manage_gameobject"), + # mcp__ prefix without a second separator passes through unchanged + ("mcp__toolname", "mcp__toolname"), ], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/tests/test_batch_strip_prefix.py` around lines 7 - 33, Add a parametrized case to test_strip_mcp_prefix that covers an input beginning with "mcp__" but lacking a second "__" separator (e.g., "mcp__toolname") and assert strip_mcp_prefix returns it unchanged; locate the parametrized list in Server/tests/test_batch_strip_prefix.py and add this tuple alongside the other cases to ensure strip_mcp_prefix's guard for tool_name[5:] is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Server/tests/test_batch_strip_prefix.py`:
- Around line 7-33: Add a parametrized case to test_strip_mcp_prefix that covers
an input beginning with "mcp__" but lacking a second "__" separator (e.g.,
"mcp__toolname") and assert strip_mcp_prefix returns it unchanged; locate the
parametrized list in Server/tests/test_batch_strip_prefix.py and add this tuple
alongside the other cases to ensure strip_mcp_prefix's guard for tool_name[5:]
is exercised.
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
batch_executecommands (e.g.UnityMCP:manage_gameobjectormcp__UnityMCP__manage_gameobject) caused"Unknown or unsupported command type"errors becauseCommandRegistryregisters handlers under short names onlystrip_mcp_prefix()in Python andStripMcpPrefix()in C# to normalize both colon and double-underscore prefix formats before command dispatchTest plan
strip_mcp_prefix()— all passStripMcpPrefix()— all pass via Unity Test Runnerbatch_executewith prefixed tool names from Claude Code🤖 Generated with Claude Code
Summary by Sourcery
Normalize MCP tool names in batch_execute on both the Unity client and Python server to handle namespaced tool identifiers transparently.
Bug Fixes:
Tests:
Summary by CodeRabbit
New Features
Tests