Fix execute_custom_tool bypassed when unity_instance is specified#724
Merged
Scriptwonder merged 5 commits intoCoplayDev:betafrom Feb 12, 2026
Conversation
Move execute_custom_tool handling outside the if/else block so it's executed regardless of whether unity_instance is provided. Previously, when unity_instance was specified, the code would take the if branch (match by hash/name) and return 404 if not found, but the execute_custom_tool block was only in the else branch, causing custom tools to fall through to PluginHub.send_command. Fixes CoplayDev#649 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideAdjusts CLI /api/command routing so execute_custom_tool is always handled via CustomToolService with proper session validation, regardless of whether a unity_instance is specified, while preserving Unity command routing for all other command types. Sequence diagram for updated CLI execute_custom_tool routingsequenceDiagram
actor CLI
participant ApiServer
participant SessionStore
participant CustomToolService
CLI->>ApiServer: POST /api/command
activate ApiServer
ApiServer->>SessionStore: resolve session_id, session_details (optional unity_instance)
SessionStore-->>ApiServer: session_id or none, session_details
alt unity_instance specified but not found
ApiServer-->>CLI: 404 JSONResponse Unity instance not found
else session resolved or first session selected
alt command_type is execute_custom_tool
ApiServer->>ApiServer: validate session_id and session_details
alt invalid session
ApiServer-->>CLI: 503 JSONResponse No valid Unity session
else valid session
ApiServer->>ApiServer: parse tool_name and tool_params from params
alt missing tool_name
ApiServer-->>CLI: 400 JSONResponse Missing tool_name
else invalid tool_params type
ApiServer-->>CLI: 400 JSONResponse Tool parameters must be dict
else valid tool parameters
ApiServer->>ApiServer: compute unity_instance_hint from session_details.hash or unity_instance
ApiServer->>ApiServer: resolve_project_id_for_unity_instance
alt project_id not found
ApiServer-->>CLI: 400 JSONResponse Could not resolve project id
else project_id found
ApiServer->>CustomToolService: execute_tool(project_id, tool_name, unity_instance_hint, tool_params)
CustomToolService-->>ApiServer: result
ApiServer-->>CLI: JSONResponse result.model_dump()
end
end
end
else command_type is not execute_custom_tool
ApiServer->>ApiServer: fall through to Unity routing
ApiServer-->>CLI: (handled by PluginHub in separate path)
end
end
deactivate ApiServer
Flow diagram for CLI command routing with execute_custom_tool handlingflowchart TD
A_start[Start cli_command_route] --> B_getParams[Read command_type, params, unity_instance]
B_getParams --> C_hasUnityInstance{unity_instance specified?}
C_hasUnityInstance -->|yes| D_findSession[Lookup session by unity_instance]
C_hasUnityInstance -->|no| E_skipLookup[Skip direct lookup]
D_findSession --> F_checkFound{session_id found?}
F_checkFound -->|no| G_404[Return 404 Unity instance not found]
F_checkFound -->|yes| H_haveSession[Have session_id and session_details]
E_skipLookup --> I_hasSessionFromElse{session_id already set?}
I_hasSessionFromElse -->|yes| H_haveSession
I_hasSessionFromElse -->|no| J_useFirst[Set session_id, session_details to first available session]
J_useFirst --> H_haveSession
H_haveSession --> K_commandType{command_type == execute_custom_tool?}
K_commandType -->|yes| L_validateSession{session_id and session_details valid?}
L_validateSession -->|no| M_503[Return 503 No valid Unity session]
L_validateSession -->|yes| N_parseParams[Extract tool_name, tool_params from params]
N_parseParams --> O_hasToolName{tool_name present?}
O_hasToolName -->|no| P_400_name[Return 400 Missing tool_name]
O_hasToolName -->|yes| Q_paramsType{tool_params is dict?}
Q_paramsType -->|no| R_400_params[Return 400 Tool parameters must be dict]
Q_paramsType -->|yes| S_computeHint[Set unity_instance_hint from session_details.hash or unity_instance]
S_computeHint --> T_resolveProject[resolve_project_id_for_unity_instance]
T_resolveProject --> U_hasProject{project_id found?}
U_hasProject -->|no| V_400_project[Return 400 Could not resolve project id]
U_hasProject -->|yes| W_executeTool[CustomToolService.execute_tool]
W_executeTool --> X_returnTool[Return JSONResponse result.model_dump]
K_commandType -->|no| Y_unityRouting[Send command to Unity via PluginHub.send_command]
Y_unityRouting --> Z_returnUnity[Return Unity command result]
G_404 --> AA_end[End]
M_503 --> AA_end
P_400_name --> AA_end
R_400_params --> AA_end
V_400_project --> AA_end
X_returnTool --> AA_end
Z_returnUnity --> AA_end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughMoves and expands execute_custom_tool handling in the Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Client
participant API_Server as Server(/api/command)
participant CTS as CustomToolService
participant PH as PluginHub
participant Unity
end
Client->>API_Server: POST /api/command { command_type, unity_instance?, params, ... }
API_Server->>API_Server: validate session / unity_instance
alt command_type == "execute_custom_tool"
API_Server->>CTS: resolve project_id (unity_instance_hint) and execute_tool(tool_name, params)
CTS-->>API_Server: model_dump (result)
API_Server-->>Client: 200 { model_dump }
else other command_type
API_Server->>PH: PluginHub.send_command(command)
PH-->>Unity: deliver command
PH-->>API_Server: command result
API_Server-->>Client: command result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `Server/src/main.py:398-407` </location>
<code_context>
+ )
+
+ # If no specific unity_instance requested, use first available session
+ if not session_id:
session_id = next(iter(sessions.sessions.keys()))
session_details = sessions.sessions.get(session_id)
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against empty `sessions.sessions` before calling `next(iter(...))` to avoid `StopIteration` at runtime.
In the no-`unity_instance` path, `session_id = next(iter(sessions.sessions.keys()))` will raise `StopIteration` when there are no active sessions. Since `execute_custom_tool` now guards on missing `session_id`/`session_details`, it would be better to mirror that behavior here by checking that `sessions.sessions` is non-empty before calling `next(...)`, and returning a 503 (or similar) if no sessions are available, instead of letting an exception bubble up.
</issue_to_address>
### Comment 2
<location> `Server/src/main.py:408-411` </location>
<code_context>
session_id = next(iter(sessions.sessions.keys()))
session_details = sessions.sessions.get(session_id)
- if command_type == "execute_custom_tool":
- tool_name = None
+ # Custom tool execution - must be checked BEFORE the final PluginHub.send_command call
+ # This applies to both cases: with or without explicit unity_instance
+ if command_type == "execute_custom_tool":
+ if not session_id or not session_details:
+ return JSONResponse(
+ {"success": False,
</code_context>
<issue_to_address>
**suggestion:** Align the `execute_custom_tool` session validation with how `session_id` is derived so the guard can actually be effective.
The new guard is unlikely to run as intended because `session_id` is always set via `next(iter(sessions.sessions.keys()))` first: if that succeeds, `session_id` is truthy and `session_details` will almost never be `None`; if it fails (empty `sessions.sessions`), we never reach the guard because `next(...)` raises.
To make the 503 path reliably reachable:
- Move the default-session selection (`if not session_id: ...`) into the `execute_custom_tool` block and guard against an empty `sessions.sessions`, or
- Extract a helper that safely returns `(session_id, session_details)` or `None`, and let `execute_custom_tool` handle the `None` case consistently.
```suggestion
# If no specific unity_instance requested, use first available session (if any)
if not session_id:
try:
session_id = next(iter(sessions.sessions.keys()))
except StopIteration:
session_id = None
session_details = None
else:
session_details = sessions.sessions.get(session_id)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
When no active sessions exist, next(iter(sessions.sessions.keys())) raises StopIteration. Catch this exception and set session_id and session_details to None, allowing execute_custom_tool guard to work as intended. Suggested by Sourcery review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Per Sourcery review: moving the default session selection inside execute_custom_tool ensures the empty sessions guard works correctly. If sessions becomes empty between the initial check and this code, the StopIteration will now properly trigger the session validation guard and return a 503 error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a routing bug in the local CLI /api/command endpoint where command_type: "execute_custom_tool" could be skipped when unity_instance was provided, ensuring custom tool execution is handled consistently regardless of instance selection.
Changes:
- Moves
execute_custom_toolhandling so it runs before the fallbackPluginHub.send_commandpath. - Ensures
unity_instanceresolution/validation happens in a way that supports both explicit and implicit (first available) session selection. - Adds an explicit “valid session required” check before executing a custom tool.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🤖 Fix all issues with AI agents
In `@Server/src/main.py`:
- Around line 410-472: The default session selection logic (the block that sets
session_id = next(iter(sessions.sessions.keys())) and session_details) currently
lives inside the execute_custom_tool branch causing non-custom commands to get a
None session_id; move that default-selection block so it runs before the
execute_custom_tool check (i.e., apply to all command types) so
PluginHub.send_command receives a valid session_id; ensure you keep the
StopIteration fallback (setting session_id/session_details to None) and the
subsequent existing checks that return 503/400 unchanged, and update any
references to unity_instance_hint/project resolution
(resolve_project_id_for_unity_instance and CustomToolService.execute_tool) to
run only inside the execute_custom_tool branch as before.
🧹 Nitpick comments (1)
Server/src/main.py (1)
414-420:StopIterationhandler is unreachable.At this point,
sessions.sessionsis guaranteed non-empty (line 378 returns 503 otherwise), and we only enter this block whenunity_instancewas not provided (the 404 at line 399 handles the "provided but not found" case). Sonext(iter(sessions.sessions.keys()))will always succeed. Thetry/except StopIterationis dead code.Not harmful, but if you adopt the fix suggested above (moving default session selection out of the custom-tool block), this becomes moot.
Contributor
Author
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
5 tasks
msanatan
pushed a commit
to msanatan/unity-mcp
that referenced
this pull request
Feb 25, 2026
…playDev#724) * Fix execute_custom_tool bypassed when unity_instance is specified Move execute_custom_tool handling outside the if/else block so it's executed regardless of whether unity_instance is provided. Previously, when unity_instance was specified, the code would take the if branch (match by hash/name) and return 404 if not found, but the execute_custom_tool block was only in the else branch, causing custom tools to fall through to PluginHub.send_command. Fixes CoplayDev#649 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Handle empty sessions gracefully when selecting default session When no active sessions exist, next(iter(sessions.sessions.keys())) raises StopIteration. Catch this exception and set session_id and session_details to None, allowing execute_custom_tool guard to work as intended. Suggested by Sourcery review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Move default session selection into execute_custom_tool block Per Sourcery review: moving the default session selection inside execute_custom_tool ensures the empty sessions guard works correctly. If sessions becomes empty between the initial check and this code, the StopIteration will now properly trigger the session validation guard and return a 503 error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: remove unintended uv.lock changes from PR * 优化 execute_custom_tool 逻辑以优先使用可用会话 --------- Co-authored-by: whatevertogo <whatevertogo@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
msanatan
pushed a commit
to msanatan/unity-mcp
that referenced
this pull request
Feb 25, 2026
…playDev#724) * Fix execute_custom_tool bypassed when unity_instance is specified Move execute_custom_tool handling outside the if/else block so it's executed regardless of whether unity_instance is provided. Previously, when unity_instance was specified, the code would take the if branch (match by hash/name) and return 404 if not found, but the execute_custom_tool block was only in the else branch, causing custom tools to fall through to PluginHub.send_command. Fixes CoplayDev#649 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Handle empty sessions gracefully when selecting default session When no active sessions exist, next(iter(sessions.sessions.keys())) raises StopIteration. Catch this exception and set session_id and session_details to None, allowing execute_custom_tool guard to work as intended. Suggested by Sourcery review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Move default session selection into execute_custom_tool block Per Sourcery review: moving the default session selection inside execute_custom_tool ensures the empty sessions guard works correctly. If sessions becomes empty between the initial check and this code, the StopIteration will now properly trigger the session validation guard and return a 503 error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: remove unintended uv.lock changes from PR * 优化 execute_custom_tool 逻辑以优先使用可用会话 --------- Co-authored-by: whatevertogo <whatevertogo@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
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
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
Fix bug where
execute_custom_toolwas bypassed when bothunity_instanceandcommand_type: "execute_custom_tool"were specified in the CLI/api/commandREST endpoint.Type of Change
Changes Made
execute_custom_toolhandling outside of theif/elseblock that checks forunity_instanceexecute_custom_toolto ensure a valid session existsunity_instanceis specified, the first available session is usedRoot Cause: The original code had
execute_custom_toolhandling only in theelsebranch (whenunity_instanceis not specified). Whenunity_instancewas provided, the code would find the session but then fall through toPluginHub.send_command, bypassing custom tool resolution.Testing/Screenshots/Recordings
unity_instancespecified +execute_custom_tool→ now executes custom toolunity_instancenot specified +execute_custom_tool→ still worksunity_instancespecified + other commands → still routes to Unityunity_instancenot specified + other commands → still routes to UnityDocumentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdRelated Issues
Fixes #649
Additional Notes
This is a pre-existing logic issue that was identified during code review of PR #644 by CodeRabbit.
Summary by Sourcery
Ensure CLI custom tool execution is correctly routed through the custom tool handler before falling back to Unity command dispatch.
Bug Fixes:
Enhancements:
Summary by CodeRabbit
Bug Fixes
Refactor