feat(stdio): add MCP tool toggle support with tools/list_changed notifications#763
Open
whatevertogo wants to merge 16 commits intoCoplayDev:betafrom
Open
feat(stdio): add MCP tool toggle support with tools/list_changed notifications#763whatevertogo wants to merge 16 commits intoCoplayDev:betafrom
whatevertogo wants to merge 16 commits intoCoplayDev:betafrom
Conversation
Align stdio transport tool visibility with HTTP transport behavior: - Add enabled_tools field to stdio status heartbeat file - Filter tools/list based on Unity-side tool enable/disable state - Add manage_editor actions: set_mcp_tool_enabled, get_mcp_tool_enabled, list_mcp_tools - Immediately refresh status file on tool toggle (no heartbeat delay) - Fail-open: missing/invalid status files skip filtering (backward compatible) Unity changes: - ManageEditor.cs: add tool toggle actions with self-lock protection - StdioBridgeHost.cs: include enabled_tools in heartbeat payload Server changes: - manage_editor.py: add new action parameters and bool coercion - unity_instance_middleware.py: stdio tools/list filtering with multi-instance support Tests: - Python: stdio tool filter tests, param coercion tests - Unity: ManageEditorToolToggleTests, TransportCommandDispatcherToolToggleTests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implement MCP tools/list_changed notifications to inform clients when the available tool list changes: - Send notification immediately after set_mcp_tool_enabled succeeds - Start background watcher to poll stdio status file changes - Track MCP sessions and notify on new connections (on_message, on_notification) - Add status file TTL (15s default) to ignore stale status files - Clean up stale sessions when notification send fails New environment variables: - UNITY_MCP_STDIO_TOOLS_WATCH_INTERVAL_SECONDS (default: 1.0, min: 0.2) - UNITY_MCP_STDIO_STATUS_TTL_SECONDS (default: 15.0) Tests: - test_manage_editor_set_mcp_tool_enabled_sends_tool_list_changed - test_stdio_list_tools_ignores_stale_status_files_before_union - test_unity_instance_middleware_tool_list_notifications.py (new) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideAdds stdio-aware MCP tool enable/disable support aligned with HTTP transport: Unity now records enabled tools in the stdio status heartbeat and exposes manage_editor actions to toggle/query/list tools; the Python server filters stdio tools/list based on these status files, sends tools/list_changed notifications on changes and session events, and supports richer per-call unity_instance routing including port-based resolution, with tests and docs updated accordingly. Sequence diagram for stdio MCP tool toggle and tools/list_changed notificationssequenceDiagram
actor User
participant AIClient as AI_client
participant ManageEditorTool as manage_editor_tool_py
participant UnityEditor as Unity_Editor_stdio
participant ManageEditorCS as ManageEditor_cs
participant StdioBridgeHost as StdioBridgeHost_cs
participant StatusFile as Stdio_status_file
participant Server as MCP_Server
participant Middleware as UnityInstanceMiddleware_py
participant Session as MCP_Client_Session
User->>AIClient: Request to disable tool manage_scene
AIClient->>ManageEditorTool: manage_editor(action=set_mcp_tool_enabled, tool_name=manage_scene, enabled=false)
ManageEditorTool->>Server: Send stdio command to Unity (set_mcp_tool_enabled)
Server-->>UnityEditor: Forward stdio command
UnityEditor->>ManageEditorCS: HandleCommand(action=set_mcp_tool_enabled)
ManageEditorCS->>ManageEditorCS: Validate toolName and enabled
ManageEditorCS->>ManageEditorCS: SetMcpToolEnabled(toolName, enabled)
ManageEditorCS->>ToolDiscovery: SetToolEnabled(metadata.Name, false)
ManageEditorCS->>StdioBridgeHost: RefreshStatusFile(reason=tool_toggle)
StdioBridgeHost->>StdioBridgeHost: heartbeatSeq++
StdioBridgeHost->>StatusFile: WriteHeartbeat(project_hash, enabled_tools, last_heartbeat,...)
StatusFile-->>StdioBridgeHost: JSON written
StdioBridgeHost-->>ManageEditorCS: Success
ManageEditorCS-->>UnityEditor: SuccessResponse(success=true, data={toolName, enabled})
UnityEditor-->>Server: Response
Server-->>ManageEditorTool: Response JSON
ManageEditorTool->>ManageEditorTool: Coerce enabled, unwrap success
ManageEditorTool->>Session: ctx.send_tool_list_changed()
Session-->>AIClient: notifications/tools/list_changed
par Background_stdio_tools_watcher
Middleware->>Middleware: start_stdio_tools_watcher()
loop Watch_status_files
Middleware->>StatusFile: _list_stdio_status_payloads()
StatusFile-->>Middleware: payloads(enabled_tools, project_hash,...)
Middleware->>Middleware: _build_stdio_tools_state_signature()
Middleware->>Session: _notify_tool_list_changed_to_sessions(reason=stdio_state_changed)
Session-->>AIClient: notifications/tools/list_changed
end
end
AIClient->>Server: tools/list
Server->>Middleware: on_list_tools
Middleware->>Middleware: _should_filter_tool_listing() -> stdio
Middleware->>Middleware: _resolve_enabled_tool_names_for_context(ctx)
Middleware->>Middleware: _resolve_enabled_tool_names_for_stdio_context(active_instance)
Middleware->>StatusFile: _list_stdio_status_payloads()
StatusFile-->>Middleware: payloads with enabled_tools
Middleware-->>Server: enabled_tool_names set
Server-->>AIClient: Filtered tools/list (manage_scene removed)
Updated class diagram for Unity and server components handling stdio MCP tool togglesclassDiagram
class ManageEditor_cs {
<<static>>
+object HandleCommand(JObject params)
-object SetActiveTool(string toolName)
-object SetMcpToolEnabled(string toolName, bool enabled)
-object GetMcpToolEnabled(string toolName)
-object ListMcpTools()
-void RefreshStdioStatusFile()
}
class StdioBridgeHost_cs {
<<static>>
-int currentUnityPort
-int heartbeatSeq
+void WriteHeartbeat(bool reloading, string reason)
+void RefreshStatusFile(string reason)
}
class ToolDiscovery {
+ToolMetadata GetToolMetadata(string toolName)
+void SetToolEnabled(string toolName, bool enabled)
+bool IsToolEnabled(string toolName)
+IEnumerable~ToolMetadata~ DiscoverAllTools()
+IEnumerable~ToolMetadata~ GetEnabledTools()
}
class ToolMetadata {
+string Name
+bool AutoRegister
+bool IsBuiltIn
}
class UnityInstanceMiddleware_py {
-dict~str,str~ _active_by_key
-RLock _lock
-RLock _metadata_lock
-RLock _session_lock
-set~str~ _unity_managed_tool_names
-dict~str,str~ _tool_alias_to_unity_target
-set~str~ _server_only_tool_names
-float _last_tool_visibility_refresh
-float _tool_visibility_refresh_interval_seconds
-bool _has_logged_empty_registry_warning
-dict~str,object~ _tracked_sessions
-asyncio.Task _stdio_tools_watch_task
-tuple _last_stdio_tools_state_signature
+str get_session_key(ctx)
+void set_active_instance(ctx, str instance_id)
+str|get None get_active_instance(ctx)
+void clear_active_instance(ctx)
+async void on_call_tool(MiddlewareContext context, call_next)
+async void on_message(MiddlewareContext context, call_next)
+async void on_notification(MiddlewareContext context, call_next)
+async void on_read_resource(MiddlewareContext context, call_next)
+async list on_list_tools(MiddlewareContext context, call_next)
+async void start_stdio_tools_watcher()
+async void stop_stdio_tools_watcher()
-bool _is_stdio_transport()
-bool _track_session_from_context(fastmcp_context)
-async void _notify_tool_list_changed_to_sessions(str reason)
-tuple _build_stdio_tools_state_signature()
-float _get_stdio_tools_watch_interval_seconds()
-async void _run_stdio_tools_watch_loop(float interval_seconds)
-bool _should_filter_tool_listing()
-async set~str~|None _resolve_enabled_tool_names_for_context(MiddlewareContext context)
-set~str~|None _resolve_enabled_tool_names_for_stdio_context(str|None active_instance)
-list~dict~ _list_stdio_status_payloads()
-str|None _extract_project_hash_from_filename(Path status_file)
-float _get_stdio_status_ttl_seconds()
-datetime|None _parse_heartbeat_datetime(object raw_heartbeat)
-async list _discover_instances(ctx)
-async str _resolve_instance_value(str value, ctx)
-async str|None _maybe_autoselect_instance(ctx)
-async void _inject_unity_instance(MiddlewareContext context)
}
class manage_editor_tool_py {
+async dict manage_editor(Context ctx, str action, bool|str|None wait_for_completion, str|None tool_name, bool|str|None enabled, str|None tag_name, str|None layer_name)
-logger logger
}
class set_active_instance_tool_py {
+async dict set_active_instance(Context ctx, str instance)
}
class MCP_Session {
+str session_id
+async void send_tool_list_changed()
}
class MCPServiceLocator {
<<static>>
+ToolDiscovery ToolDiscovery
}
ManageEditor_cs ..> MCPServiceLocator : uses
MCPServiceLocator ..> ToolDiscovery : exposes
ManageEditor_cs ..> ToolDiscovery : queries_and_sets
ManageEditor_cs ..> StdioBridgeHost_cs : calls_RefreshStatusFile
StdioBridgeHost_cs ..> ToolDiscovery : reads_enabled_tools_for_status
UnityInstanceMiddleware_py ..> MCP_Session : tracks_sessions
UnityInstanceMiddleware_py ..> manage_editor_tool_py : filters_tools_list
UnityInstanceMiddleware_py ..> set_active_instance_tool_py : shares_routing_logic
manage_editor_tool_py ..> UnityInstanceMiddleware_py : uses_unity_instance_state
manage_editor_tool_py ..> MCP_Session : sends_tools_list_changed
set_active_instance_tool_py ..> UnityInstanceMiddleware_py : set_active_instance
UnityInstanceMiddleware_py ..> StdioBridgeHost_cs : reads_status_files
UnityInstanceMiddleware_py ..> ToolDiscovery : aligns_unity_managed_tool_names
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Unity-side tool enable/disable toggles and status-file fields (project_hash, enabled_tools); server gains a stdio tools watcher, per-session stdio tracking, port/Name@hash per-call resolution, filtering of tools by enabled set, and notifications when visibility changes. Changes
Sequence DiagramsequenceDiagram
participant Editor as Unity Editor
participant StatusFile as Status File
participant Watcher as Stdio Tools Watcher
participant Server as MCP Server
participant Sessions as Active Sessions
Editor->>StatusFile: WriteHeartbeat(project_hash, enabled_tools)
Editor->>StatusFile: RefreshStatusFile(reason)
loop periodic (watcher)
Watcher->>StatusFile: Read recent status files
Watcher->>Watcher: Build tools state signature
alt signature changed
Watcher->>Server: Notify tools/list_changed(reason)
Server->>Sessions: Broadcast tools/list_changed
end
end
Sessions->>Server: Request tools/list
Server->>Watcher: Read enabled_tools / filter by project_hash
Server->>Sessions: Return filtered tool list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Author
|
Note: Please do NOT merge this PR yet. I have only performed basic testing and use them for testing. If you have time, additional testing would be appreciated. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
_notify_tool_list_changed_to_sessions, eachsend_tool_list_changedis awaited sequentially, which could stall the watcher loop or request handling when many sessions are connected; consider usingasyncio.gather(with error handling) to fan out notifications concurrently while still pruning stale sessions.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_notify_tool_list_changed_to_sessions`, each `send_tool_list_changed` is awaited sequentially, which could stall the watcher loop or request handling when many sessions are connected; consider using `asyncio.gather` (with error handling) to fan out notifications concurrently while still pruning stale sessions.
## Individual Comments
### Comment 1
<location> `Server/src/services/tools/manage_editor.py:30-34` </location>
<code_context>
tool_name: Annotated[str,
- "Tool name when setting active tool"] | None = None,
+ "Tool name when setting active tool or updating/querying MCP tool enabled state"] | None = None,
+ enabled: Annotated[bool | str,
+ "Optional. Required for set_mcp_tool_enabled (accepts true/false or 'true'/'false')"] | None = None,
tag_name: Annotated[str,
</code_context>
<issue_to_address>
**issue (bug_risk):** Only include the `enabled` parameter in Unity command params when it’s actually provided
`enabled_value` is always added to `params`, even when the caller omits `enabled` and it’s `None`. Unity’s `ManageEditor.HandleCommand` then sees an `enabled` field for `set_mcp_tool_enabled`, fails to coerce it to a boolean, and errors.
To improve this:
- Skip adding `"enabled"` to `params` when `enabled_value is None`.
- Additionally, when `action == "set_mcp_tool_enabled"` and `enabled_value is None`, consider returning a clear validation error from the MCP tool instead of relying on Unity’s error.
This keeps parameter validation on the MCP side and avoids confusing Unity-side failures.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
And the May not good as you think,but i think it's ok |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@MCPForUnity/Editor/Tools/ManageEditor.cs`:
- Around line 300-310: Guard the call to StdioBridgeHost.RefreshStatusFile in
RefreshStdioStatusFile so you only touch the stdio status when the stdio bridge
is actually active: check a runtime flag like StdioBridgeHost.IsRunning (or
equivalent) before calling RefreshStatusFile("tool_toggle") to avoid creating
stale status files when using HTTP transport, and keep the existing try/catch
logging (McpLog.Warn) for any unexpected exceptions from RefreshStatusFile.
In `@Server/README.md`:
- Around line 167-193: Add the two new environment variable docs to the
"Environment variables" section: document
UNITY_MCP_STDIO_TOOLS_WATCH_INTERVAL_SECONDS (interval in seconds to poll/watch
Unity status file for tool enable/disable changes) and
UNITY_MCP_STDIO_STATUS_TTL_SECONDS (time-to-live in seconds for cached Unity
status before refresh). Include default values, expected type (integer seconds),
and a short sentence about their effect on tools/list behavior and immediate
status-file refresh. Place these entries near the existing env var list around
the current "Environment variables" section so they are discoverable with the
other settings.
In `@Server/src/transport/unity_instance_middleware.py`:
- Around line 184-202: The branches handling enabled_raw for set and list are
duplicated; collapse them by checking isinstance(enabled_raw, (set, list)) and
run the same normalization logic to produce enabled_tools from
payload.get("enabled_tools"). Update the code that currently references
enabled_raw and enabled_tools so the tuple(sorted(...)) comprehension (filtering
isinstance(tool_name, str) and tool_name) is executed once for the combined
branch, and keep the else branch that sets enabled_tools = () unchanged.
- Around line 601-688: _list_stdio_status_payloads performs blocking filesystem
operations (glob, stat, open, json.load) on the event loop which can stall async
code; change it to run the blocking work in a thread using asyncio.to_thread
(either wrap the entire _list_stdio_status_payloads body in a to_thread call or
split out a helper like _list_stdio_status_payloads_blocking and call it via
await asyncio.to_thread(...)). Ensure callers such as
_resolve_enabled_tool_names_for_context and on_list_tools await the async
wrapper; keep logic identical but move all synchronous FS/JSON operations into
the threaded helper so the event loop is never blocked.
In `@Server/tests/test_unity_instance_middleware_tool_list_notifications.py`:
- Around line 79-108: The test
test_stdio_tools_watcher_notifies_on_signature_change is timing-sensitive and
may flake; replace the fixed asyncio.sleep with a deterministic synchronization:
create an asyncio.Event in the test, have the fake
_build_stdio_tools_state_signature implementation set the event when it has been
called the desired number of times (e.g., after the second signature change),
then await event.wait() with a timeout before stopping the watcher; update
references to middleware._build_stdio_tools_state_signature,
middleware.start_stdio_tools_watcher, middleware.stop_stdio_tools_watcher, and
middleware._notify_tool_list_changed_to_sessions accordingly so the test waits
on the event instead of sleeping.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportCommandDispatcherToolToggleTests.cs`:
- Around line 83-100: The test currently uses reflection to locate
TransportCommandDispatcher and call ExecuteCommandJsonAsync; replace that with a
direct call using the internal type made accessible by InternalsVisibleTo: call
TransportCommandDispatcher.ExecuteCommandJsonAsync(commandJson,
CancellationToken.None) directly (remove the Type.GetType, GetMethod,
MethodInfo.Invoke and Task cast code) and return the task result via
.GetAwaiter().GetResult() as before; ensure you reference the
TransportCommandDispatcher and ExecuteCommandJsonAsync symbols directly in the
test.
- Around line 78-81: Replace the numeric negative assertion on the 'error'
string with a clearer NUnit negative containment assertion: in the test method
inside TransportCommandDispatcherToolToggleTests (where 'error' is assigned from
response["error"]), swap Assert.Less(error.IndexOf("disabled in the Unity
Editor", StringComparison.OrdinalIgnoreCase), 0) for
StringAssert.DoesNotContain("disabled in the Unity Editor", error,
StringComparison.OrdinalIgnoreCase) or the equivalent Assert.That(error,
Does.Not.Contain("disabled in the Unity Editor").IgnoreCase) to improve
readability and failure messages.
🧹 Nitpick comments (5)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/ManageEditor.cs`: - Around line 300-310: Guard the call to StdioBridgeHost.RefreshStatusFile in RefreshStdioStatusFile so you only touch the stdio status when the stdio bridge is actually active: check a runtime flag like StdioBridgeHost.IsRunning (or equivalent) before calling RefreshStatusFile("tool_toggle") to avoid creating stale status files when using HTTP transport, and keep the existing try/catch logging (McpLog.Warn) for any unexpected exceptions from RefreshStatusFile. In `@Server/src/transport/unity_instance_middleware.py`: - Around line 184-202: The branches handling enabled_raw for set and list are duplicated; collapse them by checking isinstance(enabled_raw, (set, list)) and run the same normalization logic to produce enabled_tools from payload.get("enabled_tools"). Update the code that currently references enabled_raw and enabled_tools so the tuple(sorted(...)) comprehension (filtering isinstance(tool_name, str) and tool_name) is executed once for the combined branch, and keep the else branch that sets enabled_tools = () unchanged. - Around line 601-688: _list_stdio_status_payloads performs blocking filesystem operations (glob, stat, open, json.load) on the event loop which can stall async code; change it to run the blocking work in a thread using asyncio.to_thread (either wrap the entire _list_stdio_status_payloads body in a to_thread call or split out a helper like _list_stdio_status_payloads_blocking and call it via await asyncio.to_thread(...)). Ensure callers such as _resolve_enabled_tool_names_for_context and on_list_tools await the async wrapper; keep logic identical but move all synchronous FS/JSON operations into the threaded helper so the event loop is never blocked. In `@Server/tests/test_unity_instance_middleware_tool_list_notifications.py`: - Around line 79-108: The test test_stdio_tools_watcher_notifies_on_signature_change is timing-sensitive and may flake; replace the fixed asyncio.sleep with a deterministic synchronization: create an asyncio.Event in the test, have the fake _build_stdio_tools_state_signature implementation set the event when it has been called the desired number of times (e.g., after the second signature change), then await event.wait() with a timeout before stopping the watcher; update references to middleware._build_stdio_tools_state_signature, middleware.start_stdio_tools_watcher, middleware.stop_stdio_tools_watcher, and middleware._notify_tool_list_changed_to_sessions accordingly so the test waits on the event instead of sleeping. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportCommandDispatcherToolToggleTests.cs`: - Around line 78-81: Replace the numeric negative assertion on the 'error' string with a clearer NUnit negative containment assertion: in the test method inside TransportCommandDispatcherToolToggleTests (where 'error' is assigned from response["error"]), swap Assert.Less(error.IndexOf("disabled in the Unity Editor", StringComparison.OrdinalIgnoreCase), 0) for StringAssert.DoesNotContain("disabled in the Unity Editor", error, StringComparison.OrdinalIgnoreCase) or the equivalent Assert.That(error, Does.Not.Contain("disabled in the Unity Editor").IgnoreCase) to improve readability and failure messages.MCPForUnity/Editor/Tools/ManageEditor.cs (1)
300-310:RefreshStdioStatusFile— consider thatStdioBridgeHostmay not be active.When using HTTP transport (not stdio), calling
StdioBridgeHost.RefreshStatusFilemay be a no-op or could write an unnecessary status file. The current exception handling prevents failures, but it might be worth a guard check (e.g.,StdioBridgeHost.IsRunning) to avoid writing a stale/orphaned status file when stdio isn't the active transport.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/ManageEditor.cs` around lines 300 - 310, Guard the call to StdioBridgeHost.RefreshStatusFile in RefreshStdioStatusFile so you only touch the stdio status when the stdio bridge is actually active: check a runtime flag like StdioBridgeHost.IsRunning (or equivalent) before calling RefreshStatusFile("tool_toggle") to avoid creating stale status files when using HTTP transport, and keep the existing try/catch logging (McpLog.Warn) for any unexpected exceptions from RefreshStatusFile.Server/src/transport/unity_instance_middleware.py (2)
184-202: Duplicate branches forsetandlisttypes.The
isinstance(enabled_raw, set)andisinstance(enabled_raw, list)branches (lines 185-200) contain identical logic. They can be collapsed.♻️ Suggested simplification
- if isinstance(enabled_raw, set): - enabled_tools = tuple( - sorted( - tool_name - for tool_name in enabled_raw - if isinstance(tool_name, str) and tool_name - ) - ) - elif isinstance(enabled_raw, list): - enabled_tools = tuple( - sorted( - tool_name - for tool_name in enabled_raw - if isinstance(tool_name, str) and tool_name - ) - ) - else: + if isinstance(enabled_raw, (set, list)): + enabled_tools = tuple( + sorted( + tool_name + for tool_name in enabled_raw + if isinstance(tool_name, str) and tool_name + ) + ) + else:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/transport/unity_instance_middleware.py` around lines 184 - 202, The branches handling enabled_raw for set and list are duplicated; collapse them by checking isinstance(enabled_raw, (set, list)) and run the same normalization logic to produce enabled_tools from payload.get("enabled_tools"). Update the code that currently references enabled_raw and enabled_tools so the tuple(sorted(...)) comprehension (filtering isinstance(tool_name, str) and tool_name) is executed once for the combined branch, and keep the else branch that sets enabled_tools = () unchanged.
601-688: Synchronous file I/O on the async event loop.
_list_stdio_status_payloadsperforms synchronousglob,stat,open, andjson.loadcalls. This is invoked from:
- The watcher loop (periodic, low impact)
on_list_tools→_resolve_enabled_tool_names_for_context(on every tool-list request)For small, local status files this is unlikely to be a practical problem, but under pathological conditions (e.g., network-mounted home directory, slow disk) it could stall the event loop.
If this ever becomes an issue, wrapping the I/O in
asyncio.to_threadwould be the standard fix. Not blocking for this PR, just noting it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/transport/unity_instance_middleware.py` around lines 601 - 688, _list_stdio_status_payloads performs blocking filesystem operations (glob, stat, open, json.load) on the event loop which can stall async code; change it to run the blocking work in a thread using asyncio.to_thread (either wrap the entire _list_stdio_status_payloads body in a to_thread call or split out a helper like _list_stdio_status_payloads_blocking and call it via await asyncio.to_thread(...)). Ensure callers such as _resolve_enabled_tool_names_for_context and on_list_tools await the async wrapper; keep logic identical but move all synchronous FS/JSON operations into the threaded helper so the event loop is never blocked.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportCommandDispatcherToolToggleTests.cs (1)
78-81: Consider a more readable negative assertion.
Assert.Less(error.IndexOf(...), 0)works but obscures intent. NUnit providesStringAssert.DoesNotContain(orAssert.ThatwithDoes.Not.Contain) which reads more clearly and produces a better failure message.♻️ Suggested improvement
- Assert.Less(error.IndexOf("disabled in the Unity Editor", StringComparison.OrdinalIgnoreCase), 0); + Assert.That(error, Does.Not.Contain("disabled in the Unity Editor").IgnoreCase);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportCommandDispatcherToolToggleTests.cs` around lines 78 - 81, Replace the numeric negative assertion on the 'error' string with a clearer NUnit negative containment assertion: in the test method inside TransportCommandDispatcherToolToggleTests (where 'error' is assigned from response["error"]), swap Assert.Less(error.IndexOf("disabled in the Unity Editor", StringComparison.OrdinalIgnoreCase), 0) for StringAssert.DoesNotContain("disabled in the Unity Editor", error, StringComparison.OrdinalIgnoreCase) or the equivalent Assert.That(error, Does.Not.Contain("disabled in the Unity Editor").IgnoreCase) to improve readability and failure messages.Server/tests/test_unity_instance_middleware_tool_list_notifications.py (1)
79-108: Timing-sensitive watcher test could be flaky under load.The test relies on
asyncio.sleep(0.35)being sufficient for at least one watcher iteration at 0.2s interval. On a loaded CI runner, event-loop scheduling delays could cause the watcher to complete zero iterations before the sleep returns.Consider using a more deterministic synchronization mechanism (e.g., an
asyncio.Eventset inside the fake signature function after the second call, thenawait event.wait()with a timeout instead of a fixed sleep).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/tests/test_unity_instance_middleware_tool_list_notifications.py` around lines 79 - 108, The test test_stdio_tools_watcher_notifies_on_signature_change is timing-sensitive and may flake; replace the fixed asyncio.sleep with a deterministic synchronization: create an asyncio.Event in the test, have the fake _build_stdio_tools_state_signature implementation set the event when it has been called the desired number of times (e.g., after the second signature change), then await event.wait() with a timeout before stopping the watcher; update references to middleware._build_stdio_tools_state_signature, middleware.start_stdio_tools_watcher, middleware.stop_stdio_tools_watcher, and middleware._notify_tool_list_changed_to_sessions accordingly so the test waits on the event instead of sleeping.
...ts/UnityMCPTests/Assets/Tests/EditMode/Services/TransportCommandDispatcherToolToggleTests.cs
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds MCP tool toggle support for stdio transport, bringing it into feature parity with HTTP transport. Users can now enable/disable individual MCP tools from the Unity Editor, with changes immediately reflected in the AI client via tools/list filtering and tools/list_changed notifications.
Changes:
- Added three new actions to
manage_editortool:set_mcp_tool_enabled,get_mcp_tool_enabled, andlist_mcp_tools, with self-protection to prevent disablingmanage_editoritself - Unity stdio status files now include
enabled_toolsandproject_hashfields, with immediate refresh on toggle (no heartbeat delay) - Python middleware implements stdio
tools/listfiltering based on status files, session tracking for MCP clients, and a background watcher that detects tool state changes and sendstools/list_changednotifications
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| MCPForUnity/Editor/Tools/ManageEditor.cs | Adds three new MCP tool management actions with manage_editor self-lock protection |
| MCPForUnity/Editor/Services/Transport/Transports/StdioBridgeHost.cs | Adds enabled_tools and project_hash to status file heartbeat; exposes RefreshStatusFile for immediate updates |
| Server/src/transport/unity_instance_middleware.py | Implements stdio tools/list filtering, session tracking, background watcher for state changes, and tools/list_changed notifications |
| Server/src/services/tools/manage_editor.py | Adds parameter validation for new actions and sends tools/list_changed after successful toggle |
| Server/src/main.py | Manages stdio tools watcher lifecycle in server lifespan |
| Server/README.md | Documents new manage_editor actions and stdio tool toggle behavior |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageEditorToolToggleTests.cs | Tests for new manage_editor actions including self-lock protection |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportCommandDispatcherToolToggleTests.cs | Integration tests for disabled tool blocking |
| Server/tests/test_manage_editor_param_coercion.py | Tests boolean parameter coercion and tools/list_changed notification |
| Server/tests/test_unity_instance_middleware_stdio_tool_filter.py | Comprehensive tests for stdio filtering logic including edge cases |
| Server/tests/test_unity_instance_middleware_tool_list_notifications.py | Tests for session tracking and notification delivery |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1026
to
+1036
| string[] enabledTools = Array.Empty<string>(); | ||
| try | ||
| { | ||
| var toolMetadata = MCPServiceLocator.ToolDiscovery.GetEnabledTools(); | ||
| enabledTools = toolMetadata | ||
| ?.Select(tool => tool?.Name) | ||
| .Where(name => !string.IsNullOrWhiteSpace(name)) | ||
| .Distinct(StringComparer.Ordinal) | ||
| .OrderBy(name => name, StringComparer.Ordinal) | ||
| .ToArray() | ||
| ?? Array.Empty<string>(); |
There was a problem hiding this comment.
The enabled_tools array is built by filtering GetEnabledTools() which only returns enabled tools. This means when all Unity-managed tools are disabled, enabled_tools will be an empty array. However, an empty array is semantically different from a missing field - it explicitly states "no tools are enabled" rather than "state unknown".
The Python middleware correctly handles this case by checking for an empty set and filtering out all Unity-managed tools while keeping server-only tools visible. However, this semantic difference could be documented more clearly - an empty array means "all Unity tools disabled" while a missing field means "backward compatibility: no filtering".
|
|
||
| Tool toggle changes trigger an immediate status-file refresh, so `tools/list` | ||
| updates do not depend on waiting for the next heartbeat. | ||
|
|
There was a problem hiding this comment.
The documentation describes the new tool toggle functionality but doesn't document the new environment variables mentioned in the PR description:
- UNITY_MCP_STDIO_TOOLS_WATCH_INTERVAL_SECONDS (default: 1.0, min: 0.2)
- UNITY_MCP_STDIO_STATUS_TTL_SECONDS (default: 15.0)
These should be added to the environment variables section of the README to maintain consistency with other configuration options.
Suggested change
| #### Tool toggle environment variables | |
| These environment variables control how often the server checks for tool/status updates | |
| and how long status information is considered valid: | |
| - `UNITY_MCP_STDIO_TOOLS_WATCH_INTERVAL_SECONDS` | |
| Interval in seconds between checks for tool/status file changes. | |
| Default: `1.0` (minimum: `0.2`). | |
| - `UNITY_MCP_STDIO_STATUS_TTL_SECONDS` | |
| Time-to-live in seconds for status information before it is considered stale. | |
| Default: `15.0`. |
… tool watch interval
Any tool call can now include a unity_instance parameter to route that
specific call to a target Unity instance without changing the session
default and without requiring a set_active_instance call first.
The middleware pops unity_instance from tool call arguments before
Pydantic validation runs, resolves it (port number, hash prefix, or
Name@hash), and injects it into request-scoped state for that call only.
- Port numbers resolve to the matching Name@hash via status file lookup
rather than synthetic direct:{port} IDs, so the transport layer can
route them correctly
- HTTP mode rejects port-based targeting with a clear error
- set_active_instance now also accepts port numbers for consistency
- Multi-instance scenarios log available instances with ports when
auto-select cannot choose
- _discover_instances() helper DRYs up transport-aware instance
discovery previously duplicated across the codebase
- Server instructions updated to document both routing approaches
- 18 new tests covering pop behaviour, per-call vs session routing,
port resolution, transport modes, and edge cases
Closes CoplayDev#697
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ol toggle Merge PR CoplayDev#772 which adds per-call unity_instance routing support: - Middleware intercepts unity_instance from tool call arguments - Supports port number (stdio), Name@hash, and hash prefix resolution - Per-call routing does not persist to session state Conflicts resolved in unity_instance_middleware.py: - Kept stdio tool toggle methods (session tracking, notifications) - Integrated per-call routing before session fallback - Added section comments for code organization Co-Authored-By: dsarno <2432103+dsarno@users.noreply.github.com>
…togo/unity-mcp into feat/stdio-tool-toggle-from-dev
Add comprehensive edge case tests (34 tests) for stdio tool filtering: - enabled_tools parsing (empty list, null elements, wrong type) - project_hash handling (missing, empty, filename fallback) - heartbeat parsing (timezone formats, TTL boundary) - TTL and watch interval configuration validation - session tracking edge cases - watcher lifecycle (start/stop, error recovery) - signature building and deduplication - status directory handling - concurrent operations safety Code review fixes: - Guard RefreshStdioStatusFile to only run when StdioBridgeHost.IsRunning - Use asyncio.gather for concurrent notification dispatch - Collapse duplicate enabled_raw handling for set/list types - Replace timing-based test with deterministic Event synchronization - Use direct call instead of reflection in C# tests (InternalsVisibleTo) - Use StringAssert.DoesNotContain for clearer negative assertion Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add tests for handling future/partial schema without enabled_tools field and unknown extra fields in status files to ensure backward/forward compatibility.
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The instance-resolution logic for
unity_instance(port/name/hash handling) is now duplicated betweenUnityInstanceMiddleware._resolve_instance_valueand theset_active_instancetool; consider centralizing this into a shared helper to avoid divergence in behavior and error messaging over time. - In
ManageEditor.ListMcpToolsthe broadcatch (Exception e)returns an error to the client but only surfaces the message string; adding an MCP log entry (e.g., viaMcpLog.Warn) would make diagnosing unexpected failures in tool discovery much easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The instance-resolution logic for `unity_instance` (port/name/hash handling) is now duplicated between `UnityInstanceMiddleware._resolve_instance_value` and the `set_active_instance` tool; consider centralizing this into a shared helper to avoid divergence in behavior and error messaging over time.
- In `ManageEditor.ListMcpTools` the broad `catch (Exception e)` returns an error to the client but only surfaces the message string; adding an MCP log entry (e.g., via `McpLog.Warn`) would make diagnosing unexpected failures in tool discovery much easier.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
Server/src/transport/unity_instance_middleware.py (3)
190-207: Duplicate branches forsetandlisttypes produce identical logic.The
isinstance(enabled_raw, set)andisinstance(enabled_raw, list)branches execute the same sorted-tuple comprehension. Collapse them into a single check.♻️ Proposed simplification
- if isinstance(enabled_raw, set): - enabled_tools = tuple( - sorted( - tool_name - for tool_name in enabled_raw - if isinstance(tool_name, str) and tool_name - ) - ) - elif isinstance(enabled_raw, list): - enabled_tools = tuple( - sorted( - tool_name - for tool_name in enabled_raw - if isinstance(tool_name, str) and tool_name - ) - ) - else: + if isinstance(enabled_raw, (set, list)): + enabled_tools = tuple( + sorted( + tool_name + for tool_name in enabled_raw + if isinstance(tool_name, str) and tool_name + ) + ) + else: enabled_tools = ()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/transport/unity_instance_middleware.py` around lines 190 - 207, The branches for checking enabled_raw as set and list in the code that assigns enabled_tools are duplicated; replace the two separate isinstance checks with a single combined check (e.g., isinstance(enabled_raw, (set, list))) and keep the existing comprehension that filters strings and sorts into a tuple, leaving the else branch returning (). Update the logic around the enabled_raw -> enabled_tools assignment (references: variable names enabled_raw and enabled_tools) so only one branch handles both collection types and produces the same sorted tuple result.
224-239: Watcher loop swallows all non-cancellation exceptions silently atdebuglevel.If
_build_stdio_tools_state_signatureor_notify_tool_list_changed_to_sessionsraises a persistent error (e.g., permissions issue on the status directory), the loop will silently retry every interval, flooding debug logs without any escalation. Consider adding a consecutive-failure counter that escalates towarningafter N iterations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/transport/unity_instance_middleware.py` around lines 224 - 239, The watcher loop in _run_stdio_tools_watch_loop currently catches all exceptions and logs them at debug, which hides persistent failures; add a consecutive-failure counter inside _run_stdio_tools_watch_loop that is reset to 0 after a successful iteration (i.e., after calling _build_stdio_tools_state_signature and, when changed, _notify_tool_list_changed_to_sessions) and incremented inside the except Exception block, and when the counter exceeds a threshold N (choose e.g., 5) log at warning level (using logger.warning with exc_info=True and contextual text) instead of debug; ensure asyncio.CancelledError is still re-raised and that the counter is cleared on success so only consecutive failures escalate.
117-123: Transport mode check is repeated across many methods — consider a cached property or instance field.
(config.transport_mode or "stdio").lower()is evaluated in_is_stdio_transport,_should_filter_tool_listing,_resolve_enabled_tool_names_for_context,_resolve_instance_value,_discover_instances, and_maybe_autoselect_instance. Sinceconfig.transport_modedoesn't change after startup, a single cached read would reduce repetition and risk of inconsistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/transport/unity_instance_middleware.py` around lines 117 - 123, The repeated inline check "(config.transport_mode or 'stdio').lower() == 'stdio'" should be read once and cached: add a normalized transport_mode boolean or string field on the UnityInstanceMiddleware instance (e.g., self._transport_mode or self._is_stdio_transport_mode) during construction or via a `@cached_property`, initialize it to (config.transport_mode or "stdio").lower() (or compare to "stdio" immediately), and replace all uses in _is_stdio_transport, _should_filter_tool_listing, _resolve_enabled_tool_names_for_context, _resolve_instance_value, _discover_instances, and _maybe_autoselect_instance to reference that cached field instead of re-evaluating the expression.Server/src/services/tools/set_active_instance.py (2)
27-58: Port-resolution logic duplicates_resolve_instance_valuein the middleware.The middleware's
_resolve_instance_valuealready handles port numbers, Name@hash, and hash-prefix resolution with the same semantics.set_active_instancecould delegate to it (catchingValueErrorand converting to{"success": False, "error": ...}dicts) to keep a single source of truth for resolution logic. Not urgent but worth tracking to prevent drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/tools/set_active_instance.py` around lines 27 - 58, The port-resolution block in set_active_instance duplicates middleware logic; replace the manual port discovery with a call to the middleware's resolver: obtain middleware via get_unity_instance_middleware(), call its _resolve_instance_value(ctx, instance_or_value) to get the resolved_id (wrap in try/except to catch ValueError), on success call middleware.set_active_instance(ctx, resolved_id) and return the same success dict including middleware.get_session_key(ctx), and on ValueError return {"success": False, "error": str(err)} so set_active_instance delegates resolution to _resolve_instance_value and preserves existing return shapes.
27-58:valueis computed twice identically.
value = (instance or "").strip()appears at line 28 and again at line 91. The second assignment is redundant since the port-detection early-return at lines 29-58 meansvalueis unchanged when execution reaches line 91. Remove the duplicate on line 91.♻️ Proposed fix
- value = (instance or "").strip() - if not value: + if not value:Also applies to: 91-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/tools/set_active_instance.py` around lines 27 - 58, In set_active_instance, there is a redundant re-assignment of value using "value = (instance or \"\").strip()" later in the function; remove the duplicate so value is only computed once at the top (keep the initial assignment), and ensure downstream logic (port-based detection that returns early and subsequent code that uses value) remains unchanged; search for the exact string "value = (instance or \"\").strip()" and delete the later occurrence to avoid the duplicate computation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Server/src/transport/unity_instance_middleware.py`:
- Around line 593-607: In on_message, avoid sending tools/list_changed to the
session that just registered; change the logic in on_message (which calls
_track_session_from_context(context.fastmcp_context)) so that if
_track_session_from_context reports a new session you do not call
_notify_tool_list_changed_to_sessions for that newly-registered session —
instead only notify other already-tracked sessions (i.e., defer notifying this
session until on_notification handles "notifications/initialized"); keep the
existing behavior in on_notification (which calls _track_session_from_context
and awaits _notify_tool_list_changed_to_sessions when context.method ==
"notifications/initialized").
---
Nitpick comments:
In `@Server/src/services/tools/set_active_instance.py`:
- Around line 27-58: The port-resolution block in set_active_instance duplicates
middleware logic; replace the manual port discovery with a call to the
middleware's resolver: obtain middleware via get_unity_instance_middleware(),
call its _resolve_instance_value(ctx, instance_or_value) to get the resolved_id
(wrap in try/except to catch ValueError), on success call
middleware.set_active_instance(ctx, resolved_id) and return the same success
dict including middleware.get_session_key(ctx), and on ValueError return
{"success": False, "error": str(err)} so set_active_instance delegates
resolution to _resolve_instance_value and preserves existing return shapes.
- Around line 27-58: In set_active_instance, there is a redundant re-assignment
of value using "value = (instance or \"\").strip()" later in the function;
remove the duplicate so value is only computed once at the top (keep the initial
assignment), and ensure downstream logic (port-based detection that returns
early and subsequent code that uses value) remains unchanged; search for the
exact string "value = (instance or \"\").strip()" and delete the later
occurrence to avoid the duplicate computation.
In `@Server/src/transport/unity_instance_middleware.py`:
- Around line 190-207: The branches for checking enabled_raw as set and list in
the code that assigns enabled_tools are duplicated; replace the two separate
isinstance checks with a single combined check (e.g., isinstance(enabled_raw,
(set, list))) and keep the existing comprehension that filters strings and sorts
into a tuple, leaving the else branch returning (). Update the logic around the
enabled_raw -> enabled_tools assignment (references: variable names enabled_raw
and enabled_tools) so only one branch handles both collection types and produces
the same sorted tuple result.
- Around line 224-239: The watcher loop in _run_stdio_tools_watch_loop currently
catches all exceptions and logs them at debug, which hides persistent failures;
add a consecutive-failure counter inside _run_stdio_tools_watch_loop that is
reset to 0 after a successful iteration (i.e., after calling
_build_stdio_tools_state_signature and, when changed,
_notify_tool_list_changed_to_sessions) and incremented inside the except
Exception block, and when the counter exceeds a threshold N (choose e.g., 5) log
at warning level (using logger.warning with exc_info=True and contextual text)
instead of debug; ensure asyncio.CancelledError is still re-raised and that the
counter is cleared on success so only consecutive failures escalate.
- Around line 117-123: The repeated inline check "(config.transport_mode or
'stdio').lower() == 'stdio'" should be read once and cached: add a normalized
transport_mode boolean or string field on the UnityInstanceMiddleware instance
(e.g., self._transport_mode or self._is_stdio_transport_mode) during
construction or via a `@cached_property`, initialize it to (config.transport_mode
or "stdio").lower() (or compare to "stdio" immediately), and replace all uses in
_is_stdio_transport, _should_filter_tool_listing,
_resolve_enabled_tool_names_for_context, _resolve_instance_value,
_discover_instances, and _maybe_autoselect_instance to reference that cached
field instead of re-evaluating the expression.
Comment on lines
593
to
607
| async def on_message(self, context: MiddlewareContext, call_next): | ||
| if self._is_stdio_transport(): | ||
| is_new_session = self._track_session_from_context(context.fastmcp_context) | ||
| if is_new_session: | ||
| await self._notify_tool_list_changed_to_sessions("session_registered") | ||
|
|
||
| return await call_next(context) | ||
|
|
||
| async def on_notification(self, context: MiddlewareContext, call_next): | ||
| if self._is_stdio_transport(): | ||
| self._track_session_from_context(context.fastmcp_context) | ||
| if context.method == "notifications/initialized": | ||
| await self._notify_tool_list_changed_to_sessions("client_initialized") | ||
|
|
||
| return await call_next(context) |
Contributor
There was a problem hiding this comment.
Premature tools/list_changed notification in on_message before client sends initialized.
on_message fires on the very first client message (the initialize request), tracking the session and immediately sending tools/list_changed to it. Per the MCP protocol, the server should not send notifications until after the client sends the notifications/initialized notification. The on_notification hook at line 601 already handles the correct notifications/initialized trigger.
The failure is caught and the session is pruned (self-corrects via re-tracking in on_notification), so this isn't a crash risk — but it's a wasted round-trip, a spurious debug log, and a protocol-level concern.
Consider deferring notification in on_message to only sessions that were already tracked (i.e., notify other sessions about a new peer, not the newly registered one):
Proposed fix
async def on_message(self, context: MiddlewareContext, call_next):
if self._is_stdio_transport():
is_new_session = self._track_session_from_context(context.fastmcp_context)
- if is_new_session:
- await self._notify_tool_list_changed_to_sessions("session_registered")
+ # Don't notify the new session itself — it hasn't sent `initialized` yet.
+ # Other already-tracked sessions will learn about tool changes via the
+ # background watcher or `on_notification` when `initialized` arrives.
return await call_next(context)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Server/src/transport/unity_instance_middleware.py` around lines 593 - 607, In
on_message, avoid sending tools/list_changed to the session that just
registered; change the logic in on_message (which calls
_track_session_from_context(context.fastmcp_context)) so that if
_track_session_from_context reports a new session you do not call
_notify_tool_list_changed_to_sessions for that newly-registered session —
instead only notify other already-tracked sessions (i.e., defer notifying this
session until on_notification handles "notifications/initialized"); keep the
existing behavior in on_notification (which calls _track_session_from_context
and awaits _notify_tool_list_changed_to_sessions when context.method ==
"notifications/initialized").
When set_mcp_tool_enabled is called, now reregisters tools with HTTP transport (PluginHub) in addition to refreshing stdio status file. This ensures MCP clients receive tools/list_changed notifications for both transport modes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dsarno
reviewed
Feb 19, 2026
Collaborator
dsarno
left a comment
dsarno
left a comment
There was a problem hiding this comment.
Review
Good feature -- the stdio tool toggle brings needed parity with HTTP. A few items to clean up:
1. Duplicated branches in _build_stdio_tools_state_signature (unity_instance_middleware.py)
The set and list branches are identical:
if isinstance(enabled_raw, set):
enabled_tools = tuple(sorted(...))
elif isinstance(enabled_raw, list):
enabled_tools = tuple(sorted(...)) # same codeSimplify to isinstance(enabled_raw, (set, list)).
2. Port resolution logic duplicated
set_active_instance.py and _resolve_instance_value() in the middleware both implement nearly identical port-to-Name@hash resolution (~30 lines each). set_active_instance could delegate to the middleware method to DRY this up.
3. Double try-catch on status file refresh (C#)
ManageEditor.RefreshStdioStatusFile() wraps StdioBridgeHost.RefreshStatusFile() in a try-catch, but RefreshStatusFile itself already has a try-catch. The outer catch will never fire. Remove one.
4. Potential double notification on session init
on_message notifies on new sessions, and on_notification notifies unconditionally on notifications/initialized. If on_message fires first, the client gets two tools/list_changed in quick succession. Harmless but noisy -- worth a comment or guard.
5. _session_lock is RLock but no recursive locking occurs
A plain Lock() would be clearer about intent since there is no recursive acquisition.
6. Consider protecting set_active_instance from disabling
Only manage_editor is self-lock protected. If set_active_instance gets disabled, multi-instance users lose the ability to switch (per-call unity_instance still works, but the dedicated tool will not show). Worth at least documenting why only manage_editor is protected.
7. Timing-dependent test
test_stdio_tools_watcher_notifies_on_signature_change sleeps 0.35s with a 0.2s interval -- could be flaky in slow CI. Consider an event-based sync or a more generous margin.
8. _list_stdio_status_payloads density
~90 lines with 5 levels of nesting. Extracting per-file parsing into a helper like _parse_single_status_file(path, now_utc, ttl) -> dict | None would improve readability.
Items 1-3 are the most straightforward to fix. Rest are suggestions.
Contributor
Author
|
got it
…---Original---
From: ***@***.***>
Date: Thu, Feb 19, 2026 11:35 AM
To: ***@***.***>;
Cc: ***@***.******@***.***>;
Subject: Re: [CoplayDev/unity-mcp] feat(stdio): add MCP tool toggle supportwith tools/list_changed notifications (PR #763)
@dsarno commented on this pull request.
Review
Good feature -- the stdio tool toggle brings needed parity with HTTP. A few items to clean up:
1. Duplicated branches in _build_stdio_tools_state_signature (unity_instance_middleware.py)
The set and list branches are identical:
if isinstance(enabled_raw, set): enabled_tools = tuple(sorted(...)) elif isinstance(enabled_raw, list): enabled_tools = tuple(sorted(...)) # same code
Simplify to isinstance(enabled_raw, (set, list)).
2. Port resolution logic duplicated
set_active_instance.py and _resolve_instance_value() in the middleware both implement nearly identical ***@***.*** resolution (~30 lines each). set_active_instance could delegate to the middleware method to DRY this up.
3. Double try-catch on status file refresh (C#)
ManageEditor.RefreshStdioStatusFile() wraps StdioBridgeHost.RefreshStatusFile() in a try-catch, but RefreshStatusFile itself already has a try-catch. The outer catch will never fire. Remove one.
4. Potential double notification on session init
on_message notifies on new sessions, and on_notification notifies unconditionally on notifications/initialized. If on_message fires first, the client gets two tools/list_changed in quick succession. Harmless but noisy -- worth a comment or guard.
5. _session_lock is RLock but no recursive locking occurs
A plain Lock() would be clearer about intent since there is no recursive acquisition.
6. Consider protecting set_active_instance from disabling
Only manage_editor is self-lock protected. If set_active_instance gets disabled, multi-instance users lose the ability to switch (per-call unity_instance still works, but the dedicated tool will not show). Worth at least documenting why only manage_editor is protected.
7. Timing-dependent test
test_stdio_tools_watcher_notifies_on_signature_change sleeps 0.35s with a 0.2s interval -- could be flaky in slow CI. Consider an event-based sync or a more generous margin.
8. _list_stdio_status_payloads density
~90 lines with 5 levels of nesting. Extracting per-file parsing into a helper like _parse_single_status_file(path, now_utc, ttl) -> dict | None would improve readability.
Items 1-3 are the most straightforward to fix. Rest are suggestions.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
…anches-in-middleware Refactor instance resolution, simplify stdio tools handling, and remove redundant status-file try/catch
…togo/unity-mcp into feat/stdio-tool-toggle-from-dev
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Server/src/services/tools/set_active_instance.py (1)
29-35: Calling private_resolve_instance_valuefrom outside the class.
middleware._resolve_instance_value(value, ctx)crosses the encapsulation boundary set by the_prefix convention. Since this delegation pattern is intentional and permanent (it's the entire resolution path), consider promoting the method to a public API (resolve_instance_value) onUnityInstanceMiddlewareso the call site matches the intended interface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/services/tools/set_active_instance.py` around lines 29 - 35, The call is using a private method (_resolve_instance_value) from outside its class; add a public API on UnityInstanceMiddleware named resolve_instance_value that forwards to the existing _resolve_instance_value (or refactor the implementation into the public method and keep the private helper), then replace the call in set_active_instance.py from middleware._resolve_instance_value(value, ctx) to middleware.resolve_instance_value(value, ctx); keep the existing ValueError handling intact so the exception behavior does not change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Server/src/transport/unity_instance_middleware.py`:
- Around line 513-526: The ValueError raised by _resolve_instance_value inside
the on_call_tool middleware is being swallowed by FastMCP and shows a generic
error; catch ValueError around the call to self._resolve_instance_value(raw_str,
ctx) (the per-call unity_instance handling that pops unity_instance from
context.message.arguments and sets active_instance) and re-raise a fastmcp
ToolError containing the original ValueError message so the user-visible
validation text is delivered; do the same for any explicit RuntimeError paths
here if desired (convert to ToolError) so client-facing errors are
user-friendly.
- Around line 766-770: The sorted(..., key=lambda path: path.stat().st_mtime,
reverse=True) can raise OSError if a file is deleted between glob() and stat(),
aborting the whole scan; implement a per-file safe mtime helper (e.g.,
get_mtime(path) or safe_mtime(path)) that wraps path.stat().st_mtime in
try/except OSError and returns a sentinel (like 0 or float("-inf")) for
missing/removed files, then use that helper as the sort key (sorted(...,
key=safe_mtime, reverse=True)); keep the outer except OSError for
directory-level errors only.
---
Duplicate comments:
In `@Server/src/transport/unity_instance_middleware.py`:
- Around line 585-591: The on_message handler currently calls
_notify_tool_list_changed_to_sessions when _track_session_from_context returns a
new session, but that runs on the client's initial initialize message (before
notifications/initialized) and violates MCP ordering; remove the notification
call from on_message so it only tracks the session (keep _is_stdio_transport and
_track_session_from_context usage) and let the existing on_notification handler
perform the tools/list_changed notification; specifically, delete or disable the
await self._notify_tool_list_changed_to_sessions("session_registered") call in
on_message and rely on the on_notification flow to send the event.
---
Nitpick comments:
In `@Server/src/services/tools/set_active_instance.py`:
- Around line 29-35: The call is using a private method
(_resolve_instance_value) from outside its class; add a public API on
UnityInstanceMiddleware named resolve_instance_value that forwards to the
existing _resolve_instance_value (or refactor the implementation into the public
method and keep the private helper), then replace the call in
set_active_instance.py from middleware._resolve_instance_value(value, ctx) to
middleware.resolve_instance_value(value, ctx); keep the existing ValueError
handling intact so the exception behavior does not change.
Comment on lines
513
to
526
| # Per-call routing: check if this tool call explicitly specifies unity_instance. | ||
| # context.message.arguments is a mutable dict on CallToolRequestParams; resource | ||
| # reads use ReadResourceRequestParams which has no .arguments, so this is a no-op for them. | ||
| # We pop the key here so Pydantic's type_adapter.validate_python() never sees it. | ||
| active_instance: str | None = None | ||
| msg_args = getattr(getattr(context, "message", None), "arguments", None) | ||
| if isinstance(msg_args, dict) and "unity_instance" in msg_args: | ||
| raw = msg_args.pop("unity_instance") | ||
| if raw is not None: | ||
| raw_str = str(raw).strip() | ||
| if raw_str: | ||
| # Raises ValueError with a user-friendly message on invalid input. | ||
| active_instance = await self._resolve_instance_value(raw_str, ctx) | ||
| logger.debug("Per-call unity_instance resolved to: %s", active_instance) |
Contributor
There was a problem hiding this comment.
ValueError from _resolve_instance_value surfaces as a generic tool error, not the user-friendly message.
ValueError (a regular Python exception) raised from within a FastMCP on_call_tool middleware handler is caught by FastMCP and sends a generic error to the client — the LLM sees something like "Error calling tool", not the helpful message. The idiomatic FastMCP approach is to raise ToolError from on_call_tool for user-visible validation failures, so the user-friendly message in the ValueError is actually delivered.
This also applies to the existing RuntimeError at line 507 for missing API keys, but that's pre-existing. The per-call unity_instance routing added in this PR is new and introduces a new call path.
🐛 Proposed fix
+from fastmcp.exceptions import ToolError
# ...inside _inject_unity_instance:
if raw_str:
- # Raises ValueError with a user-friendly message on invalid input.
- active_instance = await self._resolve_instance_value(raw_str, ctx)
+ try:
+ active_instance = await self._resolve_instance_value(raw_str, ctx)
+ except ValueError as exc:
+ raise ToolError(str(exc)) from exc
logger.debug("Per-call unity_instance resolved to: %s", active_instance)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Server/src/transport/unity_instance_middleware.py` around lines 513 - 526,
The ValueError raised by _resolve_instance_value inside the on_call_tool
middleware is being swallowed by FastMCP and shows a generic error; catch
ValueError around the call to self._resolve_instance_value(raw_str, ctx) (the
per-call unity_instance handling that pops unity_instance from
context.message.arguments and sets active_instance) and re-raise a fastmcp
ToolError containing the original ValueError message so the user-visible
validation text is delivered; do the same for any explicit RuntimeError paths
here if desired (convert to ToolError) so client-facing errors are
user-friendly.
Comment on lines
+766
to
+770
| status_files = sorted( | ||
| status_dir.glob("unity-mcp-status-*.json"), | ||
| key=lambda path: path.stat().st_mtime, | ||
| reverse=True, | ||
| ) |
Contributor
There was a problem hiding this comment.
TOCTOU: OSError in the sorted() stat() key lambda aborts the entire directory scan.
If any status file is deleted between glob() and the sorted() call, path.stat() raises OSError which bubbles out of the lambda, is caught by the outer except OSError at line 771, and returns [] — discarding all remaining files for that poll cycle. The intent of the outer catch is to handle a missing directory, not a mid-scan file deletion.
🐛 Proposed fix — use a per-file safe mtime helper
+ def _safe_mtime(p: Path) -> float:
+ try:
+ return p.stat().st_mtime
+ except OSError:
+ return 0.0
+
try:
status_files = sorted(
status_dir.glob("unity-mcp-status-*.json"),
- key=lambda path: path.stat().st_mtime,
+ key=_safe_mtime,
reverse=True,
)
except OSError as exc:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Server/src/transport/unity_instance_middleware.py` around lines 766 - 770,
The sorted(..., key=lambda path: path.stat().st_mtime, reverse=True) can raise
OSError if a file is deleted between glob() and stat(), aborting the whole scan;
implement a per-file safe mtime helper (e.g., get_mtime(path) or
safe_mtime(path)) that wraps path.stat().st_mtime in try/except OSError and
returns a sentinel (like 0 or float("-inf")) for missing/removed files, then use
that helper as the sort key (sorted(..., key=safe_mtime, reverse=True)); keep
the outer except OSError for directory-level errors only.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportCommandDispatcherToolToggleTests.cs (1)
61-79: Consider strengthening the positive-path assertion.The test only checks that the response does not contain
"disabled in the Unity Editor". Any other error (e.g., tool-not-registered, unknown-action) also satisfies this assertion, meaning the test would pass vacuously even if the enabled-path is broken. Consider asserting the concrete response — for example, thatresponse["status"]equals"error"with an"Unknown action"message — to make the test's intent explicit and guard against regressions.💡 Suggested strengthening
StringAssert.DoesNotContain("disabled in the Unity Editor", error); +// The "ping" action is not implemented by manage_scene; verify the dispatcher +// reached the handler and returned a known "unknown action" error rather than a +// tool-disabled or routing error. +Assert.AreEqual("error", response["status"]?.ToString()); +StringAssert.Contains("Unknown action", error);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportCommandDispatcherToolToggleTests.cs` around lines 61 - 79, The test ExecuteCommandJsonAsync_WhenToolEnabled_DoesNotReturnDisabledError currently only asserts the absence of the "disabled in the Unity Editor" message which can pass vacuously; update it to assert the concrete expected response from TransportCommandDispatcher.ExecuteCommandJsonAsync given the "ping" action — e.g., assert response["status"] equals the expected status (for example "error" or "ok" depending on intended behavior) and assert response["error"] or response["message"] contains the specific expected text (e.g., "Unknown action" if ping is unrecognized) so the test explicitly verifies the positive path rather than just the absence of one error string.MCPForUnity/Editor/Tools/ManageEditor.cs (1)
303-316: Remove the redundant outer try-catch block inRefreshStdioStatusFile.
StdioBridgeHost.RefreshStatusFilealready catches and handles all exceptions internally (lines 981-989 of StdioBridgeHost.cs), so the outer try-catch inRefreshStdioStatusFileis dead code and will never execute. The inner method guarantee exceptions will not propagate, making the outer exception handler unreachable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MCPForUnity/Editor/Tools/ManageEditor.cs` around lines 303 - 316, Remove the redundant outer try-catch in RefreshStdioStatusFile: since StdioBridgeHost.RefreshStatusFile already handles all exceptions, delete the try/catch and simply call StdioBridgeHost.RefreshStatusFile("tool_toggle") after the IsRunning check (keep the early return using StdioBridgeHost.IsRunning); target the RefreshStdioStatusFile method and remove the surrounding try/catch block to eliminate unreachable exception handling.Server/tests/test_unity_instance_middleware_stdio_edge_cases.py (2)
71-80:_build_contextduplicated verbatim intest_unity_instance_middleware_tool_list_notifications.py(lines 11–20).Move to a shared
conftest.pyto keep both suites in sync if the signature ever changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/tests/test_unity_instance_middleware_stdio_edge_cases.py` around lines 71 - 80, Both test files define the helper function _build_context identically; extract this helper into the shared test configuration so both suites stay in sync: create the function _build_context in conftest.py (keeping the same signature: session_id: str, session_obj: object, method: str = "tools/list") and either leave it as a plain helper or convert it into a pytest fixture (name _build_context or build_context) depending on how tests call it, then remove the duplicate definitions from test_unity_instance_middleware_stdio_edge_cases.py and test_unity_instance_middleware_tool_list_notifications.py so they use the shared conftest version.
571-605:asyncio.sleep(0.5)creates timing-dependency; prefer event-based synchronization.With a 0.2 s interval only 2 watcher iterations are guaranteed in 0.5 s on an unloaded machine; under CI load the assertion
call_count["value"] >= 3can fail. The notification test intest_unity_instance_middleware_tool_list_notifications.pyalready uses anasyncio.Event+wait_forfor this purpose — apply the same pattern here (e.g., set an event after the third successful call and wait for it with a generous timeout).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/tests/test_unity_instance_middleware_stdio_edge_cases.py` around lines 571 - 605, The test uses asyncio.sleep which introduces flakiness; replace the timing-based wait with an event-based synchronization: create an asyncio.Event in the test, modify the _fake_signature replacement used for middleware._build_stdio_tools_state_signature to set the event once call_count["value"] reaches the third successful iteration (e.g., after incrementing to >=3), and then await asyncio.wait_for(event.wait(), timeout=2.0) instead of asyncio.sleep; keep the existing start_stdio_tools_watcher() and ensure stop_stdio_tools_watcher() is still called in the finally block so the watcher is cleaned up.Server/src/transport/unity_instance_middleware.py (2)
787-851: Extract per-file parsing logic into a helper (reviewer item#8).The inner loop body (~60 lines of open → JSON validate → TTL check → hash extraction) is dense enough that it merits its own
_parse_single_status_file(path: Path, now_utc: datetime, ttl: float) -> dict | Nonehelper, leaving_list_stdio_status_payloadsas a thin orchestrator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/transport/unity_instance_middleware.py` around lines 787 - 851, Extract the dense per-file parsing logic in unity_instance_middleware._list_stdio_status_payloads into a new helper method named _parse_single_status_file(self, status_file: Path, now_utc: datetime, status_ttl_seconds: float) -> dict | None that performs the open→json validate→enabled_tools extraction→mtime/heartbeat freshness check→project_hash fallback and returns either {"project_hash": str, "enabled_tools": set[str]} or None (preserving all current logger.debug messages and exception handling). Then simplify _list_stdio_status_payloads to iterate status_files and for each call self._parse_single_status_file(status_file, now_utc, status_ttl_seconds), appending non-None results to payloads; keep existing symbols like _extract_project_hash_from_filename and _parse_heartbeat_datetime in the helper and preserve behavior for file_hash, freshness calculation, and TTL filtering.
302-305:isinstance(exc, (SystemExit, KeyboardInterrupt)): raiseis dead code in every newexcept Exception:block.In Python 3,
SystemExitandKeyboardInterruptboth inherit directly fromBaseException, notException.except Exception:can never capture them in the first place — they propagate naturally. The same dead guard is repeated at lines 441–443, 479–481, 487–489, and 568–570.♻️ Proposed cleanup (representative block at lines 302–305)
- except Exception as exc: - if isinstance(exc, (SystemExit, KeyboardInterrupt)): - raise - logger.debug("PluginHub instance discovery failed (%s)", type(exc).__name__, exc_info=True) + except Exception: + logger.debug("PluginHub instance discovery failed (%s)", type(exc).__name__, exc_info=True)Apply the same simplification to all five identical blocks.
Also applies to: 312-315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/transport/unity_instance_middleware.py` around lines 302 - 305, The isinstance(exc, (SystemExit, KeyboardInterrupt)) guard inside the except Exception: blocks is dead code and should be removed; leave the exception handling logic and logging intact (e.g., the logger.debug call that logs "PluginHub instance discovery failed (%s)", type(exc).__name__, exc_info=True) and similar logger.debug lines, but delete the redundant if isinstance(...) raise checks in each except Exception block (the blocks around the logger.debug at the shown location and the other identical blocks at lines noted in the review). Ensure this cleanup is applied consistently to all five occurrences.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Server/tests/test_unity_instance_middleware_stdio_edge_cases.py`:
- Around line 318-342: The test test_heartbeat_exactly_at_ttl_boundary_is_stale
is flaky due to a 100ms timing window between creating slightly_fresh and when
_list_stdio_status_payloads samples datetime.now; fix it by making the
timestamps deterministic: capture a single fixed now (e.g., fixed_now =
datetime.now(timezone.utc)) at the start of the test and compute
boundary_heartbeat and slightly_fresh from that fixed_now, or alternatively
monkeypatch datetime.now used by _list_stdio_status_payloads to return that
fixed_now; update references in the test to use the fixed_now-based ISO strings
so TTL checks in _list_stdio_status_payloads are stable (refer to
test_heartbeat_exactly_at_ttl_boundary_is_stale, slightly_fresh,
boundary_heartbeat, and _list_stdio_status_payloads).
---
Duplicate comments:
In `@Server/src/transport/unity_instance_middleware.py`:
- Around line 771-776: The current sorted(...) call uses a lambda calling
path.stat() which can raise OSError if a file is deleted between glob() and key
evaluation; replace that one-shot sorted with a two-step safe collection:
iterate over status_dir.glob("unity-mcp-status-*.json"), for each path call
path.stat().st_mtime inside a try/except OSError to skip missing/broken entries,
append (path, mtime) tuples to a list, then sort that list by mtime
(reverse=True) and extract the paths into status_files; update any code that
expects status_files to still receive a list of Path objects.
- Around line 529-532: The ValueError raised by _resolve_instance_value in the
per-call unity_instance block is not translated to a ToolError and thus the
user-friendly message is lost; update the call in the middleware (inside the "if
raw_str:" branch where active_instance is set) to catch ValueError from
_resolve_instance_value and re-raise or wrap it as a ToolError (preserving the
original message and exception context) so FastMCP sees a ToolError instead of a
raw ValueError.
- Around line 591-596: The current on_message sends a "tools/list_changed" via
_notify_tool_list_changed_to_sessions as soon as _track_session_from_context
reports a new session, which can happen on the initial "initialize" request
before the client emits notifications/initialized; change the logic so we only
notify after the client has sent its notifications/initialized handshake.
Concretely: update on_message to consult the session's initialization flag (or
add one) instead of treating the first inbound message as fully ready—e.g., have
_track_session_from_context return or set a session attribute like
session.client_initialized (or expose a method like
session.is_notifications_initialized) and only call
_notify_tool_list_changed_to_sessions("session_registered") when that
flag/method is true (or delay enqueueing the notification until you observe the
notifications/initialized event handler), keeping usage of
context.fastmcp_context and the existing helper names (_is_stdio_transport,
_track_session_from_context, _notify_tool_list_changed_to_sessions) to locate
and implement the change.
---
Nitpick comments:
In `@MCPForUnity/Editor/Tools/ManageEditor.cs`:
- Around line 303-316: Remove the redundant outer try-catch in
RefreshStdioStatusFile: since StdioBridgeHost.RefreshStatusFile already handles
all exceptions, delete the try/catch and simply call
StdioBridgeHost.RefreshStatusFile("tool_toggle") after the IsRunning check (keep
the early return using StdioBridgeHost.IsRunning); target the
RefreshStdioStatusFile method and remove the surrounding try/catch block to
eliminate unreachable exception handling.
In `@Server/src/transport/unity_instance_middleware.py`:
- Around line 787-851: Extract the dense per-file parsing logic in
unity_instance_middleware._list_stdio_status_payloads into a new helper method
named _parse_single_status_file(self, status_file: Path, now_utc: datetime,
status_ttl_seconds: float) -> dict | None that performs the open→json
validate→enabled_tools extraction→mtime/heartbeat freshness check→project_hash
fallback and returns either {"project_hash": str, "enabled_tools": set[str]} or
None (preserving all current logger.debug messages and exception handling). Then
simplify _list_stdio_status_payloads to iterate status_files and for each call
self._parse_single_status_file(status_file, now_utc, status_ttl_seconds),
appending non-None results to payloads; keep existing symbols like
_extract_project_hash_from_filename and _parse_heartbeat_datetime in the helper
and preserve behavior for file_hash, freshness calculation, and TTL filtering.
- Around line 302-305: The isinstance(exc, (SystemExit, KeyboardInterrupt))
guard inside the except Exception: blocks is dead code and should be removed;
leave the exception handling logic and logging intact (e.g., the logger.debug
call that logs "PluginHub instance discovery failed (%s)", type(exc).__name__,
exc_info=True) and similar logger.debug lines, but delete the redundant if
isinstance(...) raise checks in each except Exception block (the blocks around
the logger.debug at the shown location and the other identical blocks at lines
noted in the review). Ensure this cleanup is applied consistently to all five
occurrences.
In `@Server/tests/test_unity_instance_middleware_stdio_edge_cases.py`:
- Around line 71-80: Both test files define the helper function _build_context
identically; extract this helper into the shared test configuration so both
suites stay in sync: create the function _build_context in conftest.py (keeping
the same signature: session_id: str, session_obj: object, method: str =
"tools/list") and either leave it as a plain helper or convert it into a pytest
fixture (name _build_context or build_context) depending on how tests call it,
then remove the duplicate definitions from
test_unity_instance_middleware_stdio_edge_cases.py and
test_unity_instance_middleware_tool_list_notifications.py so they use the shared
conftest version.
- Around line 571-605: The test uses asyncio.sleep which introduces flakiness;
replace the timing-based wait with an event-based synchronization: create an
asyncio.Event in the test, modify the _fake_signature replacement used for
middleware._build_stdio_tools_state_signature to set the event once
call_count["value"] reaches the third successful iteration (e.g., after
incrementing to >=3), and then await asyncio.wait_for(event.wait(), timeout=2.0)
instead of asyncio.sleep; keep the existing start_stdio_tools_watcher() and
ensure stop_stdio_tools_watcher() is still called in the finally block so the
watcher is cleaned up.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/TransportCommandDispatcherToolToggleTests.cs`:
- Around line 61-79: The test
ExecuteCommandJsonAsync_WhenToolEnabled_DoesNotReturnDisabledError currently
only asserts the absence of the "disabled in the Unity Editor" message which can
pass vacuously; update it to assert the concrete expected response from
TransportCommandDispatcher.ExecuteCommandJsonAsync given the "ping" action —
e.g., assert response["status"] equals the expected status (for example "error"
or "ok" depending on intended behavior) and assert response["error"] or
response["message"] contains the specific expected text (e.g., "Unknown action"
if ping is unrecognized) so the test explicitly verifies the positive path
rather than just the absence of one error string.
Comment on lines
+318
to
+342
| async def test_heartbeat_exactly_at_ttl_boundary_is_stale(monkeypatch, tmp_path): | ||
| """Heartbeat exactly at TTL boundary should be considered stale (>).""" | ||
| monkeypatch.setattr(config, "transport_mode", "stdio") | ||
| monkeypatch.setenv("UNITY_MCP_STATUS_DIR", str(tmp_path)) | ||
| monkeypatch.setenv("UNITY_MCP_STDIO_STATUS_TTL_SECONDS", "10") | ||
|
|
||
| # Heartbeat exactly TTL seconds ago (boundary case: > not >=) | ||
| boundary_heartbeat = (datetime.now(timezone.utc) - timedelta(seconds=10)).isoformat() | ||
| slightly_fresh = (datetime.now(timezone.utc) - timedelta(seconds=9.9)).isoformat() | ||
|
|
||
| _write_status_file( | ||
| tmp_path / "unity-mcp-status-boundary.json", | ||
| {"project_hash": "boundary", "enabled_tools": ["manage_scene"], "last_heartbeat": boundary_heartbeat}, | ||
| ) | ||
| _write_status_file( | ||
| tmp_path / "unity-mcp-status-fresh.json", | ||
| {"project_hash": "fresh", "enabled_tools": ["manage_asset"], "last_heartbeat": slightly_fresh}, | ||
| ) | ||
|
|
||
| middleware = UnityInstanceMiddleware() | ||
| names = await _filter_tool_names(middleware, _build_fastmcp_context(None)) | ||
|
|
||
| # Boundary should be stale, fresh should be included | ||
| assert "manage_asset" in names | ||
| assert "manage_scene" not in names |
Contributor
There was a problem hiding this comment.
TTL boundary test has a 100 ms timing window — flaky under slow CI.
slightly_fresh is created as 9.9 s before the test runs, but now_utc inside _list_stdio_status_payloads is sampled at call time. If setup + filter execution exceeds ~100 ms (realistic on loaded CI), slightly_fresh is also older than 10 s and the assertion "manage_scene" not in names passes for the wrong reason while "manage_asset" may also be dropped, turning the test into a false negative.
🛡️ Suggested fix — use a larger margin or mock `datetime.now`
- boundary_heartbeat = (datetime.now(timezone.utc) - timedelta(seconds=10)).isoformat()
- slightly_fresh = (datetime.now(timezone.utc) - timedelta(seconds=9.9)).isoformat()
+ # Use a much larger safety margin to avoid timing-sensitivity.
+ boundary_heartbeat = (datetime.now(timezone.utc) - timedelta(seconds=10)).isoformat()
+ slightly_fresh = (datetime.now(timezone.utc) - timedelta(seconds=5)).isoformat()Or patch datetime.now inside _list_stdio_status_payloads to a fixed value for deterministic TTL evaluation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Server/tests/test_unity_instance_middleware_stdio_edge_cases.py` around lines
318 - 342, The test test_heartbeat_exactly_at_ttl_boundary_is_stale is flaky due
to a 100ms timing window between creating slightly_fresh and when
_list_stdio_status_payloads samples datetime.now; fix it by making the
timestamps deterministic: capture a single fixed now (e.g., fixed_now =
datetime.now(timezone.utc)) at the start of the test and compute
boundary_heartbeat and slightly_fresh from that fixed_now, or alternatively
monkeypatch datetime.now used by _list_stdio_status_payloads to return that
fixed_now; update references in the test to use the fixed_now-based ISO strings
so TTL checks in _list_stdio_status_payloads are stable (refer to
test_heartbeat_exactly_at_ttl_boundary_is_stale, slightly_fresh,
boundary_heartbeat, and _list_stdio_status_payloads).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Server/tests/test_unity_instance_middleware_tool_list_notifications.py (2)
107-109: Prefix unusedreasonparameter with_to satisfy Ruff ARG001.✏️ Proposed fix
- async def _fake_notify(reason: str): + async def _fake_notify(_reason: str): notification_event.set() return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/tests/test_unity_instance_middleware_tool_list_notifications.py` around lines 107 - 109, The test helper function _fake_notify defines an unused parameter named reason which triggers Ruff ARG001; rename the parameter to _reason (or simply _) in the _fake_notify signature so the parameter remains present for callers but is marked as intentionally unused, leaving the body unchanged and keeping notification_event.set() behavior intact.
131-138: SwallowedTimeoutErrorobscures the root cause when the watcher fails to notify.If the event never fires within 1 s, the
except asyncio.TimeoutError: passlets the test fall through toassert_awaited_with, which then raises a less informativeAssertionError. Re-raising makes timeout failures self-diagnosing.✏️ Proposed fix
try: await asyncio.wait_for(notification_event.wait(), timeout=1.0) - except asyncio.TimeoutError: - pass + except asyncio.TimeoutError as exc: + raise AssertionError( + "Watcher did not fire tools/list_changed within 1 s" + ) from exc finally: await middleware.stop_stdio_tools_watcher()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/tests/test_unity_instance_middleware_tool_list_notifications.py` around lines 131 - 138, The test swallows asyncio.TimeoutError which hides why the watcher failed; change the except block so the TimeoutError is re-raised (or not caught) instead of silently passing: when awaiting asyncio.wait_for(notification_event.wait(), timeout=1.0) allow the TimeoutError to propagate (or re-raise it) so failures surface, while still running finally to call middleware.stop_stdio_tools_watcher(); keep the final assertion middleware._notify_tool_list_changed_to_sessions.assert_awaited_with("stdio_state_changed") unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Server/src/transport/unity_instance_middleware.py`:
- Around line 163-171: The current _should_send_initial_tool_list_refresh treats
session_id None as always True which causes repeated broadcasts; change its
behavior so when session_id is None it returns False (i.e., do not trigger an
initial refresh for unresolved sessions) or alternatively record a sentinel to
make it idempotent; update the logic in _should_send_initial_tool_list_refresh
(and ensure on_notification/_track_session_from_context callers expect that
unresolved contexts will not cause a tools/list_changed blast) by either
returning False immediately for None or storing a dedicated marker in
_initial_refresh_notified_sessions so repeated None calls no longer trigger
refreshes.
---
Duplicate comments:
In `@Server/src/transport/unity_instance_middleware.py`:
- Around line 557-560: The ValueError raised by _resolve_instance_value when
resolving per-call unity_instance (inside the raw_str check in
unity_instance_middleware) is currently allowed to propagate as a generic
Exception; wrap the await self._resolve_instance_value(raw_str, ctx) call in a
try/except that catches ValueError and re-raises it as
fastmcp.exceptions.ToolError (preserving the original validation message), so
the middleware returns the intended user-friendly error to the LLM/client.
- Around line 619-628: The current on_message handler sends a tools/list_changed
notification immediately for new stdio sessions (via _is_stdio_transport,
_track_session_from_context, _try_get_session_id_from_context and
_notify_tool_list_changed_to_sessions) which can occur before the client sends
notifications/initialized; change the logic so on_message no longer triggers the
initial tools/list refresh for new sessions — instead mark the session as
"awaiting_initial_tool_list" (or similar) and let the existing
notifications/initialized handler (or on_notification) call
_should_send_initial_tool_list_refresh and _notify_tool_list_changed_to_sessions
when the client has sent initialized; update any deduplication state used by
_should_send_initial_tool_list_refresh accordingly so the initial refresh is
sent only after notifications/initialized.
- Around line 875-879: The sorted() call that builds status_files uses a key
lambda that calls path.stat().st_mtime, which can raise OSError if a file is
deleted between glob() and stat(), aborting the whole scan; replace that lambda
with a small helper function (e.g., safe_mtime(path)) that catches
OSError/FileNotFoundError and returns a fallback (e.g., 0 or None) so missing
files are ignored instead of propagating, then call sorted(status_dir.glob(...),
key=safe_mtime, reverse=True) to ensure a per-file safe mtime lookup and avoid
returning [] on a single deleted file.
---
Nitpick comments:
In `@Server/tests/test_unity_instance_middleware_tool_list_notifications.py`:
- Around line 107-109: The test helper function _fake_notify defines an unused
parameter named reason which triggers Ruff ARG001; rename the parameter to
_reason (or simply _) in the _fake_notify signature so the parameter remains
present for callers but is marked as intentionally unused, leaving the body
unchanged and keeping notification_event.set() behavior intact.
- Around line 131-138: The test swallows asyncio.TimeoutError which hides why
the watcher failed; change the except block so the TimeoutError is re-raised (or
not caught) instead of silently passing: when awaiting
asyncio.wait_for(notification_event.wait(), timeout=1.0) allow the TimeoutError
to propagate (or re-raise it) so failures surface, while still running finally
to call middleware.stop_stdio_tools_watcher(); keep the final assertion
middleware._notify_tool_list_changed_to_sessions.assert_awaited_with("stdio_state_changed")
unchanged.
Comment on lines
+163
to
+171
| def _should_send_initial_tool_list_refresh(self, session_id: str | None) -> bool: | ||
| if session_id is None: | ||
| return True | ||
|
|
||
| with self._session_lock: | ||
| if session_id in self._initial_refresh_notified_sessions: | ||
| return False | ||
| self._initial_refresh_notified_sessions.add(session_id) | ||
| return True |
Contributor
There was a problem hiding this comment.
_should_send_initial_tool_list_refresh(None) is not idempotent — risks repeated notifications.
When session_id is None (e.g., _try_get_session_id_from_context raises/returns None), the function returns True on every invocation without recording anything in _initial_refresh_notified_sessions. In on_notification, this guard is called unconditionally whenever context.method == "notifications/initialized" regardless of the _track_session_from_context return value. A client that sends multiple notifications/initialized messages without a resolvable session ID would cause repeated tools/list_changed blasts to all healthy tracked sessions.
🐛 Proposed fix
def _should_send_initial_tool_list_refresh(self, session_id: str | None) -> bool:
if session_id is None:
- return True
+ return False # Can't deduplicate without a session ID; skip to avoid noisy repeats.
with self._session_lock:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Server/src/transport/unity_instance_middleware.py` around lines 163 - 171,
The current _should_send_initial_tool_list_refresh treats session_id None as
always True which causes repeated broadcasts; change its behavior so when
session_id is None it returns False (i.e., do not trigger an initial refresh for
unresolved sessions) or alternatively record a sentinel to make it idempotent;
update the logic in _should_send_initial_tool_list_refresh (and ensure
on_notification/_track_session_from_context callers expect that unresolved
contexts will not cause a tools/list_changed blast) by either returning False
immediately for None or storing a dedicated marker in
_initial_refresh_notified_sessions so repeated None calls no longer trigger
refreshes.
Contributor
Author
|
To use Codex here, create a Codex account and connect to github. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+131
to
+132
| var httpClient = new FakeTransportClient(isConnected: true, "http"); | ||
| var stdioClient = new FakeTransportClient(isConnected: false, "stdio"); |
There was a problem hiding this comment.
These FakeTransportClient constructor calls won’t compile: C# does not allow positional arguments after a named argument. Use either all positional args or name all args (e.g., name: "http" / name: "stdio").
Suggested change
| var httpClient = new FakeTransportClient(isConnected: true, "http"); | |
| var stdioClient = new FakeTransportClient(isConnected: false, "stdio"); | |
| var httpClient = new FakeTransportClient(isConnected: true, name: "http"); | |
| var stdioClient = new FakeTransportClient(isConnected: false, name: "stdio"); |
Comment on lines
+160
to
+162
| transportManager.Configure( | ||
| () => new FakeTransportClient(isConnected: true, "http"), | ||
| () => new FakeTransportClient(isConnected: false, "stdio")); |
There was a problem hiding this comment.
These FakeTransportClient constructor calls won’t compile because they pass a positional argument after a named one. Make the arguments consistently positional or consistently named.
Comment on lines
+180
to
+181
| var httpClient = new FakeTransportClient(isConnected: false, "http"); | ||
| var stdioClient = new FakeTransportClient(isConnected: false, "stdio"); |
There was a problem hiding this comment.
These FakeTransportClient constructor calls won’t compile (positional args after named args are not permitted in C#). Switch to all positional args or name all parameters.
Comment on lines
+208
to
+209
| var httpClient = new FakeTransportClient(isConnected: true, "http", throwOnReregister: true); | ||
| var stdioClient = new FakeTransportClient(isConnected: false, "stdio"); |
There was a problem hiding this comment.
This FakeTransportClient constructor call mixes named and positional arguments (named isConnected: followed by positional "http"), which is a compile-time error in C#. Use all positional or fully named arguments (including name:).
Comment on lines
+236
to
+237
| var httpClient = new FakeTransportClient(isConnected: true, "http"); | ||
| var stdioClient = new FakeTransportClient(isConnected: false, "stdio"); |
There was a problem hiding this comment.
These FakeTransportClient constructor calls mix named and positional arguments, which won’t compile. Use new FakeTransportClient(true, "http") style or name every argument.
|
|
||
| def _should_send_initial_tool_list_refresh(self, session_id: str | None) -> bool: | ||
| if session_id is None: | ||
| return True |
There was a problem hiding this comment.
Returning True when session_id is None can cause notifications/initialized to trigger a tools/list_changed broadcast even when the current session can’t be identified (e.g., session_id access raised and _try_get_session_id_from_context returned None). Consider returning False for None here, or only sending the initial refresh when a valid session_id is available / the session is tracked.
Suggested change
| return True | |
| return False |
Comment on lines
363
to
364
| # Port number (stdio only) — resolve to Name@hash via status file lookup | ||
| if value.isdigit(): |
There was a problem hiding this comment.
The comment says port-based targeting is resolved “via status file lookup”, but the implementation resolves ports by discovering instances and matching inst.port. Update the comment to match the actual behavior to avoid confusion during future maintenance.
Comment on lines
+28
to
+31
| middleware = get_unity_instance_middleware() | ||
| try: | ||
| resolved_id = await middleware._resolve_instance_value(value, ctx) | ||
| except ValueError as exc: |
There was a problem hiding this comment.
set_active_instance is calling UnityInstanceMiddleware._resolve_instance_value (a private/underscore method). To avoid coupling public tool behavior to a private API, consider promoting this to a public method (or moving the shared resolution logic into a helper) and calling that instead.
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
2 participants
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.
Description
This PR implements MCP tool toggle support for stdio transport, bringing feature parity with HTTP transport. It enables dynamic tool enable/disable in Unity Editor with automatic MCP client notification via
tools/list_changed.Key features:
tools/list_changednotifications for both stdio and HTTP transportsunity_instancerouting for multi-instance scenariosType of Change
Changes Made
Core Features
1. stdio Transport Tool Toggle (
b7929ed)enabled_toolsfield to stdio status heartbeat filetools/listbased on Unity-side tool enable/disable statemanage_editoractions:set_mcp_tool_enabled,get_mcp_tool_enabled,list_mcp_toolsUnity changes:
ManageEditor.cs: add tool toggle actions with self-lock protectionStdioBridgeHost.cs: includeenabled_toolsin heartbeat payloadServer changes:
manage_editor.py: add new action parameters and bool coercionunity_instance_middleware.py: stdiotools/listfiltering with multi-instance support2. tools/list_changed Notifications (
b88905f)set_mcp_tool_enabledsucceedsNew environment variables:
UNITY_MCP_STDIO_TOOLS_WATCH_INTERVAL_SECONDS(default: 1.0, min: 0.2)UNITY_MCP_STDIO_STATUS_TTL_SECONDS(default: 15.0)3. Configuration Options (
640aef8)4. HTTP Transport Reregistration (
01920d2)set_mcp_tool_enabledis called, reregister tools with HTTP transport (PluginHub)tools/list_changednotifications for both transport modesTests Added
stdio Tool Filtering Tests (
7036b45)34 comprehensive edge case tests:
enabled_toolsparsing (empty list, null elements, wrong type)project_hashhandling (missing, empty, filename fallback)Status File Schema Compatibility Tests (
5b475a1)enabled_toolsfieldTesting/Screenshots/Recordings
Python Tests
Unity Tests
Open
TestProjects/UnityMCPTestsin Unity Editor and run:ManageEditorToolToggleTestsTransportCommandDispatcherToolToggleTestsDocumentation Updates
manage_editoractions:set_mcp_tool_enabled,get_mcp_tool_enabled,list_mcp_toolsRelated Issues
Relates to stdio transport feature parity with HTTP transport.
Compatibility with PR #772: This branch merges
CoplayDev:beta(including @dsarno's PR #772 for per-callunity_instancerouting) and maintains full compatibility. The tool toggle implementation works seamlessly with the middleware-based instance routing mechanism.Additional Notes
Architecture Overview
Files Changed Summary
ManageEditor.cs,StdioBridgeHost.csManageEditorToolToggleTests.cs,TransportCommandDispatcherToolToggleTests.csunity_instance_middleware.py,manage_editor.pytest_unity_instance_middleware_*.pySummary by Sourcery
Add stdio-aware MCP tool enable/disable controls synchronized between the Unity Editor and the server, including status-file-based tools/list filtering, change notifications, and per-call Unity instance routing improvements.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Documentation
Tests