Add per-call Unity instance targeting#1277
Open
Qommon wants to merge 6 commits into
Open
Conversation
Add per-call Unity instance selectors for tools, resources, and local HTTP commands while keeping active-instance state session-scoped and effective targets request-local. Preserve stdio discovery and retry behavior, and update coverage and multi-instance routing documentation.
Keep numeric HTTP project names ahead of hash and port shorthand, and preserve mcp_for_unity_tool's legacy group positional argument.
Contributor
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesThe PR adds canonical Unity instance resolution across MCP middleware and HTTP endpoints, request-scoped per-call routing, targetable tool/resource metadata, targeted resource templates, and explicit-target stdio retry behavior. Tests and documentation cover routing precedence, concurrency, transport differences, and recovery. ChangesUnity instance targeting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant HTTPClient
participant MainAPI
participant UnityInstanceResolver
participant UnitySession
HTTPClient->>MainAPI: POST /api/command with unity_instance
MainAPI->>UnityInstanceResolver: resolve_instance_identifier
UnityInstanceResolver-->>MainAPI: canonical instance id
MainAPI->>UnitySession: execute command for resolved session
UnitySession-->>MainAPI: command response
MainAPI-->>HTTPClient: JSON response or structured targeting error
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 |
Qommon
marked this pull request as ready for review
July 19, 2026 21:44
![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 (6)
Server/tests/integration/test_instance_autoselect.py (1)
110-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
MagicMockinstead of manually stubbingtypes.ModuleType.Replacing whole modules in
sys.moduleswith an emptytypes.ModuleTypeis brittle. IfUnityInstanceMiddlewareeventually imports other, unrelated symbols fromtransport.plugin_hubortransport.legacy.unity_connection, the test will crash with anAttributeError.Using
unittest.mock.MagicMockfor the module stubs automatically provisions any requested attributes, preventing unexpected import failures. It also provides built-in assertions, eliminating the need to manually capture arguments with local lists likerefresh_values. Consider applying this cleaner pattern here and in similar tests below.♻️ Proposed refactor
- plugin_hub = types.ModuleType("transport.plugin_hub") - - class PluginHub: - `@classmethod` - def is_configured(cls) -> bool: - return False - - plugin_hub.PluginHub = PluginHub - monkeypatch.setitem(sys.modules, "transport.plugin_hub", plugin_hub) + from unittest.mock import MagicMock + plugin_hub_mock = MagicMock() + plugin_hub_mock.PluginHub.is_configured.return_value = False + monkeypatch.setitem(sys.modules, "transport.plugin_hub", plugin_hub_mock) monkeypatch.delitem(sys.modules, "transport.unity_instance_middleware", raising=False) from transport.unity_instance_middleware import UnityInstanceMiddleware monkeypatch.setattr(config, "transport_mode", "stdio") middleware = UnityInstanceMiddleware() ctx = DummyContext() - refresh_values = [] - - class PoolStub: - def discover_all_instances(self, force_refresh=False): - refresh_values.append(force_refresh) - return [SimpleNamespace(id="UnityMCPTests@cc8756d4")] - - unity_connection = types.ModuleType("transport.legacy.unity_connection") - unity_connection.get_unity_connection_pool = lambda: PoolStub() - monkeypatch.setitem(sys.modules, "transport.legacy.unity_connection", unity_connection) + + mock_pool = MagicMock() + mock_pool.discover_all_instances.return_value = [SimpleNamespace(id="UnityMCPTests@cc8756d4")] + unity_connection_mock = MagicMock() + unity_connection_mock.get_unity_connection_pool.return_value = mock_pool + monkeypatch.setitem(sys.modules, "transport.legacy.unity_connection", unity_connection_mock) instances = await middleware._discover_instances(ctx) assert [instance.id for instance in instances] == ["UnityMCPTests@cc8756d4"] - assert refresh_values == [True] + mock_pool.discover_all_instances.assert_called_once_with(force_refresh=True)🤖 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/integration/test_instance_autoselect.py` around lines 110 - 140, Update the module stubs in the instance auto-selection tests to use unittest.mock.MagicMock instead of manually created types.ModuleType instances. Replace local argument-capture lists such as refresh_values with MagicMock call assertions, and apply the same pattern to the similar tests below while preserving their existing behavior.Server/src/main.py (1)
482-491: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove dead code.
Since
_resolve_http_instance_targetnow raises anInstanceTargetErrorthat is caught and returned immediately above,session_idwill always be populated if a validunity_instancewas provided. This fallback check is now dead code and can be safely removed.♻️ Proposed refactor
- # If a specific unity_instance was requested but not found, return an error - # (Check done here so execute_custom_tool can also validate the instance) - if unity_instance and not session_id: - return JSONResponse( - { - "success": False, - "error": f"Unity instance '{unity_instance}' not found", - }, - status_code=404, - )🤖 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/main.py` around lines 482 - 491, Remove the fallback `if unity_instance and not session_id` block from the surrounding request handler, including its 404 `JSONResponse`; `_resolve_http_instance_target` and its existing `InstanceTargetError` handling already cover invalid Unity instances, while valid targets populate `session_id`.Server/tests/test_instance_targeting_http.py (1)
57-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a raw string for regex patterns.
The pattern passed to
match=contains regex metacharacters (.*). It's recommended to use a raw string to avoid unintended escape sequence behaviors and to satisfy linters like Ruff (RUF043).♻️ Proposed refactor
- with pytest.raises(InstanceTargetError, match="Project name 'Project'.*multiple"): + with pytest.raises(InstanceTargetError, match=r"Project name 'Project'.*multiple"):🤖 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_instance_targeting_http.py` at line 57, Update the pytest.raises call’s match pattern in the relevant instance-targeting test to use a raw string literal, preserving the existing regex and expected error message.Source: Linters/SAST tools
Server/tests/test_transport_characterization.py (1)
1094-1094: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a raw string for the
match=regex.pytest.raises(match=...)runsre.search, so.*is intentional regex; a raw string documents that intent and clears Ruff RUF043 at both sites.
Server/tests/test_transport_characterization.py#L1094-L1094: change tomatch=r"target-hash.*no longer available".Server/tests/test_transport_characterization.py#L1139-L1139: change tomatch=r"target-hash.*no longer available".🤖 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` at line 1094, Update both pytest.raises match expressions in Server/tests/test_transport_characterization.py at lines 1094-1094 and 1139-1139 to use raw regex strings for the existing target-hash pattern, preserving the current matching behavior and clearing Ruff RUF043.Source: Linters/SAST tools
Server/src/services/registry/tool_registry.py (1)
75-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
.pop()instead ofif key in dict: del dict[key].Ruff (RUF051) flags this pattern;
tool_kwargs.pop("unity_targetable", None)is more idiomatic.♻️ Proposed fix
tool_kwargs = dict(kwargs) # Create a copy to avoid side effects - if "unity_target" in tool_kwargs: - del tool_kwargs["unity_target"] - if "unity_targetable" in tool_kwargs: - del tool_kwargs["unity_targetable"] - if "group" in tool_kwargs: - del tool_kwargs["group"] + tool_kwargs.pop("unity_target", None) + tool_kwargs.pop("unity_targetable", None) + tool_kwargs.pop("group", None)🤖 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/services/registry/tool_registry.py` around lines 75 - 80, In the tool_kwargs cleanup block, replace the membership checks and deletions for unity_target, unity_targetable, and group with dict.pop calls using a None default, preserving the behavior when keys are absent.Source: Linters/SAST tools
Server/src/transport/unity_instance_middleware.py (1)
536-537: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
setattrwith a direct attribute assignment (Ruff B010).The attribute name is a constant, so
setattroffers nothing over assignment.♻️ Proposed tweak
clean_message = copy(message) - setattr(clean_message, "uri", clean_uri) + clean_message.uri = clean_uri🤖 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/unity_instance_middleware.py` around lines 536 - 537, In the affected middleware code, replace the setattr call using the constant attribute name with direct attribute assignment on the target object, preserving the assigned value and surrounding 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.
Inline comments:
In `@Server/src/services/tools/execute_custom_tool.py`:
- Around line 19-20: Update the MCP tool declaration in execute_custom_tool to
replace group=None with an allowed registered group, selecting the value
appropriate for this tool such as scripting_ext or core, while preserving
unity_targetable=True.
In `@Server/src/transport/legacy/unity_connection.py`:
- Around line 584-599: Update the instance-discovery method to avoid holding
_pool_lock during PortDiscovery.discover_all_unity_instances(). Add and
initialize a dedicated _scan_lock, use _pool_lock only for cache reads and
updates, and re-check the cache after acquiring _scan_lock before performing a
forced scan.
- Around line 547-549: Update the `except Exception as e` block in the
connection or rediscovery flow to raise `target_unavailable_error` with explicit
exception chaining from `e`, preserving the original traceback while keeping the
existing error behavior unchanged.
---
Nitpick comments:
In `@Server/src/main.py`:
- Around line 482-491: Remove the fallback `if unity_instance and not
session_id` block from the surrounding request handler, including its 404
`JSONResponse`; `_resolve_http_instance_target` and its existing
`InstanceTargetError` handling already cover invalid Unity instances, while
valid targets populate `session_id`.
In `@Server/src/services/registry/tool_registry.py`:
- Around line 75-80: In the tool_kwargs cleanup block, replace the membership
checks and deletions for unity_target, unity_targetable, and group with dict.pop
calls using a None default, preserving the behavior when keys are absent.
In `@Server/src/transport/unity_instance_middleware.py`:
- Around line 536-537: In the affected middleware code, replace the setattr call
using the constant attribute name with direct attribute assignment on the target
object, preserving the assigned value and surrounding behavior.
In `@Server/tests/integration/test_instance_autoselect.py`:
- Around line 110-140: Update the module stubs in the instance auto-selection
tests to use unittest.mock.MagicMock instead of manually created
types.ModuleType instances. Replace local argument-capture lists such as
refresh_values with MagicMock call assertions, and apply the same pattern to the
similar tests below while preserving their existing behavior.
In `@Server/tests/test_instance_targeting_http.py`:
- Line 57: Update the pytest.raises call’s match pattern in the relevant
instance-targeting test to use a raw string literal, preserving the existing
regex and expected error message.
In `@Server/tests/test_transport_characterization.py`:
- Line 1094: Update both pytest.raises match expressions in
Server/tests/test_transport_characterization.py at lines 1094-1094 and 1139-1139
to use raw regex strings for the existing target-hash pattern, preserving the
current matching behavior and clearing Ruff RUF043.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39fbde90-c6d0-470c-9078-ca06fc62462a
📒 Files selected for processing (23)
Server/src/main.pyServer/src/services/custom_tool_service.pyServer/src/services/registry/__init__.pyServer/src/services/registry/resource_registry.pyServer/src/services/registry/tool_registry.pyServer/src/services/resources/__init__.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/transport/legacy/unity_connection.pyServer/src/transport/unity_instance_middleware.pyServer/tests/integration/test_helpers.pyServer/tests/integration/test_inline_unity_instance.pyServer/tests/integration/test_instance_autoselect.pyServer/tests/test_instance_targeting_http.pyServer/tests/test_tool_registry_metadata.pyServer/tests/test_transport_characterization.pywebsite/docs/architecture/remote-auth.mdwebsite/docs/guides/multi-instance.mdwebsite/docs/reference/tools/core/manage_material.mdwebsite/docs/reference/tools/vfx/manage_texture.md
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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.
Description
This PR adds per-call Unity Editor instance targeting in the Python transport layer. The normal session-scoped active instance remains the recommended workflow; an explicit selector is an optional override for one call. We do need this feature for concurrent agent workflows in our practical development.
Type of Change
What
/api/command, HTTP/api/custom-tools?instance=..., targetable MCP Tools, and Unity-backed MCP Resources.per-call target > session active instance > existing beta auto/default behavior.paramsremain unchanged.Name@hash, unique hash prefixes, stdio port selectors, HTTP numeric hashes, active selection, single-instance auto-selection, and the existing REST fallback behavior.batch_executeinner-command routing out of the Unity payload.No Unity C# production code,
Commandmodel, WebSocket execute envelope, orHandleCommand(JObject @params)signature was changed. This change adds routing logic on the Python side only.Why
In practical development, multiple Agents frequently work concurrently against multiple Unity Editors and/or separate repository copies. A session-wide active instance is useful for normal work, but it cannot safely express overlapping calls that must go to different Editors. A request-local explicit target provides atomic per-call routing without switching or mutating the session default.
Compatibility / Package Source
6000.0.75f1.file:reference to the current working tree'sMCPForUnitypackage.AlphaandBeta) imported and compiled the package successfully.Testing / Screenshots / Recordings
Server\.venv\Scripts\python.exe -m pytest tests/ -q(1351 passed, 3 skipped, 3 warnings).The E2E used one local Python Server, two real Unity Editor processes, and one FastMCP Client with one MCP session. It verified:
/api/commandand/api/custom-tools?instance=...targeting A/B and overlapping HTTP calls.The repository's documented
uv run --frozen pytestinvocation was also attempted but could not start because of the local uv trampoline error (uv trampoline failed to canonicalize script path); the full suite passed through the repository virtualenv invocation above.Documentation Updates
tools/UPDATE_DOCS_PROMPT.mdwas not used.Related Issues
None.
Additional Notes
This is one implementation generated by GPT 5.6 Luna Max. It works well in the tested scenario, and I would appreciate maintainers' review, feedback, and suggestions.
Summary by CodeRabbit
New Features
unity_instancerouting option.Bug Fixes
Documentation