Auto-select sole Unity instance when unset #115
Conversation
…y format
- Fix add_component to check top-level componentProperties when using
componentsToAdd as string array (e.g., ["Rigidbody"])
- Previously only worked with componentName or object array formats
- Makes API more consistent with modify action
- Add comprehensive unit tests for all three parameter formats
This allows the intuitive pattern:
{
"componentsToAdd": ["Rigidbody"],
"componentProperties": {"Rigidbody": {"mass": 5.0}}
}
|
Warning Rate limit exceeded@dsarno has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 21 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
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. WalkthroughA new asynchronous auto-selection method is added to UnityInstanceMiddleware to auto-select a sole discovered Unity instance by querying PluginHub sessions or probing the stdio transport pool when no active instance is set; errors are caught and logged, and existing injection flow is preserved. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Middleware as UnityInstanceMiddleware
participant Session as SessionStore
participant PluginHub
participant Stdio as StdioTransportPool
Note over Middleware,Session: Request enters middleware
Client->>Middleware: request (ctx)
Middleware->>Session: get active_instance
alt active_instance exists
Session-->>Middleware: instance_id
else no active_instance
Middleware->>PluginHub: query sessions (HTTP transports)
alt PluginHub returns single ID
PluginHub-->>Middleware: single_instance_id
Middleware->>Session: set active_instance(single_instance_id)
else PluginHub returns none or multiple
PluginHub-->>Middleware: none/multiple
Middleware->>Stdio: probe pool for discovered IDs
alt Stdio returns single ID
Stdio-->>Middleware: single_instance_id
Middleware->>Session: set active_instance(single_instance_id)
else Stdio returns none/multiple
Stdio-->>Middleware: none/multiple
end
end
end
Note over Middleware: proceed with injection using session.active_instance (if any)
Middleware->>Client: continue to next handler
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
### Motivation - Automatically select the only connected Unity instance when no active instance is set to reduce manual configuration steps. - Prefer resolving instances through `PluginHub` when available and fall back to stdio discovery when not. - Force-refresh stdio discovery to avoid choosing stale or closed instances during auto-selection. - Be defensive about failures and avoid clearing an active instance due to transient PluginHub resolution errors. ### Description - Add `UnityInstanceMiddleware._maybe_autoselect_instance` which probes `PluginHub.get_sessions()` and falls back to `get_unity_connection_pool().discover_all_instances(force_refresh=True)` to find a sole instance. - Integrate the auto-selection into `_inject_unity_instance` so that when `get_active_instance` returns none the middleware will attempt to auto-select and `set_active_instance` on success. - Log successes via `logger.info` and capture probe failures with debug-level logging to avoid noisy errors. - Wrap discovery and resolution calls with exception handling to ensure robustness and to prevent clearing state on transient errors. ### Testing - No automated tests were run.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (2)
Server/src/transport/unity_instance_middleware.py (2)
86-140: Excellent defensive auto-selection logic with good fallback strategy.The implementation correctly prioritizes PluginHub resolution and falls back to stdio discovery with
force_refresh=Trueas intended. The logging strategy (info for success, debug for failures) and thread-safety viaset_active_instanceare well done.Suggestion: Refine exception handling while maintaining defensive posture.
The broad
Exceptioncatching (lines 110, 132, 134) is flagged by static analysis. While being defensive against transient failures is appropriate per the PR objectives, consider catching specific expected exceptions explicitly and re-raising system exceptions:🔎 Proposed refinement for exception handling
For the PluginHub probe (lines 93-114):
try: sessions_data = await PluginHub.get_sessions() sessions = sessions_data.sessions or {} ids: list[str] = [] for session_info in sessions.values(): project = getattr(session_info, "project", None) or "Unknown" hash_value = getattr(session_info, "hash", None) if hash_value: ids.append(f"{project}@{hash_value}") if len(ids) == 1: chosen = ids[0] self.set_active_instance(ctx, chosen) logger.info( "Auto-selected sole Unity instance via PluginHub: %s", chosen, ) return chosen - except Exception: + except (ConnectionError, ValueError, KeyError, TimeoutError, AttributeError) as exc: logger.debug( - "PluginHub auto-select probe failed; falling back to stdio", + "PluginHub auto-select probe failed (%s); falling back to stdio", + type(exc).__name__, exc_info=True, ) + except Exception as exc: + if isinstance(exc, (SystemExit, KeyboardInterrupt)): + raise + logger.debug( + "PluginHub auto-select probe failed with unexpected error (%s); falling back to stdio", + type(exc).__name__, + exc_info=True, + )Apply similar refinement to the stdio probe (lines 117-133) and outer handler (lines 134-138).
96-101: Optional: Consider extracting instance ID construction logic.The instance ID construction pattern (
f"{project}@{hash_value}") is duplicated between this method andset_active_instance.py(lines 21-30). While the duplication is minimal, extracting this into a shared helper function could improve maintainability if the ID format changes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Server/src/transport/unity_instance_middleware.py
🧰 Additional context used
🧬 Code graph analysis (1)
Server/src/transport/unity_instance_middleware.py (4)
Server/src/transport/unity_transport.py (1)
_current_transport(23-25)Server/src/transport/plugin_hub.py (2)
PluginHub(37-514)get_sessions(213-227)Server/src/services/tools/set_active_instance.py (1)
set_active_instance(15-112)Server/src/transport/legacy/unity_connection.py (2)
get_unity_connection_pool(652-665)discover_all_instances(449-478)
🪛 Ruff (0.14.10)
Server/src/transport/unity_instance_middleware.py
110-110: Do not catch blind exception: Exception
(BLE001)
132-132: Do not catch blind exception: Exception
(BLE001)
134-134: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
Server/src/transport/unity_instance_middleware.py (1)
147-148: Clean integration of auto-selection logic.The conditional auto-selection only triggers when no active instance is set, preserving existing behavior and avoiding unnecessary work. The integration correctly awaits the async call and maintains the flow of the existing injection logic.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Greptile SummaryThis PR implements auto-selection of the sole Unity instance when no active instance is set, reducing manual configuration steps for users.
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Middleware as UnityInstanceMiddleware
participant PluginHub
participant StdioPool as UnityConnectionPool
participant Context
Client->>Middleware: on_call_tool(context)
Middleware->>Middleware: _inject_unity_instance(context)
Middleware->>Middleware: get_active_instance(ctx)
alt No active instance set
Middleware->>Middleware: _maybe_autoselect_instance(ctx)
alt PluginHub configured
Middleware->>PluginHub: get_sessions()
PluginHub-->>Middleware: SessionList with sessions
Middleware->>Middleware: Filter sessions with hash
Middleware->>Middleware: Build ids list (project@hash)
alt Exactly 1 instance found
Middleware->>Middleware: set_active_instance(ctx, chosen)
Middleware->>Middleware: Log success
Middleware-->>Middleware: Return chosen instance
else Not exactly 1 instance
Middleware->>StdioPool: Fallback to stdio discovery
end
else PluginHub not configured or failed
Middleware->>StdioPool: discover_all_instances(force_refresh=True)
StdioPool-->>Middleware: List of UnityInstanceInfo
Middleware->>Middleware: Extract and filter ids
alt Exactly 1 instance found
Middleware->>Middleware: set_active_instance(ctx, chosen)
Middleware->>Middleware: Log success
Middleware-->>Middleware: Return chosen instance
else Not exactly 1 instance
Middleware-->>Middleware: Return None
end
end
end
alt Active instance available
Middleware->>Context: set_state("unity_instance", active_instance)
alt PluginHub configured
Middleware->>PluginHub: _resolve_session_id(active_instance)
alt Resolution successful
PluginHub-->>Middleware: session_id
Middleware->>Context: set_state("unity_session_id", session_id)
else Resolution failed
Middleware->>Middleware: Log debug (leave instance unchanged)
end
end
end
Middleware->>Client: Proceed with tool call
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try: | ||
| from transport.unity_transport import _current_transport | ||
|
|
||
| transport = _current_transport() | ||
| if PluginHub.is_configured(): | ||
| try: | ||
| sessions_data = await PluginHub.get_sessions() | ||
| sessions = sessions_data.sessions or {} | ||
| ids: list[str] = [] | ||
| for session_info in sessions.values(): | ||
| project = getattr(session_info, "project", None) or "Unknown" | ||
| hash_value = getattr(session_info, "hash", None) | ||
| if hash_value: | ||
| ids.append(f"{project}@{hash_value}") | ||
| if len(ids) == 1: | ||
| chosen = ids[0] | ||
| self.set_active_instance(ctx, chosen) | ||
| logger.info( | ||
| "Auto-selected sole Unity instance via PluginHub: %s", | ||
| chosen, | ||
| ) | ||
| return chosen | ||
| except Exception: | ||
| logger.debug( |
There was a problem hiding this comment.
style: Session filtering skips sessions with missing hash but doesn't validate that hash is non-empty string
| try: | |
| from transport.unity_transport import _current_transport | |
| transport = _current_transport() | |
| if PluginHub.is_configured(): | |
| try: | |
| sessions_data = await PluginHub.get_sessions() | |
| sessions = sessions_data.sessions or {} | |
| ids: list[str] = [] | |
| for session_info in sessions.values(): | |
| project = getattr(session_info, "project", None) or "Unknown" | |
| hash_value = getattr(session_info, "hash", None) | |
| if hash_value: | |
| ids.append(f"{project}@{hash_value}") | |
| if len(ids) == 1: | |
| chosen = ids[0] | |
| self.set_active_instance(ctx, chosen) | |
| logger.info( | |
| "Auto-selected sole Unity instance via PluginHub: %s", | |
| chosen, | |
| ) | |
| return chosen | |
| except Exception: | |
| logger.debug( | |
| for session_info in sessions.values(): | |
| project = getattr(session_info, "project", None) or "Unknown" | |
| hash_value = getattr(session_info, "hash", None) | |
| if hash_value and isinstance(hash_value, str) and hash_value.strip(): | |
| ids.append(f"{project}@{hash_value}") |
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/src/transport/unity_instance_middleware.py
Line: 88:111
Comment:
**style:** Session filtering skips sessions with missing `hash` but doesn't validate that `hash` is non-empty string
```suggestion
for session_info in sessions.values():
project = getattr(session_info, "project", None) or "Unknown"
hash_value = getattr(session_info, "hash", None)
if hash_value and isinstance(hash_value, str) and hash_value.strip():
ids.append(f"{project}@{hash_value}")
```
How can I resolve this? If you propose a fix, please make it concise.| exc_info=True, | ||
| ) | ||
|
|
||
| if transport != "http": | ||
| try: | ||
| from transport.legacy.unity_connection import get_unity_connection_pool | ||
|
|
||
| pool = get_unity_connection_pool() | ||
| instances = pool.discover_all_instances(force_refresh=True) | ||
| ids = [getattr(inst, "id", None) for inst in instances] | ||
| ids = [inst_id for inst_id in ids if inst_id] | ||
| if len(ids) == 1: | ||
| chosen = ids[0] | ||
| self.set_active_instance(ctx, chosen) | ||
| logger.info( | ||
| "Auto-selected sole Unity instance via stdio discovery: %s", | ||
| chosen, |
There was a problem hiding this comment.
style: The import is inside the try block. If the import fails, the exception will be caught silently, and auto-selection will just return None without any indication that the import failed.
| exc_info=True, | |
| ) | |
| if transport != "http": | |
| try: | |
| from transport.legacy.unity_connection import get_unity_connection_pool | |
| pool = get_unity_connection_pool() | |
| instances = pool.discover_all_instances(force_refresh=True) | |
| ids = [getattr(inst, "id", None) for inst in instances] | |
| ids = [inst_id for inst_id in ids if inst_id] | |
| if len(ids) == 1: | |
| chosen = ids[0] | |
| self.set_active_instance(ctx, chosen) | |
| logger.info( | |
| "Auto-selected sole Unity instance via stdio discovery: %s", | |
| chosen, | |
| from transport.legacy.unity_connection import get_unity_connection_pool | |
| try: | |
| pool = get_unity_connection_pool() | |
| instances = pool.discover_all_instances(force_refresh=True) | |
| ids = [getattr(inst, "id", None) for inst in instances] | |
| ids = [inst_id for inst_id in ids if inst_id] | |
| if len(ids) == 1: | |
| chosen = ids[0] | |
| self.set_active_instance(ctx, chosen) | |
| logger.info( | |
| "Auto-selected sole Unity instance via stdio discovery: %s", | |
| chosen, | |
| ) | |
| return chosen | |
| except Exception: | |
| logger.debug("Stdio auto-select probe failed", exc_info=True) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/src/transport/unity_instance_middleware.py
Line: 113:129
Comment:
**style:** The import is inside the try block. If the import fails, the exception will be caught silently, and auto-selection will just return None without any indication that the import failed.
```suggestion
from transport.legacy.unity_connection import get_unity_connection_pool
try:
pool = get_unity_connection_pool()
instances = pool.discover_all_instances(force_refresh=True)
ids = [getattr(inst, "id", None) for inst in instances]
ids = [inst_id for inst_id in ids if inst_id]
if len(ids) == 1:
chosen = ids[0]
self.set_active_instance(ctx, chosen)
logger.info(
"Auto-selected sole Unity instance via stdio discovery: %s",
chosen,
)
return chosen
except Exception:
logger.debug("Stdio auto-select probe failed", exc_info=True)
```
How can I resolve this? If you propose a fix, please make it concise.| ) | ||
| return chosen | ||
| except Exception: | ||
| logger.debug("Stdio auto-select probe failed", exc_info=True) | ||
| except Exception: | ||
| logger.debug( | ||
| "Auto-select path encountered an unexpected error", |
There was a problem hiding this comment.
style: The outermost try-except is redundant since all internal operations already have exception handling
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/src/transport/unity_instance_middleware.py
Line: 130:136
Comment:
**style:** The outermost try-except is redundant since all internal operations already have exception handling
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| async def _inject_unity_instance(self, context: MiddlewareContext) -> None: | ||
| """Inject active Unity instance into context if available.""" | ||
| ctx = context.fastmcp_context |
There was a problem hiding this comment.
style: Auto-selection runs on every tool call when no active instance is set, potentially causing redundant discovery scans. Consider caching the "no instances available" result temporarily to avoid repeated expensive operations.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/src/transport/unity_instance_middleware.py
Line: 142:144
Comment:
**style:** Auto-selection runs on every tool call when no active instance is set, potentially causing redundant discovery scans. Consider caching the "no instances available" result temporarily to avoid repeated expensive operations.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.…r HTTP ### Motivation - Avoid performing `stdio` instance discovery when the server transport is `http` to prevent unintended/scalable stdio probing in HTTP deployments. - Prefer using `PluginHub.get_sessions()` for auto-selection when PluginHub is configured and only fall back to stdio discovery for local `stdio` transport. - Reduce noisy failures by defensive exception handling and debug-level logging during auto-select probes. - Prevent clearing or flipping active instance state due to transient PluginHub resolution errors. ### Description - Added an import and call to `transport.unity_transport._current_transport()` and store the result in `transport` to detect the active transport mode. - Keep the `PluginHub`-based probe as the primary auto-select path and only run `get_unity_connection_pool().discover_all_instances(force_refresh=True)` when `transport != "http"`. - Log successful auto-selection via `logger.info` and record probe failures at `debug` level, and ensure the `_maybe_autoselect_instance` returns `None` when no sole instance is found. - Retained defensive exception handling so transient errors do not clear or unset the current `active_instance` during `_inject_unity_instance` flow. ### Testing - No automated tests were executed for this change. - Basic static checks (linting/format) were not reported as failing in the rollout logs. - Runtime validation was not performed as part of the automated rollout. - Manual verification is recommended for both `http` and `stdio` transport modes to confirm behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
Server/src/transport/unity_instance_middleware.py (4)
125-154: Move import outside the try block for clarity.The import at line 127 is inside the try block. If the import fails, the exception will be caught silently and auto-selection will return
Nonewithout any indication that the import failed, making debugging more difficult.🔎 Proposed fix
+ from transport.legacy.unity_connection import get_unity_connection_pool + if transport != "http": try: - from transport.legacy.unity_connection import get_unity_connection_pool - pool = get_unity_connection_pool() instances = pool.discover_all_instances(force_refresh=True) ids = [getattr(inst, "id", None) for inst in instances] ids = [inst_id for inst_id in ids if inst_id] if len(ids) == 1: chosen = ids[0] self.set_active_instance(ctx, chosen) logger.info( "Auto-selected sole Unity instance via stdio discovery: %s", chosen, ) return chosen
155-164: Redundant outer exception handler.The outermost try-except block (lines 155-162) appears redundant since all internal operations (PluginHub probe and stdio discovery) already have comprehensive exception handling. This extra layer adds complexity without clear benefit.
97-101: Validate thathash_valueis a non-empty string.Line 100 checks
if hash_value:but doesn't validate thathash_valueis a non-empty string. An empty string would pass the check but create an invalid instance ID like"Project@". This could lead to downstream errors when attempting to use the malformed ID.🔎 Proposed fix
for session_info in sessions.values(): project = getattr(session_info, "project", None) or "Unknown" hash_value = getattr(session_info, "hash", None) - if hash_value: + if hash_value and isinstance(hash_value, str) and hash_value.strip(): ids.append(f"{project}@{hash_value}")
171-172: Consider caching to avoid redundant discovery scans.Auto-selection runs on every tool call when no active instance is set. If no Unity instances are available, this triggers potentially expensive discovery operations (especially
discover_all_instances(force_refresh=True)) on every single invocation.Consider caching the "no instances available" result temporarily (e.g., for 5-10 seconds) to avoid repeated expensive scans when instances genuinely aren't available.
🧹 Nitpick comments (2)
Server/src/transport/unity_instance_middleware.py (2)
89-91: Consider the stability of using_current_transport()as a private API.The underscore prefix suggests
_current_transportis a private function. Depending on private APIs from other modules may introduce fragility if the implementation changes. If this is intentional internal coupling, consider documenting the dependency or promoting it to a public API.
110-123: Consider simplifying the exception handling pattern.Both the PluginHub and stdio paths use a two-tier exception handling strategy: first catching specific exceptions, then catching all
Exceptionwith special handling forSystemExit/KeyboardInterrupt. This pattern is quite defensive but adds complexity.Additionally, including
AttributeErrorin the specific exception list could mask bugs in the code itself (e.g., typos in attribute names).Consider whether a single exception handler would suffice, or whether
AttributeErrorshould be handled separately to avoid masking code bugs.Also applies to: 141-154
…tion handling ### Motivation - Avoid probing `stdio` when the server transport is `http` and prefer PluginHub session resolution instead. - Prevent transient errors from clearing or flipping the active instance by making auto-selection defensive. - Reduce noisy exception handling by catching expected errors explicitly and re-raising system-level interrupts. - Improve observability by logging successful auto-selection at `info` and probe failures at `debug`. ### Description - Added `async def _maybe_autoselect_instance(self, ctx)` which calls `transport.unity_transport._current_transport()` and tries PluginHub `get_sessions()` first, falling back to stdio discovery only when `transport != "http"`. - Limited stdio discovery to `get_unity_connection_pool().discover_all_instances(force_refresh=True)` and only auto-select when exactly one instance is found, and set the session via `set_active_instance`. - Replaced broad `except Exception:` catches with targeted exception tuples and a final handler that re-raises `SystemExit`/`KeyboardInterrupt`, and added clearer `info`/`debug` logging messages. - Updated `_inject_unity_instance` to call `_maybe_autoselect_instance` when there is no `active_instance` and to validate PluginHub session resolution defensively. ### Testing - No automated tests were executed as part of this rollout.
Motivation
PluginHubwhen available and fall back to stdio discovery when not.Description
UnityInstanceMiddleware._maybe_autoselect_instancewhich probesPluginHub.get_sessions()and falls back toget_unity_connection_pool().discover_all_instances(force_refresh=True)to find a sole instance._inject_unity_instanceso that whenget_active_instancereturns none the middleware will attempt to auto-select andset_active_instanceon success.logger.infoand capture probe failures with debug-level logging to avoid noisy errors.Testing
Codex Task
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.