Add testing and move menu items to resources #316
Conversation
We haven't connected them as yet, still thinking about how to do this neatly
…egistration logs to debug level
…source decorator The Context parameter doesn't work on our version of FastMCP
…ately This works but it spams logs like crazy, maybe there's a better/simpler way
…nd command execution
…rror don't cause Pydantic errors
This connection module needs a lookover. The retries should be an exponential backoff and we could structure why it's failing so much
…and helper function
…d get commands An MCP resource for retrieval, and a simple command to execute. Because it's a resource, it's easier for the user to see what's in the menu items
…l examples to use async/await We'll eventually put a section for resources
|
Caution Review failedThe pull request is closed. WalkthroughAdds async command handling, resource discovery/registration, and a Unity test runner; refactors command dispatch and telemetry; replaces manage_menu_item with execute_menu_item; introduces Python MCPResponse and resource endpoints; updates logging prefixes, docs, tests, and Docker/pyproject dependencies. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as MCP Client
participant Bridge as MCPForUnityBridge
participant Registry as CommandRegistry
participant Handler as Tool/Resource Handler
participant TCS as TaskCompletionSource
Client->>Bridge: Send framed command (id, name, params)
Bridge->>Bridge: Validate/trim JSON, handle ping
Bridge->>Registry: ExecuteCommand(name, params, tcs)
alt Synchronous
Registry->>Handler: Invoke handler(params)
Handler-->>Registry: result (object)
Registry-->>Bridge: Complete tcs / immediate response
Bridge-->>Client: Send framed response
else Asynchronous
Registry->>Handler: Start async handler (returns Task)
Registry->>Registry: Keep QueuedCommand.IsExecuting = true
Handler-->>Registry: Completes later and calls CompleteAsyncCommand(tcs, result)
Registry->>TCS: SetResult / SetException
Bridge->>Bridge: EditorApplication.delayCall cleanup next frame
Bridge-->>Client: Send framed response on completion
end
opt Errors
Bridge->>Client: Structured error payload (with params summary) via McpLog
end
sequenceDiagram
autonumber
participant User as API Consumer
participant Server as FastMCP Server
participant ResReg as resources.register_all_resources
participant Resource as get_tests / get_menu_items
participant Conn as unity_connection.async_send_command_with_retry
participant Unity as Unity Bridge
Server->>ResReg: register_all_resources(mcp)
ResReg-->>Server: resources registered (wrapped with telemetry)
User->>Server: HTTP call to resource URI (e.g., /mcpforunity://tests)
Server->>Resource: Invoke decorated resource
Resource->>Conn: async_send_command_with_retry("get_tests", params)
Conn->>Unity: Send framed command
Unity-->>Conn: dict response or MCPResponse
Conn-->>Resource: dict | MCPResponse
Resource-->>Server: Return GetTestsResponse or passthrough
Server-->>User: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
MCPForUnity/UnityMcpServer~/src/tools/read_console.py (1)
14-28: Critical: Function must be async to use await.The function uses
await ctx.info(...)at Line 28, but the function signature at Line 14 declares it as a regular function (def read_console) rather than an async function. This will cause aSyntaxErrorat runtime.Apply this diff to fix the issue:
-def read_console( +async def read_console(MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)
120-129: Bug: prefab path uses possibly None folderIf saveAsPrefab is True and prefab_folder is None, constructed_path becomes "None/.prefab". Default or validation is missing.
Apply this fix to default the folder or fail clearly:
- if "prefabPath" not in params: - if "name" not in params or not params["name"]: - return {"success": False, "message": "Cannot create default prefab path: 'name' parameter is missing."} - # Use the provided prefab_folder (which has a default) and the name to construct the path - constructed_path = f"{prefab_folder}/{params['name']}.prefab" + if "prefabPath" not in params: + if "name" not in params or not params["name"]: + return {"success": False, "message": "Cannot create prefab path: 'name' parameter is missing."} + # Use provided folder when available; otherwise default to Assets + folder = params.get("prefabFolder") or "Assets" + constructed_path = f"{folder.rstrip('/')}/{params['name']}.prefab" # Ensure clean path separators (Unity prefers '/') params["prefabPath"] = constructed_path.replace("\\", "/")MCPForUnity/UnityMcpServer~/src/tools/manage_script.py (5)
356-363: Critical: Function must be declared asasync defto useawait.The
create_scriptfunction usesawait ctx.info(...)on line 363, but the function definition on line 356 is not marked asasync def. In Python, you cannot useawaitin a non-async function—this will cause aSyntaxErrorat runtime.Apply this diff to fix the function declaration:
-def create_script( +async def create_script( ctx: Context, path: Annotated[str, "Path under Assets/ to create the script at, e.g., 'Assets/Scripts/My.cs'"], contents: Annotated[str, "Contents of the script to create. Note, this is Base64 encoded over transport."], script_type: Annotated[str, "Script type (e.g., 'C#')"] | None = None, namespace: Annotated[str, "Namespace for the script"] | None = None, ) -> dict[str, Any]:
394-399: Critical: Function must be declared asasync defto useawait.The
delete_scriptfunction usesawait ctx.info(...)on line 399, but the function definition on line 394 is not marked asasync def. This will cause aSyntaxErrorat runtime.Apply this diff to fix the function declaration:
-def delete_script( +async def delete_script( ctx: Context, uri: Annotated[str, "URI of the script to delete under Assets/ directory, unity://path/Assets/... or file://... or Assets/..."] ) -> dict[str, Any]:
409-417: Critical: Function must be declared asasync defto useawait.The
validate_scriptfunction usesawait ctx.info(...)on line 417, but the function definition on line 409 is not marked asasync def. This will cause aSyntaxErrorat runtime.Apply this diff to fix the function declaration:
-def validate_script( +async def validate_script( ctx: Context, uri: Annotated[str, "URI of the script to validate under Assets/ directory, unity://path/Assets/... or file://... or Assets/..."], level: Annotated[Literal['basic', 'standard'], "Validation level"] = "basic", include_diagnostics: Annotated[bool, "Include full diagnostics and summary"] = False ) -> dict[str, Any]:
443-454: Critical: Function must be declared asasync defto useawait.The
manage_scriptfunction usesawait ctx.info(...)on line 454, but the function definition on line 443 is not marked asasync def. This will cause aSyntaxErrorat runtime.Apply this diff to fix the function declaration:
-def manage_script( +async def manage_script( ctx: Context, action: Annotated[Literal['create', 'read', 'delete'], "Perform CRUD operations on C# scripts."], name: Annotated[str, "Script name (no .cs extension)", "Name of the script to create"], path: Annotated[str, "Asset path (default: 'Assets/')", "Path under Assets/ to create the script at, e.g., 'Assets/Scripts/My.cs'"], contents: Annotated[str, "Contents of the script to create", "C# code for 'create'/'update'"] | None = None, script_type: Annotated[str, "Script type (e.g., 'C#')", "Type hint (e.g., 'MonoBehaviour')"] | None = None, namespace: Annotated[str, "Namespace for the script"] | None = None, ) -> dict[str, Any]:
536-540: Critical: Function must be declared asasync defto useawait.The
get_shafunction usesawait ctx.info(...)on line 540, but the function definition on line 536 is not marked asasync def. This will cause aSyntaxErrorat runtime.Apply this diff to fix the function declaration:
-def get_sha( +async def get_sha( ctx: Context, uri: Annotated[str, "URI of the script to edit under Assets/ directory, unity://path/Assets/... or file://... or Assets/..."] ) -> dict[str, Any]:MCPForUnity/Editor/Tools/CommandRegistry.cs (1)
1064-1116: Detect and route async handlers in Bridge.ExecuteCommand
Bridge.ExecuteCommand(Command) callsCommandRegistry.GetHandler(...), which throws for any async handler (IsAsync). In MCPForUnity/Editor/MCPForUnityBridge.cs (L1065–1116), update this method to detect when a handler is async and invokeCommandRegistry.ExecuteCommand(commandName, @params, tcs)(awaiting the returned Task) instead ofGetHandler(...), or ensure async commands never reach this code path.
🧹 Nitpick comments (23)
docs/CUSTOM_TOOLS.md (2)
27-33: Async example looks good; verify command call and update logging guidance
- Good change to async and using await ctx.info.
- If send_command_with_retry is async now, await it; if it’s sync and does I/O, run it via asyncio.to_thread to avoid blocking.
- Align Best Practices to “await ctx.info()”.
- Union type uses Python 3.10+. Confirm minimum Python version or adjust typing.
Examples:
# If async: response = await send_command_with_retry("my_custom_tool", params) # If sync: import asyncio response = await asyncio.to_thread(send_command_with_retry, "my_custom_tool", params)
154-159: Same nits for the screenshot tool example
- Keep await ctx.info (good).
- Await send_command_with_retry if async, or wrap the sync call with asyncio.to_thread to prevent blocking.
- Ensure Best Practices section says “await ctx.info()”.
Snippet:
# If async: response = await send_command_with_retry("capture_screenshot", params) # If sync: import asyncio response = await asyncio.to_thread(send_command_with_retry, "capture_screenshot", params)MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (1)
136-142: Avoid blocking the event loopsend_command_with_retry is synchronous; calling it inside async handlers can block. Consider offloading to a thread executor or making it async.
Example:
+ import asyncio, functools @@ - response = send_command_with_retry("manage_gameobject", params) + loop = asyncio.get_running_loop() + response = await loop.run_in_executor( + None, functools.partial(send_command_with_retry, "manage_gameobject", params) + )MCPForUnity/Editor/Resources/McpForUnityResourceAttribute.cs (2)
9-11: Make attribute usage explicit and non-inheritablePrevent unintended inheritance and clarify intent.
-[AttributeUsage(AttributeTargets.Class, AllowMultiple = false)] -public class McpForUnityResourceAttribute : Attribute +[AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = false)] +public sealed class McpForUnityResourceAttribute : Attribute
16-26: Nullability noteResourceName can be null (auto-derive). If nullable reference types are enabled, consider declaring string? to avoid warnings.
MCPForUnity/UnityMcpServer~/src/registry/resource_registry.py (2)
31-41: Guard against duplicate registrationsRepeated imports/tests can append the same function multiple times.
def decorator(func: Callable) -> Callable: - resource_name = name if name is not None else func.__name__ - _resource_registry.append({ + resource_name = name if name is not None else func.__name__ + if any(r.get('func') is func for r in _resource_registry): + return func + _resource_registry.append({ 'func': func, 'uri': uri, 'name': resource_name, 'description': description, 'kwargs': kwargs })
46-49: Return defensive copiesget_registered_resources returns a shallow copy of the list; callers can still mutate inner dicts.
-def get_registered_resources() -> list[dict[str, Any]]: - """Get all registered resources""" - return _resource_registry.copy() +def get_registered_resources() -> list[dict[str, Any]]: + """Get all registered resources (defensive copy)""" + return [entry.copy() for entry in _resource_registry]MCPForUnity/UnityMcpServer~/src/tools/manage_scene.py (1)
47-53: Avoid blocking the event loopsend_command_with_retry is synchronous; wrap in executor or provide an async variant to keep the loop responsive.
- response = send_command_with_retry("manage_scene", params) + import asyncio, functools + loop = asyncio.get_running_loop() + response = await loop.run_in_executor( + None, functools.partial(send_command_with_retry, "manage_scene", params) + )MCPForUnity/UnityMcpServer~/src/tools/manage_shader.py (2)
29-41: Validate contents and avoid sending for non-create/updateRequire contents for create/update; ignore contents for read/delete to prevent accidental payloads.
- # Base64 encode the contents if they exist to avoid JSON escaping issues - if contents is not None: - if action in ['create', 'update']: - # Encode content for safer transmission - params["encodedContents"] = base64.b64encode( - contents.encode('utf-8')).decode('utf-8') - params["contentsEncoded"] = True - else: - params["contents"] = contents + # Enforce and encode contents appropriately + if action in ['create', 'update']: + if not contents: + return {"success": False, "message": "Parameter 'contents' is required for 'create'/'update'."} + params["encodedContents"] = base64.b64encode(contents.encode('utf-8')).decode('utf-8') + params["contentsEncoded"] = True + # For 'read'/'delete', ignore any provided contents
42-46: Avoid blocking the event loopAs with other tools, send_command_with_retry should be offloaded or made async.
- response = send_command_with_retry("manage_shader", params) + import asyncio, functools + loop = asyncio.get_running_loop() + response = await loop.run_in_executor( + None, functools.partial(send_command_with_retry, "manage_shader", params) + )MCPForUnity/Editor/Services/ITestRunnerService.cs (1)
16-16: Consider using a typed model for test data.The method returns
IReadOnlyList<Dictionary<string, string>>, which is flexible but less type-safe. Consider introducing a typed test data model (e.g.,TestInfo) for stronger compile-time guarantees and better IDE support.MCPForUnity/UnityMcpServer~/src/telemetry_decorator.py (1)
126-126: Consider minor refactoring suggestions from static analysis.While not critical, consider these optional improvements:
- Move
success = Trueto anelseblock after the try block for clearer success/failure separation- The broad
Exceptioncatching is intentional for telemetry robustness, so the BLE001 warnings can be safely ignoredAlso applies to: 135-135, 151-151, 160-160
MCPForUnity/UnityMcpServer~/src/unity_connection.py (2)
439-451: Consider returning MCPResponse consistently.The function now returns either
dict[str, Any]orMCPResponse, creating a mixed return type. For API consistency, consider always returningMCPResponsethroughout the function, including the success path at line 321.Apply this pattern to wrap all return paths:
async def async_send_command_with_retry(...) -> MCPResponse: try: ... - return resp.get('result', {}) + result = resp.get('result', {}) + return MCPResponse(success=True, data=result) except Exception as e: return MCPResponse(success=False, error=str(e))
231-232: Remove redundantNoneguard forparams
All current call sites always pass aparamsdict, so theparams: Dict[str, Any] = Nonedefault and itsif params is Nonebranch can be removed; makeparamsa required argument or default to an empty dict instead.MCPForUnity/Editor/Tools/RunTests.cs (1)
1-7: Consider consolidating ModeParser.This file imports
MCPForUnity.Editor.Resources.Tests(line 4) to accessModeParser, which is defined asinternalinGetTests.cs. SinceModeParseris internal, this cross-namespace access works because they're in the same assembly. However, for better clarity, consider makingModeParsera shared utility or documenting this dependency.Consider one of these options:
- Move
ModeParserto a shared utilities namespace- Add a comment explaining the internal access
- Make
ModeParserpublic inGetTests.csif it's intended for broader useExample comment:
using MCPForUnity.Editor.Resources.Tests; // For ModeParser (internal utility)MCPForUnity/UnityMcpServer~/src/resources/menu_items.py (1)
15-25: Consider adding parameter support for refresh and search.The function currently hardcodes
refresh=Falseandsearch=""(lines 19-22), but the comment mentions FastMCP query parameter support. If this is planned for future versions, consider either:
- Adding parameters now:
async def get_menu_items(refresh: bool = False, search: str = "") -> GetMenuItemsResponse- Adding a TODO comment to track this enhancement
MCPForUnity/Editor/Tools/CommandRegistry.cs (7)
39-41: Avoid target-typednew()for wider Unity/C# compatibilitySome Unity editor versions don’t support C# 9 target-typed
new(). Use the explicit constructor to be safe.- private static readonly Dictionary<string, HandlerInfo> _handlers = new(); + private static readonly Dictionary<string, HandlerInfo> _handlers = new Dictionary<string, HandlerInfo>();
194-203: Ensure lazy initialization to prevent “unknown command” before Initialize() is calledIf a caller forgets to call Initialize(), this throws. Make GetHandlerInfo initialize on first use.
- private static HandlerInfo GetHandlerInfo(string commandName) + private static HandlerInfo GetHandlerInfo(string commandName) { + if (!_initialized) + { + Initialize(); + } if (!_handlers.TryGetValue(commandName, out var handler)) { throw new InvalidOperationException( $"Unknown or unsupported command type: {commandName}" ); } return handler; }
67-107: Discovery looks good; consider filtering assemblies and richer error logsScanning all non-dynamic assemblies works. For faster domain reloads, consider filtering by assembly name prefix (e.g., “MCPForUnity”) and include stack traces in failure logs.
- McpLog.Error($"Failed to auto-discover MCP commands: {ex.Message}"); + McpLog.Error($"Failed to auto-discover MCP commands: {ex.Message}\n{ex.StackTrace}");
109-189: Registration path is solid; minor robustnessNice handling of duplicate names and Task vs non-Task. Optional: validate attribute values (empty ResourceName/CommandName) before falling back to ToSnakeCase, and log which name was chosen for traceability.
233-249: Guard against null TCS for async handlersIf a caller passes null TCS for an async command, later code will NRE. Fail fast.
- public static object ExecuteCommand(string commandName, JObject @params, TaskCompletionSource<string> tcs) + public static object ExecuteCommand(string commandName, JObject @params, TaskCompletionSource<string> tcs) { var handlerInfo = GetHandlerInfo(commandName); if (handlerInfo.IsAsync) { + if (tcs == null) throw new ArgumentNullException(nameof(tcs), $"Async command '{commandName}' requires a TaskCompletionSource"); ExecuteAsyncHandler(handlerInfo, @params, commandName, tcs); return null; }
225-233: Doc mismatch: mentions IEnumerator/coroutines but implementation is Task-basedThe summary says coroutines/IEnumerator, but code detects Task/Task. Update the comment to avoid confusion.
- /// Execute a command handler, supporting both synchronous and asynchronous (coroutine) handlers. - /// If the handler returns an IEnumerator, it will be executed as a coroutine. + /// Execute a command handler, supporting both synchronous and asynchronous handlers. + /// Async handlers are identified by returning Task or Task<T>.
302-346: Fire-and-forget async void is fine here; consider explicit context hop if main-thread is requiredIf handlers touch Unity APIs, enforce main-thread execution (e.g., via captured SynchronizationContext or EditorApplication.delayCall). Today, success/failure completion may occur off-thread depending on the caller’s context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
MCPForUnity/UnityMcpServer~/src/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (61)
MCPForUnity/Editor/Helpers/McpLog.cs(2 hunks)MCPForUnity/Editor/MCPForUnityBridge.cs(20 hunks)MCPForUnity/Editor/Resources.meta(1 hunks)MCPForUnity/Editor/Resources/McpForUnityResourceAttribute.cs(1 hunks)MCPForUnity/Editor/Resources/McpForUnityResourceAttribute.cs.meta(1 hunks)MCPForUnity/Editor/Resources/MenuItems.meta(1 hunks)MCPForUnity/Editor/Resources/MenuItems/GetMenuItems.cs(1 hunks)MCPForUnity/Editor/Resources/MenuItems/GetMenuItems.cs.meta(1 hunks)MCPForUnity/Editor/Resources/Tests.meta(1 hunks)MCPForUnity/Editor/Resources/Tests/GetTests.cs(1 hunks)MCPForUnity/Editor/Resources/Tests/GetTests.cs.meta(1 hunks)MCPForUnity/Editor/Services/ITestRunnerService.cs(1 hunks)MCPForUnity/Editor/Services/ITestRunnerService.cs.meta(1 hunks)MCPForUnity/Editor/Services/MCPServiceLocator.cs(4 hunks)MCPForUnity/Editor/Services/TestRunnerService.cs(1 hunks)MCPForUnity/Editor/Services/TestRunnerService.cs.meta(1 hunks)MCPForUnity/Editor/Tools/CommandRegistry.cs(5 hunks)MCPForUnity/Editor/Tools/ExecuteMenuItem.cs(2 hunks)MCPForUnity/Editor/Tools/ExecuteMenuItem.cs.meta(1 hunks)MCPForUnity/Editor/Tools/ManageEditor.cs(1 hunks)MCPForUnity/Editor/Tools/MenuItems/ManageMenuItem.cs(0 hunks)MCPForUnity/Editor/Tools/MenuItems/MenuItemsReader.cs(0 hunks)MCPForUnity/Editor/Tools/RunTests.cs(1 hunks)MCPForUnity/Editor/Tools/RunTests.cs.meta(1 hunks)MCPForUnity/UnityMcpServer~/src/Dockerfile(1 hunks)MCPForUnity/UnityMcpServer~/src/__init__.py(0 hunks)MCPForUnity/UnityMcpServer~/src/config.py(2 hunks)MCPForUnity/UnityMcpServer~/src/models.py(1 hunks)MCPForUnity/UnityMcpServer~/src/pyproject.toml(1 hunks)MCPForUnity/UnityMcpServer~/src/registry/__init__.py(1 hunks)MCPForUnity/UnityMcpServer~/src/registry/resource_registry.py(1 hunks)MCPForUnity/UnityMcpServer~/src/registry/tool_registry.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/__init__.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/menu_items.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/tests.py(1 hunks)MCPForUnity/UnityMcpServer~/src/server.py(4 hunks)MCPForUnity/UnityMcpServer~/src/telemetry.py(11 hunks)MCPForUnity/UnityMcpServer~/src/telemetry_decorator.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/__init__.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/execute_menu_item.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_editor.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_menu_item.py(0 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_prefabs.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_scene.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_script.py(8 hunks)MCPForUnity/UnityMcpServer~/src/tools/manage_shader.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/read_console.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py(3 hunks)MCPForUnity/UnityMcpServer~/src/tools/run_tests.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/script_apply_edits.py(2 hunks)MCPForUnity/UnityMcpServer~/src/unity_connection.py(5 hunks)README-zh.md(1 hunks)README.md(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources/GetMenuItemsTests.cs(3 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/CommandRegistryTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ExecuteMenuItemTests.cs(3 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/ManageMenuItemTests.cs(0 hunks)docs/CUSTOM_TOOLS.md(2 hunks)
💤 Files with no reviewable changes (5)
- MCPForUnity/UnityMcpServer~/src/init.py
- MCPForUnity/UnityMcpServer~/src/tools/manage_menu_item.py
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/ManageMenuItemTests.cs
- MCPForUnity/Editor/Tools/MenuItems/ManageMenuItem.cs
- MCPForUnity/Editor/Tools/MenuItems/MenuItemsReader.cs
🧰 Additional context used
🧬 Code graph analysis (21)
MCPForUnity/Editor/Services/ITestRunnerService.cs (1)
MCPForUnity/Editor/Services/TestRunnerService.cs (3)
TestRunResult(241-293)TestRunResult(243-247)TestRunResult(267-292)
MCPForUnity/UnityMcpServer~/src/registry/resource_registry.py (1)
MCPForUnity/UnityMcpServer~/src/registry/tool_registry.py (1)
decorator(30-39)
MCPForUnity/UnityMcpServer~/src/resources/__init__.py (2)
MCPForUnity/UnityMcpServer~/src/telemetry_decorator.py (1)
telemetry_resource(110-164)MCPForUnity/UnityMcpServer~/src/registry/resource_registry.py (1)
get_registered_resources(46-48)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPForUnity/Editor/Services/TestRunnerService.cs (3)
TestRunnerService(17-236)TestRunnerService(26-30)Dispose(94-111)
MCPForUnity/UnityMcpServer~/src/telemetry_decorator.py (1)
MCPForUnity/UnityMcpServer~/src/telemetry.py (1)
record_resource_usage(449-467)
MCPForUnity/UnityMcpServer~/src/tools/execute_menu_item.py (1)
MCPForUnity/UnityMcpServer~/src/models.py (1)
MCPResponse(5-9)
MCPForUnity/Editor/Resources/MenuItems/GetMenuItems.cs (1)
MCPForUnity/Editor/Tools/ExecuteMenuItem.cs (1)
HandleCommand(22-51)
MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
MCPForUnity/UnityMcpServer~/src/models.py (1)
MCPResponse(5-9)
MCPForUnity/Editor/Tools/RunTests.cs (4)
MCPForUnity/Editor/Resources/Tests/GetTests.cs (4)
Task(18-36)Task(46-74)ModeParser(77-105)TryParse(79-104)MCPForUnity/Editor/Services/ITestRunnerService.cs (2)
Task(16-16)Task(21-21)MCPForUnity/Editor/Services/TestRunnerService.cs (9)
Task(32-57)Task(59-92)Task(154-182)TestRunResult(241-293)TestRunResult(243-247)TestRunResult(267-292)ToSerializable(257-265)ToSerializable(314-325)ToSerializable(356-368)MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(8-67)
MCPForUnity/UnityMcpServer~/src/resources/menu_items.py (2)
MCPForUnity/UnityMcpServer~/src/models.py (1)
MCPResponse(5-9)MCPForUnity/UnityMcpServer~/src/registry/resource_registry.py (1)
mcp_for_unity_resource(10-43)
MCPForUnity/Editor/Services/TestRunnerService.cs (3)
MCPForUnity/Editor/Resources/Tests/GetTests.cs (2)
Task(18-36)Task(46-74)MCPForUnity/Editor/Services/ITestRunnerService.cs (2)
Task(16-16)Task(21-21)MCPForUnity/Editor/Tools/RunTests.cs (1)
Task(18-71)
MCPForUnity/Editor/Tools/CommandRegistry.cs (2)
MCPForUnity/Editor/MCPForUnityBridge.cs (7)
System(697-742)System(744-748)System(750-769)System(771-788)Task(480-520)Task(522-694)ExecuteCommand(1065-1117)MCPForUnity/Editor/Resources/Tests/GetTests.cs (2)
Task(18-36)Task(46-74)
MCPForUnity/UnityMcpServer~/src/registry/__init__.py (2)
MCPForUnity/UnityMcpServer~/src/registry/tool_registry.py (1)
clear_tool_registry(49-51)MCPForUnity/UnityMcpServer~/src/registry/resource_registry.py (3)
mcp_for_unity_resource(10-43)get_registered_resources(46-48)clear_resource_registry(51-53)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources/GetMenuItemsTests.cs (1)
MCPForUnity/Editor/Resources/MenuItems/GetMenuItems.cs (1)
HandleCommand(21-37)
MCPForUnity/Editor/Resources/Tests/GetTests.cs (4)
MCPForUnity/Editor/Services/ITestRunnerService.cs (2)
Task(16-16)Task(21-21)MCPForUnity/Editor/Services/TestRunnerService.cs (3)
Task(32-57)Task(59-92)Task(154-182)MCPForUnity/Editor/Tools/RunTests.cs (1)
Task(18-71)MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(8-67)
MCPForUnity/UnityMcpServer~/src/tools/run_tests.py (1)
MCPForUnity/UnityMcpServer~/src/models.py (1)
MCPResponse(5-9)
MCPForUnity/UnityMcpServer~/src/server.py (1)
MCPForUnity/UnityMcpServer~/src/resources/__init__.py (1)
register_all_resources(20-62)
MCPForUnity/Editor/Tools/ExecuteMenuItem.cs (1)
MCPForUnity/Editor/Resources/MenuItems/GetMenuItems.cs (1)
HandleCommand(21-37)
MCPForUnity/Editor/MCPForUnityBridge.cs (3)
MCPForUnity/Editor/Resources/MenuItems/GetMenuItems.cs (2)
List(39-47)HandleCommand(21-37)MCPForUnity/Editor/Tools/CommandRegistry.cs (2)
CommandRegistry(37-407)ExecuteCommand(233-249)MCPForUnity/Editor/Tools/ExecuteMenuItem.cs (1)
HandleCommand(22-51)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ExecuteMenuItemTests.cs (1)
MCPForUnity/Editor/Tools/ExecuteMenuItem.cs (1)
HandleCommand(22-51)
MCPForUnity/UnityMcpServer~/src/resources/tests.py (2)
MCPForUnity/UnityMcpServer~/src/models.py (1)
MCPResponse(5-9)MCPForUnity/UnityMcpServer~/src/registry/resource_registry.py (1)
mcp_for_unity_resource(10-43)
🪛 markdownlint-cli2 (0.18.1)
README.md
48-48: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
README-zh.md
48-48: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🪛 Ruff (0.13.3)
MCPForUnity/UnityMcpServer~/src/resources/__init__.py
38-38: Do not catch blind exception: Exception
(BLE001)
MCPForUnity/UnityMcpServer~/src/telemetry_decorator.py
126-126: Consider moving this statement to an else block
(TRY300)
135-135: Do not catch blind exception: Exception
(BLE001)
151-151: Consider moving this statement to an else block
(TRY300)
160-160: Do not catch blind exception: Exception
(BLE001)
MCPForUnity/UnityMcpServer~/src/resources/menu_items.py
7-7: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
MCPForUnity/UnityMcpServer~/src/resources/tests.py
17-17: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (65)
MCPForUnity/UnityMcpServer~/src/config.py (1)
20-20: Verify the dramatic increase in total connection wait time.The combination of these changes significantly increases the maximum wait time:
- Old behavior: 1.0s timeout × 10 retries = ~10 seconds max
- New behavior: 30.0s timeout × 5 retries = ~150 seconds max
This 15x increase in total wait time could impact user experience if connections fail quickly. Please confirm this is intentional and appropriate for Unity's startup/connection characteristics, especially given the new async command handling and resource support introduced in this PR.
Also applies to: 33-33
MCPForUnity/UnityMcpServer~/src/tools/manage_prefabs.py (1)
11-48: No issue with send_command_with_retry call send_command_with_retry is defined as a synchronous function, so invoking it withoutawaitin this async context is correct.MCPForUnity/UnityMcpServer~/src/tools/manage_asset.py (1)
35-35: LGTM!The async logging call is correct since
manage_assetis already declared as an async function.MCPForUnity/Editor/Tools/ManageEditor.cs (1)
209-209: LGTM!The explicit namespace qualification for
UnityEngine.Resourcesprevents potential conflicts with the newly introducedMCPForUnity.Editor.Resourcesnamespace in this PR.MCPForUnity/Editor/Tools/ExecuteMenuItem.cs.meta (1)
1-11: LGTM!Standard Unity metadata file for the ExecuteMenuItem.cs asset. The structure and GUID are properly formatted.
MCPForUnity/UnityMcpServer~/src/tools/resource_tools.py (3)
145-145: LGTM! Consistent async logging migration.The change from
ctx.info()toawait ctx.info()aligns correctly with the async function signature and ensures the logging operation completes properly in the async context.
205-205: LGTM! Consistent async logging migration.The change from
ctx.info()toawait ctx.info()aligns correctly with the async function signature and ensures the logging operation completes properly in the async context.
360-360: LGTM! Consistent async logging migration.The change from
ctx.info()toawait ctx.info()aligns correctly with the async function signature and ensures the logging operation completes properly in the async context.MCPForUnity/Editor/Services/TestRunnerService.cs.meta (1)
1-11: LGTM!Standard Unity metadata file for the TestRunnerService.cs asset. The structure and GUID are properly formatted.
MCPForUnity/UnityMcpServer~/src/Dockerfile (1)
1-15: LGTM!The Dockerfile updates follow standard practices:
- Uses Python 3.13-slim base image
- Copies application files to the working directory
- Synchronizes dependencies with
uv syncThe structure is clean and follows Docker best practices.
MCPForUnity/Editor/Tools/RunTests.cs.meta (1)
1-11: LGTM!Standard Unity metadata file for the RunTests.cs asset. The structure and GUID are properly formatted.
MCPForUnity/UnityMcpServer~/src/tools/manage_editor.py (2)
12-12: LGTM! Consistent async migration.The function signature correctly transitions to
async def, aligning with the broader async migration pattern across the codebase.
25-25: LGTM! Consistent async logging.The change to
await ctx.info()properly awaits the logging operation, ensuring it completes before proceeding.MCPForUnity/UnityMcpServer~/src/tools/script_apply_edits.py (2)
357-357: LGTM! Consistent async migration.The function signature correctly transitions to
async def, aligning with the broader async migration pattern across the codebase.
369-369: LGTM! Consistent async logging.The change to
await ctx.info()properly awaits the logging operation, ensuring it completes before proceeding.MCPForUnity/UnityMcpServer~/src/pyproject.toml (1)
7-12: Approve Pydantic v2 upgrade.
No Pydantic v1 patterns detected; upgrading to v2.12.0 is safe.MCPForUnity/UnityMcpServer~/src/tools/manage_gameobject.py (2)
11-16: Async conversion looks goodSignature migrated to async; parameters preserved. Good.
67-67: Correct: await ctx.infoAwaiting the context logger prevents warnings in fastmcp.
MCPForUnity/UnityMcpServer~/src/tools/manage_scene.py (1)
9-19: Async conversion looks goodHandler signature and await ctx.info align with fastmcp.
MCPForUnity/UnityMcpServer~/src/telemetry.py (2)
396-406: Telemetry initialization pattern OKLazy global instance with a background worker is fine.
420-447: Resource usage telemetry addition looks solidAPI surface is consistent with tool usage metrics.
MCPForUnity/UnityMcpServer~/src/tools/manage_script.py (1)
78-89: LGTM! Async function signature properly updated.The function is correctly declared as
async defand the new optional parameters (precondition_sha256andstrict) are well-integrated into the existing flow.MCPForUnity/Editor/Services/ITestRunnerService.cs.meta (1)
2-2: LGTM! Unity meta file GUID update.This is a standard Unity asset metadata update accompanying the new
ITestRunnerService.csinterface file.MCPForUnity/Editor/Resources/MenuItems/GetMenuItems.cs.meta (1)
2-2: LGTM! Unity meta file GUID update.This is a standard Unity asset metadata update accompanying the new
GetMenuItems.csresource file.README.md (1)
48-48: LGTM! Documentation updated to reflect tool rename.The documentation correctly reflects the rename from
manage_menu_itemtoexecute_menu_item, with an updated description emphasizing the execution behavior.MCPForUnity/Editor/Helpers/McpLog.cs (1)
8-10: LGTM! Improved log level visual distinction.The refactor from a single
Prefixto three color-coded prefixes (LogPrefix,WarnPrefix,ErrorPrefix) enhances log readability by providing clear visual distinction between log levels in the Unity console.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/CommandRegistryTests.cs (1)
44-44: LGTM! Test updated to reflect tool rename.The test expectation correctly reflects the rename from
manage_menu_itemtoexecute_menu_item, ensuring test coverage remains accurate.README-zh.md (1)
48-48: LGTM! Chinese documentation updated to reflect tool rename.The Chinese documentation correctly mirrors the changes in README.md, updating from
manage_menu_itemtoexecute_menu_itemwith the appropriate translated description.MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
1-2: LGTM! Clean integration of test runner service.The new
ITestRunnerServiceis properly integrated into the service locator pattern, following the established conventions for lazy initialization, registration, and disposal. Theusing System;directive correctly supports theIDisposableinterface usage.Also applies to: 13-13, 30-33, 48-49, 57-65
MCPForUnity/UnityMcpServer~/src/tools/__init__.py (3)
12-12: LGTM: Registry API surface updated.The removal of
mcp_for_unity_toolfrom the public exports aligns with the broader registry refactoring to centralize tool registration.Also applies to: 17-17
58-58: LGTM: Reduced log verbosity.Changing per-tool registration messages to debug level reduces clutter while maintaining observability when needed.
54-56: Verify decorator wrapping order.Telemetry is now applied before
mcp.tool, so the server registers the telemetry-wrapped function rather than wrapping the registered tool afterward. Confirm this reversal is intentional and thatmcp.toolhandles pre-wrapped callables correctly and aligns with your desired telemetry scope.MCPForUnity/UnityMcpServer~/src/models.py (1)
5-9: LGTM: Well-designed response model.The
MCPResponsemodel provides a clean, standardized structure for API responses with appropriate field types and optional values.MCPForUnity/UnityMcpServer~/src/server.py (2)
10-10: LGTM: Resource registration added.The addition of resource registration follows the established pattern for tool registration and properly integrates with the MCP server initialization.
Also applies to: 166-167
176-176: LGTM: Prompt text updated to reflect new command names.The asset creation strategy prompt has been correctly updated to reference
execute_menu_iteminstead of the deprecatedmanage_menu_item.Also applies to: 188-188
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ExecuteMenuItemTests.cs (1)
3-3: LGTM: Tests updated to reflect implementation rename.The test class and method calls have been consistently updated to use
ExecuteMenuItem.HandleCommandinstead of the previousMenuItemExecutor.Execute, maintaining the same test coverage.Also applies to: 5-7, 14-14, 23-23, 33-33
MCPForUnity/UnityMcpServer~/src/tools/execute_menu_item.py (1)
13-25: LGTM: Well-implemented menu item execution tool.The function properly:
- Uses type annotations for clarity
- Filters out None values before sending
- Handles both dict and non-dict responses appropriately
- Aligns with the C# side expectations (camelCase parameter names)
MCPForUnity/UnityMcpServer~/src/telemetry_decorator.py (1)
110-164: LGTM: Resource telemetry decorator properly implemented.The new
telemetry_resourcedecorator:
- Mirrors the proven pattern from
telemetry_tool- Handles both synchronous and asynchronous resources
- Ensures telemetry is recorded in finally blocks even on exceptions
- Properly limits log verbosity with shared counter
MCPForUnity/UnityMcpServer~/src/registry/tool_registry.py (1)
49-49: Rename complete: no remainingclear_registryreferences
Ripgrep search found no usages of the old name; all callers have been updated.MCPForUnity/UnityMcpServer~/src/resources/__init__.py (2)
36-40: LGTM on broad exception handling.The broad exception catch is appropriate here. Module import failures during resource discovery should not crash the entire registration process. The warning log provides sufficient visibility for debugging.
55-59: Decorator application order is correct and consistent with existingtoolsusage. No changes required.MCPForUnity/UnityMcpServer~/src/registry/__init__.py (1)
4-22: LGTM!The export updates cleanly separate tool and resource registry APIs, improving clarity and maintainability. The renaming from
clear_registrytoclear_tool_registrymakes the scope explicit.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources/GetMenuItemsTests.cs (1)
1-69: LGTM!The test migration to the new
GetMenuItems.HandleCommandAPI is thorough and covers the essential scenarios: listing all items, searching with no matches, and searching with matches. The tests properly validate both success responses and data structure.MCPForUnity/Editor/Tools/ExecuteMenuItem.cs (1)
1-53: LGTM!The refactoring from
MenuItemExecutor.ExecutetoExecuteMenuItem.HandleCommandis clean and aligns with the new tool registration pattern. The core logic, error handling, and safety blacklist remain intact.MCPForUnity/Editor/Resources/Tests/GetTests.cs (1)
77-105: LGTM on ModeParser implementation.The mode parsing logic is clean, with clear error messages and case-insensitive comparison. The separation into a dedicated parser class improves reusability.
MCPForUnity/Editor/Resources/MenuItems/GetMenuItems.cs (3)
18-19: LGTM on cache initialization strategy.Using
[InitializeOnLoadMethod]to build the cache on load ensures menu items are available immediately. This is an appropriate optimization for relatively static data.
21-37: LGTM on HandleCommand implementation.The command handling logic is clean:
- Extracts
refreshandsearchparameters with sensible defaults- Delegates to internal method for cache management
- Applies case-insensitive search filtering when requested
- Returns a clear success message with count
49-69: LGTM on Refresh implementation.The menu item scanning logic is solid:
- Uses
TypeCache.GetMethodsWithAttribute<MenuItem>for efficient discovery- Extracts
menuItemvalues from all attributes (handles multiple attributes per method)- Filters empty strings, deduplicates with ordinal comparison, and sorts ordinally
- Proper error handling ensures cache is initialized even on failure
MCPForUnity/Editor/Tools/RunTests.cs (3)
31-43: LGTM on timeout parsing with defensive error handling.The timeout parameter parsing is robust:
- Defaults to 600 seconds
- Validates positive integer values
- Silently falls back to default on parse errors (swallowing the exception is acceptable here since it's a nice-to-have parameter)
56-62: LGTM on timeout detection.Using
Task.WhenAnyto race the test run against a timeout is the correct pattern. The identity checkcompleted != runTaskproperly detects timeouts.
45-54: Good defensive error handling for test service startup.Wrapping
RunTestsAsyncinitiation in a try-catch and returning a structured error response prevents unhandled exceptions from propagating. This is especially important for async operations where the task might fail immediately.MCPForUnity/UnityMcpServer~/src/resources/menu_items.py (1)
6-7: LGTM! The static analysis hint is a false positive.The
data: list[str] = []field is correctly defined as a Pydantic model field default, not a class variable. The Ruff RUF012 hint suggestingtyping.ClassVaris a false positive in this context.MCPForUnity/UnityMcpServer~/src/tools/run_tests.py (1)
1-57: LGTM! Well-structured implementation.The tool function and data models are well-organized:
- Clear, hierarchical data models for test results
- Proper async context logging with
await ctx.info()- Conditional parameter building for optional timeout
- Consistent response wrapping pattern
MCPForUnity/Editor/MCPForUnityBridge.cs (3)
21-40: LGTM! Clear structure for async command tracking.The new
OutboundandQueuedCommandstructures properly support the async command execution model:
IsExecutingflag prevents concurrent processing- Clear separation of concerns between queue item state and network framing
69-1127: LGTM! Consistent logging migration.The comprehensive migration from
Debug.LogtoMcpLog.Info/Error/Warnimproves observability with:
- Structured log levels (Info, Warn, Error)
- Consistent prefixing via McpLog helpers
- Better alignment with MCP logging conventions
917-942: Remove explicit queue‐clear suggestion: Unity’s domain reload resets all static fields (includingcommandQueue), so no manual clear inOnBeforeAssemblyReloadis needed.Likely an incorrect or invalid review comment.
MCPForUnity/UnityMcpServer~/src/resources/tests.py (2)
16-17: LGTM! The static analysis hint is a false positive.Similar to
menu_items.py, thedata: list[TestItem] = []field is correctly defined as a Pydantic model field default. The Ruff RUF012 hint is a false positive.
20-31: LGTM! Clean resource implementation.Both resource functions follow the established pattern:
get_tests: Retrieves all testsget_tests_for_mode: Filters by mode (EditMode/PlayMode)- Consistent response wrapping with
GetTestsResponseMCPForUnity/Editor/Services/TestRunnerService.cs (4)
32-57: LGTM! Proper synchronization and context preservation.The method correctly uses:
SemaphoreSlimfor exclusive access during test discoveryConfigureAwait(false)for lock acquisition (no context needed)ConfigureAwait(true)forRetrieveTestRootAsyncto ensure Unity main thread context for Unity API calls
59-92: LGTM! Robust async execution with proper concurrency control.The implementation:
- Prevents concurrent test runs with
InvalidOperationException(lines 65-67)- Properly releases
_operationLockin both success and exception paths- Uses
TaskCompletionSourceto bridge Unity's callback-based API to async/await
154-182: LGTM! Well-handled async callback bridging.The method effectively bridges Unity's callback-based
RetrieveTestListto async/await:
EditorApplication.QueuePlayerLoopUpdate()ensures the callback is invoked even if the editor window is unfocused- 30-second timeout prevents indefinite hangs
- Proper error logging distinguishes timeouts from exceptions
184-386: LGTM! Clean tree traversal and data modeling.The implementation handles Unity's test hierarchy well:
- Recursive traversal builds complete test list (lines 184-233)
- Path tracking correctly adds/removes at each level (lines 199, 229-231)
HashSet<string>deduplicates tests by mode and full name (lines 207-209)- Data models (
TestRunResult,TestRunSummary,TestRunTestResult) provide clean serialization withToSerializable()methodsMCPForUnity/Editor/Tools/CommandRegistry.cs (3)
259-300: Async delegate creation is correct; verify ConfigureAwait choiceYou use ConfigureAwait(true). If handlers must resume on the Unity main thread, ensure ExecuteAsyncHandler is always invoked on the main thread (or capture Unity’s SynchronizationContext during Initialize and Post back). Otherwise consider ConfigureAwait(false) to avoid context captures.
101-107: Nice discovery summary logGood visibility on tools/resources counts and total handlers.
167-179: Good Task/Task detection and delegate wiringAccurately distinguishes async vs sync handlers and wraps Task results cleanly.
| /// <summary> | ||
| /// Get a synchronous command handler by name. | ||
| /// Throws if the command is asynchronous. | ||
| /// </summary> | ||
| /// <param name="commandName"></param> | ||
| /// <returns></returns> | ||
| /// <exception cref="InvalidOperationException"></exception> | ||
| public static Func<JObject, object> GetHandler(string commandName) | ||
| { | ||
| var handlerInfo = GetHandlerInfo(commandName); | ||
| if (handlerInfo.IsAsync) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Command '{commandName}' is asynchronous and must be executed via ExecuteCommand" | ||
| ); | ||
| } | ||
|
|
||
| return handlerInfo.SyncHandler; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Breaking change: GetHandler now throws for async commands; update call sites
This will throw for async handlers. Ensure all dispatchers use ExecuteCommand(...) for async-capable commands.
Run to find and update callers:
🏁 Script executed:
#!/bin/bash
rg -nP --type=cs -C3 '\bCommandRegistry\.(GetHandler|ExecuteCommand)\s*\('Length of output: 3874
Update MCPForUnityBridge calls to CommandRegistry.GetHandler
Call sites at UnityMcpBridge/Editor/MCPForUnityBridge.cs:1045 and 1093 will now throw for async commands. Ensure these invocations only target synchronous handlers or switch to CommandRegistry.ExecuteCommand for async-capable commands.
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Tools/CommandRegistry.cs around lines 205-223 the
GetHandler method now throws for asynchronous commands; update the two call
sites in UnityMcpBridge/Editor/MCPForUnityBridge.cs (around lines 1045 and 1093)
to avoid calling GetHandler for async-capable commands: either ensure the
requested command is synchronous before calling GetHandler, or switch those call
sites to use CommandRegistry.ExecuteCommand (await/handle task result) when the
command may be async. Modify the bridge logic to query GetHandlerInfo (or a new
helper) to detect IsAsync and route to the appropriate sync handler or
ExecuteCommand path, preserving existing parameter/result handling and error
propagation.
| fileFormatVersion: 2 | ||
| guid: 552680d3640c64564b19677d789515c3 | ||
| folderAsset: yes | ||
| DefaultImporter: | ||
| externalObjects: {} | ||
| userData: | ||
| assetBundleName: | ||
| assetBundleVariant: |
There was a problem hiding this comment.
🧩 Analysis chain
Looks correct for a Unity folder .meta; please verify no GUID collisions and that the folder exists.
Metadata is standard (folderAsset: yes, DefaultImporter). Since Resources relies on exact casing and GUID uniqueness, verify:
- The corresponding Resources directory exists at the same path and casing.
- No duplicate GUIDs across .meta files.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
META='TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta'
DIR='TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources'
# 1) Ensure the folder exists with exact casing
if [[ ! -d "$DIR" ]]; then
echo "Missing directory: $DIR"
exit 1
fi
if [[ "$(basename "$DIR")" != "Resources" ]]; then
echo "Incorrect casing for Resources directory: $(basename "$DIR")"
exit 1
fi
# 2) Ensure this is a folder meta
grep -qE '^folderAsset:\s+yes$' "$META" || { echo "folderAsset: yes not set in $META"; exit 1; }
# 3) Extract this GUID
THIS_GUID=$(rg -n '^guid:\s*([0-9a-f]+)$' "$META" -r '$1')
echo "GUID for $META: $THIS_GUID"
# 4) Check for duplicate GUIDs across all .meta files
# Output duplicates and fail if any are found
DUP_OUTPUT=$(rg -n '^guid:\s*[0-9a-f]+$' -g '*.meta' \
| sed -E 's/^([^:]+):[0-9]+:guid:\s*([0-9a-f]+)/\2 \1/' \
| sort \
| awk '{
count[$1]++; files[$1]=(files[$1] ? files[$1] ", " $2 : $2)
} END {
for (g in count) if (count[g]>1) print "DUP_GUID " g " in: " files[g]
}')
if [[ -n "$DUP_OUTPUT" ]]; then
echo "$DUP_OUTPUT"
exit 1
fi
echo "No duplicate GUIDs found. Resources folder/meta looks good."Length of output: 10702
Resolve duplicate GUIDs across .meta files
Multiple GUID collisions were detected (e.g., between MCPForUnity and UnityMcpBridge .meta files). Unity requires each asset/folder to have a unique GUID—regenerate GUIDs for the colliding .meta files to prevent asset reference conflicts.
🤖 Prompt for AI Agents
In TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta (lines 1-8)
this .meta file shares a GUID that collides with other .meta files; regenerate a
unique GUID for this file to avoid Unity asset reference conflicts by either:
(a) deleting this .meta and letting Unity reimport the folder to create a new
GUID, or (b) replacing the guid value with a newly generated GUID (use Unity's
GUID generation tool or a UUID v4), then save and reimport; after changing the
GUID, search the project for the old GUID and update any asset references to the
new GUID (or re-link assets in the editor) to ensure no broken references
remain.
There was a problem hiding this comment.
That's OK, UnityMcpBridge is legacy isn't going to be updated moving forward. It's still there for backwards compatibility, but MCPForUnity is the only project for the Unity plugin
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for GUID collisions within MCPForUnity only (excluding UnityMcpBridge)
echo "Checking for GUID collisions within MCPForUnity directory only..."
DUP_OUTPUT=$(rg -n '^guid:\s*[0-9a-f]+$' MCPForUnity/ -g '*.meta' \
| sed -E 's/^([^:]+):[0-9]+:guid:\s*([0-9a-f]+)/\2 \1/' \
| sort \
| awk '{
count[$1]++; files[$1]=(files[$1] ? files[$1] ", " $2 : $2)
} END {
for (g in count) if (count[g]>1) print "DUPLICATE: " g " appears in: " files[g]
}')
if [[ -n "$DUP_OUTPUT" ]]; then
echo "⚠️ Found GUID collisions within MCPForUnity:"
echo "$DUP_OUTPUT"
exit 1
else
echo "✅ No GUID collisions found within MCPForUnity directory."
fi
Length of output: 122
@msanatan, thank you for the clarification! I've confirmed there are no GUID collisions within the MCPForUnity directory itself—all GUIDs are unique where it matters. Since UnityMcpBridge is legacy and maintained only for backwards compatibility, the cross-directory GUID overlap is not a concern.
The Resources.meta file and its GUID are good to go. ✅
✏️ Learnings added
Learnt from: msanatan
PR: CoplayDev/unity-mcp#316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.000Z
Learning: UnityMcpBridge is a legacy project kept for backwards compatibility; MCPForUnity is the only active Unity plugin project. GUID collisions between UnityMcpBridge and MCPForUnity are acceptable.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: dsarno
PR: CoplayDev/unity-mcp#260
File: UnityMcpBridge/UnityMcpServer~/src/server_version.txt:1-1
Timestamp: 2025-09-04T01:01:11.927Z
Learning: The UnityMcpBridge project is not maintaining changelogs yet, so don't suggest adding changelog entries for version bumps.
This reverts commit 012ea6b.
… GetTests commands
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
MCPForUnity/UnityMcpServer~/src/telemetry.py (1)
27-27: Critical: Import-time version read still fragile.The version reading logic remains vulnerable to import-time crashes. Although switching to
tomlihelps with Python 3.10 compatibility, the core issue from the previous review persists:open("pyproject.toml", "rb")will crash if the file is missing (e.g., in packaged contexts, wrong CWD, or CI environments), andtomli.load()or dictionary access can raise exceptions if the file is malformed.Apply this diff to add error handling and fallback version:
+import os + import tomli def get_package_version() -> str: """ - Open pyproject.toml and parse version - We use the tomli library instead of tomllib to support Python 3.10 + Resolve version from env or pyproject.toml when available; never raise. """ - with open("pyproject.toml", "rb") as f: - data = tomli.load(f) - return data["project"]["version"] + # Allow explicit override + env_ver = os.environ.get("UNITY_MCP_VERSION") + if env_ver: + return env_ver + + try: + if os.path.exists("pyproject.toml"): + with open("pyproject.toml", "rb") as f: + data = tomli.load(f) + return str(data.get("project", {}).get("version") or "0.0.0") + except Exception: + pass + + return "0.0.0" MCP_VERSION = get_package_version()Also applies to: 39-50
🧹 Nitpick comments (1)
MCPForUnity/Editor/Resources/Tests/GetTests.cs (1)
46-74: Inconsistent string validation method.Line 50 uses
string.IsNullOrEmptywhileModeParser.TryParse(line 84) usesstring.IsNullOrWhiteSpace. Since the mode parser will catch whitespace-only strings anyway, this validation is redundant. However, for consistency with the parser's behavior, useIsNullOrWhiteSpace:-if (string.IsNullOrEmpty(modeStr)) +if (string.IsNullOrWhiteSpace(modeStr)) { return Response.Error("'mode' parameter is required"); }Alternatively, remove this check entirely and let
ModeParser.TryParsehandle all validation:string modeStr = @params["mode"]?.ToString(); -if (string.IsNullOrWhiteSpace(modeStr)) -{ - return Response.Error("'mode' parameter is required"); -} - if (!ModeParser.TryParse(modeStr, out var parsedMode, out var parseError)) { return Response.Error(parseError); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MCPForUnity/Editor/Resources/Tests/GetTests.cs(1 hunks)MCPForUnity/UnityMcpServer~/src/config.py(2 hunks)MCPForUnity/UnityMcpServer~/src/telemetry.py(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- MCPForUnity/UnityMcpServer~/src/config.py
🧰 Additional context used
🧬 Code graph analysis (1)
MCPForUnity/Editor/Resources/Tests/GetTests.cs (4)
MCPForUnity/Editor/Services/ITestRunnerService.cs (2)
Task(16-16)Task(21-21)MCPForUnity/Editor/Tools/RunTests.cs (1)
Task(18-71)MCPForUnity/Editor/Services/TestRunnerService.cs (3)
Task(32-57)Task(59-92)Task(154-182)MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(8-67)
🔇 Additional comments (4)
MCPForUnity/UnityMcpServer~/src/telemetry.py (3)
59-59: LGTM!The new
RESOURCE_RETRIEVALenum member is correctly added and aligns with the new resource usage tracking functionality introduced later in the file.
83-84: LGTM!The type annotation updates consistently modernize the code to use Python 3.10+ union syntax (
|) and built-in generic types (dict,list), improving readability and aligning with current best practices.Also applies to: 213-214, 267-267, 291-294, 398-398, 409-411, 416-416, 421-421, 471-471, 484-484
450-468: LGTM!The new
record_resource_usagefunction correctly implements resource telemetry tracking, following the established pattern fromrecord_tool_usage. Error truncation, duration rounding, and data structure are consistent with existing telemetry functions.MCPForUnity/Editor/Resources/Tests/GetTests.cs (1)
77-105: LGTM!The
ModeParserimplementation is clean, well-structured, and handles all expected cases:
- Proper validation of null/whitespace input
- Case-insensitive mode matching
- Clear error messages
- Consistent use of
outparameters
| public static async Task<object> HandleCommand(JObject @params) | ||
| { | ||
| McpLog.Info("[GetTests] Retrieving tests for all modes"); | ||
| IReadOnlyList<Dictionary<string, string>> result; | ||
|
|
||
| try | ||
| { | ||
| result = await MCPServiceLocator.Tests.GetTestsAsync(mode: null).ConfigureAwait(true); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| McpLog.Error($"[GetTests] Error retrieving tests: {ex.Message}\n{ex.StackTrace}"); | ||
| return Response.Error("Failed to retrieve tests"); | ||
| } | ||
|
|
||
| string message = $"Retrieved {result.Count} tests"; | ||
|
|
||
| return Response.Success(message, result); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Previous review concerns have been addressed.
The unnecessary null check and conversion logic from the previous review have been successfully removed. The code now directly assigns the result and uses it without defensive checks, which is correct since GetTestsAsync returns a non-null collection.
However, verify that @params cannot be null when this method is invoked. If there's no upstream validation, add a null check:
public static async Task<object> HandleCommand(JObject @params)
{
+ if (@params == null)
+ {
+ return Response.Error("Parameters cannot be null");
+ }
+
McpLog.Info("[GetTests] Retrieving tests for all modes");Run the following script to check if other resource handlers validate @params for null:
🏁 Script executed:
#!/bin/bash
# Description: Check if other resource/tool handlers validate @params for null
# Search for HandleCommand methods that check @params for null
rg -nP -C3 --type=cs 'HandleCommand\s*\(\s*JObject\s+@params\s*\)' -A 10 | rg -C2 '@params\s*(==|is)\s*null'
# Search for all HandleCommand signatures to see the pattern
rg -nP --type=cs 'public\s+static\s+(?:async\s+)?Task<object>\s+HandleCommand\s*\(\s*JObject\s+@params\s*\)'Length of output: 2983
Add null-check for @params in both HandleCommand methods
Precede each HandleCommand(JObject @params) with:
public static async Task<object> HandleCommand(JObject @params)
{
+ if (@params == null)
+ return Response.Error("Parameters cannot be null");
McpLog.Info(...);
…
}[must apply same change at lines 18 and 46]
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static async Task<object> HandleCommand(JObject @params) | |
| { | |
| McpLog.Info("[GetTests] Retrieving tests for all modes"); | |
| IReadOnlyList<Dictionary<string, string>> result; | |
| try | |
| { | |
| result = await MCPServiceLocator.Tests.GetTestsAsync(mode: null).ConfigureAwait(true); | |
| } | |
| catch (Exception ex) | |
| { | |
| McpLog.Error($"[GetTests] Error retrieving tests: {ex.Message}\n{ex.StackTrace}"); | |
| return Response.Error("Failed to retrieve tests"); | |
| } | |
| string message = $"Retrieved {result.Count} tests"; | |
| return Response.Success(message, result); | |
| } | |
| public static async Task<object> HandleCommand(JObject @params) | |
| { | |
| if (@params == null) | |
| return Response.Error("Parameters cannot be null"); | |
| McpLog.Info("[GetTests] Retrieving tests for all modes"); | |
| IReadOnlyList<Dictionary<string, string>> result; | |
| try | |
| { | |
| result = await MCPServiceLocator.Tests.GetTestsAsync(mode: null).ConfigureAwait(true); | |
| } | |
| catch (Exception ex) | |
| { | |
| McpLog.Error($"[GetTests] Error retrieving tests: {ex.Message}\n{ex.StackTrace}"); | |
| return Response.Error("Failed to retrieve tests"); | |
| } | |
| string message = $"Retrieved {result.Count} tests"; | |
| return Response.Success(message, result); | |
| } |
🤖 Prompt for AI Agents
In MCPForUnity/Editor/Resources/Tests/GetTests.cs around lines 18-36 (and the
second HandleCommand at ~line 46), the HandleCommand(JObject @params) methods
lack a null-check for the incoming @params; add an early guard that checks if
@params is null and returns Response.Error("Missing parameters") (or similar)
before any logging or service calls to avoid NullReferenceExceptions; apply the
same null-check to both HandleCommand methods at the specified lines.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
MCPForUnity/UnityMcpServer~/src/resources/menu_items.py (2)
19-22: Consider supporting dynamic query parameters.The
refreshandsearchparameters are hardcoded, but the comment indicates that FastMCP supports query parameters. If this feature is available or planned, consider accepting these as function parameters to allow callers to customize the behavior.If query parameter support is available, refactor the function signature:
-async def get_menu_items() -> GetMenuItemsResponse: +async def get_menu_items(ctx: Context, refresh: bool = True, search: str = "") -> GetMenuItemsResponse: """Provides a list of all menu items.""" - # Later versions of FastMCP support these as query parameters - # See: https://gofastmcp.com/servers/resources#query-parameters params = { - "refresh": True, - "search": "", + "refresh": refresh, + "search": search, }
24-25: Add defensive error handling for unexpected response formats.The return statement assumes that if
responseis not a dict, it's a valid response object. Consider adding validation or error handling for cases whereasync_send_command_with_retryreturns an unexpected type or error state.response = await async_send_command_with_retry("get_menu_items", params) -return GetMenuItemsResponse(**response) if isinstance(response, dict) else response +if isinstance(response, dict): + return GetMenuItemsResponse(**response) +if isinstance(response, GetMenuItemsResponse): + return response +# Handle unexpected response type +return GetMenuItemsResponse( + success=False, + error=f"Unexpected response type: {type(response).__name__}" +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
MCPForUnity/UnityMcpServer~/src/resources/menu_items.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
MCPForUnity/UnityMcpServer~/src/resources/menu_items.py (2)
MCPForUnity/UnityMcpServer~/src/models.py (1)
MCPResponse(5-9)MCPForUnity/UnityMcpServer~/src/registry/resource_registry.py (1)
mcp_for_unity_resource(10-43)
🪛 Ruff (0.14.0)
MCPForUnity/UnityMcpServer~/src/resources/menu_items.py
7-7: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🔇 Additional comments (3)
MCPForUnity/UnityMcpServer~/src/resources/menu_items.py (3)
1-3: LGTM!The imports follow a clean, absolute style within the package. They correctly reference the core modules for MCP response models, resource registration, and Unity communication.
6-7: LGTM! Static analysis hint is a false positive.The class correctly extends
MCPResponseand defines a typeddatafield. The static analysis warning (RUF012) about the mutable default[]is a false positive—Pydantic handles mutable defaults correctly by creating a new list instance for each model instance.
15-15: Noctx: Contextparameter needed Resource handlers (get_tests,get_tests_for_mode, etc.) consistently omitctxand operate correctly without it.Likely an incorrect or invalid review comment.
1 participant
Fix ctx.info callsCloses #307, and I guess closes #299. This is part of #287 but it doesn't close that one
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores