Fix HTTP instance routing and per-project session IDs #99
Conversation
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on December 7. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe changes implement per-project session scoping in Unity and add HTTP-based transport support for instance discovery and routing. Session IDs are now stored with project-specific hash-derived keys. Server-side instance resolution and active instance management are enhanced to support HTTP transport via PluginHub alongside existing stdio/TCP pools, with Name@hash instance identification support. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SetActiveInstance as set_active_instance.py
participant PluginHub
participant UnityPool as Unity Connection Pool
participant Middleware
Client->>SetActiveInstance: set_active_instance(instance: "Project@hash" or "hash")
alt HTTP Transport
SetActiveInstance->>PluginHub: get_sessions()
PluginHub-->>SetActiveInstance: [session_data, ...]
SetActiveInstance->>SetActiveInstance: Build instances from sessions
else Stdio Transport
SetActiveInstance->>UnityPool: discover_all_instances()
UnityPool-->>SetActiveInstance: [instance_data, ...]
end
SetActiveInstance->>SetActiveInstance: Resolve instance ID/hash from input
SetActiveInstance->>SetActiveInstance: Match against discovered instances
alt Match Found
SetActiveInstance->>Middleware: Store active instance
Middleware-->>SetActiveInstance: OK
SetActiveInstance-->>Client: {success: true}
else No Match
SetActiveInstance-->>Client: {error: "Instance not found"}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
|
@CodeRabbit please review |
|
@dsarno: I'll review the changes in this pull request. ✅ Actions performedReview triggered.
|
Greptile OverviewGreptile SummaryMade session IDs project-specific by scoping EditorPrefs keys with project hashes, and fixed HTTP transport instance routing to properly parse Key Changes:
Issues Found:
Confidence Score: 4/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Client as MCP Client
participant Middleware as UnityInstanceMiddleware
participant Tool as set_active_instance
participant Hub as PluginHub
participant Registry as PluginRegistry
participant Unity as Unity Editor
Note over Client,Unity: HTTP Transport Mode
Unity->>Hub: WebSocket connect + register(session_id, project_hash)
Hub->>Registry: register(session_id, project_name, project_hash)
Registry-->>Hub: PluginSession
Client->>Tool: set_active_instance("ProjectName@hash")
Tool->>Hub: get_sessions()
Hub->>Registry: list_sessions()
Registry-->>Hub: sessions dict
Hub-->>Tool: sessions with project, hash
alt Full ID format (Name@hash)
Tool->>Tool: Match against inst.id
else Hash only
Tool->>Tool: Match against inst.hash.startswith(value)
end
Tool->>Middleware: set_active_instance(ctx, resolved_id)
Tool-->>Client: success + resolved instance ID
Client->>Hub: send_command_for_instance(unity_instance, cmd, params)
Hub->>Hub: _resolve_session_id(unity_instance)
alt unity_instance contains "@"
Hub->>Hub: Extract hash from "Name@hash"
else
Hub->>Hub: Use as-is (hash only)
end
Hub->>Registry: get_session_id_by_hash(target_hash)
Registry-->>Hub: session_id
Hub->>Unity: send_command(session_id, cmd, params)
Unity-->>Hub: command_result
Hub-->>Client: result
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (_cachedProjectHash == "default" || string.IsNullOrEmpty(_cachedProjectHash)) | ||
| { | ||
| UpdateIdentityCache(); | ||
| } |
There was a problem hiding this comment.
logic: If Application.dataPath is unavailable, UpdateIdentityCache() returns early (line 52-54) leaving _cachedProjectHash as "default". This check will always be true in that scenario, but subsequent GetProjectHash() will still return "default". Multiple projects failing hash computation would share MCPForUnity.WebSocketSessionId_default, violating per-project session ID isolation
Prompt To Fix With AI
This is a comment left during a code review.
Path: MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs
Line: 132:135
Comment:
**logic:** If `Application.dataPath` is unavailable, `UpdateIdentityCache()` returns early (line 52-54) leaving `_cachedProjectHash` as "default". This check will always be true in that scenario, but subsequent `GetProjectHash()` will still return "default". Multiple projects failing hash computation would share `MCPForUnity.WebSocketSessionId_default`, violating per-project session ID isolation
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs (2)
125-145: Per-project session key derivation looks solid; consider clarifying the “default” fallback behaviorDeriving
projectSpecificKeyfrom_cachedProjectHashand forcing a cache refresh when it is"default"or empty gives you proper per-project isolation in the common case and avoids relying on the async initializer.One edge-case to be aware of: if
UpdateIdentityCache()cannot ever compute a real hash (e.g.,Application.dataPathmissing or consistently failing), multiple such projects will still shareSessionPrefKey_default. If that scenario matters (batch runners, unusual editor setups), you might want to either document that behavior, or add a secondary discriminator (e.g., project name/path) before falling back to"default".
163-174: ResetSessionId correctly scopes deletion to the current project; optional legacy-cleanupMirroring the on-demand cache refresh and project-hash-based key in
ResetSessionIdensures tests only clear the current project’s session id rather than a global key, which matches the new per-project semantics.If you want a clean migration from the old global key, you could optionally also delete the legacy
SessionPrefKey(without hash suffix) once per process so that stale global ids don’t linger in EditorPrefs, while keeping the main behavior as-is.Server/plugin_hub.py (1)
225-238: Name@hash parsing and hash-based lookup align with routing intentAllowing
unity_instanceto be either bare hash orName@hashand normalizing both intotarget_hashforget_session_id_by_hashkeeps the API flexible while preserving deterministic selection when a specific hash is given.One minor future-proofing tweak you might consider: if project names are ever allowed to contain
'@',unity_instance.partition("@")will split on the first delimiter and treat the rest (including any additional'@') as the hash. Usingunity_instance.rpartition("@")(orrsplit("@", 1)) would instead always take the last segment as the hash while leaving any earlier'@'in the name.Server/tests/integration/test_instance_routing_comprehensive.py (1)
19-244: HTTP routing tests accurately exercise the new discovery pathThe HTTP tests are wired up well: they set
UNITY_MCP_TRANSPORT="http", patchtools.set_active_instance.PluginHub.get_sessionsandget_unity_instance_middleware, and then assert thatset_active_instance_toolupdatesUnityInstanceMiddlewareto the expectedProject@hashfor both explicitName@hashand hash-only inputs. That directly validates the new HTTP-aware path inset_active_instance.If you want to harden behavior further, you could add one or two cases where the hash prefix is ambiguous or doesn’t exist, asserting that the error messages (and success flags) match what the stdio path already guarantees. That would help ensure both transports stay behaviorally aligned as the routing logic evolves.
Server/tools/set_active_instance.py (1)
2-48: Async, transport-aware discovery is correct; tighten a couple of edge casesThe refactor to make
set_active_instanceasync and to branch on_is_http_transport()looks good: the HTTP code path pulls sessions fromplugin_hub.PluginHub.get_sessions(), normalizes them intoSimpleNamespaceobjects withid/hash/name, and then reuses the existing Name@hash / hash-prefix resolution and middleware storage logic. The stdio path continues to rely onget_unity_connection_pool().discover_all_instances(force_refresh=True), so existing behavior is preserved when not using HTTP.Two small improvements worth considering:
- Input validation: when
instanceis empty or only whitespace,value = instance.strip()becomes""and the hash-prefix branch will see every instance as a match (startswith("")), returning the “matches multiple instances” error. A short early check forif not value:returning a clearer validation error would make this behavior more intentional.- Transport detection reuse:
_is_http_transport()is now duplicated here and inServer/resources/unity_instances.py. Pulling this into a tiny shared helper module (or config utility) would avoid drift if you ever introduce additional transports or rename"http"/"stdio".Neither of these blocks the current behavior, but they would make the tool a bit more predictable and maintainable.
Server/resources/unity_instances.py (1)
13-105: HTTP/stdio branches produce a consistent API; just be explicit about PluginHub schema couplingThe transport split is clean: when
_is_http_transport()is true you queryplugin_hub.PluginHub.get_sessions()and buildinstancesfrom its session dict, and otherwise you fall back toget_unity_connection_pool().discover_all_instances(...). Both paths return the same top-level shape (success,transport,instance_count,instances, optionalwarning), and the duplicate-name warning text is shared, which is helpful for clients.One thing to keep in mind is that the HTTP branch assumes
PluginHub.get_sessions()always returns'project'and'hash'keys (accessed viasession_info["project"]/["hash"]). Today that’s true, but if the PluginHub schema ever changes, this will surface as aKeyErrorthat then gets wrapped by the outerexceptblock. To make that coupling clearer and a bit safer you could either:
- Switch to
.getwith an explicit check and a targeted error message when the schema is malformed, or- Add a short comment here stating that
PluginHub.get_sessionsis the canonical producer of this structure.Also,
_is_http_transport()duplicates the helper inServer/tools/set_active_instance.py; sharing a single implementation would reduce the chance of the two drifting apart in future changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs(2 hunks)Server/plugin_hub.py(1 hunks)Server/resources/unity_instances.py(2 hunks)Server/tests/integration/test_instance_routing_comprehensive.py(2 hunks)Server/tools/set_active_instance.py(1 hunks)
dsarno
force-pushed
the
fix/http-instance-routing
branch
from
8a2e04a to
2c865bf
Compare
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.