Feature/session based instance routing #369
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
…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
- 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
- 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)
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughThis PR introduces multi-instance Unity Editor support by replacing the single global connection with a UnityConnectionPool that discovers and manages multiple instances. Port management now falls back to new ports if existing ones become unavailable. Tools and resources are refactored to route commands through a per-session middleware, enabling deterministic targeting of specific instances. New resources and tools expose instance discovery and selection capabilities. Material color property detection now auto-identifies appropriate properties. Status file tracking includes project metadata for improved instance management. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Server
participant Middleware as UnityInstanceMiddleware
participant Pool as UnityConnectionPool
participant Instance as UnityInstance
Client->>Server: Call tool (e.g., manage_asset)
Server->>Middleware: on_call_tool(ctx)
Middleware->>Middleware: _get_session_key(ctx)
Middleware->>Middleware: get_active_instance(session_key)
Middleware->>Server: inject "unity_instance" into ctx.state
Server->>Server: get_unity_instance_from_context(ctx)
Server->>Pool: get_connection(instance_id)
Pool->>Pool: _resolve_instance_id(instance_id)
Pool->>Instance: return UnityConnection(instance_id)
Server->>Instance: send_command_with_retry(cmd, params, instance_id)
Instance-->>Server: response
Server-->>Client: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (58)
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 |
Collaborator
Author
|
thank you @sakurachan for all the great work on this! and @msanatan for your help! |
This was referenced Nov 5, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
set_active_instancetool to route subsequent commands to a specific Unity instance.unity_instancesresource to list all running Unity Editor instances.Bug Fixes
Improvements