Add per-call unity_instance routing via tool arguments#772
Merged
dsarno merged 1 commit intoCoplayDev:betafrom Feb 18, 2026
Merged
Add per-call unity_instance routing via tool arguments#772dsarno merged 1 commit intoCoplayDev:betafrom
dsarno merged 1 commit intoCoplayDev:betafrom
Conversation
Contributor
Reviewer's GuideImplements per-call Unity instance routing via a new Sequence diagram for per-call unity_instance routing via middlewaresequenceDiagram
actor Client
participant MCPServer
participant UnityInstanceMiddleware
participant Discovery as InstanceDiscovery
participant ToolHandler
Client->>MCPServer: CallToolRequest manage_scene(action="get_active", unity_instance="6401")
MCPServer->>UnityInstanceMiddleware: _inject_unity_instance(context)
UnityInstanceMiddleware->>UnityInstanceMiddleware: Read context.message.arguments
UnityInstanceMiddleware->>UnityInstanceMiddleware: Pop unity_instance from arguments
UnityInstanceMiddleware->>UnityInstanceMiddleware: _resolve_instance_value("6401", ctx)
alt value is digit and transport is stdio
UnityInstanceMiddleware->>Discovery: _discover_instances(ctx)
Discovery-->>UnityInstanceMiddleware: [instances with id, hash, port]
UnityInstanceMiddleware->>UnityInstanceMiddleware: Match instance by port
UnityInstanceMiddleware-->>UnityInstanceMiddleware: Return resolved id Name@hash
else value is digit and transport is http
UnityInstanceMiddleware-->>UnityInstanceMiddleware: Raise ValueError (port targeting not supported)
else value is Name@hash or hash prefix
UnityInstanceMiddleware->>Discovery: _discover_instances(ctx)
Discovery-->>UnityInstanceMiddleware: [instances with id, hash]
UnityInstanceMiddleware->>UnityInstanceMiddleware: Match by exact id or hash prefix
UnityInstanceMiddleware-->>UnityInstanceMiddleware: Return resolved id Name@hash or error
end
UnityInstanceMiddleware->>UnityInstanceMiddleware: Set active_instance for this request only
UnityInstanceMiddleware->>UnityInstanceMiddleware: Fallback to get_active_instance(ctx) if none
UnityInstanceMiddleware->>UnityInstanceMiddleware: Fallback to _maybe_autoselect_instance(ctx) if still none
UnityInstanceMiddleware-->>MCPServer: Context with request-scoped active_instance
MCPServer->>ToolHandler: Invoke manage_scene with validated args
ToolHandler->>ToolHandler: Use ctx active_instance for Unity routing
ToolHandler-->>Client: Tool result
Updated class diagram for Unity instance routing and set_active_instanceclassDiagram
class UnityInstanceMiddleware {
-lock
-active_by_key
+get_session_key(ctx) str
+get_active_instance(ctx) str
+set_active_instance(ctx, instance_id str) None
+clear_active_instance(ctx) None
+_inject_unity_instance(context MiddlewareContext) None
+_maybe_autoselect_instance(ctx) str
+_discover_instances(ctx) list
+_resolve_instance_value(value str, ctx) str
}
class MiddlewareContext {
+message
+get_state(key str)
+set_state(key str, value)
}
class CallToolRequestMessage {
+arguments dict
}
class PluginHub {
+is_configured() bool
+get_sessions(user_id) PluginSessions
}
class PluginSessions {
+sessions dict
}
class UnityConnectionPool {
+discover_all_instances(force_refresh bool) list
}
class SetActiveInstanceTool {
+set_active_instance(ctx Context, instance str) dict~str, Any~
}
class Context {
+set_state(key str, value)
+get_state(key str)
}
class Config {
+transport_mode str
+http_remote_hosted bool
}
UnityInstanceMiddleware --> MiddlewareContext : uses
MiddlewareContext --> CallToolRequestMessage : has
UnityInstanceMiddleware --> PluginHub : discovers_http_instances
UnityInstanceMiddleware --> UnityConnectionPool : discovers_stdio_instances
UnityInstanceMiddleware --> Config : reads
SetActiveInstanceTool --> Context : uses
SetActiveInstanceTool --> UnityConnectionPool : discovers_stdio_instances
SetActiveInstanceTool --> UnityInstanceMiddleware : sets_active_instance
SetActiveInstanceTool --> Config : reads
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 Walkthrough🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The port/hash/Name@hash resolution logic is duplicated between
_resolve_instance_valuein the middleware and theset_active_instancetool; consider centralizing this resolution into a shared helper so behavior and error messages stay consistent over time. - The informative
logger.infomessages in_maybe_autoselect_instancelog all available instance IDs when multiple are present; if deployments can have many instances or IDs are sensitive, you may want to truncate or redact this list to keep logs manageable and privacy-safe.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The port/hash/Name@hash resolution logic is duplicated between `_resolve_instance_value` in the middleware and the `set_active_instance` tool; consider centralizing this resolution into a shared helper so behavior and error messages stay consistent over time.
- The informative `logger.info` messages in `_maybe_autoselect_instance` log all available instance IDs when multiple are present; if deployments can have many instances or IDs are sensitive, you may want to truncate or redact this list to keep logs manageable and privacy-safe.
## Individual Comments
### Comment 1
<location> `Server/tests/integration/test_inline_unity_instance.py:347-356` </location>
<code_context>
+def test_set_active_instance_port_stdio(monkeypatch):
</code_context>
<issue_to_address>
**suggestion (testing):** Add a failure-path test for set_active_instance when a port number does not match any instance
The new tests exercise the success and HTTP rejection cases, but not the failure path for an unknown port in stdio mode. Please add a test (similar to `test_port_number_not_found_raises`) that calls `set_active_instance` in stdio mode with a port not in the fake pool and asserts it returns `success: False` with the expected error message, including the `Available: ...` list. This will better lock in the user-facing error behavior and prevent regressions.
Suggested implementation:
```python
# ---------------------------------------------------------------------------
# set_active_instance tool: port number support
# ---------------------------------------------------------------------------
def test_set_active_instance_unknown_port_stdio(monkeypatch):
"""set_active_instance returns failure in stdio mode when the port is not in the instance pool."""
monkeypatch.setattr(config, "transport_mode", "stdio")
monkeypatch.setattr(config, "http_remote_hosted", False)
from types import SimpleNamespace
from transport.unity_instance_middleware import UnityInstanceMiddleware, set_unity_instance_middleware
mw = UnityInstanceMiddleware()
set_unity_instance_middleware(mw)
# Pool has only port 6401, we'll ask for 6402.
mw._unity_instance_pool = [
SimpleNamespace(id="Proj@abc123", hash="abc123", port=6401),
]
# The new tests for the success path should already demonstrate
# how set_active_instance is invoked; we mirror that here.
# If set_active_instance is a top-level helper instead of a method,
# adjust this call to match that helper.
result = asyncio.run(mw.set_active_instance("6402"))
assert result["success"] is False
# Lock in the user-visible error message including the available ports list.
assert "No Unity instance found listening on port 6402" in result["message"]
assert "Available: 6401" in result["message"]
def test_set_active_instance_port_stdio(monkeypatch):
```
You will likely need to make these small adjustments to align with the rest of your test file:
1. **Instance pool attribute name**: If the middleware uses a different attribute than `_unity_instance_pool` to store its pool (e.g. `unity_instance_pool` or similar), update that line accordingly.
2. **Invocation of `set_active_instance`**:
- If there is a module-level function (e.g. `from tools.inline_unity_instance import set_active_instance`) used by the existing tests, call that instead of `mw.set_active_instance`.
- Mirror exactly how `test_set_active_instance_port_stdio` invokes `set_active_instance` (async vs sync, parameter shape, etc.), and only change the port value to one that is *not* in the fake pool (e.g. `"6402"`).
3. **Imports**:
- If `SimpleNamespace` and/or `asyncio` are already imported at the top of the file, you can remove the local import of `SimpleNamespace` and the test will still work.
4. **Error text**:
- If your existing error message wording or capitalization for the unknown-port path is slightly different (or uses a different prefix before the `Available:` list), update the string assertions to match the actual message, keeping the explicit check that the `Available: ...` portion is present.
</issue_to_address>
### Comment 2
<location> `Server/tests/integration/test_inline_unity_instance.py:177-127` </location>
<code_context>
+# Port number resolution (stdio)
+# ---------------------------------------------------------------------------
+
+def test_port_number_resolves_to_name_hash_stdio(monkeypatch):
+ """Bare port number resolves to the matching Name@hash in stdio mode."""
+ instances = [
+ SimpleNamespace(id="Proj@abc123", hash="abc123", port=6401),
+ SimpleNamespace(id="Other@def456", hash="def456", port=6402),
+ ]
+ mw = _make_middleware(monkeypatch, transport="stdio", pool_instances=instances)
+
+ ctx = DummyContext()
+ ctx.client_id = "client-1"
+ mw_ctx = DummyMiddlewareContext(ctx, arguments={"unity_instance": "6401"})
+
+ asyncio.run(mw._inject_unity_instance(mw_ctx))
+
+ assert ctx.get_state("unity_instance") == "Proj@abc123"
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for non-string unity_instance values to exercise the str() casting path
Since `_inject_unity_instance` does `raw_str = str(raw).strip()`, it accepts non-string `unity_instance` values (e.g., integers). All current tests cover only string inputs. Please add a test where `unity_instance` is an `int` (e.g., `6401`) and confirm it behaves like the string case, plus one for a non-numeric type to verify the expected fallback/error behavior. This will lock in the implicit `str()` casting behavior for future refactors.
Suggested implementation:
```python
# ---------------------------------------------------------------------------
# Port number resolution (stdio)
# ---------------------------------------------------------------------------
def test_port_number_resolves_to_name_hash_stdio(monkeypatch):
"""Bare port number resolves to the matching Name@hash in stdio mode."""
instances = [
SimpleNamespace(id="Proj@abc123", hash="abc123", port=6401),
SimpleNamespace(id="Other@def456", hash="def456", port=6402),
]
mw = _make_middleware(monkeypatch, transport="stdio", pool_instances=instances)
ctx = DummyContext()
ctx.client_id = "client-1"
mw_ctx = DummyMiddlewareContext(ctx, arguments={"unity_instance": "6401"})
asyncio.run(mw._inject_unity_instance(mw_ctx))
assert ctx.get_state("unity_instance") == "Proj@abc123"
def test_port_number_resolves_to_name_hash_stdio_int(monkeypatch):
"""Int unity_instance is accepted and resolved via str() casting in stdio mode."""
instances = [
SimpleNamespace(id="Proj@abc123", hash="abc123", port=6401),
SimpleNamespace(id="Other@def456", hash="def456", port=6402),
]
mw = _make_middleware(monkeypatch, transport="stdio", pool_instances=instances)
ctx = DummyContext()
ctx.client_id = "client-1"
# Note: unity_instance passed as int, exercising str() casting path
mw_ctx = DummyMiddlewareContext(ctx, arguments={"unity_instance": 6401})
asyncio.run(mw._inject_unity_instance(mw_ctx))
assert ctx.get_state("unity_instance") == "Proj@abc123"
def test_non_string_unity_instance_non_numeric_stdio(monkeypatch):
"""Non-numeric, non-string unity_instance exercises the str() casting fallback path."""
instances = [
SimpleNamespace(id="Proj@abc123", hash="abc123", port=6401),
SimpleNamespace(id="Other@def456", hash="def456", port=6402),
]
mw = _make_middleware(monkeypatch, transport="stdio", pool_instances=instances)
class CustomUnityInstance:
def __str__(self) -> str:
# Use a value that will not resolve to any known instance to hit the fallback path
return "not-a-port-or-instance-id"
ctx = DummyContext()
ctx.client_id = "client-1"
mw_ctx = DummyMiddlewareContext(
ctx,
arguments={"unity_instance": CustomUnityInstance()},
)
# Depending on implementation, this may either:
# * Leave unity_instance unset, or
# * Raise a specific error when resolution fails.
# We assume resolution failure raises a ValueError; adjust if the implementation differs.
with pytest.raises(ValueError):
asyncio.run(mw._inject_unity_instance(mw_ctx))
# Ensure no unity_instance was set when resolution fails
assert ctx.get_state("unity_instance") is None
When a tool call includes unity_instance in its arguments, the middleware:
```
1. Ensure `pytest` is imported at the top of the file (e.g., `import pytest`) if it is not already present.
2. If `_inject_unity_instance` uses a different exception type on resolution failure, update `with pytest.raises(ValueError):` to the appropriate exception class to match the actual behavior.
3. If the real behavior on resolution failure is to *not* raise but to silently ignore/clear the value, remove the `pytest.raises(...)` context and instead assert the expected fallback behavior (e.g., `ctx.get_state("unity_instance") is None` and/or that no active instance changes).
</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
+347
to
+356
| def test_set_active_instance_port_stdio(monkeypatch): | ||
| """set_active_instance accepts a port number in stdio mode and resolves to Name@hash.""" | ||
| monkeypatch.setattr(config, "transport_mode", "stdio") | ||
| monkeypatch.setattr(config, "http_remote_hosted", False) | ||
|
|
||
| from transport.unity_instance_middleware import UnityInstanceMiddleware, set_unity_instance_middleware | ||
| mw = UnityInstanceMiddleware() | ||
| set_unity_instance_middleware(mw) | ||
|
|
||
| pool_instance = SimpleNamespace(id="Proj@abc123", hash="abc123", port=6401) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a failure-path test for set_active_instance when a port number does not match any instance
The new tests exercise the success and HTTP rejection cases, but not the failure path for an unknown port in stdio mode. Please add a test (similar to test_port_number_not_found_raises) that calls set_active_instance in stdio mode with a port not in the fake pool and asserts it returns success: False with the expected error message, including the Available: ... list. This will better lock in the user-facing error behavior and prevent regressions.
Suggested implementation:
# ---------------------------------------------------------------------------
# set_active_instance tool: port number support
# ---------------------------------------------------------------------------
def test_set_active_instance_unknown_port_stdio(monkeypatch):
"""set_active_instance returns failure in stdio mode when the port is not in the instance pool."""
monkeypatch.setattr(config, "transport_mode", "stdio")
monkeypatch.setattr(config, "http_remote_hosted", False)
from types import SimpleNamespace
from transport.unity_instance_middleware import UnityInstanceMiddleware, set_unity_instance_middleware
mw = UnityInstanceMiddleware()
set_unity_instance_middleware(mw)
# Pool has only port 6401, we'll ask for 6402.
mw._unity_instance_pool = [
SimpleNamespace(id="Proj@abc123", hash="abc123", port=6401),
]
# The new tests for the success path should already demonstrate
# how set_active_instance is invoked; we mirror that here.
# If set_active_instance is a top-level helper instead of a method,
# adjust this call to match that helper.
result = asyncio.run(mw.set_active_instance("6402"))
assert result["success"] is False
# Lock in the user-visible error message including the available ports list.
assert "No Unity instance found listening on port 6402" in result["message"]
assert "Available: 6401" in result["message"]
def test_set_active_instance_port_stdio(monkeypatch):You will likely need to make these small adjustments to align with the rest of your test file:
- Instance pool attribute name: If the middleware uses a different attribute than
_unity_instance_poolto store its pool (e.g.unity_instance_poolor similar), update that line accordingly. - Invocation of
set_active_instance:- If there is a module-level function (e.g.
from tools.inline_unity_instance import set_active_instance) used by the existing tests, call that instead ofmw.set_active_instance. - Mirror exactly how
test_set_active_instance_port_stdioinvokesset_active_instance(async vs sync, parameter shape, etc.), and only change the port value to one that is not in the fake pool (e.g."6402").
- If there is a module-level function (e.g.
- Imports:
- If
SimpleNamespaceand/orasyncioare already imported at the top of the file, you can remove the local import ofSimpleNamespaceand the test will still work.
- If
- Error text:
- If your existing error message wording or capitalization for the unknown-port path is slightly different (or uses a different prefix before the
Available:list), update the string assertions to match the actual message, keeping the explicit check that theAvailable: ...portion is present.
- If your existing error message wording or capitalization for the unknown-port path is slightly different (or uses a different prefix before the
|
|
||
| asyncio.run(mw._inject_unity_instance(mw_ctx)) | ||
|
|
||
| assert ctx.get_state("unity_instance") == "Proj@abc123" |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for non-string unity_instance values to exercise the str() casting path
Since _inject_unity_instance does raw_str = str(raw).strip(), it accepts non-string unity_instance values (e.g., integers). All current tests cover only string inputs. Please add a test where unity_instance is an int (e.g., 6401) and confirm it behaves like the string case, plus one for a non-numeric type to verify the expected fallback/error behavior. This will lock in the implicit str() casting behavior for future refactors.
Suggested implementation:
# ---------------------------------------------------------------------------
# Port number resolution (stdio)
# ---------------------------------------------------------------------------
def test_port_number_resolves_to_name_hash_stdio(monkeypatch):
"""Bare port number resolves to the matching Name@hash in stdio mode."""
instances = [
SimpleNamespace(id="Proj@abc123", hash="abc123", port=6401),
SimpleNamespace(id="Other@def456", hash="def456", port=6402),
]
mw = _make_middleware(monkeypatch, transport="stdio", pool_instances=instances)
ctx = DummyContext()
ctx.client_id = "client-1"
mw_ctx = DummyMiddlewareContext(ctx, arguments={"unity_instance": "6401"})
asyncio.run(mw._inject_unity_instance(mw_ctx))
assert ctx.get_state("unity_instance") == "Proj@abc123"
def test_port_number_resolves_to_name_hash_stdio_int(monkeypatch):
"""Int unity_instance is accepted and resolved via str() casting in stdio mode."""
instances = [
SimpleNamespace(id="Proj@abc123", hash="abc123", port=6401),
SimpleNamespace(id="Other@def456", hash="def456", port=6402),
]
mw = _make_middleware(monkeypatch, transport="stdio", pool_instances=instances)
ctx = DummyContext()
ctx.client_id = "client-1"
# Note: unity_instance passed as int, exercising str() casting path
mw_ctx = DummyMiddlewareContext(ctx, arguments={"unity_instance": 6401})
asyncio.run(mw._inject_unity_instance(mw_ctx))
assert ctx.get_state("unity_instance") == "Proj@abc123"
def test_non_string_unity_instance_non_numeric_stdio(monkeypatch):
"""Non-numeric, non-string unity_instance exercises the str() casting fallback path."""
instances = [
SimpleNamespace(id="Proj@abc123", hash="abc123", port=6401),
SimpleNamespace(id="Other@def456", hash="def456", port=6402),
]
mw = _make_middleware(monkeypatch, transport="stdio", pool_instances=instances)
class CustomUnityInstance:
def __str__(self) -> str:
# Use a value that will not resolve to any known instance to hit the fallback path
return "not-a-port-or-instance-id"
ctx = DummyContext()
ctx.client_id = "client-1"
mw_ctx = DummyMiddlewareContext(
ctx,
arguments={"unity_instance": CustomUnityInstance()},
)
# Depending on implementation, this may either:
# * Leave unity_instance unset, or
# * Raise a specific error when resolution fails.
# We assume resolution failure raises a ValueError; adjust if the implementation differs.
with pytest.raises(ValueError):
asyncio.run(mw._inject_unity_instance(mw_ctx))
# Ensure no unity_instance was set when resolution fails
assert ctx.get_state("unity_instance") is None
When a tool call includes unity_instance in its arguments, the middleware:- Ensure
pytestis imported at the top of the file (e.g.,import pytest) if it is not already present. - If
_inject_unity_instanceuses a different exception type on resolution failure, updatewith pytest.raises(ValueError):to the appropriate exception class to match the actual behavior. - If the real behavior on resolution failure is to not raise but to silently ignore/clear the value, remove the
pytest.raises(...)context and instead assert the expected fallback behavior (e.g.,ctx.get_state("unity_instance") is Noneand/or that no active instance changes).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Server/src/transport/unity_instance_middleware.py (1)
107-149:_discover_instancespartially duplicates the discovery logic in_maybe_autoselect_instance.Both methods query PluginHub sessions and fall back to the stdio pool with similar exception handling. Consider extracting the shared transport-aware discovery into
_discover_instancesand having_maybe_autoselect_instancecall it, which would reduce the duplication and ensure both paths stay in sync for future changes.Not blocking since the two methods serve different purposes (raw discovery vs. auto-select-and-persist).
Also applies to: 225-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/transport/unity_instance_middleware.py` around lines 107 - 149, The discovery logic is duplicated between _discover_instances and _maybe_autoselect_instance; refactor by extracting the transport-aware discovery (querying PluginHub via PluginHub.get_sessions with the same user_id/get_state lookup and the stdio fallback using get_unity_connection_pool() and pool.discover_all_instances(force_refresh=True), including the current exception handling for SystemExit/KeyboardInterrupt and logger.debug) into _discover_instances, and then have _maybe_autoselect_instance call _discover_instances to obtain the instances and continue its auto-select-and-persist behavior, ensuring transport selection (config.transport_mode or "stdio") and http_remote_hosted/user_id resolution are preserved.
🤖 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/transport/unity_instance_middleware.py`:
- Around line 151-223: The port-resolution path in _resolve_instance_value can
fail when PluginHub-provided sessions (from
PluginHub.is_configured/PluginHub.configure) are returned without a .port
attribute; update _resolve_instance_value so after calling
self._discover_instances(ctx) for a numeric value it checks whether any returned
instance has a port (e.g., using getattr(inst, "port", None)) and if none do,
call the pool-based discovery (e.g., re-invoke or call an alternate discovery
path in _discover_instances) to obtain instances with ports before attempting
the port lookup; ensure this fallback preserves the same error messages and uses
the same id/port extraction logic so port-based targeting works even when
PluginHub returns port-less sessions.
---
Nitpick comments:
In `@Server/src/transport/unity_instance_middleware.py`:
- Around line 107-149: The discovery logic is duplicated between
_discover_instances and _maybe_autoselect_instance; refactor by extracting the
transport-aware discovery (querying PluginHub via PluginHub.get_sessions with
the same user_id/get_state lookup and the stdio fallback using
get_unity_connection_pool() and pool.discover_all_instances(force_refresh=True),
including the current exception handling for SystemExit/KeyboardInterrupt and
logger.debug) into _discover_instances, and then have _maybe_autoselect_instance
call _discover_instances to obtain the instances and continue its
auto-select-and-persist behavior, ensuring transport selection
(config.transport_mode or "stdio") and http_remote_hosted/user_id resolution are
preserved.
whatevertogo
added a commit
to whatevertogo/unity-mcp
that referenced
this pull request
Feb 18, 2026
…ol toggle Merge PR CoplayDev#772 which adds per-call unity_instance routing support: - Middleware intercepts unity_instance from tool call arguments - Supports port number (stdio), Name@hash, and hash prefix resolution - Per-call routing does not persist to session state Conflicts resolved in unity_instance_middleware.py: - Kept stdio tool toggle methods (session tracking, notifications) - Integrated per-call routing before session fallback - Added section comments for code organization Co-Authored-By: dsarno <2432103+dsarno@users.noreply.github.com>
3 tasks
Any tool call can now include a unity_instance parameter to route that
specific call to a target Unity instance without changing the session
default and without requiring a set_active_instance call first.
The middleware pops unity_instance from tool call arguments before
Pydantic validation runs, resolves it (port number, hash prefix, or
Name@hash), and injects it into request-scoped state for that call only.
- Port numbers resolve to the matching Name@hash via status file lookup
rather than synthetic direct:{port} IDs, so the transport layer can
route them correctly
- HTTP mode rejects port-based targeting with a clear error
- set_active_instance now also accepts port numbers for consistency
- Multi-instance scenarios log available instances with ports when
auto-select cannot choose
- _discover_instances() helper DRYs up transport-aware instance
discovery previously duplicated across the codebase
- Server instructions updated to document both routing approaches
- 18 new tests covering pop behaviour, per-call vs session routing,
port resolution, transport modes, and edge cases
Closes CoplayDev#697
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dsarno
force-pushed
the
feature/inline-unity-instance-routing
branch
from
b87baaf to
0cb5a0d
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Server/src/services/tools/set_active_instance.py (1)
28-28:valueis computed identically on both Line 28 and Line 91.Line 91 recomputes
value = (instance or "").strip()which is guaranteed to yield the same result as Line 28 (we only reach Line 91 when the digit check at Line 29 was false, meaningvalueis already non-digit and non-empty or empty). Consider reusing the variable from Line 28 and removing the duplicate assignment at Line 91.Remove redundant assignment
- value = (instance or "").strip() - if not value: + if not value:Also applies to: 91-91
🤖 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` at line 28, Remove the duplicate computation of value in set_active_instance (the variable computed as (instance or "").strip()); keep the first assignment at the top (value = (instance or "").strip()) and remove the repeated assignment at the later branch (currently at Line 91), and update any subsequent logic to reuse that original value (e.g., references in the digit check and the later non-digit branch) so there is a single source of truth for value within the function.Server/tests/integration/test_inline_unity_instance.py (2)
32-72: Test helper is well-designed; module-patching approach is sound but fragile.The
_make_middlewarehelper usesmonkeypatch.delitem(sys.modules, ...)to force re-import of the middleware module with a patchedPluginHub. This is effective for test isolation but couples tests tightly to import-time behavior — if the middleware module ever cachesPluginHubat import time differently, these tests will silently break. Consider adding a brief comment at line 59 noting why the module is evicted (so the FakePluginHub is picked up on re-import).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/tests/integration/test_inline_unity_instance.py` around lines 32 - 72, Add a short inline comment in the _make_middleware helper next to the monkeypatch.delitem(sys.modules, "transport.unity_instance_middleware", raising=False) call explaining that the middleware module is intentionally evicted from sys.modules so that re-importing transport.unity_instance_middleware will pick up the patched transport.plugin_hub.PluginHub (FakePluginHub); reference the helper name _make_middleware and the FakePluginHub class in the comment so future readers understand this import-time dependency and test isolation intent.
347-374: Test isolation: global middleware singleton not cleaned up after this test.
set_unity_instance_middleware(mw)at Line 354 mutates the module-level global_unity_instance_middleware, butmonkeypatchonly reverts attribute/item patches — it won't undo the side-effect of callingset_unity_instance_middleware. If a later test in the same process callsget_unity_instance_middleware(), it may receive this test'smwinstance.Consider patching the global via
monkeypatch.setattron the module attribute instead:Patch via monkeypatch for automatic teardown
- from transport.unity_instance_middleware import UnityInstanceMiddleware, set_unity_instance_middleware + import transport.unity_instance_middleware as uim + from transport.unity_instance_middleware import UnityInstanceMiddleware mw = UnityInstanceMiddleware() - set_unity_instance_middleware(mw) + monkeypatch.setattr(uim, "_unity_instance_middleware", mw)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/tests/integration/test_inline_unity_instance.py` around lines 347 - 374, The test currently calls set_unity_instance_middleware(mw) which mutates the module-level singleton _unity_instance_middleware without automatic teardown; instead, use monkeypatch to set the module attribute so it will be reverted after the test: replace the direct call to set_unity_instance_middleware with monkeypatch.setattr on the transport.unity_instance_middleware module's _unity_instance_middleware (or on get_unity_instance_middleware/set_unity_instance_middleware symbols) to inject mw, ensuring the global singleton is restored by monkeypatch; reference symbols: set_unity_instance_middleware, get_unity_instance_middleware, _unity_instance_middleware, UnityInstanceMiddleware.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Server/src/transport/unity_instance_middleware.py`:
- Around line 107-149: The PluginHub branch in _discover_instances builds
SimpleNamespace objects (constructed at the PluginHub.get_sessions loop) without
a .port attribute while the pool.discover_all_instances results include port
info, causing downstream port-based lookups to fail when PluginHub returns
sessions; fix by ensuring the SimpleNamespace created in the PluginHub loop
includes a .port attribute (set to the real port if available on session_info,
otherwise set to None or a sentinel) so the objects returned by
_discover_instances have a consistent shape for later port resolution and
matching with pool.discover_all_instances.
---
Nitpick comments:
In `@Server/src/services/tools/set_active_instance.py`:
- Line 28: Remove the duplicate computation of value in set_active_instance (the
variable computed as (instance or "").strip()); keep the first assignment at the
top (value = (instance or "").strip()) and remove the repeated assignment at the
later branch (currently at Line 91), and update any subsequent logic to reuse
that original value (e.g., references in the digit check and the later non-digit
branch) so there is a single source of truth for value within the function.
In `@Server/tests/integration/test_inline_unity_instance.py`:
- Around line 32-72: Add a short inline comment in the _make_middleware helper
next to the monkeypatch.delitem(sys.modules,
"transport.unity_instance_middleware", raising=False) call explaining that the
middleware module is intentionally evicted from sys.modules so that re-importing
transport.unity_instance_middleware will pick up the patched
transport.plugin_hub.PluginHub (FakePluginHub); reference the helper name
_make_middleware and the FakePluginHub class in the comment so future readers
understand this import-time dependency and test isolation intent.
- Around line 347-374: The test currently calls
set_unity_instance_middleware(mw) which mutates the module-level singleton
_unity_instance_middleware without automatic teardown; instead, use monkeypatch
to set the module attribute so it will be reverted after the test: replace the
direct call to set_unity_instance_middleware with monkeypatch.setattr on the
transport.unity_instance_middleware module's _unity_instance_middleware (or on
get_unity_instance_middleware/set_unity_instance_middleware symbols) to inject
mw, ensuring the global singleton is restored by monkeypatch; reference symbols:
set_unity_instance_middleware, get_unity_instance_middleware,
_unity_instance_middleware, UnityInstanceMiddleware.
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
Adds per-call
unity_instancerouting so AI clients can target specific Unity instances on any tool call without requiringset_active_instancefirst. Supersedes the CLI flags approach from #697.unity_instancefrom tool call arguments, resolves it (Name@hash, hash prefix, or port number in stdio mode), and sets request-scoped state — without persisting to the sessionset_active_instanceextended to also accept port numbers (stdio mode) for consistencyHow it works
Any tool call can include
unity_instanceas an extra parameter:The middleware pops the key before Pydantic validation, resolves it to a validated instance ID, and sets it in request-scoped state for that call only. Subsequent calls without
unity_instancerevert to the session-persisted instance (or auto-select).Design decisions
unity_instancedoes NOT change the session default. This avoids confusion when alternating between instancesName@hashby querying the connection pool, not synthetic IDsset_active_instanceworkflow unchanged;unity_instanceparameter is purely additiveCloses #697
Test plan
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Summary by Sourcery
Add support for per-call Unity instance routing via a unity_instance tool argument while keeping session-pinned routing, and extend instance selection ergonomics across transports.
New Features:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests
Documentation