Fix HTTP instance routing and per-project session IDs #4
Conversation
|
@CodeRabbit you there? do a review of this, dude! |
dsarno
force-pushed
the
fix/http-instance-routing
branch
from
8a2e04a to
2c865bf
Compare
|
@coderabbit_ai how about now? |
|
@msanatan: I'll review the changes in this PR right away! ✅ Actions performedReview triggered.
|
WalkthroughThe pull request enhances Unity session identity management across the editor and server layers. Changes introduce project-scoped session IDs with fallback mechanisms for deterministic per-project hashing, add support for compound session resolution formats ( Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Editor<br/>(ProjectIdentityUtility)
participant Prefs as EditorPrefs
participant Hash as Hash Cache
participant WD as Working Dir<br/>(Fallback)
Editor->>Prefs: GetProjectHash() called
Editor->>Hash: EnsureIdentityCache()
Hash-->>Editor: Cache ready
alt Cache hit
Editor-->>Editor: Return cached hash
else Cache miss
Editor->>Prefs: Read dataPath-based hash
alt dataPath available
Prefs-->>Editor: Return hash
else dataPath unavailable (batch mode)
Editor->>WD: TryComputeFallbackProjectHash()
WD->>WD: Hash(CWD + "/Assets")
WD-->>Editor: Deterministic hash
end
end
Editor->>Prefs: GetOrCreateSessionId(project_hash)
alt Project key exists
Prefs-->>Editor: Return existing GUID
else Project key missing, legacy key exists
Prefs->>Prefs: Read legacy key
Prefs-->>Editor: Return legacy GUID
else Both keys missing
Editor->>Editor: Generate new GUID
Editor->>Prefs: Store under project key
Editor-->>Editor: Return new GUID
end
sequenceDiagram
participant Client as Client/MCP
participant Tool as set_active_instance
participant Hub as PluginHub
participant Registry as Session Registry
Client->>Tool: set_active_instance("MyProject@abc123")
Tool->>Tool: Check transport type
alt HTTP transport
Tool->>Hub: get_sessions()
Hub->>Registry: Query all sessions
Registry-->>Hub: Session list
Hub-->>Tool: Sessions with (name, hash, id)
Tool->>Tool: Extract "abc123" from input
Tool->>Registry: get_session_id_by_hash("abc123")
Registry-->>Tool: Session ID or error
Tool-->>Client: Set active + response
else Stdio transport (fallback)
Tool->>Tool: Query Unity pool
Tool->>Tool: Resolve by hash/name
Tool-->>Client: Set active + response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
Server/resources/unity_instances.py (1)
13-14: Code duplication:_is_http_transporthelper.This helper is duplicated in
Server/tools/set_active_instance.py(lines 12-14). Both delegate to_core_is_http_transportfromunity_transport. While this keeps modules loosely coupled, consider whether a shared import of_core_is_http_transportdirectly (or re-exporting from a common location) might reduce duplication.This is a minor duplication and the current approach maintains module independence, so it's acceptable as-is.
MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs (2)
73-75: Fallback identity cache is solid; consider extending the same robustness to project name
EnsureIdentityCache()+TryComputeFallbackProjectHash()give you deterministic hashes even whenApplication.dataPathis unavailable, which directly addresses the “everything collapses to 'default'” problem. One minor gap: the fallback path only updates_cachedProjectHash, andGetProjectName()doesn’t invokeEnsureIdentityCache(), so in those edge cases the name will remain"Unknown"even though you’ve derived a meaningful hash. If you ever rely onGetProjectName()in such contexts, it might be worth either callingEnsureIdentityCache()there as well and/or deriving a fallback name fromDirectory.GetCurrentDirectory()(e.g., mirroringComputeProjectNameon the same synthetic.../Assetspath).Also applies to: 180-202, 204-222
19-19: Project-specific reset logic matches new key format; legacy guard is fine but could be simplifiedThe project-specific cleanup in
ResetSessionId()("{SessionPrefKey}_{projectHash}") correctly mirrors the new storage key, so tests and tools can reliably clear the right session. The_legacyKeyClearedflag ensures the oldSessionPrefKeyis only removed once, which is fine for a one-time migration scenario. The trade-off is that if an older plugin version in the same editor session recreatesSessionPrefKey, subsequentResetSessionId()calls won’t delete it again. If mixed-version usage within one editor session is at all likely, you could drop the boolean and just always check/delete the legacy key; otherwise this implementation is acceptable as-is.Also applies to: 159-172
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs(4 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)
🧰 Additional context used
🪛 Ruff (0.14.5)
Server/resources/unity_instances.py
54-56: Abstract raise to an inner function
(TRY301)
54-56: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (5)
Server/plugin_hub.py (1)
225-232: Hash extraction logic handles edge cases defensively.The rpartition logic correctly extracts the hash from
Name@hashor treats the full string as a hash. One edge case: ifunity_instanceis"Name@"(trailing@with no suffix), line 230 setstarget_hashtoNone, which triggers the fallback path (first available session) rather than returning an error. This is lenient but safe.Server/tests/integration/test_instance_routing_comprehensive.py (1)
172-295: Comprehensive HTTP transport test coverage.The four new tests validate the HTTP transport path for
set_active_instance:
- Full
Name@hashformat resolution- Hash-only resolution
- Missing hash error handling
- Ambiguous hash prefix error handling
The tests properly mock
PluginHub.get_sessionsand validate both success and error scenarios.Server/tools/set_active_instance.py (1)
57-62: Good defensive validation for empty input.The explicit check for empty or whitespace-only instance identifiers prevents confusing errors downstream and provides a clear, actionable message.
Server/resources/unity_instances.py (1)
58-65: HTTP instance structure looks correct.The instance dictionaries include appropriate fields for HTTP transport, including the transport-specific
connected_attimestamp andsession_idfor reference. This aligns with the docstring updates noting which fields are transport-specific.MCPForUnity/Editor/Helpers/ProjectIdentityUtility.cs (1)
127-142: Per-project session ID scoping via EditorPrefs key suffix looks correctUsing
GetProjectHash()to suffixSessionPrefKeyand storing the GUID under"{SessionPrefKey}_{projectHash}"gives you the per-project isolation you need while preserving the legacy hash semantics. Thetry/catcharoundEditorPrefsand the fallback to an in-memoryGuidin error/batch scenarios are reasonable safeguards; I don’t see correctness issues here.
| if not project or not project_hash: | ||
| raise ValueError( | ||
| "PluginHub session missing required 'project' or 'hash' fields." | ||
| ) |
There was a problem hiding this comment.
Strict validation may be too restrictive.
If any session from PluginHub is missing project or hash fields, the entire operation fails (raises ValueError caught at line 115). This differs from Server/tools/set_active_instance.py (lines 32-33), which silently skips sessions without a hash.
Consider whether this strict validation is intentional:
- Current behavior: One malformed session breaks the entire
unity://instanceslisting - Alternative: Skip invalid sessions with a warning, allowing valid sessions to be returned
The strict approach catches data quality issues early, but may impact availability if PluginHub occasionally returns incomplete session data.
Static analysis notes: Ruff suggests abstracting the raise to an inner function (TRY301) and avoiding long messages outside the exception class (TRY003), though these are minor style concerns.
🧰 Tools
🪛 Ruff (0.14.5)
54-56: Abstract raise to an inner function
(TRY301)
54-56: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In Server/resources/unity_instances.py around lines 53-56 the code raises a
ValueError when a PluginHub session lacks 'project' or 'hash', which causes the
whole unity://instances listing to fail; change this to mirror
Server/tools/set_active_instance.py by skipping malformed sessions instead of
raising: log a warning (include minimal context like session id or index) and
continue to the next session so valid sessions are returned; keep the validation
check, but replace the exception with a warning + continue, and if needed
centralize the check into a small helper to satisfy static analysis (reduce
message length in exception handling).
| # Discover running instances based on transport | ||
| if _is_http_transport(): | ||
| sessions_data = await PluginHub.get_sessions() | ||
| sessions = sessions_data.get("sessions", {}) if isinstance(sessions_data, dict) else {} | ||
| instances = [] | ||
| for session_id, session in sessions.items(): | ||
| project = session.get("project") or session.get("project_name") or "Unknown" | ||
| hash_value = session.get("hash") | ||
| if not hash_value: | ||
| continue | ||
| inst_id = f"{project}@{hash_value}" | ||
| instances.append(SimpleNamespace( | ||
| id=inst_id, | ||
| hash=hash_value, | ||
| name=project, | ||
| session_id=session_id, | ||
| )) | ||
| else: | ||
| pool = get_unity_connection_pool() | ||
| instances = pool.discover_all_instances(force_refresh=True) |
There was a problem hiding this comment.
Inconsistent error handling compared to unity_instances.py.
This HTTP transport discovery path differs from Server/resources/unity_instances.py in how it handles sessions with missing fields:
This file (set_active_instance.py):
- Line 30: Falls back to
"Unknown"if project name is missing - Lines 32-33: Silently skips sessions without
hash(continues)
unity_instances.py:
- Line 50: No
"Unknown"fallback for project name - Lines 53-56: Raises
ValueErrorifprojectorhashis missing
This inconsistency means:
set_active_instanceis more lenient (skips bad sessions, uses "Unknown" for missing names)unity_instancesis stricter (fails if any session has missing required fields)
Consider aligning the validation logic:
- Should both silently skip invalid sessions, or both fail fast?
- Should
"Unknown"be an acceptable fallback, or should project name be required?
The lenient approach in this file seems more resilient (one bad session doesn't break everything), while the strict approach in unity_instances.py might catch data quality issues earlier. Document the intended behavior or unify the validation strategy.
🤖 Prompt for AI Agents
In Server/tools/set_active_instance.py around lines 24-43, the HTTP discovery
code is inconsistent with Server/resources/unity_instances.py: remove the
fallback to "Unknown" and the silent continue for missing fields and instead
validate sessions the same way as unity_instances.py by raising a ValueError
when required fields (project or hash) are missing; update the code to require
session.get("project") (no "Unknown" fallback), require session.get("hash") and
raise a clear ValueError if either is absent, and add a brief comment/docstring
indicating the strict validation choice so the behavior is consistent with
unity_instances.py.
|
Closed by #5 which solves the merge conflict |
2 participants
Problem: Instance routing didn't work under http. When two Unity projects connect over HTTP, they share a single WebSocket session ID and the server can’t reliably route commands.
unity_instancesandset_active_instanceonly worked in stdio mode, so HTTP clients lost track of which editor they were targeting.Fix: Each project now stores a session ID under a hash-scoped EditorPrefs key, so simultaneous editors keep their own sessions.
unity_instancesenumerates HTTP sessions inPluginHub, andset_active_instance(plusPluginHub._resolve_session_id) understands bothProject@hashand hash-only inputs while running asynchronously.Result: After these changes, the domain-reload pipeline and tools (
create_script,list_resources, etc.) continue targeting the intended Unity instance no matter which transport is in use.Test:
uvx[YOUR_PATH]/Server pytest Server/tests/integration/test_instance_routing_comprehensive.pySummary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.