Feature/session based instance routing #95
Conversation
- Fix partial framed response handling in port discovery Add _recv_exact() helper to ensure complete frame reading Prevents healthy Unity instances from being misidentified as offline - Remove unused default_conn variables in server.py (2 files) Fixes Ruff F841 lint error that would block CI/CD - Preserve sync/async nature of resources in wrapper Check if original function is coroutine before wrapping Prevents 'dict object is not awaitable' runtime errors - Fix reconnection to preserve instance_id Add instance_id tracking to UnityConnection dataclass Reconnection now targets the same Unity instance instead of any available one Prevents operations from being applied to wrong project - Add instance logging to manage_asset for debugging Helps troubleshoot multi-instance scenarios 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
Address 3 CodeRabbit review comments:
1. Critical: Guard reconnection fallback to prevent wrong instance routing
- When instance_id is set but rediscovery fails, now raises ConnectionError
- Added 'from e' to preserve exception chain for better debugging
- Prevents silently connecting to different Unity instance
- Ensures multi-instance routing integrity
2. Minor: Guard __annotations__ access in resource registration
- Use getattr(func, '__annotations__', {}) instead of direct access
- Prevents AttributeError for functions without type hints
3. Minor: Remove unused get_type_hints import
- Clean up unused import in resources/__init__.py
All changes applied to both Server/ and MCPForUnity/ directories.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
- Fix sorting logic for instances without heartbeat data: use epoch timestamp instead of current time to properly deprioritize instances with None last_heartbeat - Use logger.exception() instead of logger.error() in disconnect_all() to include stack traces for better debugging 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…ting
Replaces module-level session_state.py with UnityInstanceMiddleware class
that follows FastMCP best practices. Middleware intercepts all tool calls
via on_call_tool hook and injects active Unity instance into request state.
Key changes:
- Add UnityInstanceMiddleware class with on_call_tool hook
- Tools now use ctx.get_state("unity_instance") instead of direct session_state calls
- Remove unity_instance parameter from all tool schemas to prevent LLM hallucination
- Convert list_unity_instances tool to unity_instances resource (read-only data)
- Update error messages to reference unity://instances resource
- Add set_state/get_state methods to DummyContext test helper
- All 67 tests passing (55 passed, 5 skipped, 7 xpassed)
Architecture benefits:
- Centralized session management in middleware
- Standard FastMCP patterns (middleware + request state)
- Cleaner separation of concerns
- Prevents AI hallucination of invalid instance IDs
Convert MCP resources from URI templates with query parameters to static
resources to fix discoverability in MCP clients like Claude Code.
Changes:
- Remove {?force_refresh} from unity://instances
- Remove {?unity_instance} from mcpforunity://menu-items
- Remove {?unity_instance} from mcpforunity://tests
- Keep {mode} path parameter in mcpforunity://tests/{mode} (legitimate)
Root cause: Query parameters {?param} trigger ResourceTemplate registration,
which are listed via resources/templates/list instead of resources/list.
Claude Code's ListMcpResourcesTool only queries resources/list, making
templates undiscoverable.
Solution: Remove optional query parameters from URIs. Instance routing is
handled by middleware/context, and force_refresh was cache control that
doesn't belong in resource identity.
Impact: Resources now discoverable via standard resources/list endpoint and
work with all MCP clients including Claude Code and Cursor.
Requires FastMCP >=2.13.0 for proper RFC 6570 query parameter support.
Material Property Improvements (ManageAsset.cs): - Add GetMainColorPropertyName() helper that auto-detects shader color properties - Tries _BaseColor (URP), _Color (Standard), _MainColor, _Tint, _TintColor - Update both named and array color property handling to use auto-detection - Add warning messages when color properties don't exist on materials - Split HasProperty check from SetColor to enable error reporting This fixes the issue where simple color array format [r,g,b,a] defaulted to _Color property, causing silent failures with URP Lit shader which uses _BaseColor. Server Resource Sync: - Sync Server/resources with MCPForUnity/UnityMcpServer~/src/resources - Remove query parameters from resource URIs for discoverability - Use session-based instance routing via get_unity_instance_from_context()
…text PROBLEM: Instance routing was failing - scripts went to wrong Unity instances. Script1 (intended: ramble) -> went to UnityMCPTests ❌ Script2 (intended: UnityMCPTests) -> went to ramble ❌ ROOT CAUSE: Two incompatible approaches for accessing active instance: 1. Middleware: ctx.set_state() / ctx.get_state() - used by most tools 2. Legacy: ctx.request_context.meta - used by script tools Script tools were reading from wrong location, middleware had no effect. FIX: 1. Updated get_unity_instance_from_context() to read from ctx.get_state() 2. Removed legacy request_context.meta code path (98 lines removed) 3. Single source of truth: middleware state only TESTING: - Added comprehensive test suite (21 tests) covering all scenarios - Tests middleware state management, session isolation, race conditions - Tests reproduce exact 4-script failure scenario - All 88 tests pass (76 passed + 5 skipped + 7 xpassed) - Verified fix with live 4-script test: 100% success rate Files changed: - Server/tools/__init__.py: Simplified from 75 lines to 15 lines - MCPForUnity/UnityMcpServer~/src/tools/__init__.py: Same simplification - tests/test_instance_routing_comprehensive.py: New comprehensive test suite
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds multi-Unity-instance support: connection pool and per-session instance middleware, instance-aware port discovery/probing and models, context-based routing for tools/resources, instance listing and selection tools, editor asset fixes, dependency bumps, and tests for instance routing and middleware injection. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP_Server
participant Middleware as UnityInstanceMiddleware
participant Tool
participant Pool as UnityConnectionPool
participant Editor as Unity Editor
Client->>MCP_Server: Invoke resource/tool (with Context)
MCP_Server->>Middleware: on_call_tool(ctx)
Middleware-->>MCP_Server: inject `unity_instance` into ctx.state (if set)
MCP_Server->>Tool: call tool(ctx)
Tool->>Tool: resolve unity_instance from ctx
Tool->>Pool: send_with_unity_instance(fn, unity_instance, ...)
Pool->>Pool: resolve instance -> connection
Pool->>Editor: dispatch command
Editor-->>Pool: response
Pool-->>Tool: response
Tool-->>Client: return result
sequenceDiagram
participant Operator
participant CLI
participant Server
participant Discovery as PortDiscovery
participant Pool as UnityConnectionPool
participant Editors as Unity Instances
Operator->>CLI: start server (--default-instance?)
CLI->>Server: pass default instance
Server->>Discovery: discover_all_unity_instances()
Discovery->>Editors: probe ports (framed ping/pong)
Editors-->>Discovery: valid responses
Discovery-->>Server: list of UnityInstanceInfo
Server->>Pool: initialize pool with discovered instances
Server->>Server: register UnityInstanceMiddleware
Server-->>Operator: server ready
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR implements session-based instance routing for managing multiple Unity Editor instances simultaneously. Previously, the MCP server could only target one Unity instance at a time globally. Now, each client session can independently select and route commands to different Unity instances.
Key Changes
- New
UnityInstanceMiddleware: Stores active Unity instance per session ID, injecting it into request state for all tool calls - New
set_active_instancetool: Allows users to select which Unity instance their session should target - New
unity_instancesresource: Provides discoverable list of all running Unity instances - Updated all tools: Extract
unity_instancefrom context state and pass to connection layer for routing - Enhanced C# bridge: Improved port conflict handling, added
project_nameandunity_versionto status files - Comprehensive test coverage: 21 tests validating middleware state management, session isolation, and routing correctness
Architecture
The routing flow follows a clean separation of concerns:
- User calls
set_active_instance→ middleware stores instance per session - Middleware intercepts every tool call → injects instance into
ctx.set_state("unity_instance", ...) - Tools extract instance via
get_unity_instance_from_context(ctx)orctx.get_state() - Connection pool resolves instance identifier to TCP connection
Issues Found
- Inconsistent instance extraction: Some tools use
ctx.get_state()directly while others use helper functionget_unity_instance_from_context()(style issue, both work) - Dead import in unused decorator:
with_unity_instancedecorator imports non-existentsession_statemodule, will fail if ever used (syntax issue)
Confidence Score: 4/5
- Safe to merge with minor code cleanup recommended
- Strong implementation with comprehensive tests and clear architecture. Score of 4 (not 5) due to: (1) unused decorator contains broken import that would fail at runtime if called, and (2) inconsistent patterns across tools for instance extraction. Core routing logic is sound and well-tested.
- Server/tools/init.py (fix broken import in unused decorator), Server/tools/manage_gameobject.py and Server/tools/manage_asset.py (consider standardizing on helper function)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| Server/unity_instance_middleware.py | 5/5 | New middleware implementing session-based instance routing with thread-safe state management |
| Server/server.py | 5/5 | Integrated UnityInstanceMiddleware into server startup and registered with FastMCP |
| Server/unity_connection.py | 4/5 | Expanded connection pool with instance resolution logic, added default instance support |
| Server/tools/init.py | 5/5 | Added helper functions for extracting and forwarding unity_instance from context state |
| tests/test_instance_routing_comprehensive.py | 5/5 | Comprehensive test suite validating middleware state management and session isolation |
| Server/tools/manage_gameobject.py | 3/5 | Changed to use ctx.get_state() directly instead of helper function |
| Server/tools/manage_asset.py | 3/5 | Changed to use ctx.get_state() directly instead of helper function |
Sequence Diagram
sequenceDiagram
participant Client as MCP Client
participant Server as FastMCP Server
participant Middleware as UnityInstanceMiddleware
participant Tool as Tool (e.g., manage_script)
participant Pool as UnityConnectionPool
participant Unity as Unity Editor Instance
Note over Client,Unity: Session-Based Instance Routing Flow
Client->>Server: set_active_instance(instance="MyProject@abc123")
Server->>Tool: Execute set_active_instance
Tool->>Middleware: set_active_instance(ctx, "MyProject@abc123")
Middleware->>Middleware: Store in _active_by_key[session_id]
Tool-->>Client: {"success": true, "instance": "MyProject@abc123"}
Note over Client,Unity: Subsequent Tool Call Routes to Active Instance
Client->>Server: manage_script(action="create", path="MyScript.cs")
Server->>Middleware: on_call_tool(context)
Middleware->>Middleware: Lookup active instance for session_id
Middleware->>Middleware: ctx.set_state("unity_instance", "MyProject@abc123")
Middleware->>Tool: call_next() - Execute manage_script
Tool->>Tool: unity_instance = get_unity_instance_from_context(ctx)
Tool->>Tool: unity_instance = "MyProject@abc123"
Tool->>Pool: send_command_with_retry(cmd, params, instance_id="MyProject@abc123")
Pool->>Pool: Resolve instance ID to connection
Pool->>Unity: TCP connection to MyProject on port 5555
Unity-->>Pool: Response
Pool-->>Tool: Response
Tool-->>Client: {"success": true, "data": {...}}
Note over Client,Unity: Different Session Uses Different Instance
Client->>Server: set_active_instance(instance="OtherProject@def456") [session2]
Server->>Middleware: set_active_instance(ctx2, "OtherProject@def456")
Middleware->>Middleware: Store in _active_by_key[session2_id]
Client->>Server: manage_gameobject(action="create") [session2]
Server->>Middleware: on_call_tool(context2)
Middleware->>Middleware: ctx2.set_state("unity_instance", "OtherProject@def456")
Middleware->>Tool: call_next()
Tool->>Pool: send_command_with_retry(instance_id="OtherProject@def456")
Pool->>Unity: TCP to OtherProject on port 5556
Unity-->>Client: Response routed to correct instance
57 files reviewed, 2 comments
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Server/tools/manage_prefabs.py (1)
27-47: Ensurecomponent_propertiesis sent through to Unity.The new
component_propertiesargument is accepted but never added toparams, so the Unity side never receives the payload. Any caller providing component data will silently fail. Please include it in the command parameters before dispatch.if search_inactive is not None: params["searchInactive"] = bool(search_inactive) + if component_properties: + params["componentProperties"] = component_propertiesMCPForUnity/UnityMcpServer~/src/tools/manage_prefabs.py (1)
27-47: Forwardcomponent_propertiesin the request payload.Just like the server-side copy, this function accepts
component_propertiesbut never forwards it to Unity, so any supplied component JSON is dropped. Add it toparamsbefore callingsend_with_unity_instance.if search_inactive is not None: params["searchInactive"] = bool(search_inactive) + if component_properties: + params["componentProperties"] = component_properties
🧹 Nitpick comments (8)
MCPForUnity/UnityMcpServer~/src/pyproject.toml (1)
9-9: Verify that fastmcp 2.13.0 supports the new multi-instance routing features.The bump from 2.12.5 to 2.13.0 aligns with the PR's introduction of per-session instance middleware and context-aware routing. However, confirm that this version provides any necessary APIs or capabilities required by the new connection pool and instance management code.
Also consider tightening the version constraint from
>=2.13.0to>=2.13.0,<3.0.0or similar for production stability, to prevent unexpected breaking changes in future major versions.MCPForUnity/Editor/MCPForUnityBridge.cs (1)
1218-1250: Robust project name extraction with good defensive coding.The project name derivation logic properly handles edge cases with try-catch and fallback to "Unknown". The addition of
project_nameandunity_versionto the heartbeat payload supports the multi-instance routing described in the PR objectives.Note:
ComputeProjectHash(lines 1276-1294) is duplicated in both this file andPortManager.cs(lines 302-320). Consider extracting to a shared utility class to reduce duplication.MCPForUnity/UnityMcpServer~/src/tools/manage_editor.py (1)
6-28: Use helper to readunity_instancesafely
Line 27 callsctx.get_state(...)directly. Several call-sites (tests, lightweight contexts) rely on the defensive helper to avoid AttributeError whenctxlacksget_state. For consistency with the other tools and to keep unit-test stubs working, please swap toget_unity_instance_from_context(ctx).-from tools import send_with_unity_instance +from tools import get_unity_instance_from_context, send_with_unity_instance @@ - unity_instance = ctx.get_state("unity_instance") + unity_instance = get_unity_instance_from_context(ctx)tests/test_manage_asset_json_parsing.py (1)
21-24: Silence Ruff ARG001 in stub transportsRuff flags these test doubles because
cmd,params, andkwargsnow go unused. Renaming them to_cmd,_params, etc., or explicitly deleting them here (and in the other stubs below) will quiet the warnings without changing behavior.Also applies to: 46-48, 68-70, 92-94, 120-122
MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)
69-71: Reuse the shared context helper for instance lookup.We already ship
get_unity_instance_from_contextintools.__init__, which gracefully handles contexts that don’t exposeget_stateand keeps the lookup logic in one place. Please switch to that helper here both for consistency with other tools in this PR and to avoid tight coupling to the concrete Context implementation.-from tools import send_with_unity_instance +from tools import get_unity_instance_from_context, send_with_unity_instance @@ - unity_instance = ctx.get_state("unity_instance") + unity_instance = get_unity_instance_from_context(ctx)Server/tools/manage_shader.py (1)
21-23: Leverage the existing unity-instance helper.For parity with other tools (and to keep us resilient to alternate Context implementations), please swap the direct
ctx.get_statecall forget_unity_instance_from_context(ctx)fromtools.__init__.-from tools import send_with_unity_instance +from tools import get_unity_instance_from_context, send_with_unity_instance @@ - unity_instance = ctx.get_state("unity_instance") + unity_instance = get_unity_instance_from_context(ctx)MCPForUnity/UnityMcpServer~/src/tools/manage_scene.py (1)
5-22: Reuse the context helper for unity_instance lookup.Other endpoints already rely on
get_unity_instance_from_contextso they work even if a Context doesn’t exposeget_statedirectly. Aligning here keeps that safety net and centralizes the lookup logic.-from tools import send_with_unity_instance +from tools import send_with_unity_instance, get_unity_instance_from_context @@ - unity_instance = ctx.get_state("unity_instance") + unity_instance = get_unity_instance_from_context(ctx)Server/tools/resource_tools.py (1)
46-76: LGTM! Instance-aware project root resolution implemented correctly.The function now extracts the Unity instance from context and routes the
get_project_rootcommand through the selected instance using the wrapper. The response handling correctly checks for success and safely accesses nested data.For slightly better readability, consider reformatting lines 71-72 to avoid breaking the chained
.get()calls mid-expression:- pr = Path(response.get("data", {}).get( - "projectRoot", "")).expanduser().resolve() + pr = Path( + response.get("data", {}).get("projectRoot", "") + ).expanduser().resolve()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (57)
MCPForUnity/Editor/Helpers/PortManager.cs(1 hunks)MCPForUnity/Editor/MCPForUnityBridge.cs(3 hunks)MCPForUnity/Editor/Tools/ManageAsset.cs(5 hunks)MCPForUnity/UnityMcpServer~/src/models.py(2 hunks)MCPForUnity/UnityMcpServer~/src/port_discovery.py(3 hunks)MCPForUnity/UnityMcpServer~/src/pyproject.toml(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/__init__.py(2 hunks)MCPForUnity/UnityMcpServer~/src/resources/menu_items.py(2 hunks)MCPForUnity/UnityMcpServer~/src/resources/tests.py(2 hunks)MCPForUnity/UnityMcpServer~/src/resources/unity_instances.py(1 hunks)MCPForUnity/UnityMcpServer~/src/server.py(6 hunks)MCPForUnity/UnityMcpServer~/src/tools/__init__.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/debug_request_context.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/execute_menu_item.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py(3 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_editor.py(3 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py(3 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_prefabs.py(3 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_scene.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_script.py(12 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_shader.py(3 hunks)MCPForUnity/UnityMcpServer~/src/tools/read_console.py(3 hunks)MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py(7 hunks)MCPForUnity/UnityMcpServer~/src/tools/run_tests.py(3 hunks)MCPForUnity/UnityMcpServer~/src/tools/script_apply_edits.py(8 hunks)MCPForUnity/UnityMcpServer~/src/tools/set_active_instance.py(1 hunks)MCPForUnity/UnityMcpServer~/src/unity_connection.py(8 hunks)MCPForUnity/UnityMcpServer~/src/unity_instance_middleware.py(1 hunks)Server/models.py(2 hunks)Server/port_discovery.py(3 hunks)Server/pyproject.toml(1 hunks)Server/resources/__init__.py(2 hunks)Server/resources/menu_items.py(2 hunks)Server/resources/tests.py(2 hunks)Server/resources/unity_instances.py(1 hunks)Server/server.py(6 hunks)Server/tools/__init__.py(2 hunks)Server/tools/execute_menu_item.py(2 hunks)Server/tools/manage_asset.py(3 hunks)Server/tools/manage_editor.py(3 hunks)Server/tools/manage_gameobject.py(3 hunks)Server/tools/manage_prefabs.py(3 hunks)Server/tools/manage_scene.py(2 hunks)Server/tools/manage_script.py(12 hunks)Server/tools/manage_shader.py(3 hunks)Server/tools/read_console.py(3 hunks)Server/tools/resource_tools.py(7 hunks)Server/tools/run_tests.py(3 hunks)Server/tools/script_apply_edits.py(8 hunks)Server/tools/set_active_instance.py(1 hunks)Server/unity_connection.py(8 hunks)Server/unity_instance_middleware.py(1 hunks)tests/test_helpers.py(1 hunks)tests/test_instance_routing_comprehensive.py(1 hunks)tests/test_instance_targeting_resolution.py(1 hunks)tests/test_manage_asset_json_parsing.py(9 hunks)tests/test_manage_asset_param_coercion.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T05:19:59.258Z
Learnt from: dsarno
Repo: dsarno/unity-mcp PR: 85
File: tests/test_telemetry_queue_worker.py:33-37
Timestamp: 2025-10-21T05:19:59.258Z
Learning: In the unity-mcp repository, pyproject.toml is located in MCPForUnity/UnityMcpServer~/src/, not at the repository root. When testing code that imports telemetry.py, the working directory should be changed to SRC (MCPForUnity/UnityMcpServer~/src/) so that telemetry.py's get_package_version() can find pyproject.toml via relative path.
Applied to files:
MCPForUnity/UnityMcpServer~/src/tools/resource_tools.pyMCPForUnity/UnityMcpServer~/src/resources/tests.pyServer/tools/resource_tools.py
🧬 Code graph analysis (50)
MCPForUnity/UnityMcpServer~/src/resources/menu_items.py (4)
MCPForUnity/UnityMcpServer~/src/models.py (1)
MCPResponse(6-10)MCPForUnity/UnityMcpServer~/src/registry/resource_registry.py (1)
mcp_for_unity_resource(10-43)MCPForUnity/UnityMcpServer~/src/tools/__init__.py (2)
get_unity_instance_from_context(64-80)async_send_with_unity_instance(96-106)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
async_send_command_with_retry(710-742)
MCPForUnity/UnityMcpServer~/src/tools/execute_menu_item.py (2)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (1)
async_send_with_unity_instance(96-106)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
async_send_command_with_retry(710-742)
Server/resources/menu_items.py (4)
Server/models.py (1)
MCPResponse(6-10)Server/registry/resource_registry.py (1)
mcp_for_unity_resource(10-43)Server/tools/__init__.py (2)
get_unity_instance_from_context(64-80)async_send_with_unity_instance(96-106)Server/unity_connection.py (1)
async_send_command_with_retry(710-742)
Server/resources/unity_instances.py (2)
MCPForUnity/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection_pool(623-636)discover_all_instances(432-459)tests/test_helpers.py (2)
info(36-37)error(46-47)
MCPForUnity/UnityMcpServer~/src/tools/set_active_instance.py (2)
MCPForUnity/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection_pool(623-636)discover_all_instances(432-459)MCPForUnity/UnityMcpServer~/src/unity_instance_middleware.py (2)
get_unity_instance_middleware(16-20)set_active_instance(58-62)
Server/unity_instance_middleware.py (2)
Server/tools/set_active_instance.py (1)
set_active_instance(12-45)tests/test_helpers.py (1)
set_state(49-51)
tests/test_manage_asset_json_parsing.py (3)
tests/test_helpers.py (1)
DummyContext(16-55)tests/test_script_tools.py (5)
fake_async(175-178)fake_send(75-78)fake_send(101-103)fake_send(128-130)fake_send(149-151)tests/test_instance_targeting_resolution.py (2)
fake_send(30-34)fake_send(68-70)
Server/tools/script_apply_edits.py (2)
Server/tools/__init__.py (2)
get_unity_instance_from_context(64-80)send_with_unity_instance(83-93)Server/unity_connection.py (1)
send_command_with_retry(670-707)
MCPForUnity/UnityMcpServer~/src/tools/read_console.py (2)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (1)
send_with_unity_instance(83-93)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(670-707)
Server/tools/__init__.py (3)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (4)
get_unity_instance_from_context(64-80)send_with_unity_instance(83-93)async_send_with_unity_instance(96-106)with_unity_instance(109-175)Server/unity_instance_middleware.py (1)
get_active_instance(64-68)tests/test_helpers.py (1)
info(36-37)
MCPForUnity/Editor/Helpers/PortManager.cs (1)
MCPForUnity/Editor/MCPForUnityBridge.cs (1)
IsDebugEnabled(72-75)
Server/tools/set_active_instance.py (2)
Server/unity_connection.py (2)
get_unity_connection_pool(623-636)discover_all_instances(432-459)Server/unity_instance_middleware.py (2)
get_unity_instance_middleware(16-20)set_active_instance(58-62)
Server/resources/__init__.py (1)
Server/telemetry_decorator.py (1)
telemetry_resource(110-164)
MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py (3)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (3)
get_unity_instance_from_context(64-80)send_with_unity_instance(83-93)async_send_with_unity_instance(96-106)Server/tools/resource_tools.py (1)
_resolve_project_root(46-108)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(670-707)
Server/resources/tests.py (4)
Server/models.py (1)
MCPResponse(6-10)Server/registry/resource_registry.py (1)
mcp_for_unity_resource(10-43)Server/tools/__init__.py (2)
get_unity_instance_from_context(64-80)async_send_with_unity_instance(96-106)Server/unity_connection.py (1)
async_send_command_with_retry(710-742)
MCPForUnity/UnityMcpServer~/src/tools/script_apply_edits.py (2)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (2)
get_unity_instance_from_context(64-80)send_with_unity_instance(83-93)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(670-707)
MCPForUnity/UnityMcpServer~/src/tools/manage_shader.py (3)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (1)
send_with_unity_instance(83-93)tests/test_helpers.py (1)
get_state(53-55)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(670-707)
Server/tools/manage_scene.py (3)
Server/tools/__init__.py (1)
send_with_unity_instance(83-93)Server/unity_connection.py (1)
send_command_with_retry(670-707)tests/test_helpers.py (1)
get_state(53-55)
MCPForUnity/UnityMcpServer~/src/resources/tests.py (3)
MCPForUnity/UnityMcpServer~/src/models.py (1)
MCPResponse(6-10)MCPForUnity/UnityMcpServer~/src/tools/__init__.py (2)
get_unity_instance_from_context(64-80)async_send_with_unity_instance(96-106)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
async_send_command_with_retry(710-742)
MCPForUnity/UnityMcpServer~/src/resources/unity_instances.py (2)
MCPForUnity/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection_pool(623-636)discover_all_instances(432-459)tests/test_helpers.py (2)
info(36-37)error(46-47)
MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (3)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (1)
send_with_unity_instance(83-93)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(670-707)tests/test_helpers.py (1)
get_state(53-55)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (5)
MCPForUnity/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_tool(17-107)MCPForUnity/UnityMcpServer~/src/registry/tool_registry.py (1)
get_registered_tools(44-46)MCPForUnity/UnityMcpServer~/src/module_discovery.py (1)
discover_modules(13-55)MCPForUnity/UnityMcpServer~/src/unity_instance_middleware.py (1)
get_active_instance(64-68)tests/test_helpers.py (1)
info(36-37)
Server/port_discovery.py (1)
Server/models.py (1)
UnityInstanceInfo(13-35)
tests/test_instance_targeting_resolution.py (3)
tests/test_helpers.py (2)
DummyContext(16-55)set_state(49-51)MCPForUnity/UnityMcpServer~/src/unity_instance_middleware.py (4)
UnityInstanceMiddleware(29-85)set_unity_instance_middleware(23-26)set_active_instance(58-62)get_active_instance(64-68)MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)
manage_gameobject(13-211)
tests/test_instance_routing_comprehensive.py (3)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (1)
get_unity_instance_from_context(64-80)Server/tools/__init__.py (1)
get_unity_instance_from_context(64-80)tests/test_helpers.py (3)
set_state(49-51)get_state(53-55)info(36-37)
MCPForUnity/UnityMcpServer~/src/unity_instance_middleware.py (2)
MCPForUnity/UnityMcpServer~/src/tools/set_active_instance.py (1)
set_active_instance(12-45)tests/test_helpers.py (1)
set_state(49-51)
MCPForUnity/UnityMcpServer~/src/tools/debug_request_context.py (1)
MCPForUnity/UnityMcpServer~/src/unity_instance_middleware.py (3)
get_unity_instance_middleware(16-20)_get_session_key(42-56)get_active_instance(64-68)
Server/tools/manage_editor.py (3)
Server/tools/__init__.py (1)
send_with_unity_instance(83-93)tests/test_helpers.py (1)
get_state(53-55)Server/unity_connection.py (1)
send_command_with_retry(670-707)
Server/tools/manage_gameobject.py (3)
Server/tools/__init__.py (1)
send_with_unity_instance(83-93)Server/unity_connection.py (1)
send_command_with_retry(670-707)tests/test_helpers.py (1)
get_state(53-55)
Server/tools/read_console.py (3)
Server/tools/__init__.py (1)
send_with_unity_instance(83-93)tests/test_helpers.py (1)
get_state(53-55)Server/unity_connection.py (1)
send_command_with_retry(670-707)
Server/tools/manage_shader.py (2)
Server/tools/__init__.py (1)
send_with_unity_instance(83-93)Server/unity_connection.py (1)
send_command_with_retry(670-707)
MCPForUnity/UnityMcpServer~/src/port_discovery.py (1)
MCPForUnity/UnityMcpServer~/src/models.py (1)
UnityInstanceInfo(13-35)
MCPForUnity/UnityMcpServer~/src/resources/__init__.py (1)
MCPForUnity/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_resource(110-164)
Server/tools/resource_tools.py (3)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (3)
get_unity_instance_from_context(64-80)send_with_unity_instance(83-93)async_send_with_unity_instance(96-106)Server/tools/__init__.py (3)
get_unity_instance_from_context(64-80)send_with_unity_instance(83-93)async_send_with_unity_instance(96-106)Server/unity_connection.py (1)
send_command_with_retry(670-707)
Server/tools/manage_asset.py (3)
Server/tools/__init__.py (1)
async_send_with_unity_instance(96-106)tests/test_helpers.py (1)
get_state(53-55)Server/unity_connection.py (1)
async_send_command_with_retry(710-742)
Server/tools/run_tests.py (3)
Server/tools/__init__.py (1)
async_send_with_unity_instance(96-106)tests/test_helpers.py (1)
get_state(53-55)Server/unity_connection.py (1)
async_send_command_with_retry(710-742)
Server/unity_connection.py (2)
Server/port_discovery.py (3)
PortDiscovery(29-307)discover_unity_port(132-176)discover_all_unity_instances(224-307)Server/models.py (2)
MCPResponse(6-10)UnityInstanceInfo(13-35)
Server/tools/execute_menu_item.py (2)
Server/tools/__init__.py (1)
async_send_with_unity_instance(96-106)Server/unity_connection.py (1)
async_send_command_with_retry(710-742)
MCPForUnity/UnityMcpServer~/src/server.py (3)
MCPForUnity/UnityMcpServer~/src/unity_connection.py (5)
get_unity_connection_pool(623-636)UnityConnectionPool(415-615)discover_all_instances(432-459)get_connection(564-604)disconnect_all(606-615)MCPForUnity/UnityMcpServer~/src/unity_instance_middleware.py (2)
UnityInstanceMiddleware(29-85)set_unity_instance_middleware(23-26)MCPForUnity/UnityMcpServer~/src/telemetry.py (2)
record_telemetry(423-427)RecordType(62-72)
MCPForUnity/UnityMcpServer~/src/tools/run_tests.py (3)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (1)
async_send_with_unity_instance(96-106)tests/test_helpers.py (1)
get_state(53-55)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
async_send_command_with_retry(710-742)
Server/tools/manage_prefabs.py (2)
Server/tools/__init__.py (1)
send_with_unity_instance(83-93)Server/unity_connection.py (1)
send_command_with_retry(670-707)
MCPForUnity/UnityMcpServer~/src/unity_connection.py (2)
MCPForUnity/UnityMcpServer~/src/port_discovery.py (3)
PortDiscovery(29-307)discover_unity_port(132-176)discover_all_unity_instances(224-307)MCPForUnity/UnityMcpServer~/src/models.py (1)
UnityInstanceInfo(13-35)
MCPForUnity/UnityMcpServer~/src/tools/manage_prefabs.py (3)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (1)
send_with_unity_instance(83-93)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(670-707)tests/test_helpers.py (1)
get_state(53-55)
MCPForUnity/UnityMcpServer~/src/tools/manage_script.py (3)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (2)
get_unity_instance_from_context(64-80)send_with_unity_instance(83-93)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(670-707)Server/tools/manage_script.py (1)
_split_uri(13-62)
MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py (3)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (1)
async_send_with_unity_instance(96-106)tests/test_helpers.py (1)
get_state(53-55)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
async_send_command_with_retry(710-742)
MCPForUnity/UnityMcpServer~/src/tools/manage_scene.py (3)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (1)
send_with_unity_instance(83-93)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(670-707)tests/test_helpers.py (1)
get_state(53-55)
Server/server.py (3)
Server/unity_connection.py (5)
get_unity_connection_pool(623-636)UnityConnectionPool(415-615)discover_all_instances(432-459)get_connection(564-604)disconnect_all(606-615)Server/unity_instance_middleware.py (2)
UnityInstanceMiddleware(29-85)set_unity_instance_middleware(23-26)Server/telemetry.py (2)
record_telemetry(423-427)RecordType(62-72)
MCPForUnity/UnityMcpServer~/src/tools/manage_editor.py (2)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (1)
send_with_unity_instance(83-93)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(670-707)
Server/tools/manage_script.py (2)
Server/tools/__init__.py (2)
get_unity_instance_from_context(64-80)send_with_unity_instance(83-93)Server/unity_connection.py (1)
send_command_with_retry(670-707)
MCPForUnity/Editor/MCPForUnityBridge.cs (2)
MCPForUnity/Editor/Helpers/PortManager.cs (4)
PortManager(17-321)GetPortWithFallback(42-74)IsDebugEnabled(19-23)ComputeProjectHash(302-320)MCPForUnity/Editor/Helpers/McpLog.cs (5)
McpLog(6-32)Error(28-31)IsDebugEnabled(12-15)Info(17-21)Warn(23-26)
🪛 Ruff (0.14.3)
Server/resources/unity_instances.py
58-58: Consider moving this statement to an else block
(TRY300)
60-60: Do not catch blind exception: Exception
(BLE001)
64-64: Use explicit conversion flag
Replace with conversion flag
(RUF010)
Server/unity_instance_middleware.py
19-19: Avoid specifying long messages outside the exception class
(TRY003)
tests/test_manage_asset_json_parsing.py
21-21: Unused function argument: cmd
(ARG001)
21-21: Unused function argument: params
(ARG001)
21-21: Unused function argument: kwargs
(ARG001)
46-46: Unused function argument: cmd
(ARG001)
46-46: Unused function argument: params
(ARG001)
46-46: Unused function argument: kwargs
(ARG001)
68-68: Unused function argument: cmd
(ARG001)
68-68: Unused function argument: params
(ARG001)
68-68: Unused function argument: kwargs
(ARG001)
92-92: Unused function argument: cmd
(ARG001)
92-92: Unused function argument: params
(ARG001)
92-92: Unused function argument: kwargs
(ARG001)
120-120: Unused function argument: cmd
(ARG001)
120-120: Unused function argument: params
(ARG001)
120-120: Unused function argument: kwargs
(ARG001)
Server/tools/__init__.py
17-23: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
77-78: try-except-pass detected, consider logging the exception
(S110)
77-77: Do not catch blind exception: Exception
(BLE001)
132-132: Do not catch blind exception: Exception
(BLE001)
136-136: Do not catch blind exception: Exception
(BLE001)
149-150: try-except-pass detected, consider logging the exception
(S110)
149-149: Do not catch blind exception: Exception
(BLE001)
165-165: Store a reference to the return value of loop.create_task
(RUF006)
169-170: try-except-pass detected, consider logging the exception
(S110)
169-169: Do not catch blind exception: Exception
(BLE001)
MCPForUnity/UnityMcpServer~/src/resources/unity_instances.py
58-58: Consider moving this statement to an else block
(TRY300)
60-60: Do not catch blind exception: Exception
(BLE001)
64-64: Use explicit conversion flag
Replace with conversion flag
(RUF010)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py
17-23: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
77-78: try-except-pass detected, consider logging the exception
(S110)
77-77: Do not catch blind exception: Exception
(BLE001)
132-132: Do not catch blind exception: Exception
(BLE001)
136-136: Do not catch blind exception: Exception
(BLE001)
149-150: try-except-pass detected, consider logging the exception
(S110)
149-149: Do not catch blind exception: Exception
(BLE001)
164-164: Store a reference to the return value of loop.create_task
(RUF006)
167-168: try-except-pass detected, consider logging the exception
(S110)
167-167: Do not catch blind exception: Exception
(BLE001)
Server/port_discovery.py
106-106: Consider moving this statement to an else block
(TRY300)
107-107: Do not catch blind exception: Exception
(BLE001)
110-110: Do not catch blind exception: Exception
(BLE001)
219-219: Consider moving this statement to an else block
(TRY300)
220-220: Do not catch blind exception: Exception
(BLE001)
262-263: try-except-pass detected, consider logging the exception
(S110)
262-262: Do not catch blind exception: Exception
(BLE001)
300-300: Do not catch blind exception: Exception
(BLE001)
tests/test_instance_targeting_resolution.py
68-68: Unused function argument: command_type
(ARG001)
68-68: Unused function argument: params
(ARG001)
tests/test_instance_routing_comprehensive.py
110-110: Unused function argument: ctx
(ARG001)
202-202: Unused function argument: ctx
(ARG001)
MCPForUnity/UnityMcpServer~/src/unity_instance_middleware.py
19-19: Avoid specifying long messages outside the exception class
(TRY003)
MCPForUnity/UnityMcpServer~/src/tools/debug_request_context.py
30-30: Do not catch blind exception: Exception
(BLE001)
MCPForUnity/UnityMcpServer~/src/port_discovery.py
106-106: Consider moving this statement to an else block
(TRY300)
107-107: Do not catch blind exception: Exception
(BLE001)
110-110: Do not catch blind exception: Exception
(BLE001)
219-219: Consider moving this statement to an else block
(TRY300)
220-220: Do not catch blind exception: Exception
(BLE001)
262-263: try-except-pass detected, consider logging the exception
(S110)
262-262: Do not catch blind exception: Exception
(BLE001)
300-300: Do not catch blind exception: Exception
(BLE001)
Server/unity_connection.py
256-256: Do not catch blind exception: Exception
(BLE001)
366-368: Abstract raise to an inner function
(TRY301)
366-368: Avoid specifying long messages outside the exception class
(TRY003)
476-478: Avoid specifying long messages outside the exception class
(TRY003)
515-519: Avoid specifying long messages outside the exception class
(TRY003)
526-528: Avoid specifying long messages outside the exception class
(TRY003)
558-562: Avoid specifying long messages outside the exception class
(TRY003)
590-593: Avoid specifying long messages outside the exception class
(TRY003)
MCPForUnity/UnityMcpServer~/src/server.py
71-71: Unused function argument: server
(ARG001)
128-128: Do not catch blind exception: Exception
(BLE001)
Server/tools/manage_prefabs.py
27-27: Unused function argument: component_properties
(ARG001)
MCPForUnity/UnityMcpServer~/src/unity_connection.py
256-256: Do not catch blind exception: Exception
(BLE001)
366-368: Abstract raise to an inner function
(TRY301)
366-368: Avoid specifying long messages outside the exception class
(TRY003)
476-478: Avoid specifying long messages outside the exception class
(TRY003)
515-519: Avoid specifying long messages outside the exception class
(TRY003)
526-528: Avoid specifying long messages outside the exception class
(TRY003)
558-562: Avoid specifying long messages outside the exception class
(TRY003)
590-593: Avoid specifying long messages outside the exception class
(TRY003)
MCPForUnity/UnityMcpServer~/src/tools/manage_prefabs.py
27-27: Unused function argument: component_properties
(ARG001)
tests/test_manage_asset_param_coercion.py
49-49: Unused function argument: cmd
(ARG001)
49-49: Unused function argument: kwargs
(ARG001)
Server/server.py
71-71: Unused function argument: server
(ARG001)
128-128: Do not catch blind exception: Exception
(BLE001)
Server/tools/manage_script.py
108-108: Undefined name send_with_unity_instance
(F821)
488-488: Undefined name send_with_unity_instance
(F821)
🔇 Additional comments (39)
MCPForUnity/Editor/Helpers/PortManager.cs (2)
63-67: Good improvement to port fallback logic.When the stored port remains busy after waiting, the code now correctly finds and saves an alternative available port instead of returning the busy one. This aligns well with the multi-instance support described in the PR objectives.
71-73: LGTM - clearer variable naming.The rename from
newPorttofoundPortis a minor improvement in clarity.MCPForUnity/Editor/MCPForUnityBridge.cs (1)
492-506: Good cleanup logic for status files.Properly removes the per-project status file on shutdown with appropriate error handling and debug logging.
MCPForUnity/Editor/Tools/ManageAsset.cs (2)
914-977: Excellent defensive color property handling.The changes add robust validation:
- Auto-detects the main color property when not specified
- Verifies property existence before attempting to set
- Only marks material as modified when the value actually changes
- Provides helpful warnings with guidance on common color properties
This prevents silent failures and improves the user experience when working with different shader types.
1164-1183: Well-designed color property detection helper.The helper method intelligently tries common color properties in order (_BaseColor for URP, _Color for Standard, etc.) with a safe fallback. This aligns well with the warning messages in the calling code and supports multiple render pipelines.
Server/models.py (1)
13-35: UnityInstanceInfo serialization looks solidField coverage and ISO conversion give downstream consumers consistent shapes while handling missing heartbeats safely.
MCPForUnity/UnityMcpServer~/src/tools/manage_shader.py (1)
21-47: Instance-aware routing hooked in cleanly
ctx.get_statefallback keeps existing behavior whilesend_with_unity_instancecentralizes theinstance_idinjection, so the command path stays resilient to missing state.MCPForUnity/UnityMcpServer~/src/tools/execute_menu_item.py (1)
22-28: Async instance routing handled correctlyUsing
ctx.get_stateplusasync_send_with_unity_instancekeeps the retry helper untouched while ensuring the request respects the session’s chosen Unity instance.Server/tools/execute_menu_item.py (1)
22-28: Server-side tool mirrors routing pattern wellThe context state lookup and async wrapper preserve behavior while adding per-session targeting parity with the Unity MCP tool.
MCPForUnity/UnityMcpServer~/src/tools/read_console.py (1)
29-95: Console tool now respects selected instanceInstance-aware send without altering the parameter normalization keeps the original behavior intact while supporting multi-instance routing.
MCPForUnity/UnityMcpServer~/src/resources/__init__.py (1)
40-74: Template-aware registration reads nicelySplitting query-templated URIs keeps telemetry wrapping consistent and the registration count accurate, which will make the new per-instance resources easier to reason about.
Server/tools/manage_gameobject.py (2)
6-6: LGTM - Context-based instance retrieval.The transition from session_state to context-based retrieval via
ctx.get_state("unity_instance")correctly aligns with the UnityInstanceMiddleware design.Also applies to: 69-71
201-202: LGTM - Command routing via Unity instance wrapper.The routing through
send_with_unity_instancecorrectly passes the active instance to the underlying transport, enabling per-session instance targeting while preserving backward compatibility when no instance is set.Server/resources/unity_instances.py (3)
16-50: LGTM - Resource implementation is clean.The function correctly discovers instances, builds structured output, and detects duplicate project names for helpful warnings.
52-56: LGTM - Helpful duplicate name warning.The warning provides clear guidance when multiple instances share a project name, directing users to the disambiguating full format.
60-67: Broad exception catch is acceptable here.For a resource listing endpoint, catching broad exceptions ensures robustness and returns a structured error payload to clients. The error logging provides visibility for debugging.
MCPForUnity/UnityMcpServer~/src/tools/manage_script.py (3)
90-91: LGTM - Consistent instance routing in apply_text_edits.Both the read operation (for normalization) and the apply operation correctly route through the same Unity instance, ensuring coherent multi-step editing.
Also applies to: 108-117, 314-319
376-377: LGTM - Consistent instance routing across all script tools.All script management functions (create, delete, validate, manage, get_sha) correctly retrieve the active instance from context and route commands through the same instance.
Also applies to: 403-408, 418-419, 424-429, 442-443, 455-460, 485-486, 508-513, 577-578, 582-587
351-360: LGTM - Correct instance targeting in async sentinel flip.The force_sentinel_reload path correctly passes the unity_instance to ensure the reload sentinel flips on the same instance where the edit was applied.
MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py (2)
37-50: LGTM - Defensive JSON parsing with clear error handling.The properties coercion handles both string and dict inputs gracefully, with helpful warnings on parse failures and sensible defaults.
89-93: LGTM - Correct async instance routing.The async variant of the wrapper correctly routes the command through the active Unity instance while properly handling the event loop.
tests/test_instance_routing_comprehensive.py (3)
29-85: LGTM - Comprehensive middleware state tests.The basic tests correctly validate storage, retrieval, session isolation, and the fallback chain (session_id → client_id → global).
87-143: LGTM - Integration tests validate end-to-end flow.The integration tests correctly verify that middleware injects instance state and that tools can retrieve it via
get_unity_instance_from_context. The unusedctxparameter in mock functions is acceptable for signature matching.
247-316: LGTM - Regression test for sequential routing bug.This test correctly reproduces the reported bug where alternating instance selections led to incorrect routing. The test structure properly simulates the middleware flow and validates the fix.
Server/tools/manage_scene.py (1)
5-5: LGTM - Consistent instance routing pattern.The scene management tool follows the same correct pattern as other tools: retrieve instance from context, route commands through the wrapper.
Also applies to: 19-21, 49-50
MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py (2)
46-75: LGTM - Project root resolution now instance-aware.The signature change to accept Context enables proper routing of the Unity project root query through the active instance, while preserving all fallback mechanisms.
151-154: LGTM - All call sites correctly updated.All resource tools (list_resources, read_resource, find_in_file) correctly pass the context to
_resolve_project_rootand log the active instance.Also applies to: 212-214, 277-277, 368-371
Server/server.py (4)
106-131: LGTM - Clean pool initialization with discovery.The startup sequence correctly initializes the connection pool, discovers instances, and attempts default connection. The telemetry includes instance count, which provides valuable insight. The broad exception catch in the startup path is acceptable for robustness.
161-168: LGTM - Proper pool lifecycle management.The lifespan correctly yields the pool and ensures all connections are disconnected during cleanup. The comment clarifies that tools access the pool directly via the global getter.
195-199: LGTM - Middleware registered before tools.The middleware initialization and registration sequence is correct. Registering before tools ensures the middleware is in place when tool decorators are processed.
208-242: LGTM - Well-documented CLI argument handling.The argparse setup is clean with helpful documentation in the epilog. Environment variable propagation from
--default-instanceis correctly implemented. The unusedserverparameter on Line 71 is required by the FastMCP lifespan signature.MCPForUnity/UnityMcpServer~/src/resources/tests.py (1)
24-33: Context-aware routing looks solid.Fetching the Unity instance from the request context before delegating through
async_send_with_unity_instancekeeps the instance_id plumbing consistent and avoids repeating per-call wiring. Nice upgrade.tests/test_helpers.py (1)
19-55: DummyContext now mirrors FastMCP behaviors.The added state storage, logging hooks, and per-request metadata align the test helper with the real Context so the new routing tests can rely on it without shims.
MCPForUnity/UnityMcpServer~/src/resources/unity_instances.py (1)
40-56: Thanks for surfacing duplicate-name guidance.Counting duplicate project names and returning a tailored warning makes it much easier for callers to target the right instance when several projects share the same name.
Server/unity_instance_middleware.py (1)
37-68: Session-keyed cache with locking looks good.Using
_get_session_keytogether with the RLock-guarded map gives deterministic per-session routing without race conditions. Nice work.Server/tools/resource_tools.py (4)
17-17: LGTM! Context-aware Unity instance imports added correctly.The imports enable per-session instance routing, allowing tools to extract the active Unity instance from context and route commands accordingly.
143-193: LGTM! Context-aware instance routing integrated correctly.The function now accepts a
Contextparameter, extracts the Unity instance, logs the targeted instance for debugging, and passes the context to_resolve_project_rootfor instance-aware project resolution.
197-354: LGTM! Instance routing integrated consistently.The function follows the same pattern as
list_resources, extracting the Unity instance from context, logging it, and passing context through to_resolve_project_root. The implementation is consistent and correct.
358-415: LGTM! Instance routing integration complete.The function correctly integrates context-aware instance routing, maintaining consistency with the other resource functions. All call sites of
_resolve_project_rootacross the file have been properly updated to pass the context parameter.
| // Port is occupied by another instance, get a new available port | ||
| int oldPort = currentUnityPort; | ||
| currentUnityPort = PortManager.GetPortWithFallback(); | ||
|
|
||
| // Safety check: ensure we got a different port | ||
| if (currentUnityPort == oldPort) | ||
| { | ||
| McpLog.Error($"Port {oldPort} is occupied and no alternative port available"); | ||
| throw; | ||
| } | ||
|
|
||
| if (IsDebugEnabled()) | ||
| { | ||
| McpLog.Info($"Port {oldPort} occupied, switching to port {currentUnityPort}"); | ||
| } | ||
|
|
There was a problem hiding this comment.
Verify safety check logic when port becomes available.
The safety check at lines 370-374 throws an error if GetPortWithFallback() returns the same port as oldPort. However, GetPortWithFallback() internally calls WaitForPortRelease(), which might successfully reclaim the stored port if it becomes available during the wait. In that scenario, returning the same port is valid and should be used, not treated as an error.
Consider revising the logic to distinguish between:
- Port became available during wait → use it
- All ports exhausted → error
- // Safety check: ensure we got a different port
- if (currentUnityPort == oldPort)
- {
- McpLog.Error($"Port {oldPort} is occupied and no alternative port available");
- throw;
- }
+ // If GetPortWithFallback returned the same port, it became available - proceed
+ // If it returned a different port, we're switching to an alternativeCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In MCPForUnity/Editor/MCPForUnityBridge.cs around lines 365-380, the safety
check treats getting the same port as an error even though GetPortWithFallback()
may wait and legitimately return the reclaimed port; update the logic so
same-port is allowed and only treat the call as failure when no alternative port
is available. Concretely: change PortManager.GetPortWithFallback() to signal a
genuine failure (e.g., return -1/null or throw a specific exception) when no
ports remain, or add an out/bool indicator (e.g., succeeded/wasExhausted); then
here, remove the current equality-based throw and instead only log+throw when
the sentinel/failure indicator is returned, otherwise proceed and use the
returned port (and keep the existing debug/info logging).
- Standardize all 18 tools to use get_unity_instance_from_context() helper instead of direct ctx.get_state() calls for consistency - Remove dead session_state imports from with_unity_instance decorator that would cause ModuleNotFoundError at runtime - Update README.md with concise instance routing documentation
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Server/tools/manage_prefabs.py (1)
27-48: Forward component_properties to Unity
component_propertiesis accepted but never added toparams, so callers can’t actually send the data. Please forward it with the rest of the payload.Apply this diff:
if allow_overwrite is not None: params["allowOverwrite"] = bool(allow_overwrite) if search_inactive is not None: params["searchInactive"] = bool(search_inactive) + if component_properties is not None: + params["componentProperties"] = component_propertiesMCPForUnity/UnityMcpServer~/src/tools/run_tests.py (1)
1-10: Don’t await ctx.info (it isn’t awaitable)
ctx.inforeturnsNonein our contexts, soawait ctx.info(...)will raise at runtime. Handle the optional awaitable case explicitly.Apply this diff:
-"""Tool for executing Unity Test Runner suites.""" -from typing import Annotated, Literal, Any +"""Tool for executing Unity Test Runner suites.""" +import inspect +from typing import Annotated, Literal, Any @@ - response = await async_send_with_unity_instance( - async_send_command_with_retry, unity_instance, "run_tests", params - ) - await ctx.info(f"Response {response}") + response = await async_send_with_unity_instance( + async_send_command_with_retry, unity_instance, "run_tests", params + ) + log_result = ctx.info(f"Response {response}") + if inspect.isawaitable(log_result): + await log_resultAlso applies to: 75-77
🧹 Nitpick comments (1)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (1)
17-23: Optional: Sort__all__exports.Static analysis suggests sorting the
__all__list for consistency. This is a minor style improvement.Apply this diff if desired:
__all__ = [ + "async_send_with_unity_instance", + "get_unity_instance_from_context", "register_all_tools", - "get_unity_instance_from_context", "send_with_unity_instance", - "async_send_with_unity_instance", "with_unity_instance", ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/execute_menu_item.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py(3 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_editor.py(3 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py(3 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_prefabs.py(3 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_scene.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_shader.py(3 hunks)MCPForUnity/UnityMcpServer~/src/tools/read_console.py(3 hunks)MCPForUnity/UnityMcpServer~/src/tools/run_tests.py(3 hunks)README.md(3 hunks)Server/tools/__init__.py(2 hunks)Server/tools/execute_menu_item.py(2 hunks)Server/tools/manage_asset.py(3 hunks)Server/tools/manage_editor.py(3 hunks)Server/tools/manage_gameobject.py(3 hunks)Server/tools/manage_prefabs.py(3 hunks)Server/tools/manage_scene.py(2 hunks)Server/tools/manage_shader.py(3 hunks)Server/tools/read_console.py(3 hunks)Server/tools/run_tests.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- MCPForUnity/UnityMcpServer~/src/tools/read_console.py
- Server/tools/manage_editor.py
- Server/tools/manage_shader.py
- Server/tools/execute_menu_item.py
- Server/tools/read_console.py
- MCPForUnity/UnityMcpServer~/src/tools/execute_menu_item.py
🧰 Additional context used
🧬 Code graph analysis (14)
Server/tools/run_tests.py (2)
Server/tools/__init__.py (2)
get_unity_instance_from_context(64-80)async_send_with_unity_instance(96-106)Server/unity_connection.py (1)
async_send_command_with_retry(710-742)
Server/tools/manage_gameobject.py (2)
Server/tools/__init__.py (2)
get_unity_instance_from_context(64-80)send_with_unity_instance(83-93)Server/unity_connection.py (1)
send_command_with_retry(670-707)
MCPForUnity/UnityMcpServer~/src/tools/manage_shader.py (2)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (2)
get_unity_instance_from_context(64-80)send_with_unity_instance(83-93)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(670-707)
MCPForUnity/UnityMcpServer~/src/tools/manage_scene.py (2)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (2)
get_unity_instance_from_context(64-80)send_with_unity_instance(83-93)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(670-707)
MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (2)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (2)
get_unity_instance_from_context(64-80)send_with_unity_instance(83-93)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(670-707)
Server/tools/__init__.py (2)
Server/telemetry_decorator.py (1)
telemetry_tool(17-107)tests/test_helpers.py (1)
info(36-37)
MCPForUnity/UnityMcpServer~/src/tools/manage_editor.py (2)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (2)
get_unity_instance_from_context(64-80)send_with_unity_instance(83-93)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(670-707)
MCPForUnity/UnityMcpServer~/src/tools/run_tests.py (2)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (2)
get_unity_instance_from_context(64-80)async_send_with_unity_instance(96-106)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
async_send_command_with_retry(710-742)
Server/tools/manage_prefabs.py (2)
Server/tools/__init__.py (2)
get_unity_instance_from_context(64-80)send_with_unity_instance(83-93)Server/unity_connection.py (1)
send_command_with_retry(670-707)
MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py (1)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (2)
get_unity_instance_from_context(64-80)async_send_with_unity_instance(96-106)
MCPForUnity/UnityMcpServer~/src/tools/manage_prefabs.py (3)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (2)
get_unity_instance_from_context(64-80)send_with_unity_instance(83-93)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
send_command_with_retry(670-707)Server/tools/manage_prefabs.py (1)
manage_prefabs(12-57)
Server/tools/manage_scene.py (2)
Server/tools/__init__.py (2)
get_unity_instance_from_context(64-80)send_with_unity_instance(83-93)Server/unity_connection.py (1)
send_command_with_retry(670-707)
Server/tools/manage_asset.py (2)
Server/tools/__init__.py (2)
get_unity_instance_from_context(64-80)async_send_with_unity_instance(96-106)Server/unity_connection.py (1)
async_send_command_with_retry(710-742)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (1)
tests/test_helpers.py (1)
info(36-37)
🪛 markdownlint-cli2 (0.18.1)
README.md
55-55: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
64-64: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
293-293: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.3)
Server/tools/__init__.py
17-23: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
77-78: try-except-pass detected, consider logging the exception
(S110)
77-77: Do not catch blind exception: Exception
(BLE001)
132-132: Do not catch blind exception: Exception
(BLE001)
136-136: Do not catch blind exception: Exception
(BLE001)
148-149: try-except-pass detected, consider logging the exception
(S110)
148-148: Do not catch blind exception: Exception
(BLE001)
163-163: Store a reference to the return value of loop.create_task
(RUF006)
167-168: try-except-pass detected, consider logging the exception
(S110)
167-167: Do not catch blind exception: Exception
(BLE001)
Server/tools/manage_prefabs.py
27-27: Unused function argument: component_properties
(ARG001)
MCPForUnity/UnityMcpServer~/src/tools/manage_prefabs.py
27-27: Unused function argument: component_properties
(ARG001)
MCPForUnity/UnityMcpServer~/src/tools/__init__.py
17-23: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
77-78: try-except-pass detected, consider logging the exception
(S110)
77-77: Do not catch blind exception: Exception
(BLE001)
132-132: Do not catch blind exception: Exception
(BLE001)
136-136: Do not catch blind exception: Exception
(BLE001)
148-149: try-except-pass detected, consider logging the exception
(S110)
148-148: Do not catch blind exception: Exception
(BLE001)
162-162: Store a reference to the return value of loop.create_task
(RUF006)
165-166: try-except-pass detected, consider logging the exception
(S110)
165-165: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (15)
README.md (1)
282-302: Excellent addition: Multi-instance routing section is clear and user-friendly.The new "Working with Multiple Unity Instances" section is well-structured with:
- Clear step-by-step instructions (list instances → set active → use tools)
- Practical example showing the user-LLM workflow
- Appropriate level of detail for end-user documentation
This effectively communicates the new feature to users and integrates naturally with the existing documentation flow.
MCPForUnity/UnityMcpServer~/src/tools/manage_editor.py (1)
6-6: LGTM: Instance routing correctly integrated.The file successfully adopts the per-instance routing pattern by retrieving the Unity instance from context and routing commands through
send_with_unity_instance. The implementation is consistent with the broader multi-instance architecture.Also applies to: 26-27, 68-68
Server/tools/run_tests.py (1)
9-9: LGTM: Async instance routing correctly integrated.The async routing pattern is correctly implemented using
get_unity_instance_from_contextandasync_send_with_unity_instance.Also applies to: 52-52, 75-75
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (3)
64-80: LGTM: Defensive context extraction.The
get_unity_instance_from_contexthelper correctly extracts the Unity instance from middleware state with appropriate defensive checks usinggetattrand exception handling.
83-93: LGTM: Instance routing wrappers correctly implemented.Both sync and async variants correctly attach
instance_idusingsetdefault, ensuring existing values aren't overwritten. The implementation is clean and type-safe.Also applies to: 96-106
109-173: LGTM: Comprehensive decorator with async/sync support.The
with_unity_instancedecorator handles both sync and async functions correctly, with proper logging and error handling. The fire-and-forget task creation at Line 162 is intentional for non-blocking logging.MCPForUnity/UnityMcpServer~/src/tools/manage_shader.py (1)
6-6: LGTM: Instance routing correctly integrated.The shader management tool correctly adopts instance-aware routing while preserving the existing shader contents encoding logic. Implementation is consistent with the multi-instance pattern.
Also applies to: 21-23, 46-46
MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py (1)
10-10: LGTM: Async instance routing correctly integrated.The asset management tool correctly implements async instance routing while preserving input coercion logic. The
loopparameter is properly passed to the async helper.Also applies to: 37-39, 93-93
MCPForUnity/UnityMcpServer~/src/tools/manage_scene.py (1)
5-5: LGTM: Instance routing correctly integrated.The scene management tool correctly adopts instance routing while preserving all scene operation logic. Implementation is consistent with the pattern.
Also applies to: 19-21, 50-50
Server/tools/manage_gameobject.py (1)
6-6: LGTM: Instance routing correctly integrated and previous inconsistency resolved.The GameObject management tool now consistently uses
get_unity_instance_from_context(ctx)helper instead of directctx.get_state()calls, addressing the previous review comment. The routing implementation is correct and consistent with other tools.Also applies to: 69-70, 201-201
Server/tools/manage_scene.py (1)
5-5: LGTM: Instance routing correctly integrated.The scene management tool in the Server package correctly implements instance routing, mirroring the MCPForUnity implementation. The pattern is consistent across the codebase.
Also applies to: 19-21, 50-50
MCPForUnity/UnityMcpServer~/src/tools/manage_prefabs.py (4)
5-5: LGTM! Imports align with instance-aware routing.The new imports are necessary for the Unity instance routing mechanism and are properly utilized in the function.
10-10: LGTM! Description updated appropriately.The generic description correctly reflects the expanded scope of prefab operations.
14-14: LGTM! Action parameter expanded correctly.The action literal values properly reflect the expanded prefab operation capabilities.
31-31: LGTM! Unity instance routing implemented correctly.The function properly integrates with the new session-based instance routing by extracting the Unity instance from context and using the instance-aware dispatch wrapper. This aligns with the PR's objective of enabling multi-Unity-instance support.
Also applies to: 47-47
| "Allow replacing an existing prefab at the same path"] | None = None, | ||
| search_inactive: Annotated[bool, | ||
| "Include inactive objects when resolving the target name"] | None = None, | ||
| component_properties: Annotated[str, "Component properties in JSON format"] | None = None, |
There was a problem hiding this comment.
Critical: component_properties parameter is unused.
The component_properties parameter is defined in the signature but never added to the params dictionary. This breaks the API contract—callers can pass this parameter expecting it to be sent to Unity, but it will be silently ignored.
Apply this diff to forward the parameter to Unity:
if search_inactive is not None:
params["searchInactive"] = bool(search_inactive)
+ if component_properties is not None:
+ params["componentProperties"] = component_properties
response = send_with_unity_instance(send_command_with_retry, unity_instance, "manage_prefabs", params)Also applies to: 33-46
🧰 Tools
🪛 Ruff (0.14.3)
27-27: Unused function argument: component_properties
(ARG001)
🤖 Prompt for AI Agents
In MCPForUnity/UnityMcpServer~/src/tools/manage_prefabs.py around lines 27 and
also in the similar block at lines 33-46, the component_properties parameter is
accepted but never added to the params sent to Unity; update both places to
include component_properties in the params dictionary when it is not None (e.g.,
params["componentProperties"] = component_properties) so the JSON string passed
by callers is forwarded to Unity; keep the existing key naming consistent with
the API and do not alter other params.
| # Get active instance from session state | ||
| # Removed session_state import |
There was a problem hiding this comment.
Remove outdated comments.
Line 29's comment is outdated (the instance comes from context, not session state), and line 30 is a development artifact that shouldn't remain in production code.
Apply this diff to remove the comments:
- # Get active instance from session state
- # Removed session_state import
unity_instance = get_unity_instance_from_context(ctx)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Get active instance from session state | |
| # Removed session_state import | |
| unity_instance = get_unity_instance_from_context(ctx) |
🤖 Prompt for AI Agents
In MCPForUnity/UnityMcpServer~/src/tools/manage_prefabs.py around lines 29-30,
remove the two outdated/development comment lines ("# Get active instance from
session state" and "# Removed session_state import") so the file no longer
contains misleading or artifact comments; ensure surrounding code remains
unchanged and run a quick lint to confirm no trailing whitespace or leftover
markers remain.
| * `script_apply_edits`: Structured C# method/class edits (insert/replace/delete) with safer boundaries. | ||
| * `validate_script`: Fast validation (basic/standard) to catch syntax/structure issues before/after writes. | ||
| * `run_test`: Runs a tests in the Unity Editor. | ||
| * `set_active_instance`: Routes subsequent tool calls to a specific Unity instance (when multiple are running). |
There was a problem hiding this comment.
Fix markdown list indentation issues to match linting standards.
The bullet points at lines 55 and 64 have extra indentation (2 spaces) that violates MD007. These should align with the other list items at the same nesting level.
Apply this diff to fix the indentation:
- * `set_active_instance`: Routes subsequent tool calls to a specific Unity instance (when multiple are running).
+* `set_active_instance`: Routes subsequent tool calls to a specific Unity instance (when multiple are running).- * `unity_instances`: Lists all running Unity Editor instances with their details (name, path, port, status).
+* `unity_instances`: Lists all running Unity Editor instances with their details (name, path, port, status).Also applies to: 64-64
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
55-55: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🤖 Prompt for AI Agents
In README.md around lines 55 and 64, two bullet list items have extra leading
spaces breaking MD007; remove the extra indentation so these bullets align with
other list items at the same nesting level (make their leading whitespace match
the surrounding list items) to satisfy the markdown linting rule.
| ``` | ||
| User: "List all Unity instances" | ||
| LLM: [Shows ProjectA@abc123 and ProjectB@def456] | ||
| User: "Set active instance to ProjectA@abc123" | ||
| LLM: [Calls set_active_instance("ProjectA@abc123")] | ||
| User: "Create a red cube" | ||
| LLM: [Creates cube in ProjectA] | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to code block.
The code block starting at line 293 is missing a language identifier. Add text to clarify the example format and comply with MD040.
Apply this diff:
-```
+```text
User: "List all Unity instances"
LLM: [Shows ProjectA@abc123 and ProjectB@def456]
User: "Set active instance to ProjectA@abc123"
LLM: [Calls set_active_instance("ProjectA@abc123")]
User: "Create a red cube"
LLM: [Creates cube in ProjectA]
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
293-293: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
README.md around lines 293 to 302: the fenced code block lacks a language
specifier which triggers MD040; update the opening fence to include the "text"
language identifier (i.e., replace the opening ``` with ```text) so the block
reads as a text code block and ensure the closing fence remains ``` to apply the
change.
| await ctx.info(f"Processing run_tests: mode={mode}") | ||
| mode: Annotated[Literal["edit", "play"], "Unity test mode to run"] = "edit", | ||
| timeout_seconds: Annotated[int | str | None, "Optional timeout in seconds for the Unity test run (string, e.g. '30')"] = None, | ||
| ) -> dict[str, Any]: |
There was a problem hiding this comment.
Return type annotation mismatch.
The function signature declares -> dict[str, Any] but Line 77 returns RunTestsResponse(**response), which is a Pydantic model instance, not a dict. This creates a type inconsistency.
Apply this diff to align the return type:
-) -> dict[str, Any]:
+) -> dict[str, Any] | RunTestsResponse:Or, if you prefer to always return dicts, convert the model to a dict:
- return RunTestsResponse(**response) if isinstance(response, dict) else response
+ return RunTestsResponse(**response).model_dump() if isinstance(response, dict) else responseAlso applies to: 77-77
🤖 Prompt for AI Agents
In Server/tools/run_tests.py around lines 49 and 77, the function is annotated
to return dict[str, Any] but actually returns a Pydantic model instance
(RunTestsResponse(**response)) on line 77; update the function signature to
return RunTestsResponse (and add the necessary import for RunTestsResponse) so
the annotation matches the actual return type, or alternatively change the
return to RunTestsResponse(**response).dict() if you prefer to return a
dict—pick one approach and make the corresponding import/signature or .dict()
conversion consistent.
- Remove incorrect port safety check that treated reclaimed ports as errors (GetPortWithFallback may legitimately return same port if it became available) - Fix timezone-aware vs naive datetime mixing in unity_connection.py sorting (use timestamp() for comparison to avoid TypeError) - Normalize all datetime comparisons in port_discovery.py to UTC (file_mtime and last_heartbeat now consistently timezone-aware) - Add missing send_with_unity_instance import in Server/tools/manage_script.py (was causing NameError at runtime on lines 108 and 488) All 88 tests pass (76 passed + 5 skipped + 7 xpassed)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Server/tools/manage_script.py (1)
465-466: Instance routing correctly implemented for manage_script.Context extraction, logging, and the Unity command call all follow the established pattern using the
send_with_unity_instancewrapper.Optional: Consider consistent calling pattern.
The file uses both wrapped (
send_with_unity_instance) and direct (instance_id=unity_instance) patterns for Unity calls. Both are functionally correct, but using one approach consistently throughout would improve readability. This is purely a style consideration and doesn't affect correctness.Also applies to: 488-493
MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
238-267: Consider more specific exception handling.The bare
except Exceptionon line 256 could mask unexpected errors during status file reading. Consider catching specific exceptions (e.g.,FileNotFoundError,json.JSONDecodeError,OSError) to avoid hiding programming errors while still providing defensive behavior.Apply this diff to improve exception handling:
- except Exception: + except (FileNotFoundError, json.JSONDecodeError, OSError, PermissionError) as e: + logger.debug(f"Could not read status file: {e}") return None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
MCPForUnity/Editor/MCPForUnityBridge.cs(3 hunks)MCPForUnity/UnityMcpServer~/src/unity_connection.py(8 hunks)Server/port_discovery.py(3 hunks)Server/tools/manage_script.py(12 hunks)Server/unity_connection.py(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- MCPForUnity/Editor/MCPForUnityBridge.cs
🧰 Additional context used
🧬 Code graph analysis (4)
Server/port_discovery.py (1)
Server/models.py (1)
UnityInstanceInfo(13-35)
Server/unity_connection.py (2)
Server/port_discovery.py (3)
PortDiscovery(29-312)discover_unity_port(132-176)discover_all_unity_instances(224-312)Server/models.py (2)
MCPResponse(6-10)UnityInstanceInfo(13-35)
MCPForUnity/UnityMcpServer~/src/unity_connection.py (2)
Server/port_discovery.py (3)
PortDiscovery(29-312)discover_unity_port(132-176)discover_all_unity_instances(224-312)MCPForUnity/UnityMcpServer~/src/models.py (1)
UnityInstanceInfo(13-35)
Server/tools/manage_script.py (2)
Server/tools/__init__.py (2)
get_unity_instance_from_context(64-80)send_with_unity_instance(83-93)Server/unity_connection.py (1)
send_command_with_retry(673-710)
🪛 Ruff (0.14.3)
Server/port_discovery.py
106-106: Consider moving this statement to an else block
(TRY300)
107-107: Do not catch blind exception: Exception
(BLE001)
110-110: Do not catch blind exception: Exception
(BLE001)
219-219: Consider moving this statement to an else block
(TRY300)
220-220: Do not catch blind exception: Exception
(BLE001)
267-268: try-except-pass detected, consider logging the exception
(S110)
267-267: Do not catch blind exception: Exception
(BLE001)
305-305: Do not catch blind exception: Exception
(BLE001)
Server/unity_connection.py
256-256: Do not catch blind exception: Exception
(BLE001)
366-368: Abstract raise to an inner function
(TRY301)
366-368: Avoid specifying long messages outside the exception class
(TRY003)
476-478: Avoid specifying long messages outside the exception class
(TRY003)
518-522: Avoid specifying long messages outside the exception class
(TRY003)
529-531: Avoid specifying long messages outside the exception class
(TRY003)
561-565: Avoid specifying long messages outside the exception class
(TRY003)
593-596: Avoid specifying long messages outside the exception class
(TRY003)
MCPForUnity/UnityMcpServer~/src/unity_connection.py
256-256: Do not catch blind exception: Exception
(BLE001)
366-368: Abstract raise to an inner function
(TRY301)
366-368: Avoid specifying long messages outside the exception class
(TRY003)
476-478: Avoid specifying long messages outside the exception class
(TRY003)
518-522: Avoid specifying long messages outside the exception class
(TRY003)
529-531: Avoid specifying long messages outside the exception class
(TRY003)
561-565: Avoid specifying long messages outside the exception class
(TRY003)
593-596: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (33)
Server/tools/manage_script.py (8)
9-9: Import issue resolved - LGTM!The missing
send_with_unity_instanceimport flagged in the previous review has been correctly added.
90-91: Instance routing correctly implemented.The context extraction and logging follow the established pattern for multi-instance support.
108-117: Wrapped Unity call correctly structured.The
send_with_unity_instancewrapper properly routes the read command through the per-context Unity instance.
314-314: Direct instance_id parameter correctly passed.The call properly includes the
instance_idparameter for instance-aware routing.
371-372: Instance routing correctly implemented for create_script.Context extraction, logging, and the Unity command call all follow the established pattern.
Also applies to: 398-398
408-409: Instance routing correctly implemented for delete_script.Context extraction, logging, and the Unity command call all follow the established pattern.
Also applies to: 414-414
427-428: Instance routing correctly implemented for validate_script.Context extraction, logging, and the Unity command call all follow the established pattern.
Also applies to: 440-440
557-558: Instance routing correctly implemented for get_sha.Context extraction, logging, and the Unity command call all follow the established pattern.
Also applies to: 562-562
Server/unity_connection.py (11)
7-7: LGTM: Import additions support multi-instance features.The new imports (
os,List,UnityInstanceInfo) are all utilized correctly throughout the file for environment-based defaults, type annotations, and instance discovery.Also applies to: 15-15, 17-17
41-41: LGTM: Instance identifier field enables targeted reconnection.The
instance_idfield allows each connection to track its target instance, supporting instance-specific port rediscovery and status monitoring during retries.
238-267: LGTM: Instance-aware status file reading with graceful fallback.The updated logic correctly filters status files by instance hash when available, falling back to the most recent file when not. The hash extraction from
instance_id(splitting on '@') is clean. The bare exception catch at line 256 is acceptable here as a defensive fallback to prevent status read failures from crashing the retry loop.
349-377: LGTM: Instance-specific reconnection with appropriate failure behavior.The instance-aware port rediscovery correctly prioritizes the specified instance. Raising
ConnectionErrorwhen a user-specified instance can't be rediscovered (lines 366-368) is the right behavior—it prevents silently connecting to the wrong instance. The helpful error message guides users to check their instance identifier.
488-494: Fixed: Timezone-aware heartbeat sorting now uses numeric timestamps.The previous review concern about mixing naive and aware datetimes has been correctly addressed. The sort key now uses
.timestamp()which returns a float, avoiding any timezone comparison issues. Instances with no heartbeat safely fall back to0.0.
415-459: LGTM: Well-designed connection pool with intelligent caching.The pool architecture is solid:
- Thread-safe operations with
_pool_lock- 5-second instance cache reduces excessive scanning (configurable via
force_refresh)- Environment variable
UNITY_MCP_DEFAULT_INSTANCEfor default selection is user-friendly- Clean separation of concerns between discovery and connection management
461-565: LGTM: Comprehensive instance resolution with helpful error messages.The
_resolve_instance_idmethod thoroughly handles multiple identifier formats:
- Exact ID match
- Project name (with duplicate detection and clear guidance)
- Hash (including prefix matching)
- Composite format (Name@Hash or Name@Port)
- Port number
- Full path
The error messages are contextual and guide users to the correct format when resolution fails or is ambiguous. The fallback to most-recent-heartbeat instance when no identifier is provided is sensible.
567-607: LGTM: Efficient connection reuse with port tracking.The connection management correctly:
- Creates new connections on first access
- Reuses existing connections by instance ID
- Updates cached port when instances report new ports (lines 602-604)
- Logs connection state changes for observability
Note: Existing connections with
sock=Nonewill automatically reconnect on the nextsend_commandcall due to the built-in retry logic, so explicit validation here is unnecessary.
609-618: LGTM: Clean shutdown with exception isolation.The
disconnect_allmethod properly isolates exceptions so that one failing disconnection doesn't prevent cleanup of other connections. The lock ensures thread-safe clearing of the connection dictionary.
621-657: LGTM: Thread-safe pool initialization with backwards compatibility.The global pool accessor uses proper double-checked locking for efficient thread-safe initialization. The
get_unity_connectionfunction maintains backwards compatibility while seamlessly delegating to the pool—existing code continues to work while gaining multi-instance support.
673-710: LGTM: Retry helpers extended for multi-instance routing.Both synchronous and asynchronous retry helpers correctly accept the optional
instance_idparameter and delegate to the appropriate connection. The change is minimal, backwards-compatible, and maintains the existing reload-aware retry behavior.Also applies to: 713-745
MCPForUnity/UnityMcpServer~/src/unity_connection.py (10)
7-7: LGTM: Foundation for multi-instance support.The import additions and
instance_idfield properly enable tracking and managing connections to specific Unity instances for reconnection and routing.Also applies to: 15-15, 17-17, 41-41
349-377: LGTM: Instance-aware reconnection logic.The reconnection flow correctly prioritizes instance-specific rediscovery when
instance_idis set, maintaining session affinity. The fallback to generic discovery preserves backward compatibility for connections without instance tracking.
415-430: LGTM: Well-designed pool initialization.The connection pool properly initializes caching infrastructure and supports configuring a default instance via environment variable, which provides good operational flexibility.
432-459: LGTM: Efficient instance discovery with appropriate caching.The 5-second cache interval balances discovery overhead with freshness, and the
force_refreshoption enables explicit cache invalidation during reconnection scenarios.
486-494: Confirmed: Timezone comparison issue resolved.The past review concern about mixing timezone-aware and naive datetimes has been correctly addressed. Using
.timestamp()to convert to numeric values (with0.0fallback) ensures all comparisons are float-to-float, eliminating theTypeErrorfrom mixed datetime types.
461-565: LGTM: Comprehensive instance resolution with excellent error messages.The cascading resolution logic supports multiple identifier formats (exact ID, name, hash, composite name@hash/port, port number, path), and the error messages provide actionable guidance when resolution fails. The handling of ambiguous name matches (lines 507-522) is particularly helpful for users.
567-607: LGTM: Sound connection caching and lifecycle management.The connection caching correctly:
- Only stores successfully connected instances
- Updates cached connection metadata (instance_id, port) to handle port changes
- Raises clear errors on connection failures
The pool lock held during connection ensures atomic cache updates and is acceptable given connections should complete quickly.
609-619: LGTM: Proper cleanup with error resilience.The cleanup method correctly disconnects all connections and clears the cache, while preventing individual disconnection failures from blocking cleanup of remaining connections.
621-656: LGTM: Clean migration path with backward compatibility.The global pool accessor uses proper double-checked locking for thread-safe lazy initialization, and
get_unity_connection()maintains the existing API while delegating to the pool internally, ensuring smooth migration for existing callers.
673-743: LGTM: Retry helpers properly support instance routing.The retry helpers correctly accept and thread
instance_idthrough to the connection pool, enabling per-session instance routing while maintaining the existing reload-retry behavior.Server/port_discovery.py (4)
17-24: LGTM! Imports align with new functionality.The added imports (
os,struct,timezone,Dict,UnityInstanceInfo) are all utilized in the framed protocol implementation and instance discovery features.
62-112: Well-structured framed protocol implementation with backward compatibility.The implementation correctly handles Unity's framed protocol (8-byte big-endian length header + payload) and gracefully falls back to legacy mode. The
_recv_exacthelper ensures complete message reception, and the 10000-byte sanity check prevents runaway allocations. Debug logging provides appropriate visibility into probe failures.
200-221: LGTM! Robust path parsing with appropriate fallback.The method correctly extracts the project name by stripping trailing slashes and the "Assets" directory, then using
os.path.basenameto get the final directory name. The fallback to "Unknown" ensures the function always returns a valid string.
241-241: Excellent! UTC normalization resolves the previous timezone comparison issue.The implementation now correctly normalizes all timestamps to UTC:
file_mtimeusestz=timezone.utc(line 241)- Naive parsed datetimes are assigned UTC timezone (lines 263-264)
- Aware parsed datetimes are converted to UTC (lines 265-266)
This ensures that the comparison at line 282 (
existing_time >= freshness) and the sort at line 309 will never throwTypeErrordue to mixing naive and aware datetimes. Based on learningsAlso applies to: 261-268
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Log heartbeat parse failures at debug level.
The silent except ... pass hides parse errors that could indicate malformed timestamps or data corruption in status files. Adding debug logging aids troubleshooting without cluttering normal operations.
Apply this diff to add debug logging:
except Exception:
+ logger.debug(f"Failed to parse last_heartbeat '{heartbeat_str}' in {status_path.name}")
pass📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception: | |
| pass | |
| except Exception: | |
| logger.debug(f"Failed to parse last_heartbeat '{heartbeat_str}' in {status_path.name}") | |
| pass |
🧰 Tools
🪛 Ruff (0.14.3)
267-268: try-except-pass detected, consider logging the exception
(S110)
267-267: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In Server/port_discovery.py around lines 267-268, the bare "except Exception:
pass" swallows parse failures; replace it with a debug-level log that records
the exception and contextual data (e.g., the offending timestamp or file
contents) while preserving the non-failing behavior. Use the module logger to
call logger.debug with a clear message and exception info (exc_info=True or
include the exception string) so parse errors are recorded for troubleshooting
without raising.
Summary by CodeRabbit
New Features
Improvements
Tests