Replace session-global Unity instance selection with explicit routing#981
Open
CharlieHess wants to merge 1 commit intoCoplayDev:betafrom
Open
Replace session-global Unity instance selection with explicit routing#981CharlieHess wants to merge 1 commit intoCoplayDev:betafrom
CharlieHess wants to merge 1 commit intoCoplayDev:betafrom
Conversation
Contributor
Reviewer's GuideMakes Unity instance routing explicit and safer by adding per-call and per-endpoint instance selection, exposing unity_instance on Unity-backed tools/resources, introducing an instance-bound HTTP MCP endpoint, and demoting session-global set_active_instance to a compatibility fallback with adjusted behavior and messaging. Sequence diagram for per-call unity_instance routing on tools and resourcessequenceDiagram
actor Client
participant HttpApp as FastMCPHttpApp
participant Route as MCP_HTTP_Route
participant Middleware as UnityInstanceMiddleware
participant Ctx as MiddlewareContext
participant Tool as UnityBackedToolOrResource
participant PluginHub as PluginHub
participant Unity as UnityEditorInstance
Client->>HttpApp: Call tool/resource with unity_instance
HttpApp->>Route: Dispatch MCP request
Route->>Middleware: on_call / on_read_resource
Middleware->>Ctx: ctx = context.fastmcp_context
Middleware->>Middleware: _inject_user_id(ctx)
Middleware->>Middleware: _inject_bound_unity_instance(ctx)
Middleware->>Middleware: _inject_unity_instance(context, include_legacy_default=True, allow_autoselect=True)
activate Middleware
Middleware->>Middleware: requested = _extract_requested_instance(context)
alt unity_instance provided
Middleware->>Middleware: _resolve_instance_value(requested, ctx)
Middleware->>Ctx: set_state("unity_instance", active_instance)
else no unity_instance
Middleware->>Ctx: get_state("bound_unity_instance")
alt no bound_unity_instance
Middleware->>Middleware: get_active_instance(ctx)
Middleware->>Middleware: _maybe_autoselect_instance(ctx)
end
end
deactivate Middleware
Middleware->>Tool: call_next(context)
Tool->>Ctx: ctx.get_state("unity_instance")
alt unity_instance missing and multiple editors
Tool-->>Client: MCPResponse(error: selection required)
else unity_instance resolved
Tool->>PluginHub: Execute with unity_instance
PluginHub->>Unity: Perform operation
Unity-->>PluginHub: Result
PluginHub-->>Tool: Result
Tool-->>Client: MCPResponse(success)
end
Flow diagram for tool and resource registration with optional unity_instance parameterflowchart TB
subgraph Registry[Tool and Resource Registration]
direction TB
Func[Original tool/resource function]
CheckParam{Has unity_instance
parameter already?}
AddParam[add_optional_unity_instance_parameter
wraps function signature
and strips kwarg at call time]
AppendQuery[append_unity_instance_query_template
adds unity_instance to
resource URI template]
WrappedFunc[Registered function
with optional unity_instance]
UpdatedURI["Registered URI
with {?unity_instance}"]
end
Func --> CheckParam
CheckParam -->|Yes| WrappedFunc
CheckParam -->|No and tool| AddParam
CheckParam -->|No and resource| AddParam
AddParam --> WrappedFunc
WrappedFunc -->|Resource registration| AppendQuery
AppendQuery --> UpdatedURI
subgraph Runtime[Runtime behavior]
CallSite[Client calls tool/resource
with unity_instance]
UnityMW[UnityInstanceMiddleware
_extract_requested_instance]
Resolver[_resolve_instance_value]
end
CallSite --> WrappedFunc
WrappedFunc --> UnityMW
UnityMW --> Resolver
Resolver --> UnityMW
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughThis PR introduces per-call Unity instance targeting through optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware as UnityInstanceMiddleware
participant BoundResolver as Bound Instance<br/>Resolver
participant RequestResolver as Request Instance<br/>Resolver
participant LegacyResolver as Legacy<br/>Resolver
participant Tool as Tool/Resource<br/>Handler
Client->>Middleware: HTTP Request (with path, args, query)
Middleware->>BoundResolver: Extract bound_unity_instance<br/>from endpoint path
alt Bound Endpoint Present
BoundResolver-->>Middleware: bound_unity_instance=X
else No Bound Endpoint
BoundResolver-->>Middleware: bound_unity_instance=None
end
Middleware->>RequestResolver: Extract requested override<br/>from args or query
alt Requested Override Present
RequestResolver-->>Middleware: requested=Y
alt Both Bound & Requested
Middleware-->>Client: ValueError: Cannot override<br/>bound instance
else Only Requested
Middleware->>Tool: active_instance=Y
end
else No Requested Override
alt Bound Instance Present
Middleware->>Tool: active_instance=X
else No Bound Instance
Middleware->>LegacyResolver: Check legacy defaults<br/>(get_active_instance)
alt Legacy Default Available
LegacyResolver-->>Middleware: active_instance=Z
Middleware->>Tool: active_instance=Z
else No Default & Multiple Instances
Middleware-->>Client: ConnectionError: Multiple<br/>instances, pass explicitly
else No Default & Single Instance
Middleware->>Tool: active_instance=only
end
end
end
Tool->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The FastMCP compatibility helpers (
_get_http_request_for_bindingand_resolve_user_id_from_request) both implement similar try/fallback import logic; consider centralizing this into a shared utility so future FastMCP layout changes only need to be handled in one place. - In
append_unity_instance_query_template, the"unity_instance" in uricheck may skip adding the query parameter when that substring appears in the path rather than the query template; consider parsing the existing{?...}segment explicitly instead of relying on a broad substring match.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The FastMCP compatibility helpers (`_get_http_request_for_binding` and `_resolve_user_id_from_request`) both implement similar try/fallback import logic; consider centralizing this into a shared utility so future FastMCP layout changes only need to be handled in one place.
- In `append_unity_instance_query_template`, the `"unity_instance" in uri` check may skip adding the query parameter when that substring appears in the path rather than the query template; consider parsing the existing `{?...}` segment explicitly instead of relying on a broad substring match.
## Individual Comments
### Comment 1
<location path="Server/src/transport/unity_instance_middleware.py" line_range="51-58" />
<code_context>
_unity_instance_middleware = middleware
+def _get_http_request_for_binding():
+ """Resolve the current HTTP request using whichever FastMCP layout is installed."""
+ try:
+ from fastmcp.dependencies import get_http_request
+ except Exception:
+ try:
+ from fastmcp.server.dependencies import get_http_request
+ except Exception as exc: # pragma: no cover - import-layout compatibility
+ raise RuntimeError("FastMCP HTTP request dependency is unavailable") from exc
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider narrowing the broad exception handling around FastMCP imports to avoid masking unrelated errors.
Both `except Exception:` blocks will catch any error in the import path (e.g., `AttributeError`, `SyntaxError`), not just missing modules/attributes, which can hide real `fastmcp` issues. Prefer catching `ImportError` (and `AttributeError` if needed) for the first import, so only layout differences are handled while other problems surface. The second broad `except` can remain as a last-resort guard if you still want that behavior.
</issue_to_address>
### Comment 2
<location path="Server/src/transport/unity_instance_middleware.py" line_range="498-507" />
<code_context>
"""Filter MCP tool listing to the Unity-enabled set when session data is available."""
try:
- await self._inject_unity_instance(context)
+ await self._inject_user_id(context.fastmcp_context)
+ bound_instance = await self._inject_bound_unity_instance(
+ context.fastmcp_context,
+ )
except Exception as exc:
# Re-raise authentication errors so callers get a proper auth failure
if isinstance(exc, RuntimeError) and "authentication" in str(exc).lower():
raise
_diag.warning(
- "on_list_tools: _inject_unity_instance failed (%s: %s), continuing without instance",
+ "on_list_tools: user_id injection failed (%s: %s), continuing without instance",
type(exc).__name__, exc,
)
</code_context>
<issue_to_address>
**suggestion:** The log message in `on_list_tools` omits that `_inject_bound_unity_instance` can also fail, which may make debugging harder.
The `try` wraps both `_inject_user_id` and `_inject_bound_unity_instance`, but the warning only mentions user_id. If `_inject_bound_unity_instance` (or `_resolve_instance_value`) raises, the log will point to the wrong step. Please update the message to reflect both operations or to include which step failed.
</issue_to_address>
### Comment 3
<location path="Server/tests/test_transport_characterization.py" line_range="814-826" />
<code_context>
assert instance is None
+class TestLegacyUnityConnectionPoolSelection:
+ """Test stdio instance resolution when no explicit target is provided."""
+
+ def test_resolve_instance_id_requires_selection_when_multiple_instances_exist(self):
+ pool = UnityConnectionPool()
+ pool._default_instance_id = None
+ instances = [
+ SimpleNamespace(id="ProjectA@aaa111", name="ProjectA", hash="aaa111", path="/tmp/a", port=6401),
+ SimpleNamespace(id="ProjectB@bbb222", name="ProjectB", hash="bbb222", path="/tmp/b", port=6402),
+ ]
+
+ with pytest.raises(ConnectionError, match="Multiple Unity Editor instances are running"):
+ pool._resolve_instance_id(None, instances)
+
</code_context>
<issue_to_address>
**suggestion (testing):** Extend `TestLegacyUnityConnectionPoolSelection` to cover the single-instance auto-select behavior
The new test only covers the multi-instance error path. There’s also new logic for the single-instance case (`len(instances) == 1`), which logs and returns that instance. Please add a test that:
- Uses `instances` with exactly one item and `instance_identifier=None` while `_default_instance_id` is `None`.
- Asserts `_resolve_instance_id(None, instances)` returns that sole instance.
This will cover the new branch and guard against regressions to the previous “most recent instance” behavior.
```suggestion
class TestLegacyUnityConnectionPoolSelection:
"""Test stdio instance resolution when no explicit target is provided."""
def test_resolve_instance_id_requires_selection_when_multiple_instances_exist(self):
pool = UnityConnectionPool()
pool._default_instance_id = None
instances = [
SimpleNamespace(id="ProjectA@aaa111", name="ProjectA", hash="aaa111", path="/tmp/a", port=6401),
SimpleNamespace(id="ProjectB@bbb222", name="ProjectB", hash="bbb222", path="/tmp/b", port=6402),
]
with pytest.raises(ConnectionError, match="Multiple Unity Editor instances are running"):
pool._resolve_instance_id(None, instances)
def test_resolve_instance_id_auto_selects_single_instance(self):
pool = UnityConnectionPool()
pool._default_instance_id = None
instances = [
SimpleNamespace(id="ProjectA@aaa111", name="ProjectA", hash="aaa111", path="/tmp/a", port=6401),
]
selected = pool._resolve_instance_id(None, instances)
assert selected is instances[0]
assert selected.id == "ProjectA@aaa111"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+51
to
+58
| def _get_http_request_for_binding(): | ||
| """Resolve the current HTTP request using whichever FastMCP layout is installed.""" | ||
| try: | ||
| from fastmcp.dependencies import get_http_request | ||
| except Exception: | ||
| try: | ||
| from fastmcp.server.dependencies import get_http_request | ||
| except Exception as exc: # pragma: no cover - import-layout compatibility |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Consider narrowing the broad exception handling around FastMCP imports to avoid masking unrelated errors.
Both except Exception: blocks will catch any error in the import path (e.g., AttributeError, SyntaxError), not just missing modules/attributes, which can hide real fastmcp issues. Prefer catching ImportError (and AttributeError if needed) for the first import, so only layout differences are handled while other problems surface. The second broad except can remain as a last-resort guard if you still want that behavior.
Comment on lines
+498
to
+507
| await self._inject_user_id(context.fastmcp_context) | ||
| bound_instance = await self._inject_bound_unity_instance( | ||
| context.fastmcp_context, | ||
| ) | ||
| except Exception as exc: | ||
| # Re-raise authentication errors so callers get a proper auth failure | ||
| if isinstance(exc, RuntimeError) and "authentication" in str(exc).lower(): | ||
| raise | ||
| _diag.warning( | ||
| "on_list_tools: _inject_unity_instance failed (%s: %s), continuing without instance", | ||
| "on_list_tools: user_id injection failed (%s: %s), continuing without instance", |
Contributor
There was a problem hiding this comment.
suggestion: The log message in on_list_tools omits that _inject_bound_unity_instance can also fail, which may make debugging harder.
The try wraps both _inject_user_id and _inject_bound_unity_instance, but the warning only mentions user_id. If _inject_bound_unity_instance (or _resolve_instance_value) raises, the log will point to the wrong step. Please update the message to reflect both operations or to include which step failed.
Comment on lines
+814
to
+826
| class TestLegacyUnityConnectionPoolSelection: | ||
| """Test stdio instance resolution when no explicit target is provided.""" | ||
|
|
||
| def test_resolve_instance_id_requires_selection_when_multiple_instances_exist(self): | ||
| pool = UnityConnectionPool() | ||
| pool._default_instance_id = None | ||
| instances = [ | ||
| SimpleNamespace(id="ProjectA@aaa111", name="ProjectA", hash="aaa111", path="/tmp/a", port=6401), | ||
| SimpleNamespace(id="ProjectB@bbb222", name="ProjectB", hash="bbb222", path="/tmp/b", port=6402), | ||
| ] | ||
|
|
||
| with pytest.raises(ConnectionError, match="Multiple Unity Editor instances are running"): | ||
| pool._resolve_instance_id(None, instances) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Extend TestLegacyUnityConnectionPoolSelection to cover the single-instance auto-select behavior
The new test only covers the multi-instance error path. There’s also new logic for the single-instance case (len(instances) == 1), which logs and returns that instance. Please add a test that:
- Uses
instanceswith exactly one item andinstance_identifier=Nonewhile_default_instance_idisNone. - Asserts
_resolve_instance_id(None, instances)returns that sole instance.
This will cover the new branch and guard against regressions to the previous “most recent instance” behavior.
Suggested change
| class TestLegacyUnityConnectionPoolSelection: | |
| """Test stdio instance resolution when no explicit target is provided.""" | |
| def test_resolve_instance_id_requires_selection_when_multiple_instances_exist(self): | |
| pool = UnityConnectionPool() | |
| pool._default_instance_id = None | |
| instances = [ | |
| SimpleNamespace(id="ProjectA@aaa111", name="ProjectA", hash="aaa111", path="/tmp/a", port=6401), | |
| SimpleNamespace(id="ProjectB@bbb222", name="ProjectB", hash="bbb222", path="/tmp/b", port=6402), | |
| ] | |
| with pytest.raises(ConnectionError, match="Multiple Unity Editor instances are running"): | |
| pool._resolve_instance_id(None, instances) | |
| class TestLegacyUnityConnectionPoolSelection: | |
| """Test stdio instance resolution when no explicit target is provided.""" | |
| def test_resolve_instance_id_requires_selection_when_multiple_instances_exist(self): | |
| pool = UnityConnectionPool() | |
| pool._default_instance_id = None | |
| instances = [ | |
| SimpleNamespace(id="ProjectA@aaa111", name="ProjectA", hash="aaa111", path="/tmp/a", port=6401), | |
| SimpleNamespace(id="ProjectB@bbb222", name="ProjectB", hash="bbb222", path="/tmp/b", port=6402), | |
| ] | |
| with pytest.raises(ConnectionError, match="Multiple Unity Editor instances are running"): | |
| pool._resolve_instance_id(None, instances) | |
| def test_resolve_instance_id_auto_selects_single_instance(self): | |
| pool = UnityConnectionPool() | |
| pool._default_instance_id = None | |
| instances = [ | |
| SimpleNamespace(id="ProjectA@aaa111", name="ProjectA", hash="aaa111", path="/tmp/a", port=6401), | |
| ] | |
| selected = pool._resolve_instance_id(None, instances) | |
| assert selected is instances[0] | |
| assert selected.id == "ProjectA@aaa111" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Server/src/main.py (1)
959-967: Consider documenting thetimeout_graceful_shutdown=0choice.The immediate shutdown (
timeout_graceful_shutdown=0) may be intentional for a locally-run MCP server, but it means in-flight requests won't complete gracefully. If this is deliberate (e.g., to match stdio transport behavior or simplify Unity integration), a brief inline comment would clarify the intent for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/main.py` around lines 959 - 967, The uvicorn.run call in main.py sets timeout_graceful_shutdown=0 which forces immediate shutdown and prevents in-flight requests from completing; add a concise inline comment next to the uvicorn.run invocation (referencing create_http_app and the uvicorn.run call) stating the intentional rationale for using 0 (e.g., to match stdio transport behavior, simplify Unity integration, or avoid blocking on shutdown) and note any trade-offs so future maintainers understand the choice.Server/src/services/registry/resource_registry.py (1)
19-19: Document the newunity_targetparameter in the docstring.The new parameter is functional but undocumented. Consider adding it to the Args section for discoverability.
📝 Suggested docstring update
Args: name: Resource name (defaults to function name) description: Resource description + unity_target: Whether to expose unity_instance parameter for explicit routing. + When True (default), adds optional unity_instance kwarg and appends + {?unity_instance} to the URI template. Set to False for server-only + resources that don't interact with Unity instances. **kwargs: Additional arguments passed to `@mcp.resource`()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/registry/resource_registry.py` at line 19, Add an Args entry for the new parameter unity_target in the docstring of the function/method in resource_registry.py that declares unity_target: bool = True; describe its type (bool), default (True), and brief purpose (what enabling/disabling unity target does) and any side effects or expected values so callers can discover and use the parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Server/src/services/custom_tool_service.py`:
- Around line 375-377: The error message incorrectly tells callers to "Pass
unity_instance explicitly" even though global custom tools built via
self._mcp.tool(...) use signatures from _build_signature()/_build_annotations()
and do not accept unity_instance; either remove that suggestion and only
instruct callers to use set_active_instance (update the MCPResponse message in
the handler), or implement proper support: add a 'unity_instance' parameter to
the signature/annotations generation (_build_signature/_build_annotations) and
in the handler extract/pop unity_instance from kwargs before forwarding so
multi-editor routing works. Ensure you reference the MCPResponse-producing
handler and the _build_signature/_build_annotations code when making the change.
In `@Server/src/services/tools/set_active_instance.py`:
- Around line 26-27: The call to await get_state("bound_unity_instance") in
set_active_instance.py should be wrapped in a try/except to defensively handle
FastMCP missing-state exceptions: change the bound_instance assignment to call
get_state only if callable(get_state) and catch any exception from await
get_state("bound_unity_instance"), setting bound_instance = None on failure so
the instance resolution fallback can proceed; reference the existing pattern
used elsewhere (services/tools/__init__.py) and the get_state and bound_instance
symbols when locating where to add the try/except.
In `@Server/src/transport/legacy/unity_connection.py`:
- Around line 552-557: The branch that auto-selects when len(instances) == 1 is
using a potentially stale 5s-cached list; before returning instances[0] (and
logging via logger.info), refresh the discovery/source that populates instances
(i.e., call the same discovery/lookup method or explicit refresh used elsewhere)
and re-fetch the instances variable, then re-evaluate the length — only proceed
to logger.info and return instances[0] if the refreshed list still has length ==
1, otherwise fall through to the normal selection/ambiguity handling.
---
Nitpick comments:
In `@Server/src/main.py`:
- Around line 959-967: The uvicorn.run call in main.py sets
timeout_graceful_shutdown=0 which forces immediate shutdown and prevents
in-flight requests from completing; add a concise inline comment next to the
uvicorn.run invocation (referencing create_http_app and the uvicorn.run call)
stating the intentional rationale for using 0 (e.g., to match stdio transport
behavior, simplify Unity integration, or avoid blocking on shutdown) and note
any trade-offs so future maintainers understand the choice.
In `@Server/src/services/registry/resource_registry.py`:
- Line 19: Add an Args entry for the new parameter unity_target in the docstring
of the function/method in resource_registry.py that declares unity_target: bool
= True; describe its type (bool), default (True), and brief purpose (what
enabling/disabling unity target does) and any side effects or expected values so
callers can discover and use the parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f330dd17-15b2-4fa3-9b5b-b4fa043c7090
📒 Files selected for processing (21)
Server/src/main.pyServer/src/services/custom_tool_service.pyServer/src/services/registry/resource_registry.pyServer/src/services/registry/tool_registry.pyServer/src/services/registry/unity_targeting.pyServer/src/services/resources/custom_tools.pyServer/src/services/resources/gameobject.pyServer/src/services/resources/prefab.pyServer/src/services/resources/tool_groups.pyServer/src/services/resources/unity_instances.pyServer/src/services/tools/execute_custom_tool.pyServer/src/services/tools/set_active_instance.pyServer/src/transport/legacy/unity_connection.pyServer/src/transport/plugin_hub.pyServer/src/transport/unity_instance_middleware.pyServer/src/transport/unity_transport.pyServer/tests/integration/test_inline_unity_instance.pyServer/tests/test_main_http_routes.pyServer/tests/test_resource_registry_metadata.pyServer/tests/test_tool_registry_metadata.pyServer/tests/test_transport_characterization.py
Comment on lines
375
to
+377
| return MCPResponse( | ||
| success=False, | ||
| message="No active Unity instance. Call set_active_instance with Name@hash from mcpforunity://instances.", | ||
| message="No active Unity instance. Pass unity_instance explicitly or call set_active_instance with Name@hash from mcpforunity://instances.", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the custom_tool_service.py file
find . -name "custom_tool_service.py" -type fRepository: CoplayDev/unity-mcp
Length of output: 107
🏁 Script executed:
# Check the file size and structure
wc -l Server/src/services/custom_tool_service.pyRepository: CoplayDev/unity-mcp
Length of output: 109
🏁 Script executed:
# Examine the specific lines mentioned (375-377) plus surrounding context
sed -n '360,390p' Server/src/services/custom_tool_service.pyRepository: CoplayDev/unity-mcp
Length of output: 1473
🏁 Script executed:
# Find the _build_global_tool_handler method
rg -n "_build_global_tool_handler" Server/src/services/custom_tool_service.py -A 30Repository: CoplayDev/unity-mcp
Length of output: 2422
🏁 Script executed:
# Find the _build_signature and _build_annotations methods
rg -n "_build_signature|_build_annotations" Server/src/services/custom_tool_service.py -A 15Repository: CoplayDev/unity-mcp
Length of output: 1674
Error message misleadingly suggests passing unity_instance for global custom tools.
At lines 375-377, the handler tells users to "Pass unity_instance explicitly," but global custom tools registered via self._mcp.tool(...) have signatures built only from definition.parameters (via _build_signature() and _build_annotations()). The handler attempts to retrieve unity_instance from context, not from kwargs, so callers cannot actually provide it as a parameter. Either expose unity_instance in the signature and filter it from forwarded params (enabling multi-editor routing), or revise the message to only mention the set_active_instance workaround.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Server/src/services/custom_tool_service.py` around lines 375 - 377, The error
message incorrectly tells callers to "Pass unity_instance explicitly" even
though global custom tools built via self._mcp.tool(...) use signatures from
_build_signature()/_build_annotations() and do not accept unity_instance; either
remove that suggestion and only instruct callers to use set_active_instance
(update the MCPResponse message in the handler), or implement proper support:
add a 'unity_instance' parameter to the signature/annotations generation
(_build_signature/_build_annotations) and in the handler extract/pop
unity_instance from kwargs before forwarding so multi-editor routing works.
Ensure you reference the MCPResponse-producing handler and the
_build_signature/_build_annotations code when making the change.
Comment on lines
+26
to
+27
| get_state = getattr(ctx, "get_state", None) | ||
| bound_instance = await get_state("bound_unity_instance") if callable(get_state) else None |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n Server/src/services/tools/set_active_instance.py | head -40Repository: CoplayDev/unity-mcp
Length of output: 1833
🏁 Script executed:
rg "get_state" Server/src/services/tools/ -B 2 -A 2 -t pyRepository: CoplayDev/unity-mcp
Length of output: 1749
🏁 Script executed:
rg "Context.get_state\|ctx.get_state" --type py -B 2 -A 2 | head -100Repository: CoplayDev/unity-mcp
Length of output: 45
🏁 Script executed:
rg "@mcp_for_unity_tool" Server/src/services/tools/ -A 5 --type py | grep -E "(`@mcp_for_unity_tool`|group=)" | head -50Repository: CoplayDev/unity-mcp
Length of output: 3309
🏁 Script executed:
rg "group\s*=" Server/src/services/tools/ --type py -B 1Repository: CoplayDev/unity-mcp
Length of output: 3355
🏁 Script executed:
cat -n Server/src/services/tools/__init__.py | sed -n '1,40p'Repository: CoplayDev/unity-mcp
Length of output: 1788
Add defensive handling for bound_unity_instance state lookup.
Line 27 calls await get_state("bound_unity_instance") without catching exceptions. If FastMCP raises for missing state keys, normal unbound /mcp requests will fail before the instance resolution fallback can run. The codebase already demonstrates the defensive pattern in services/tools/__init__.py—wrap the call in try/except.
Suggested fix
get_state = getattr(ctx, "get_state", None)
- bound_instance = await get_state("bound_unity_instance") if callable(get_state) else None
+ try:
+ bound_instance = await get_state("bound_unity_instance") if callable(get_state) else None
+ except Exception:
+ bound_instance = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Server/src/services/tools/set_active_instance.py` around lines 26 - 27, The
call to await get_state("bound_unity_instance") in set_active_instance.py should
be wrapped in a try/except to defensively handle FastMCP missing-state
exceptions: change the bound_instance assignment to call get_state only if
callable(get_state) and catch any exception from await
get_state("bound_unity_instance"), setting bound_instance = None on failure so
the instance resolution fallback can proceed; reference the existing pattern
used elsewhere (services/tools/__init__.py) and the get_state and bound_instance
symbols when locating where to add the try/except.
Comment on lines
+552
to
+557
| if len(instances) == 1: | ||
| logger.info( | ||
| "No instance specified, auto-selecting sole instance: %s", | ||
| instances[0].id, | ||
| ) | ||
| return instances[0] |
Contributor
There was a problem hiding this comment.
Refresh discovery before auto-selecting the “sole” instance.
These instances come from a 5-second cache. If a second editor connects inside that window, len(instances) == 1 is stale and this branch still routes implicitly to the previously cached editor—the exact behavior this PR is trying to remove.
🔁 Proposed fix
else:
+ instances = self.discover_all_instances(force_refresh=True)
if len(instances) == 1:
logger.info(
"No instance specified, auto-selecting sole instance: %s",
instances[0].id,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Server/src/transport/legacy/unity_connection.py` around lines 552 - 557, The
branch that auto-selects when len(instances) == 1 is using a potentially stale
5s-cached list; before returning instances[0] (and logging via logger.info),
refresh the discovery/source that populates instances (i.e., call the same
discovery/lookup method or explicit refresh used elsewhere) and re-fetch the
instances variable, then re-evaluate the length — only proceed to logger.info
and return instances[0] if the refreshed list still has length == 1, otherwise
fall through to the normal selection/ambiguity handling.
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
set_active_instanceworks for simple single-editor flows, but it breaks down as soon as multiple Unity editors are connected at the same time. The active instance lives in mutable session state, which means one agent can retarget another agent's calls, and an unbound session can drift onto the wrong editor.The root problem is that instance selection is modeled as ambient state instead of request data. The server can resolve a target internally, but the MCP surface did not make that routing explicit, and the unbound HTTP endpoint had no safe way to pin a whole session to one editor.
This PR makes instance routing explicit. Unity-backed tools and resources now accept
unity_instance, HTTP clients can bind an entire session to/mcp/instance/{Name@hash}, andset_active_instanceis demoted to a compatibility fallback instead of the primary multi-editor pattern. The middleware also stops silently selecting a different editor when multiple instances are connected, which makes concurrent-editor workflows predictable and safe.The test coverage here focuses on the contract reviewers care about: the new tool/resource metadata, bound endpoint behavior, and the failure mode when multiple editors are available but no explicit target is provided.
Summary by Sourcery
Make Unity instance routing explicit and safe across HTTP and stdio transports, replacing implicit session-global selection with per-request and endpoint-bound targeting.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
Release Notes
New Features
/mcp/instance/{instance}for direct targeting of specific Unity instances via URL path.unity_instanceparameter for explicit instance selection in function calls as an alternative toset_active_instance.Behavior Changes
Documentation Resources