Skip to content

Conversation

@dsarno
Copy link
Owner

@dsarno dsarno commented Jan 1, 2026

Motivation

  • Prefer resolving sessions from PluginHub when HTTP transport is active to avoid probing stdio unnecessarily.
  • Auto-select the single connected Unity instance when no active instance is set to simplify developer experience.
  • Make auto-selection defensive to avoid transient errors clearing or flipping the active instance.
  • Improve observability and reduce noisy catching by tightening exception handling and making logging clearer.

Description

  • Added async def _maybe_autoselect_instance(self, ctx) which prefers PluginHub.get_sessions() for HTTP transport and falls back to stdio discovery only when transport != "http".
  • Limited stdio discovery to call get_unity_connection_pool().discover_all_instances(force_refresh=True) and only auto-select when exactly one instance is found, storing it via set_active_instance.
  • Refined exception handling to catch expected exception types explicitly, re-raise SystemExit/KeyboardInterrupt, and add clearer info/debug logs for probe results.
  • Added integration tests Server/tests/integration/test_instance_autoselect.py and updated Server/tests/integration/conftest.py to include src on sys.path and stub minimal starlette modules to allow isolated test runs.

Testing

  • Ran pytest tests/integration/test_instance_autoselect.py -v which executed the new tests and reported 2 passed.
  • The new tests validate PluginHub-based auto-selection and stdio fallback, and verify that PluginHub calls are not redundantly invoked.
  • Test suite uses asyncio.run(...) to execute middleware async paths and stubs transport submodules to isolate the unit under test.
  • No other automated tests were modified as part of this change.

Codex Task

…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.
@coderabbitai
Copy link

coderabbitai bot commented Jan 1, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link

greptile-apps bot commented Jan 1, 2026

Greptile Summary

This PR implements automatic Unity instance selection when no active instance is set, improving developer experience by removing the need to manually select instances in single-instance scenarios.

Key Changes:

  • Added _maybe_autoselect_instance() method that tries PluginHub first (when configured), then falls back to stdio discovery (when transport != "http")
  • Auto-selection only activates when exactly one Unity instance is discovered to avoid ambiguity
  • Improved exception handling with explicit catching of expected error types (ConnectionError, ValueError, KeyError, TimeoutError, AttributeError) and proper re-raising of SystemExit/KeyboardInterrupt
  • Added defensive logging with debug level for probe failures and info level for successful auto-selection
  • Integrated auto-selection into _inject_unity_instance() workflow to run before every tool/resource call when no instance is set
  • Added integration tests validating both PluginHub and stdio auto-selection paths
  • Updated test infrastructure with src path configuration and starlette module stubs

Minor Issues:

  • Test coverage could be improved by stubbing PluginHub._resolve_session_id in the PluginHub test to validate the full happy path rather than relying on exception handling

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk
  • The implementation is well-structured with defensive exception handling and clear logging. The auto-selection logic is conservative (only selects when exactly 1 instance exists) and non-destructive (doesn't affect existing manual selection). The code follows existing patterns and includes test coverage. Minor deduction for incomplete test stubbing, but this doesn't affect production behavior.
  • No files require special attention

Important Files Changed

Filename Overview
Server/src/transport/unity_instance_middleware.py Added auto-selection logic for single Unity instances via PluginHub (HTTP) and stdio fallback with defensive exception handling
Server/tests/integration/test_instance_autoselect.py Added integration tests for PluginHub and stdio auto-selection paths; missing _resolve_session_id stub in PluginHub test

Sequence Diagram

sequenceDiagram
    participant Client
    participant Middleware as UnityInstanceMiddleware
    participant PluginHub
    participant StdioPool as UnityConnectionPool
    participant Context

    Client->>Middleware: on_call_tool() / on_read_resource()
    Middleware->>Middleware: get_active_instance(ctx)
    
    alt No active instance set
        Middleware->>Middleware: _maybe_autoselect_instance(ctx)
        
        alt PluginHub.is_configured()
            Middleware->>PluginHub: get_sessions()
            PluginHub-->>Middleware: sessions_data
            
            alt Exactly 1 session found
                Middleware->>Middleware: set_active_instance(ctx, chosen)
                Middleware-->>Middleware: return chosen
            else 0 or 2+ sessions
                Note over Middleware: Fall through to stdio check
            end
        end
        
        alt transport != "http"
            Middleware->>StdioPool: discover_all_instances(force_refresh=True)
            StdioPool-->>Middleware: instances
            
            alt Exactly 1 instance found
                Middleware->>Middleware: set_active_instance(ctx, chosen)
                Middleware-->>Middleware: return chosen
            else 0 or 2+ instances
                Middleware-->>Middleware: return None
            end
        end
    end
    
    alt active_instance exists
        alt PluginHub.is_configured()
            Middleware->>PluginHub: _resolve_session_id(active_instance)
            PluginHub-->>Middleware: session_id
            Middleware->>Context: set_state("unity_session_id", session_id)
        end
        Middleware->>Context: set_state("unity_instance", active_instance)
    end
    
    Middleware->>Client: call_next(context)
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +61 to +64
asyncio.run(middleware._inject_unity_instance(middleware_context))

assert ctx.get_state("unity_instance") == "Ramble@deadbeef"
assert call_count["sessions"] == 1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Missing stub for PluginHub._resolve_session_id. When _inject_unity_instance is called with PluginHub configured, it will attempt to call _resolve_session_id(active_instance) on line 187 of the middleware. While the exception handling prevents the test from failing, the test should properly stub this method to avoid relying on exception handling and to ensure the happy path is tested.

Suggested change
asyncio.run(middleware._inject_unity_instance(middleware_context))
assert ctx.get_state("unity_instance") == "Ramble@deadbeef"
assert call_count["sessions"] == 1
asyncio.run(middleware._inject_unity_instance(middleware_context))
assert ctx.get_state("unity_instance") == "Ramble@deadbeef"
# Note: _resolve_session_id is not stubbed, so unity_session_id won't be set
# Consider adding: monkeypatch.setattr(plugin_hub.PluginHub, "_resolve_session_id", lambda inst: "session-1")
assert call_count["sessions"] == 1
Prompt To Fix With AI
This is a comment left during a code review.
Path: Server/tests/integration/test_instance_autoselect.py
Line: 61:64

Comment:
**style:** Missing stub for `PluginHub._resolve_session_id`. When `_inject_unity_instance` is called with PluginHub configured, it will attempt to call `_resolve_session_id(active_instance)` on line 187 of the middleware. While the exception handling prevents the test from failing, the test should properly stub this method to avoid relying on exception handling and to ensure the happy path is tested.

```suggestion
    asyncio.run(middleware._inject_unity_instance(middleware_context))

    assert ctx.get_state("unity_instance") == "Ramble@deadbeef"
    # Note: _resolve_session_id is not stubbed, so unity_session_id won't be set
    # Consider adding: monkeypatch.setattr(plugin_hub.PluginHub, "_resolve_session_id", lambda inst: "session-1")
    assert call_count["sessions"] == 1
```

How can I resolve this? If you propose a fix, please make it concise.

@dsarno dsarno closed this Jan 1, 2026
@dsarno dsarno deleted the codex/clarify-issue-with-custom-tools-in-repo-givkr2 branch January 2, 2026 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants