Fix: #709 - Preserve tool enabled/disabled state and filter tools in client listings #723
+1,291
−38
Conversation
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
Fixes CoplayDev#709 - Remove logic in ToolDiscoveryService.EnsurePreferenceInitialized that forced built-in tools to be re-enabled on every Unity launch - Add ReregisterToolsAsync() to IMcpTransportClient for dynamic tool list updates when toggles change in the UI - Implement tool reregistration in WebSocketTransportClient - Add ReregisterToolsAsync() to McpToolsSection to trigger updates when tool toggles are flipped - Add unit tests for ToolDiscoveryService preference persistence Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideImplements persistent Unity tool enablement preferences and server-side filtering of disabled tools from HTTP/WebSocket client tool listings, including alias/visibility metadata and user-scoped tool resolution, plus editor-side support to re-register tools over WebSocket and extensive characterization tests. Sequence diagram for Unity tool toggle and WebSocket tool reregistrationsequenceDiagram
actor UnityUser
participant McpToolsSection
participant ToolDiscoveryService
participant TransportManager
participant HttpClient as WebSocketTransportClient
participant McpServer
UnityUser->>McpToolsSection: Toggle tool checkbox
McpToolsSection->>ToolDiscoveryService: SetToolEnabled(toolName, enabled)
ToolDiscoveryService-->>McpToolsSection: Preference stored in EditorPrefs
McpToolsSection->>McpToolsSection: HandleToggleChange(tool, enabled, updateSummary, reregisterTools)
McpToolsSection->>McpToolsSection: UpdateSummary()
alt reregisterTools == true
McpToolsSection->>TransportManager: GetClient(TransportMode.Http)
TransportManager-->>McpToolsSection: HttpClient (if created)
alt HttpClient is null or not connected
McpToolsSection-->>McpToolsSection: Skip reregistration
else HttpClient exists and IsConnected
McpToolsSection->>HttpClient: ReregisterToolsAsync()
activate HttpClient
HttpClient->>HttpClient: SendRegisterToolsAsync(_lifecycleCts.Token)
HttpClient->>McpServer: register_tools(tools payload)
McpServer-->>HttpClient: Registration acknowledged
HttpClient-->>McpToolsSection: Reregistration completed
deactivate HttpClient
end
end
Sequence diagram for HTTP list_tools with Unity-aware filteringsequenceDiagram
actor Client
participant HttpTransport as HTTPTransport
participant UnityMiddleware as UnityInstanceMiddleware
participant FastMCP as FastMCPCore
participant PluginHub
participant Registry as ToolRegistry
Client->>HttpTransport: list_tools
HttpTransport->>UnityMiddleware: on_list_tools(context)
UnityMiddleware->>UnityMiddleware: _inject_unity_instance(context)
UnityMiddleware->>FastMCP: call_next(context) (list_tools)
FastMCP-->>UnityMiddleware: tools (all registered)
UnityMiddleware->>UnityMiddleware: _should_filter_tool_listing()
alt transport != http or PluginHub not configured
UnityMiddleware-->>Client: tools (unfiltered)
else Filtering enabled
UnityMiddleware->>UnityMiddleware: _refresh_tool_visibility_metadata_from_registry()
UnityMiddleware->>Registry: get_registered_tools()
Registry-->>UnityMiddleware: registry_tools
UnityMiddleware->>UnityMiddleware: Build
UnityMiddleware->>UnityMiddleware: _unity_managed_tool_names
UnityMiddleware->>UnityMiddleware: _tool_alias_to_unity_target
UnityMiddleware->>UnityMiddleware: _server_only_tool_names
UnityMiddleware->>PluginHub: get_sessions(user_id)
PluginHub-->>UnityMiddleware: sessions
UnityMiddleware->>UnityMiddleware: _resolve_candidate_project_hashes(unity_instance)
UnityMiddleware->>PluginHub: get_tools_for_project(project_hash, user_id)
PluginHub-->>UnityMiddleware: registered_tools
UnityMiddleware->>UnityMiddleware: Collect enabled_tool_names
loop For each tool in tools
UnityMiddleware->>UnityMiddleware: _is_tool_visible(tool.name, enabled_tool_names)
end
UnityMiddleware-->>Client: filtered tools
end
Sequence diagram for execute_custom_tool with user-scoped resolutionsequenceDiagram
actor Client
participant FastMCP as FastMCPCore
participant ExecuteTool as execute_custom_tool
participant CustomToolService
participant PluginHub
participant UnityInstance as UnityInstanceMiddleware
Client->>FastMCP: execute_custom_tool(tool_name, parameters)
FastMCP->>ExecuteTool: execute_custom_tool(ctx, tool_name, parameters)
ExecuteTool->>ExecuteTool: get_user_id_from_context(ctx)
ExecuteTool->>CustomToolService: execute_tool(project_id, tool_name, unity_instance, params, user_id)
activate CustomToolService
CustomToolService->>CustomToolService: get_tool_definition(project_id, tool_name, user_id)
alt tool in _project_tools
CustomToolService-->>CustomToolService: Use local definition
else not local
CustomToolService->>PluginHub: get_tool_definition(project_id, tool_name, user_id)
PluginHub-->>CustomToolService: definition
end
CustomToolService->>UnityInstance: send_with_unity_instance(..., user_id)
UnityInstance-->>CustomToolService: initial MCPResponse
alt requires_polling
CustomToolService->>CustomToolService: _poll_until_complete(..., user_id)
loop until complete or timeout
CustomToolService->>UnityInstance: send_with_unity_instance(..., user_id)
UnityInstance-->>CustomToolService: MCPResponse
end
end
CustomToolService-->>ExecuteTool: final MCPResponse
deactivate CustomToolService
ExecuteTool-->>Client: MCPResponse
Class diagram for Unity transport clients, tool UI, and registry metadataclassDiagram
class McpToolsSection {
-Dictionary~string, Toggle~ toolToggleMap
-IEnumerable~ToolMetadata~ allTools
+CreateToolRow(tool ToolMetadata) VisualElement
-HandleToggleChange(tool ToolMetadata, enabled bool, updateSummary bool, reregisterTools bool) void
-ReregisterToolsAsync() void
-SetAllToolsState(enabled bool) void
-UpdateSummary() void
}
class ToolDiscoveryService {
+IsToolEnabled(toolName string) bool
+SetToolEnabled(toolName string, enabled bool) void
-EnsurePreferenceInitialized(metadata ToolMetadata) void
}
class TransportManager {
-IMcpTransportClient _httpClient
-IMcpTransportClient _stdioClient
+StartAsync(mode TransportMode) Task~bool~
+StopAsync(mode TransportMode) Task
+VerifyAsync(mode TransportMode) Task~bool~
+GetState(mode TransportMode) TransportState
+IsRunning(mode TransportMode) bool
+GetClient(mode TransportMode) IMcpTransportClient
}
class IMcpTransportClient {
<<interface>>
+bool IsConnected
+Task~bool~ StartAsync()
+Task StopAsync()
+Task~bool~ VerifyAsync()
+Task ReregisterToolsAsync()
}
class WebSocketTransportClient {
+bool IsConnected
-CancellationTokenSource _lifecycleCts
+Task~bool~ StartAsync()
+Task StopAsync()
+Task~bool~ VerifyAsync()
+Task ReregisterToolsAsync()
-Task SendRegisterToolsAsync(token CancellationToken)
}
class StdioTransportClient {
+bool IsConnected
+Task~bool~ StartAsync()
+Task StopAsync()
+Task~bool~ VerifyAsync()
+Task ReregisterToolsAsync()
}
McpToolsSection --> ToolDiscoveryService : uses
McpToolsSection --> TransportManager : uses
TransportManager --> IMcpTransportClient : returns
WebSocketTransportClient ..|> IMcpTransportClient
StdioTransportClient ..|> IMcpTransportClient
class UnityInstanceMiddleware {
-dict~string, string~ _active_by_key
-RLock _lock
-RLock _metadata_lock
-set~string~ _unity_managed_tool_names
-dict~string, string~ _tool_alias_to_unity_target
-set~string~ _server_only_tool_names
-tuple~tuple~string, string~~ _tool_visibility_signature
-float _last_tool_visibility_refresh
-float _tool_visibility_refresh_interval_seconds
-bool _has_logged_empty_registry_warning
+async on_list_tools(context MiddlewareContext) Any
-_should_filter_tool_listing() bool
-async _resolve_enabled_tool_names_for_context(context MiddlewareContext) set~string~ or None
-_refresh_tool_visibility_metadata_from_registry() void
-_resolve_candidate_project_hashes(active_instance str or None) list~str~
-_is_tool_visible(tool_name str or None, enabled_tool_names set~str~) bool
}
class ToolRegistryDecorator {
+mcp_for_unity_tool(name str or None, description str or None, unity_target str or None, **kwargs) Callable
}
class RegisteredToolMetadata {
+func Callable
+name str
+description str or None
+unity_target str or None
+kwargs dict
}
UnityInstanceMiddleware --> RegisteredToolMetadata : reads
class PluginHub {
<<static>>
+is_configured() bool
+async get_sessions(user_id str or None) SessionList
+async get_tools_for_project(project_hash str, user_id str or None) list~Any~
+async get_tool_definition(project_hash str, tool_name str, user_id str or None) Any or None
}
UnityInstanceMiddleware --> PluginHub : queries sessions and tools
class CustomToolService {
-dict~str, dict~str, ToolDefinitionModel~~ _project_tools
+async list_registered_tools(project_id str, user_id str or None) list~ToolDefinitionModel~
+async get_tool_definition(project_id str, tool_name str, user_id str or None) ToolDefinitionModel or None
+async execute_tool(project_id str, tool_name str, unity_instance str or None, params dict~str, object~ or None, user_id str or None) MCPResponse
-async _poll_until_complete(project_id str, unity_instance str or None, tool_name str, initial_params dict~str, object~, initial_response Any, poll_action str, user_id str or None) MCPResponse
}
CustomToolService --> PluginHub : resolves tools per user
class ToolFunctions {
+execute_custom_tool
+manage_script
+create_script
+delete_script
+validate_script
+find_in_file
+script_apply_edits
+manage_script_capabilities
+set_active_instance
+debug_request_context
}
ToolFunctions --> ToolRegistryDecorator : decorated by mcp_for_unity_tool
class UnityTargetMetadata {
+manage_script : base tool
+aliases : create_script, delete_script, validate_script, find_in_file, script_apply_edits
+server_only : execute_custom_tool, manage_script_capabilities, set_active_instance, debug_request_context
}
UnityInstanceMiddleware --> UnityTargetMetadata : derives visibility from
File-Level Changes
Assessment against linked issues
Possibly linked issues
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:
📝 WalkthroughWalkthroughInitializes tool preferences without forced overrides, adds transport-level ReregisterToolsAsync and UI batching to invoke it, exposes TransportManager.GetClient publicly, implements server-side HTTP tool-list filtering with user/session scoping, and extends unit and Unity Editor tests for prefs and user-scoped tool behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware as UnityInstanceMiddleware
participant PluginHub
participant Registry as SessionRegistry
Client->>Middleware: LIST /tools (HTTP)
Middleware->>Registry: resolve_session_id(project_hash, user_id?)
alt session_id found
Middleware->>PluginHub: get_tools_for_project(project_hash, user_id)
PluginHub->>Registry: fetch session by id
Registry-->>PluginHub: session (tools list)
PluginHub-->>Middleware: tools list
Middleware->>Middleware: filter tools by enabled names/aliases/server-only rules
Middleware-->>Client: filtered tools list
else no session
Middleware-->>Client: original/unfiltered tools list
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![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
McpToolsSection.ReregisterToolsAsync, consider making the method fully async instead of usingThreadPool.QueueUserWorkItemwith.Wait(), so you can avoid blocking a thread-pool thread and more cleanly propagate cancellation or errors. - In
UnityInstanceMiddleware._resolve_enabled_tool_names_for_context, you sequentially callPluginHub.get_tools_for_projectfor each project hash; if there can be multiple active sessions, you may want to batch or parallelize these calls (or cache the results) to avoid unnecessary latency on tool listing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `McpToolsSection.ReregisterToolsAsync`, consider making the method fully async instead of using `ThreadPool.QueueUserWorkItem` with `.Wait()`, so you can avoid blocking a thread-pool thread and more cleanly propagate cancellation or errors.
- In `UnityInstanceMiddleware._resolve_enabled_tool_names_for_context`, you sequentially call `PluginHub.get_tools_for_project` for each project hash; if there can be multiple active sessions, you may want to batch or parallelize these calls (or cache the results) to avoid unnecessary latency on tool listing.
## Individual Comments
### Comment 1
<location> `MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs:248-267` </location>
<code_context>
+ }
+ }
+
+ private void ReregisterToolsAsync()
+ {
+ // Fire and forget - don't block UI
+ ThreadPool.QueueUserWorkItem(_ =>
+ {
+ try
+ {
+ var transportManager = MCPServiceLocator.TransportManager;
+ var client = transportManager.GetClient(TransportMode.Http);
+ if (client != null && client.IsConnected)
+ {
+ client.ReregisterToolsAsync().Wait();
+ }
+ }
+ catch (Exception ex)
+ {
+ McpLog.Warn($"Failed to reregister tools: {ex.Message}");
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid blocking a ThreadPool thread with .Wait(); use an async flow instead
`ThreadPool.QueueUserWorkItem` plus `client.ReregisterToolsAsync().Wait()` blocks a ThreadPool thread and can cause thread-pool pressure if this is triggered often. Since the goal is just to avoid blocking the UI and `ReregisterToolsAsync` is already async, you can make the method itself fire-and-forget and run the async call directly:
```csharp
private void ReregisterToolsAsync()
{
var transportManager = MCPServiceLocator.TransportManager;
var client = transportManager.GetClient(TransportMode.Http);
if (client == null || !client.IsConnected)
{
return;
}
_ = Task.Run(async () =>
{
try
{
await client.ReregisterToolsAsync().ConfigureAwait(false);
}
catch (Exception ex)
{
McpLog.Warn($"Failed to reregister tools: {ex.Message}");
}
});
}
```
This avoids blocking worker threads while still keeping the call off the UI thread and logging failures.
```suggestion
private void ReregisterToolsAsync()
{
// Fire and forget - don't block UI thread
var transportManager = MCPServiceLocator.TransportManager;
var client = transportManager.GetClient(TransportMode.Http);
if (client == null || !client.IsConnected)
{
return;
}
_ = System.Threading.Tasks.Task.Run(async () =>
{
try
{
await client.ReregisterToolsAsync().ConfigureAwait(false);
}
catch (Exception ex)
{
McpLog.Warn($"Failed to reregister tools: {ex.Message}");
}
});
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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: 3
🤖 Fix all issues with AI agents
In `@Server/src/transport/unity_instance_middleware.py`:
- Around line 335-340: The bare except around PluginHub.get_sessions() swallows
all errors; change it to catch Exception as e and log the error at debug level
(matching the file's existing pattern) before returning None so failures are
diagnosable; specifically update the except block that handles the await
PluginHub.get_sessions(user_id=user_id) call to log a message including user_id
and the exception (use the existing logger/process_logger and include exception
info/stacktrace), then keep the return None and preserve assignment to
sessions_data/sessions.
- Around line 362-366: The loop over project_hashes currently swallows all
exceptions from PluginHub.get_tools_for_project(project_hash); update the except
block to catch Exception as e and log the error (including e and the
project_hash) via the existing logger before continuing so failures are visible;
ensure you still continue on error but do not re-raise, and prefer a clear
message like "Failed to fetch tools for project {project_hash}: {e}" to help
debug calls to PluginHub.get_tools_for_project and the handling of
registered_tools.
In
`@TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/ToolDiscoveryServiceTests.cs.meta`:
- Line 2: The .meta file contains a hand-crafted GUID
"7a8b9c0d1e2f3a4b5c6d7e8f9a0b1c2d" which risks collisions; remove or replace
this GUID by letting Unity regenerate a random one—either delete the
corresponding .meta file and reimport the asset or use Unity's Reimport/Refresh
so the GUID is auto-generated, ensuring the GUID value in
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/ToolDiscoveryServiceTests.cs.meta
is no longer the manual hex sequence.
🧹 Nitpick comments (6)
MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs (1)
522-527: Unnecessary fully-qualified exception types.
System.OperationCanceledExceptionandSystem.Exceptionare fully qualified here, but theSystemnamespace is already imported (line 1). The rest of the file uses unqualified names (e.g., lines 326, 336).Consistency fix
- catch (System.OperationCanceledException) + catch (OperationCanceledException) { McpLog.Warn("[WebSocket] Tool reregistration cancelled"); } - catch (System.Exception ex) + catch (Exception ex) { McpLog.Error($"[WebSocket] Tool reregistration failed: {ex.Message}"); }MCPForUnity/Editor/Services/Transport/IMcpTransportClient.cs (1)
17-17: Consider adding an XML doc comment for the new interface method.The other members lack docs too, so this is consistent, but since this method has nuanced behavior (no-op for stdio, real reregistration for WebSocket), a brief
<summary>would help implementors.MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs (1)
248-267: Potential for redundant concurrent reregistrations on rapid toggles.Each individual toggle fires
ReregisterToolsAsyncon the thread pool independently. Rapid toggling can queue multiple concurrent reregistrations. They'll serialize via the WebSocket send lock, but the redundant work is wasteful.Consider a simple debounce (e.g., a
CancellationTokenSourcethat gets reset on each call, with a short delay before actually sending) to coalesce rapid individual toggles into a single reregistration. Not a blocker — the current approach is correct, just chatty.Server/src/transport/unity_instance_middleware.py (1)
58-94: Hardcoded tool name sets are a maintenance burden.These three collections (
_unity_managed_tool_names,_tool_alias_to_unity_target,_server_only_tool_names) must be manually synchronized whenever tools are added, removed, or renamed. If a new Unity-managed tool is introduced without being added here, it will silently remain visible even when disabled (due to the forward-compatibility fallback in_is_tool_visibleline 401).Consider deriving these sets from a shared manifest or registry, or at minimum adding a comment/docstring that cross-references the authoritative source so future maintainers know to update this list.
#!/bin/bash # Verify whether the hardcoded tool names in _unity_managed_tool_names match # tool definitions registered elsewhere in the codebase. echo "=== Tool definitions in C# tool files ===" rg -n --type=cs 'ToolName\s*=' -g '!*Test*' | head -40 echo "" echo "=== Python tool registrations (e.g., `@mcp.tool` or name= in tool defs) ===" rg -n --type=py 'name\s*=\s*"' -g '!*test*' | grep -i tool | head -40 echo "" echo "=== Aliases like create_script, apply_text_edits in Python ===" rg -rn 'create_script|apply_text_edits|delete_script|find_in_file|get_sha|script_apply_edits|validate_script' --type=py -g '!*test*' -g '!unity_instance_middleware.py' | head -20Server/tests/test_transport_characterization.py (1)
271-310: Good coverage of the core filtering path; consider asserting onmanage_scripttoo.The test correctly validates that:
- Enabled tools pass through (
manage_scene)- Aliases resolve to their target's enabled state (
create_script→manage_script)- Server-only tools always pass (
set_active_instance)- Disabled Unity-managed tools are filtered (
manage_asset)
manage_scriptis in theavailable_toolslist and is enabled, so it should appear in the filtered output. Adding an assertion for it would make the test more comprehensive and explicitly document that directly-enabled tools pass through.Optional: assert manage_script presence
names = [tool.name for tool in filtered] + assert "manage_script" in names assert "manage_scene" in names assert "create_script" in names assert "set_active_instance" in names assert "manage_asset" not in namesTestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/ToolDiscoveryServiceTests.cs (1)
116-124: Test assumes at least one built-in tool withAutoRegister=falseexists.Line 124 asserts that such a tool is found. If the codebase ever removes all such tools, this test will fail with a somewhat opaque message. This is acceptable for a characterization test since it documents a current invariant, but worth noting if the tool set changes significantly.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/ToolDiscoveryServiceTests.cs.meta
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes #709 by persisting Unity tool enabled/disabled preferences across restarts and hiding disabled Unity-managed tools (and aliases) from MCP client tool listings in HTTP/WebSocket mode.
Changes:
- Unity (C#): Persist tool enabled state and trigger tool re-registration when toggles change.
- Server (Python): Filter
list_toolsresults based on the enabled tool set registered for the active Unity session (including alias mapping). - Tests: Add characterization tests for transport tool listing behavior and Unity preference persistence tests.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| TestProjects/UnityMCPTests/Assets/Tests/Editor/State.meta | Adds Unity folder metadata for new test structure. |
| TestProjects/UnityMCPTests/Assets/Tests/Editor.meta | Adds Unity folder metadata for Editor tests folder. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/ToolDiscoveryServiceTests.cs.meta | Adds Unity meta for new C# test file. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/ToolDiscoveryServiceTests.cs | Adds EditMode tests validating EditorPrefs persistence and discovery behavior. |
| Server/tests/test_transport_characterization.py | Adds characterization tests for HTTP tool-list filtering and alias behavior. |
| Server/src/transport/unity_instance_middleware.py | Implements on_list_tools() filtering logic based on Unity session enabled tools. |
| MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs | Triggers tool re-registration after toggle changes / bulk enable-disable. |
| MCPForUnity/Editor/Services/Transport/Transports/WebSocketTransportClient.cs | Adds ReregisterToolsAsync() to re-send tool registration while connected. |
| MCPForUnity/Editor/Services/Transport/Transports/StdioTransportClient.cs | Adds no-op ReregisterToolsAsync() for stdio transport. |
| MCPForUnity/Editor/Services/Transport/TransportManager.cs | Exposes GetClient() publicly for UI-triggered reregistration. |
| MCPForUnity/Editor/Services/Transport/IMcpTransportClient.cs | Extends transport interface with ReregisterToolsAsync(). |
| MCPForUnity/Editor/Services/ToolDiscoveryService.cs | Adjusts preference initialization logic to avoid overriding stored disabled state. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/ToolDiscoveryServiceTests.cs
Show resolved
Hide resolved
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10754e5819
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Server/src/transport/plugin_hub.py (1)
340-354:⚠️ Potential issue | 🟠 Major
get_tool_definitionlacksuser_idparameter, inconsistent with updatedget_tools_for_project.In remote-hosted mode,
get_tools_for_project(line 330) acceptsuser_idand passes it toget_session_id_by_hash, butget_tool_definition(line 341) does not, causing user-scoped session lookups to fail. The proposed fix to adduser_idtoget_tool_definitionis correct:Proposed fix
`@classmethod` - async def get_tool_definition(cls, project_hash: str, tool_name: str) -> Any | None: + async def get_tool_definition(cls, project_hash: str, tool_name: str, user_id: str | None = None) -> Any | None: """Retrieve a specific tool definition for an active project hash.""" if cls._registry is None: return None - session_id = await cls._registry.get_session_id_by_hash(project_hash) + session_id = await cls._registry.get_session_id_by_hash(project_hash, user_id=user_id) if not session_id: return NoneNote: This fix alone is incomplete—the service layer (
CustomToolService.list_registered_toolsandget_tool_definition) also needs to accept and thread throughuser_idfrom the MCP context to reach the PluginHub methods.
🧹 Nitpick comments (1)
Server/src/transport/unity_instance_middleware.py (1)
58-94: Categorization metadata could be derived from tool decorators.The three hardcoded sets (
_unity_managed_tool_names,_tool_alias_to_unity_target,_server_only_tool_names) categorize tools for filtering, not duplicate registration—tools are auto-discovered via@mcp_for_unity_tooldecorators. The filtering logic is sound: unknown tools intentionally remain visible for forward compatibility rather than being hidden if omitted from these sets.That said, you could reduce the explicit hardcoding by adding a
categoryparameter to the decorator (e.g.,@mcp_for_unity_tool(category="unity_managed")), then deriving these sets from tool metadata at runtime. This would keep the categorization and tool definition in sync automatically, though it adds minimal ongoing maintenance compared to the current explicit sets given their size and stability.
whatevertogo
force-pushed
the
fix/preserve-tool-toggle-state-restarts
branch
from
b7759be to
ceb2396
Compare
… and improve error handling
- Pass user_id to execute_tool for user-scoped tool lookups - Update get_tool_definition to support user-scoped queries Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…oplayDev#709) Comprehensive fix for Issue CoplayDev#709: - Unity tools stay disabled across restarts (EditorPrefs persistence) - Client tool lists filter out disabled Unity tools (HTTP mode only) - Script helper tools follow their target tool's enabled state - Added graceful error fallback when tool lookup fails Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
whatevertogo
force-pushed
the
fix/preserve-tool-toggle-state-restarts
branch
from
ceb2396 to
fc81d9e
Compare
…ol visibility control
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Server/src/services/tools/manage_script.py (1)
354-370:⚠️ Potential issue | 🟡 MinorPre-existing bug: async function used as thread target will never execute.
Not introduced by this PR, but
_flip_asyncis anasync defpassed tothreading.Thread(target=...). This creates a coroutine that is never awaited — the thread completes immediately without running the body. Wrapping withasyncio.run(_flip_async())or usingasyncio.ensure_futureon an existing loop would be needed for the code to actually execute.
🧹 Nitpick comments (3)
MCPForUnity/Editor/Windows/Components/Tools/McpToolsSection.cs (1)
248-249:Asyncsuffix on avoid-returning method is misleading.By .NET convention, the
Asyncsuffix signals a method returnsTask/Task<T>. Since this is intentionally fire-and-forget, consider renaming toReregisterToolsFireAndForgetorTriggerToolReregistrationto avoid confusion with awaitable methods.Server/src/services/registry/tool_registry.py (1)
37-41: Minor: preferpopoverif key in dict+del.Ruff RUF051 flags this pattern. Using
popis more concise.Proposed simplification
- tool_kwargs = dict(kwargs) # Create a copy to avoid side effects - if "unity_target" in tool_kwargs: - del tool_kwargs["unity_target"] + tool_kwargs = dict(kwargs) # Create a copy to avoid side effects + tool_kwargs.pop("unity_target", None)Server/tests/test_transport_characterization.py (1)
284-648: Consider extracting the shared test setup into a helper or fixture to reduce deep nesting.Every
on_list_toolsfiltering test repeats the same ~15-linewith patch(...)nesting structure. A helper method or pytest fixture yielding the mocks would significantly reduce boilerplate and improve readability.Example sketch
from contextlib import asynccontextmanager `@asynccontextmanager` async def _patch_for_list_tools_filter(middleware, mock_context, *, sessions, tools_side_effect, registry=None): middleware_ctx = Mock() middleware_ctx.fastmcp_context = mock_context with patch.object(middleware, "_inject_unity_instance", new=AsyncMock()): with patch("transport.unity_instance_middleware.PluginHub.is_configured", return_value=True): with patch("transport.unity_instance_middleware.get_registered_tools", return_value=registry or _tool_registry_for_visibility_tests()): with patch("transport.unity_instance_middleware.PluginHub.get_sessions", new_callable=AsyncMock) as mock_sessions: with patch("transport.unity_instance_middleware.PluginHub.get_tools_for_project", new_callable=AsyncMock) as mock_tools: mock_sessions.return_value = sessions if callable(tools_side_effect): mock_tools.side_effect = tools_side_effect else: mock_tools.return_value = tools_side_effect yield middleware_ctx, mock_sessions, mock_tools
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a9b08351f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
1064 line for test, 40 for .meta,: add 200 lines code |
IntegerField lives in UnityEditor.UIElements, not UnityEngine.UIElements. Without this import the project fails to compile.
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.
Fixes #709 - Disabled Tools Reappear and Still Show Up in Client Lists
Description
Fixes #709 - Disabled tools now persist across Unity Editor restarts and are correctly filtered from client tool
listings.
Type of Change
Changes Made
Unity Editor (C#)
ToolDiscoveryService.cs: AddedEnsurePreferenceInitialized()method to initialize EditorPrefs for tools on firstdiscovery
McpToolsSection.cs: AddedReregisterToolsAsync()call to trigger tool reregistration when toggles changeWebSocketTransportClient.cs: ImplementedReregisterToolsAsync()to send updated tool list to serverIMcpTransportClient.cs: AddedReregisterToolsAsync()interface methodMCP Server (Python)
unity_instance_middleware.py: Addedon_list_tools()middleware method to filter tools based on Unity sessionstate
tool_registry.py: Addedunity_targetparameter to@mcp_for_unity_tooldecorator for explicit tool visibilitycontrol
create_script) follow their target tool's enabled state via_tool_alias_to_unity_targetmapping
Testing/Screenshots/Recordings
test_transport_characterization.pyToolDiscoveryServiceTests.cstest_custom_tool_service_user_scope.pyDocumentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)tools/UPDATE_DOCS.mdRelated Issues
Additional Notes
Limitations:
Behavior:
set_active_instance,debug_request_context,manage_script_capabilities,execute_custom_tool) always remain visibleSummary by Sourcery
Preserve Unity tool enablement state across sessions and ensure MCP server only lists tools that are enabled for the current Unity context and user.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Tests