Refactor instance resolution, simplify stdio tools handling, and remove redundant status-file try/catch#12
Merged
whatevertogo merged 1 commit intoFeb 19, 2026
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors Unity instance resolution to be centralized in middleware, simplifies stdio tools state handling, tightens locking semantics, and removes redundant exception handling around stdio status refresh. Sequence diagram for centralized Unity instance resolution in set_active_instance toolsequenceDiagram
actor User
participant SetActiveInstanceTool as set_active_instance_tool
participant UnityInstanceMiddleware
User->>SetActiveInstanceTool: set_active_instance(ctx, instance)
SetActiveInstanceTool->>SetActiveInstanceTool: normalize value from instance
SetActiveInstanceTool->>UnityInstanceMiddleware: _resolve_instance_value(value, ctx)
alt resolution succeeds
UnityInstanceMiddleware-->>SetActiveInstanceTool: resolved_id
SetActiveInstanceTool->>UnityInstanceMiddleware: set_active_instance(ctx, resolved_id)
SetActiveInstanceTool->>UnityInstanceMiddleware: get_session_key(ctx)
UnityInstanceMiddleware-->>SetActiveInstanceTool: session_key
SetActiveInstanceTool-->>User: success, message, instance, session_key
else resolution raises ValueError
UnityInstanceMiddleware-->>SetActiveInstanceTool: ValueError
SetActiveInstanceTool-->>User: success False, error message
end
Updated class diagram for UnityInstanceMiddleware and related toolsclassDiagram
class UnityInstanceMiddleware {
-dict~str,str~ _active_by_key
-RLock _lock
-RLock _metadata_lock
-Lock _session_lock
-set~str~ _unity_managed_tool_names
-dict~str,str~ _tool_alias_to_unity_target
-set~str~ _server_only_tool_names
+tuple _build_stdio_tools_state_signature()
+str _resolve_instance_value(str value, Context ctx)
+void set_active_instance(Context ctx, str instance_id)
+str get_session_key(Context ctx)
}
class SetActiveInstanceTool {
+dict~str,Any~ set_active_instance(Context ctx, str instance)
}
class ManageEditor {
+void RefreshStdioStatusFile()
}
class StdioBridgeHost {
+void RefreshStatusFile(str source)
}
SetActiveInstanceTool --> UnityInstanceMiddleware : uses
ManageEditor --> StdioBridgeHost : calls
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
aaa3260
into
feat/stdio-tool-toggle-from-dev
6 of 7 checks passed
|
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 Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
set_active_instancetool now relies on the private methodUnityInstanceMiddleware._resolve_instance_value; consider promoting this to a public helper or moving the logic to a shared utility to avoid cross-module dependence on a private implementation detail. - Catching a bare
ValueErrorinset_active_instanceand returningstr(exc)directly makes user-visible errors tightly coupled to the middleware implementation; you may want a more structured error contract (e.g., custom exception types or error codes) to keep internal changes from unintentionally altering API responses.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `set_active_instance` tool now relies on the private method `UnityInstanceMiddleware._resolve_instance_value`; consider promoting this to a public helper or moving the logic to a shared utility to avoid cross-module dependence on a private implementation detail.
- Catching a bare `ValueError` in `set_active_instance` and returning `str(exc)` directly makes user-visible errors tightly coupled to the middleware implementation; you may want a more structured error contract (e.g., custom exception types or error codes) to keep internal changes from unintentionally altering API responses.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR refactors Unity instance targeting and stdio tool-state handling to centralize resolution/validation logic in UnityInstanceMiddleware, and removes redundant exception handling around stdio status refresh in the Unity editor integration.
Changes:
- Simplifies stdio enabled-tools signature generation by unifying
set/listhandling. - Switches session tracking lock from
RLocktoLockto reflect non-recursive usage. - Refactors the
set_active_instancetool to reuse middleware instance-resolution logic instead of duplicating discovery/validation. - Removes redundant
try/catcharoundStdioBridgeHost.RefreshStatusFile()inManageEditor.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Server/src/transport/unity_instance_middleware.py | Uses a non-recursive session lock and simplifies stdio tools signature construction; provides centralized instance resolution helper. |
| Server/src/services/tools/set_active_instance.py | Delegates instance parsing/validation to the middleware resolver and simplifies the tool implementation. |
| MCPForUnity/Editor/Tools/ManageEditor.cs | Removes redundant exception handling around stdio status refresh. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+29
to
+30
| try: | ||
| resolved_id = await middleware._resolve_instance_value(value, ctx) |
There was a problem hiding this comment.
set_active_instance is calling UnityInstanceMiddleware._resolve_instance_value(...), which is a private (underscore-prefixed) middleware helper. Since this tool lives outside the middleware module, this creates a brittle cross-module dependency on internal implementation details. Consider exposing a public method (e.g., resolve_instance_value) or a small shared resolver utility, and have both the middleware and tool call that instead of reaching into a private method.
Suggested change
| try: | |
| resolved_id = await middleware._resolve_instance_value(value, ctx) | |
| resolver = getattr(middleware, "resolve_instance_value", None) | |
| if resolver is None or not callable(resolver): | |
| raise RuntimeError( | |
| "Unity instance resolver is not available. " | |
| "Expected UnityInstanceMiddleware.resolve_instance_value(...) to be implemented." | |
| ) | |
| try: | |
| resolved_id = await resolver(value, ctx) |
Owner
Author
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
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
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.
Motivation
StdioBridgeHost.RefreshStatusFile()since the callee already handles errors.Description
UnityInstanceMiddleware._build_stdio_tools_state_signature()by merging duplicateset/listbranches into a singleisinstance(enabled_raw, (set, list))check and normalization of tool names._session_lockfromRLock()toLock()inUnityInstanceMiddlewareto reflect non-recursive usage (Server/src/transport/unity_instance_middleware.py).set_active_instancetool to delegate all port/hash/Name@hash resolution to the middleware methodUnityInstanceMiddleware._resolve_instance_value(...), removing the duplicated resolution/discovery code and reusing middleware validation (Server/src/services/tools/set_active_instance.py).try/catchwrapper fromManageEditor.RefreshStdioStatusFile()sinceStdioBridgeHost.RefreshStatusFile()already catches and logs exceptions (MCPForUnity/Editor/Tools/ManageEditor.cs).Testing
python -m py_compile Server/src/transport/unity_instance_middleware.py Server/src/services/tools/set_active_instance.pywhich succeeded.pytestfor relevant tests, but test collection failed due to a missing runtime dependency (ModuleNotFoundError: No module named 'starlette') in the current environment, so full test execution could not be completed here.Codex Task
Summary by Sourcery
Refactor Unity instance selection and stdio tooling state handling while simplifying error and lock management across server and editor components.
Enhancements:
Summary by CodeRabbit
Refactor