Update for CLI #636
Merged
Update for CLI #636
Conversation
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
(1) Custom Tool fix (2) Include more tips and helps with CLI, including a CLI_EXAMPLE.md with @JohanHoltby's feedback!
Contributor
📝 WalkthroughWalkthroughThis PR adds custom tools listing and execution capabilities to both the CLI and server components. It introduces new CLI commands ( Changes
Sequence DiagramssequenceDiagram
participant User as User (CLI)
participant CLI as CLI Handler
participant Connection as Connection Utils
participant API as Server API
participant Service as Custom Tool Service
User->>CLI: unity-mcp tool list
CLI->>Connection: run_list_custom_tools(config)
Connection->>API: GET /api/custom-tools
API->>Service: get_custom_tools(project_id)
Service-->>API: tools list
API-->>Connection: {tools: [...], ...}
Connection-->>CLI: result dict
CLI->>CLI: format output (text/json)
CLI-->>User: Custom tools (N):<br/>- tool1<br/>- tool2
sequenceDiagram
participant User as User (CLI)
participant CLI as Editor Command
participant Suggestions as Suggestions Util
participant Connection as Connection Utils
participant API as Server API
participant Service as Custom Tool Service
User->>CLI: unity-mcp editor custom-tool "InvalidTool"
CLI->>API: execute tool "InvalidTool"
API-->>CLI: Error: tool not found
CLI->>Connection: run_list_custom_tools()
Connection->>API: GET /api/custom-tools
API->>Service: get_custom_tools(project_id)
Service-->>API: {tools: [...]}
API-->>Connection: result
Connection-->>CLI: tools list
CLI->>Suggestions: suggest_matches("InvalidTool", tool_names)
Suggestions-->>CLI: [closest_matches]
CLI->>Suggestions: format_suggestions(matches)
Suggestions-->>CLI: "Did you mean: ValidTool?"
CLI-->>User: Error + suggestion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
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 |
Contributor
Reviewer's GuideAdds CLI support for listing and executing custom tools against the active Unity project, improves instance targeting and error handling for custom tool execution, and updates CLI UX and documentation with examples and suggestion messages. Sequence diagram for CLI custom tool listingsequenceDiagram
actor Dev as CLI_user
participant CLI as unity_mcp_CLI
participant Conn as cli_utils_connection
participant HTTP as HTTP_server_main_py
participant CTS as CustomToolService
Dev->>CLI: unity-mcp tool list
CLI->>Conn: run_list_custom_tools(config)
Conn->>HTTP: GET /api/custom-tools?instance=cfg.unity_instance
HTTP->>HTTP: _normalize_instance_token(instance)
HTTP->>HTTP: PluginHub.get_sessions()
alt instance specified
HTTP->>HTTP: find matching session_details
alt no match
HTTP-->>Conn: 404 Unity instance not found
Conn-->>CLI: error JSON
CLI-->>Dev: formatted error
else match
HTTP->>HTTP: unity_instance_hint = session_details.hash
end
else no instance
HTTP->>HTTP: session_details = first session
HTTP->>HTTP: unity_instance_hint = session_details.hash
end
HTTP->>HTTP: project_id = resolve_project_id_for_unity_instance(unity_instance_hint)
alt project_id missing
HTTP-->>Conn: 400 could not resolve project id
Conn-->>CLI: error JSON
CLI-->>Dev: formatted error
else project_id resolved
HTTP->>CTS: list_registered_tools(project_id)
CTS-->>HTTP: tools list
HTTP-->>Conn: JSON {success, project_id, tools}
Conn-->>CLI: result dict
CLI-->>Dev: render tools list
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 2 issues, and left some high level feedback:
- Overriding
click.Group.resolve_commandglobally is a fairly invasive monkey patch that can affect any other click-based entry points; consider scoping this behavior to your CLI group (e.g., via subclassing or a custom group factory) rather than patching the library-level attribute. - The logic for resolving Unity sessions by
instance(hash vs project name) is now duplicated in both/api/commandand/api/custom-tools; extracting a shared helper to encapsulate this matching would reduce repetition and keep the routing logic easier to reason about. - The CLI custom tool listing currently has to handle multiple possible response shapes (
toolsvs nested underdata.tools); aligning the server response to a single, consistent structure would simplify the client-side parsing and make error handling more straightforward.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Overriding `click.Group.resolve_command` globally is a fairly invasive monkey patch that can affect any other click-based entry points; consider scoping this behavior to your CLI group (e.g., via subclassing or a custom group factory) rather than patching the library-level attribute.
- The logic for resolving Unity sessions by `instance` (hash vs project name) is now duplicated in both `/api/command` and `/api/custom-tools`; extracting a shared helper to encapsulate this matching would reduce repetition and keep the routing logic easier to reason about.
- The CLI custom tool listing currently has to handle multiple possible response shapes (`tools` vs nested under `data.tools`); aligning the server response to a single, consistent structure would simplify the client-side parsing and make error handling more straightforward.
## Individual Comments
### Comment 1
<location> `Server/src/main.py:331-337` </location>
<code_context>
"message": "MCP for Unity server is running"
})
+ def _normalize_instance_token(instance_token: str | None) -> tuple[str | None, str | None]:
+ if not instance_token:
+ return None, None
+ if "@" in instance_token:
+ name_part, _, hash_part = instance_token.partition("@")
+ return (name_part or None), (hash_part or None)
+ return None, instance_token
+
@mcp.custom_route("/api/command", methods=["POST"])
</code_context>
<issue_to_address>
**suggestion:** Consider trimming whitespace from the instance token before parsing name/hash parts.
If callers pass tokens with leading/trailing spaces (e.g. from shell/config), those spaces become part of the name/hash and can break matching. Consider normalizing with `instance_token = instance_token.strip()` and stripping `name_part`/`hash_part` before returning.
```suggestion
def _normalize_instance_token(instance_token: str | None) -> tuple[str | None, str | None]:
# Normalize whitespace on the raw token first
if instance_token is None:
return None, None
instance_token = instance_token.strip()
if not instance_token:
return None, None
if "@" in instance_token:
name_part, _, hash_part = instance_token.partition("@")
# Strip whitespace from name/hash parts and normalize empty strings to None
name_part = name_part.strip() or None
hash_part = hash_part.strip() or None
return name_part, hash_part
# No explicit name provided, treat the (trimmed) token as the hash
return None, instance_token
```
</issue_to_address>
### Comment 2
<location> `docs/guides/CLI_EXAMPLE.md:3` </location>
<code_context>
+## Unity MCP (CLI Mode)
+
+We use Unity MCP via **CLI commands** instead of MCP server connection. This avoids the reconnection issues that occur when Unity restarts.
+
+### Why CLI Instead of MCP Connection?
</code_context>
<issue_to_address>
**nitpick (typo):** Consider adding an article in "instead of MCP server connection" for smoother grammar.
For example: “We use Unity MCP via **CLI commands** instead of an MCP server connection. This avoids the reconnection issues that occur when Unity restarts.”
```suggestion
We use Unity MCP via **CLI commands** instead of an MCP server connection. This avoids the reconnection issues that occur when Unity restarts.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+331
to
+337
| def _normalize_instance_token(instance_token: str | None) -> tuple[str | None, str | None]: | ||
| if not instance_token: | ||
| return None, None | ||
| if "@" in instance_token: | ||
| name_part, _, hash_part = instance_token.partition("@") | ||
| return (name_part or None), (hash_part or None) | ||
| return None, instance_token |
Contributor
There was a problem hiding this comment.
suggestion: Consider trimming whitespace from the instance token before parsing name/hash parts.
If callers pass tokens with leading/trailing spaces (e.g. from shell/config), those spaces become part of the name/hash and can break matching. Consider normalizing with instance_token = instance_token.strip() and stripping name_part/hash_part before returning.
Suggested change
| def _normalize_instance_token(instance_token: str | None) -> tuple[str | None, str | None]: | |
| if not instance_token: | |
| return None, None | |
| if "@" in instance_token: | |
| name_part, _, hash_part = instance_token.partition("@") | |
| return (name_part or None), (hash_part or None) | |
| return None, instance_token | |
| def _normalize_instance_token(instance_token: str | None) -> tuple[str | None, str | None]: | |
| # Normalize whitespace on the raw token first | |
| if instance_token is None: | |
| return None, None | |
| instance_token = instance_token.strip() | |
| if not instance_token: | |
| return None, None | |
| if "@" in instance_token: | |
| name_part, _, hash_part = instance_token.partition("@") | |
| # Strip whitespace from name/hash parts and normalize empty strings to None | |
| name_part = name_part.strip() or None | |
| hash_part = hash_part.strip() or None | |
| return name_part, hash_part | |
| # No explicit name provided, treat the (trimmed) token as the hash | |
| return None, instance_token |
| @@ -0,0 +1,166 @@ | |||
| ## Unity MCP (CLI Mode) | |||
|
|
|||
| We use Unity MCP via **CLI commands** instead of MCP server connection. This avoids the reconnection issues that occur when Unity restarts. | |||
Contributor
There was a problem hiding this comment.
nitpick (typo): Consider adding an article in "instead of MCP server connection" for smoother grammar.
For example: “We use Unity MCP via CLI commands instead of an MCP server connection. This avoids the reconnection issues that occur when Unity restarts.”
Suggested change
| We use Unity MCP via **CLI commands** instead of MCP server connection. This avoids the reconnection issues that occur when Unity restarts. | |
| We use Unity MCP via **CLI commands** instead of an MCP server connection. This avoids the reconnection issues that occur when Unity restarts. |
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request adds CLI support for custom tool management and improves CLI usability with command suggestions and enhanced documentation.
Changes:
- Added new CLI commands
tool listandcustom_tool listto list registered custom tools for a Unity project - Implemented command suggestion feature for CLI using fuzzy matching to help users with typos
- Enhanced custom tool execution error handling with helpful suggestions when tools are not found
- Added comprehensive CLI usage examples and documentation
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/reference/CUSTOM_TOOLS.md | Added Step 3 section documenting CLI commands for listing and calling custom tools |
| docs/guides/CLI_USAGE.md | Updated to include custom tool listing commands in examples and command reference table |
| docs/guides/CLI_EXAMPLE.md | New comprehensive CLI usage guide with examples for all major command categories |
| Server/src/main.py | Added /api/custom-tools HTTP endpoint and execute_custom_tool handler in /api/command route with proper instance resolution |
| Server/src/cli/utils/suggestions.py | New utility module providing fuzzy matching for command suggestions using difflib |
| Server/src/cli/utils/connection.py | Added list_custom_tools and run_list_custom_tools functions to query custom tools via HTTP |
| Server/src/cli/main.py | Installed global command suggestion handler by monkey-patching Click's resolve_command method, registered new tool and custom_tool commands |
| Server/src/cli/commands/tool.py | New command module implementing tool list and custom_tool list commands |
| Server/src/cli/commands/editor.py | Enhanced custom-tool command with better error messages and suggestions when tool is not found |
| Server/src/cli/CLI_USAGE_GUIDE.md | Added new Custom Tools section documenting the listing and execution commands |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ] if isinstance(tools, list) else [] | ||
| matches = suggest_matches(tool_name, names) | ||
| suggestion = format_suggestions(matches) | ||
| if suggestion: |
There was a problem hiding this comment.
Potential IndexError when accessing matches[0]. While format_suggestions returning a non-None value implies matches has elements, it's safer to add an explicit check before accessing matches[0]. Consider checking if matches is non-empty before using it in the example message.
Suggested change
| if suggestion: | |
| if suggestion and matches: |
Comment on lines
+40
to
+61
| @click.group("tool") | ||
| def tool(): | ||
| """Tool management - list custom tools for the active Unity project.""" | ||
| pass | ||
|
|
||
|
|
||
| @tool.command("list") | ||
| def list_tools(): | ||
| """List custom tools registered for the active Unity project.""" | ||
| _list_custom_tools() | ||
|
|
||
|
|
||
| @click.group("custom_tool") | ||
| def custom_tool(): | ||
| """Alias for tool management (custom tools).""" | ||
| pass | ||
|
|
||
|
|
||
| @custom_tool.command("list") | ||
| def list_custom_tools(): | ||
| """List custom tools registered for the active Unity project.""" | ||
| _list_custom_tools() |
There was a problem hiding this comment.
The new CLI commands 'tool list' and 'custom_tool list' lack test coverage. The test file Server/tests/test_cli.py has comprehensive tests for other commands but no tests for these new commands. Consider adding tests similar to the existing patterns in the test file to ensure the new functionality works correctly.
|
|
||
| The MCP HTTP server still needs to be running for CLI to work. Here is an example to run the server manually on Mac: | ||
| ```bash | ||
| /opt/homebrew/bin/uvx --no-cache --refresh --from /XXX/unity-mcp/Server mcp-for-unity --transport http --http-url http://localhost:8080 |
There was a problem hiding this comment.
The path placeholder "/XXX/unity-mcp/Server" is unclear. Consider using a more standard placeholder format like "/path/to/unity-mcp/Server" or "path/to/unity-mcp/Server" to make it clearer that this should be replaced with the actual installation path.
Suggested change
| /opt/homebrew/bin/uvx --no-cache --refresh --from /XXX/unity-mcp/Server mcp-for-unity --transport http --http-url http://localhost:8080 | |
| /opt/homebrew/bin/uvx --no-cache --refresh --from /path/to/unity-mcp/Server mcp-for-unity --transport http --http-url http://localhost:8080 |
Comment on lines
+398
to
+399
| if tool_params is None: | ||
| tool_params = {} |
There was a problem hiding this comment.
The None check on line 398 is redundant because tool_params is already set to {} on line 391 if both 'parameters' and 'params' are falsy. The check on line 400 for isinstance(tool_params, dict) is sufficient to handle invalid parameter types.
Suggested change
| if tool_params is None: | |
| tool_params = {} |
| print_info(suggestion) | ||
| print_info( | ||
| f'Example: unity-mcp editor custom-tool "{matches[0]}"') | ||
| except UnityConnectionError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
Suggested change
| except UnityConnectionError: | |
| except UnityConnectionError: | |
| # Ignore failures when fetching optional custom tool suggestions. |
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.
Description
Updated CLI to include command for tool/custom_tool, and add example usage docs
Type of Change
Changes Made
(1) Custom Tool fix
(2) Include more tips and helps with CLI, including a CLI_EXAMPLE.md with @JohanHoltby's feedback!
Testing/Screenshots/Recordings
Documentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdRelated Issues
Additional Notes
Summary by Sourcery
Add CLI support for listing and executing project-scoped custom tools with improved instance resolution and user guidance.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Summary by CodeRabbit
Release Notes
New Features
unity-mcp tool listandunity-mcp custom_tool listcommands to list available custom tools for the active project.unity-mcp editor custom-toolcommand to execute custom tools with optional parameters.Documentation
✏️ Tip: You can customize this high-level summary in your review settings.