fix(server): stop silent cross-instance routing when multiple editors are connected (#1023)#1236
Open
Scriptwonder wants to merge 3 commits into
Open
Conversation
…ion state (CoplayDev#1023) UnityInstanceMiddleware previously kept its own dict (`_active_by_key`) keyed by `client_id | user:{user_id} | "global"`. The MCP protocol's identity is session-based, not client-based: - Streamable HTTP uses `Mcp-Session-Id` per the 2025-11-25 spec - stdio is intrinsically 1:1 (one subprocess = one session) - `client_id` is the peer-declared Implementation name, which is not unique — two Cursor instances both report `"cursor"` and collide The `"global"` fallback compounded this: anonymous clients all collapsed onto one record, so client A's `set_active_instance` retargeted client B (CoplayDev#1023). FastMCP already exposes session-isolated state via `ctx.set_state` / `ctx.get_state`, prefixed by `ctx.session_id` (verified in fastmcp 3.x `server/context.py:1181-1238` — reads `mcp-session-id` header on HTTP, generates a per-session UUID on stdio, caches on the session object). Switching to it removes a whole concurrency surface and closes the bug class by construction. Changes: - Replace `_active_by_key + _lock + get_session_key` with two-liner set/get/clear_active_instance backed by `ctx.set_state` under `mcpforunity.active_instance`. - `set_active_instance` tool now returns `session_id` (the real key) instead of the bogus `session_key` field. - `debug_request_context` drops `derived_key` and `all_keys_in_store` (no global dict to enumerate; per-session reads still report `active_instance` and the real `session_id`). - Drop characterization/integration tests that pinned the removed `client_id → key → "global"` behaviour. Add a regression test that proves two ctxs with private state dicts can't read each other's active instance. No protocol or wire-format change. The behavior shift is invisible to single-client setups and removes the cross-client leak for multi-client HTTP deployments.
…nnected (CoplayDev#1023) The stdio connection pool's _resolve_instance_id silently returned the most-recently-heartbeated editor when no unity_instance and no default were supplied, letting an unbound session retarget another project's Unity. Mirror the HTTP "multiple connected, no active set" guard: select the sole instance when there is exactly one, otherwise raise ConnectionError listing the available ids and requiring an explicit selection. Pairs with the cherry-picked middleware fix that keys active-instance state by ctx.session_id instead of the peer-supplied client_id.
Contributor
📝 WalkthroughWalkthroughThis PR moves active Unity instance storage to FastMCP session state, updates tool responses to return session_id and active-instance data, adds structured available-instance details to selection errors, and changes stdio resolution to fail on ambiguous multi-instance cases. Related integration and characterization tests were updated. ChangesSession-state persistence and selection flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Server/tests/test_transport_characterization.py (1)
143-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSession isolation and repeated-update coverage matches the new middleware contract.
These tests correctly reflect the FastMCP session-scoped
set_state/get_statebehavior described inunity_instance_middleware.py. One gap: the removedtest_middleware_thread_safe_updatesisn't replaced with an equivalent concurrent-write test for the new implementation —test_middleware_repeated_updates_settle_to_latestonly checks sequential writes within one session, not concurrent writes racing on the same session's state. Since FastMCP's session state store is now responsible for concurrency safety, this may be an acceptable trade-off, but worth confirming there's no remaining assumption in the middleware layer that requires local synchronization for concurrent writes to the same session.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Server/tests/test_transport_characterization.py` around lines 143 - 186, The middleware tests now cover session isolation and sequential overwrite behavior, but they no longer verify concurrent writes against the same session. Add a concurrency-focused test alongside test_middleware_isolates_multiple_sessions and test_middleware_repeated_updates_settle_to_latest that exercises UnityInstanceMiddleware.set_active_instance/get_active_instance under racing updates to one ctx, so the FastMCP session-state contract is covered without relying on any local synchronization assumptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Server/tests/test_transport_characterization.py`:
- Around line 143-186: The middleware tests now cover session isolation and
sequential overwrite behavior, but they no longer verify concurrent writes
against the same session. Add a concurrency-focused test alongside
test_middleware_isolates_multiple_sessions and
test_middleware_repeated_updates_settle_to_latest that exercises
UnityInstanceMiddleware.set_active_instance/get_active_instance under racing
updates to one ctx, so the FastMCP session-state contract is covered without
relying on any local synchronization assumptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5fee2498-ab7d-49ee-ac44-cf3c89686f67
📒 Files selected for processing (8)
Server/src/services/tools/debug_request_context.pyServer/src/services/tools/set_active_instance.pyServer/src/transport/legacy/unity_connection.pyServer/src/transport/unity_instance_middleware.pyServer/tests/integration/test_instance_routing_comprehensive.pyServer/tests/integration/test_middleware_auth_integration.pyServer/tests/test_stdio_instance_resolution.pyServer/tests/test_transport_characterization.py
💤 Files with no reviewable changes (1)
- Server/tests/integration/test_middleware_auth_integration.py
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens multi-Unity-Editor routing on the Python server to prevent stdio callers and concurrent local clients from silently targeting the wrong Unity project when multiple editors are connected (issue #1023).
Changes:
- Stdio legacy routing now refuses to “guess” a target Unity instance when 2+ editors are connected and no instance is selected, instead raising an explicit selection-required error.
- UnityInstanceMiddleware persistence is moved to FastMCP session-scoped state (per
ctx.session_id) to prevent cross-client stomping of the active instance. - Tests are updated/added to lock in multi-instance refusal behavior and per-session isolation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Server/src/transport/legacy/unity_connection.py | Stops implicit “most-recent heartbeat” routing; raises on ambiguous multi-instance selection. |
| Server/src/transport/unity_instance_middleware.py | Persists active instance via FastMCP session state instead of a process-global dict keyed by client_id. |
| Server/src/services/tools/set_active_instance.py | Updates response payload to expose session_id and aligns storage with the new middleware persistence mechanism. |
| Server/src/services/tools/debug_request_context.py | Removes enumeration of global active-instance state and reports session-scoped active instance only. |
| Server/tests/test_stdio_instance_resolution.py | Adds regression tests for stdio instance resolution guard behavior in multi-instance scenarios. |
| Server/tests/test_transport_characterization.py | Updates characterization tests to validate session-state-based persistence and session isolation for #1023. |
| Server/tests/integration/test_instance_routing_comprehensive.py | Updates integration tests to use stateful context shims compatible with the new middleware persistence behavior. |
| Server/tests/integration/test_middleware_auth_integration.py | Removes tests tied to the old get_session_key behavior that no longer applies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+557
to
567
| # 2+ instances connected and nothing pinned. Refuse to guess — | ||
| # silently routing to the most-recently-heartbeated editor lets | ||
| # an unbound session retarget another project's Unity (#1023). | ||
| # Mirror the HTTP "multiple connected, no active set" guard. | ||
| available_ids = [inst.id for inst in instances] | ||
| raise ConnectionError( | ||
| "Multiple Unity instances are connected and none is selected. " | ||
| "Pass unity_instance on the call or use set_active_instance " | ||
| f"with one of: {available_ids}. " | ||
| "Read mcpforunity://instances for current sessions." | ||
| ) |
…, not retry Live-testing follow-ups from the two-client isolation pass: - The HTTP refusal only logged the available instances server-side while the stdio guard lists them inline — an agent hitting the HTTP path needed an extra mcpforunity://instances hop to act. InstanceSelectionRequiredError now carries available_instances structurally and appends them to the message, fetched from the registry at raise time (error path only). - The transport wrapper's blanket except turned the refusal into hint="retry", which is misleading: a blind retry fails identically. Selection errors now return hint="select_instance" with reason and available_instances in data, so clients can route straight to set_active_instance.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Server/src/transport/plugin_hub.py (1)
927-965: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd
available_instancesto the post-loop ambiguity raises.
retry_on_reload=Falseskips the wait loop, soServer/src/transport/plugin_hub.py:960-965still raisesInstanceSelectionRequiredErrorwithoutavailable_instances; HTTP callers then receive an empty instance list indata.available_instances.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Server/src/transport/plugin_hub.py` around lines 927 - 965, The post-loop ambiguity checks in the plugin session selection flow are still raising InstanceSelectionRequiredError without available_instances, which causes HTTP responses to lose the instance list. Update the final raise paths in the session lookup logic in plugin_hub.py so they pass available_instances=await _available_instance_ids(), matching the earlier ambiguity branches and preserving the list for callers even when retry_on_reload is false.
🧹 Nitpick comments (1)
Server/src/transport/plugin_hub.py (1)
918-925: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBlind
except Exceptionswallows unexpected registry errors silently.Flagged by Ruff (BLE001). Since this is only used to enrich an already-erroring refusal path, at minimum log the exception so unexpected registry failures aren't invisible.
🔧 Proposed fix
async def _available_instance_ids() -> list[str]: # Error path only; one extra registry read keeps the refusal actionable. try: sessions = await cls._registry.list_sessions(user_id=user_id) return sorted( f"{s.project_name}@{s.project_hash}" for s in sessions.values()) - except Exception: + except Exception: + logger.debug("Failed to compute available_instances for selection error", exc_info=True) return []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Server/src/transport/plugin_hub.py` around lines 918 - 925, The _available_instance_ids helper in plugin_hub.py is swallowing unexpected registry failures with a bare except Exception, making registry issues invisible. Update the _available_instance_ids function to catch the exception in a named variable and log it through the existing logger or refusal-path logging before returning an empty list, so cls._registry.list_sessions failures are still observable while preserving the fallback behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Server/src/transport/plugin_hub.py`:
- Around line 927-965: The post-loop ambiguity checks in the plugin session
selection flow are still raising InstanceSelectionRequiredError without
available_instances, which causes HTTP responses to lose the instance list.
Update the final raise paths in the session lookup logic in plugin_hub.py so
they pass available_instances=await _available_instance_ids(), matching the
earlier ambiguity branches and preserving the list for callers even when
retry_on_reload is false.
---
Nitpick comments:
In `@Server/src/transport/plugin_hub.py`:
- Around line 918-925: The _available_instance_ids helper in plugin_hub.py is
swallowing unexpected registry failures with a bare except Exception, making
registry issues invisible. Update the _available_instance_ids function to catch
the exception in a named variable and log it through the existing logger or
refusal-path logging before returning an empty list, so
cls._registry.list_sessions failures are still observable while preserving the
fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e36866d-496d-4a7b-a42e-5e74da197441
📒 Files selected for processing (3)
Server/src/transport/plugin_hub.pyServer/src/transport/unity_transport.pyServer/tests/test_transport_characterization.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the dangerous half of #1023 (agents writing into the wrong project when 2+ editors are open).
Problem
Two independent routing hazards on the Python server:
UnityConnectionPool._resolve_instance_idroutes to the "most recently active" instance by heartbeat — nondeterministic per call, so consecutive tool calls can land in different projects. The reporter had editor settings overwritten, submodules force-deleted, and source files overwritten in the wrong project. HTTP was already hardened to raiseInstanceSelectionRequiredErroron ambiguity; stdio (the default transport) never got the same guard.UnityInstanceMiddleware.get_session_keyfalls back to a shared"global"/client_idkey, so two clients on one server stomp each other'sset_active_instanceselection.Fix
Name@hashids (mirroring the HTTP hub's semantics) instead of silently picking by heartbeat. Single-instance auto-routing is unchanged.client_id | user:{user_id} | "global"only where session identity genuinely doesn't exist, so concurrent clients keep isolated selections.Verification
cd Server && uv run pytest tests/ -q→ 1305 passed, 3 skipped on this branch rebased onto current beta, including newtest_stdio_instance_resolution.py(multi-instance refusal matrix) and reworked instance-routing/characterization tests.Relationship to other open work
UNITY_MCP_DEFAULT_INSTANCE/set_active_instanceremain the explicit opt-ins for scripted multi-instance workflows.Summary by CodeRabbit
select_instancehint with available instance IDs.