Allow the LLMs to read menu items, not just execute them #263
Conversation
If it ain't broke with the other clients, don't fix...
…ws compatibility issues
…ol supporting list/exists/refresh
…asset creation strategy
|
Caution Review failedThe pull request is closed. WalkthroughRenames the command/tool from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant MCP as MCP Tool (manage_menu_item)
participant Server as Python Server
participant Bridge as Unity Bridge
participant Handler as ManageMenuItem
participant Exec as MenuItemExecutor
participant Reader as MenuItemsReader
User->>MCP: manage_menu_item(action, menu_path/search/refresh)
MCP->>Server: async_send_command_with_retry("manage_menu_item", params)
Server->>Bridge: sendCommand("manage_menu_item", params)
Bridge->>Handler: HandleCommand(params)
alt action == "execute"
Handler->>Exec: Execute(params)
Exec-->>Handler: Response (scheduled)
else action == "list"
Handler->>Reader: List(params)
Reader-->>Handler: Response (array)
else action == "exists"
Handler->>Reader: Exists(params)
Reader-->>Handler: Response (exists flag)
else action == "refresh"
Handler->>Reader: RefreshCommand()
Reader-->>Handler: Response (refreshed list)
else unknown
Handler-->>Bridge: Error("Unknown action")
end
Handler-->>Bridge: Response
Bridge-->>Server: Response
Server-->>MCP: Response
MCP-->>User: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
@msanatan will review today |
|
@msanatan I pulled and tested this. Looks great to me! Minor limitation is that it can't see some core File menu items (it only sees New Scene and Save as Scene Template). But if people want to use those, |
|
Oh yeah and please bump |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
UnityMcpBridge/Editor/Tools/MenuItems/MenuItemExecutor.cs (1)
1-67: Replace leftoverexecute_menu_itemreferences
- UnityMcpBridge/UnityMcpServer~/src/reload_sentinel.py line 8 still mentions
execute_menu_itemin the deprecation message- UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py lines 330–332 call
send_command_with_retry("execute_menu_item", …)Update both to use the new
MenuItemExecutor.ExecuteAPI.
🧹 Nitpick comments (23)
UnityMcpBridge/Editor/Tools/MenuItems/MenuItemExecutor.cs (4)
25-31: Input validation: trim and early‑normalize the path.Adds resilience to stray whitespace.
- string menuPath = @params["menu_path"]?.ToString() ?? @params["menuPath"]?.ToString(); + string menuPath = (@params["menu_path"]?.ToString() ?? @params["menuPath"]?.ToString())?.Trim();
40-56: Asynchronous semantics—callers won’t know success/failure.Scheduling via delayCall is fine, but it changes behavior vs. a synchronous ExecuteMenuItem. Please confirm all call-sites/tests expect "scheduled" rather than "executed". Optionally, preflight with a cheap existence check to fail fast.
Option (optional) preflight:
// If MenuItemsReader.Exists is cheap, consider: if (!MenuItemsReader.Exists(new JObject { ["menu_path"] = menuPath })) return Response.Error($"Menu item '{menuPath}' not found or disabled.");
45-49: Log levels and success trace.A non-executed item is usually a warning, not an error. Also add a success trace for observability.
- bool executed = EditorApplication.ExecuteMenuItem(menuPath); - if (!executed) - { - Debug.LogError($"[MenuItemExecutor] Failed to execute menu item via delayCall: '{menuPath}'. It might be invalid, disabled, or context-dependent."); - } + bool executed = EditorApplication.ExecuteMenuItem(menuPath); + if (executed) + { + McpLog.Info($"[MenuItemExecutor] Executed: '{menuPath}'", always: false); + } + else + { + Debug.LogWarning($"[MenuItemExecutor] Failed to execute: '{menuPath}' (invalid/disabled/context-dependent)."); + }
57-63: Return structured payload indicating scheduling rather than execution.Helps clients programmatically reason about results and keeps parity with previous shape.
- return Response.Success($"Attempted to execute menu item: '{menuPath}'. Check Unity logs for confirmation or errors."); + return Response.Success( + "Scheduled menu item execution.", + new { scheduled = true, menuPath } + );UnityMcpBridge/Editor/Tools/CommandRegistry.cs (1)
23-25: Add back-compat alias for legacy “HandleExecuteMenuItem” callersSome clients may still call the old handler name. Add an alias that defaults action=execute to avoid breaking them.
private static readonly Dictionary<string, Func<JObject, object>> _handlers = new() { { "HandleManageScript", ManageScript.HandleCommand }, @@ { "HandleReadConsole", ReadConsole.HandleCommand }, { "HandleManageMenuItem", ManageMenuItem.HandleCommand }, + // Back-compat shim for older clients + { "HandleExecuteMenuItem", p => { + if (p == null) p = new JObject(); + if (p["action"] == null) p["action"] = "execute"; + return ManageMenuItem.HandleCommand(p); + }}, { "HandleManageShader", ManageShader.HandleCommand}, };UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
800-812: Provide runtime fallback for legacy command type “execute_menu_item”Add a case that routes old requests to ManageMenuItem with action=execute to smooth upgrades.
"manage_shader" => ManageShader.HandleCommand(paramsObject), "read_console" => ReadConsole.HandleCommand(paramsObject), "manage_menu_item" => ManageMenuItem.HandleCommand(paramsObject), + // Back-compat: route old command to new handler + "execute_menu_item" => ManageMenuItem.HandleCommand(WithAction(paramsObject, "execute")), _ => throw new ArgumentException( $"Unknown or unsupported command type: {command.type}" ),Add this helper in the class (outside the shown range):
private static JObject WithAction(JObject p, string action) { p ??= new JObject(); if (p["action"] == null || string.IsNullOrWhiteSpace(p["action"]?.ToString())) p["action"] = action; return p; }README.md (1)
45-46: Clarify capabilities and fix list indentation (MD007)Spell out all actions and align indentation with surrounding bullets.
- * `manage_menu_item`: Execute and list Unity Editor menu items (e.g., execute "File/Save Project"). + * `manage_menu_item`: Execute, list, check existence, and refresh Unity Editor menu items (e.g., "File/Save Project").If markdownlint still flags indentation, normalize bullet indent across this list to match the linter’s expected style for top-level items.
UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (1)
27-28: Optional: export a shim for external callers of register_execute_menu_item_toolsKeeps downstream scripts from breaking if they import the old symbol.
from .manage_menu_item import register_manage_menu_item_tools @@ register_manage_menu_item_tools(mcp) register_resource_tools(mcp) logger.info("MCP for Unity Server tool registration complete.") + +# Back-compat: allow old import name used by external scripts +try: + register_execute_menu_item_tools = register_manage_menu_item_tools # type: ignore[misc] +except Exception: + passTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/MenuItemExecutorTests.cs (1)
11-37: Good coverage of success/error paths — LGTMCovers missing param, blacklist, and happy path. Optional: add a test asserting underscore key
menu_pathworks too.+ [Test] + public void Execute_UnderscoreParam_ReturnsImmediateSuccess() + { + var res = MenuItemExecutor.Execute(new JObject { ["menu_path"] = "File/Save Project" }); + var jo = ToJO(res); + Assert.IsTrue((bool)jo["success"]); + }UnityMcpBridge/Editor/Tools/MenuItems/ManageMenuItem.cs (2)
20-24: DRY the valid-actions list to avoid string driftCentralize the list once and reuse in messages.
public static class ManageMenuItem { + private static readonly string[] ValidActions = { "execute", "list", "exists", "refresh" }; @@ - if (string.IsNullOrEmpty(action)) - { - return Response.Error("Action parameter is required. Valid actions are: execute, list, exists, refresh."); - } + if (string.IsNullOrEmpty(action)) + { + return Response.Error($"Action parameter is required. Valid actions are: {string.Join(", ", ValidActions)}."); + } @@ - default: - return Response.Error($"Unknown action: '{action}'. Valid actions are: execute, list, exists, refresh."); + default: + return Response.Error($"Unknown action: '{action}'. Valid actions are: {string.Join(", ", ValidActions)}.");Also applies to: 39-40
18-47: Note on enumeration limits (core File menu items)TypeCache-based scans won’t list native/builtin menu items (e.g., many under File). Consider documenting this in README and advising users to call execute/exists directly for those paths; listing/refresh may not surface them.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py (3)
15-20: Silence Ruff ARG001: unused ctxRename to _ctx.
- async def manage_menu_item( - ctx: Context, + async def manage_menu_item( + _ctx: Context, action: str, menu_path: str | None = None, search: str | None = None, refresh: bool = False, ) -> dict[str, Any]:
33-46: Pre-validate required params for execute/exists (save a roundtrip)Guard on Python side to return a fast, consistent error.
action = (action or "").lower() if not action: return {"success": False, "error": "action is required (execute|list|exists|refresh)"} + if action in ("execute", "exists") and not menu_path: + return {"success": False, "error": "'menu_path' is required for actions: execute, exists"}
53-55: Use “error” key in fallback for consistencyOther error paths use “error”; align the fallback.
- result = await async_send_command_with_retry("manage_menu_item", params_dict, loop=loop) - return result if isinstance(result, dict) else {"success": False, "message": str(result)} + result = await async_send_command_with_retry("manage_menu_item", params_dict, loop=loop) + return result if isinstance(result, dict) else {"success": False, "error": str(result)}TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/MenuItemsReaderTests.cs (3)
33-41: Also assert de-duplication.
Since the reader applies Distinct, add a quick uniqueness check to prevent regressions.if (arr.Count >= 2) { var original = arr.Select(t => (string)t).ToList(); var sorted = original.OrderBy(s => s, StringComparer.Ordinal).ToList(); CollectionAssert.AreEqual(sorted, original, "Expected menu items to be sorted ascending"); + Assert.AreEqual(original.Count, original.Distinct(StringComparer.Ordinal).Count(), + "Expected no duplicate menu item entries"); }
53-78: Exercise explicit case-insensitivity in search.
Upper/lowercase the term to prove case-insensitive matching.- var res = MenuItemsReader.List(new JObject { ["search"] = term }); + var res = MenuItemsReader.List(new JObject { ["search"] = term.ToUpperInvariant() });
89-97: Add test for snake_case parameter alias.
Covers the documented 'menu_path' alternative.[Test] public void Exists_Bogus_ReturnsFalse() { var res = MenuItemsReader.Exists(new JObject { ["menuPath"] = "Nonexistent/Menu/___unlikely___" }); var jo = ToJO(res); Assert.IsTrue((bool)jo["success"], "Expected success true"); Assert.IsNotNull(jo["data"], "Expected data field present"); Assert.IsFalse((bool)jo["data"]["exists"], "Expected exists false for bogus menu path"); } + + [Test] + public void Exists_SnakeCaseParamAlias_Works() + { + var res = MenuItemsReader.Exists(new JObject { ["menu_path"] = "Nonexistent/Menu/___unlikely___" }); + var jo = ToJO(res); + Assert.IsTrue((bool)jo["success"]); + Assert.IsFalse((bool)jo["data"]["exists"]); + }UnityMcpBridge/UnityMcpServer~/src/server.py (1)
73-75: Nit: hyphenate “third-party” and tighten phrasing.
Minor polish for user-facing copy.- "- Use `manage_menu_item` for interacting with Unity systems and third party tools like a user would.\n" + "- Use `manage_menu_item` for interacting with Unity systems and third-party tools as a user would.\n" - "- If a menu item seems missing, refresh the cache: use manage_menu_item with action='list' and refresh=true, or action='refresh'. Avoid refreshing every time; prefer refresh only when the menu set likely changed.\n" + "- If a menu item seems missing, refresh the cache (action='list' with refresh=true, or action='refresh'). Avoid refreshing every time; only refresh when the menu set likely changed.\n"TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/ManageMenuItemTests.cs (2)
29-36: Good guard against executing blocked items.
Consider asserting the error code if Response.Error code is set, to harden the check.- StringAssert.Contains("blocked for safety", (string)jo["error"], "Expected blacklist message"); + StringAssert.Contains("blocked for safety", (string)jo["error"], "Expected blacklist message"); + Assert.IsNotNull(jo["code"], "Expected an error code for machine parsing");
38-46: Add coverage for refresh action and action case-insensitivity.
Ensures full façade behavior.[Test] public void HandleCommand_Exists_MissingParam_ReturnsError() { var res = ManageMenuItem.HandleCommand(new JObject { ["action"] = "exists" }); var jo = ToJO(res); Assert.IsFalse((bool)jo["success"], "Expected success false when missing menuPath"); StringAssert.Contains("Required parameter", (string)jo["error"]); } + + [Test] + public void HandleCommand_Refresh_ReturnsList() + { + var res = ManageMenuItem.HandleCommand(new JObject { ["action"] = "refresh" }); + var jo = ToJO(res); + Assert.IsTrue((bool)jo["success"]); + Assert.AreEqual(JTokenType.Array, jo["data"].Type); + } + + [Test] + public void HandleCommand_ActionCaseInsensitivity_Works() + { + var res = ManageMenuItem.HandleCommand(new JObject { ["action"] = "LiSt" }); + var jo = ToJO(res); + Assert.IsTrue((bool)jo["success"]); + Assert.AreEqual(JTokenType.Array, jo["data"].Type); + }UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs (3)
68-79: Trim search input and short-circuit when empty.
Prevents accidental whitespace queries and small allocs.- string search = @params["search"]?.ToString(); + string search = @params["search"]?.ToString()?.Trim(); @@ - if (!string.IsNullOrEmpty(search)) + if (!string.IsNullOrEmpty(search)) { result = result.Where(s => s.IndexOf(search, StringComparison.OrdinalIgnoreCase) >= 0); }
89-103: Consider optional case-insensitive exists.
List search is case-insensitive, but exists is strict; offering an opt-in improves UX without breaking defaults.- string menuPath = @params["menu_path"]?.ToString() ?? @params["menuPath"]?.ToString(); + string menuPath = @params["menu_path"]?.ToString() ?? @params["menuPath"]?.ToString(); + bool ignoreCase = @params["ignoreCase"]?.ToObject<bool?>() ?? true; @@ - bool exists = (_cached ?? new List<string>()).Contains(menuPath); + var list = _cached ?? new List<string>(); + bool exists = ignoreCase + ? list.Any(s => string.Equals(s, menuPath, StringComparison.OrdinalIgnoreCase)) + : list.Contains(menuPath); return Response.Success($"Exists check completed for '{menuPath}'.", new { exists });
18-25: Expose cache snapshot safely.
If external misuse is a concern, consider returning a copy to prevent accidental mutation via interface leaks.- public static IReadOnlyList<string> AllMenuItems() => _cached ??= Refresh(); + public static IReadOnlyList<string> AllMenuItems() => (_cached ??= Refresh()).ToList();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
README.md(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/ManageMenuItemTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/ManageMenuItemTests.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/MenuItemExecutorTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/MenuItemExecutorTests.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/MenuItemsReaderTests.cs(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/MenuItemsReaderTests.cs.meta(1 hunks)UnityMcpBridge/Editor/MCPForUnityBridge.cs(2 hunks)UnityMcpBridge/Editor/Tools/CommandRegistry.cs(2 hunks)UnityMcpBridge/Editor/Tools/ExecuteMenuItem.cs(0 hunks)UnityMcpBridge/Editor/Tools/MenuItems.meta(1 hunks)UnityMcpBridge/Editor/Tools/MenuItems/ManageMenuItem.cs(1 hunks)UnityMcpBridge/Editor/Tools/MenuItems/ManageMenuItem.cs.meta(1 hunks)UnityMcpBridge/Editor/Tools/MenuItems/MenuItemExecutor.cs(1 hunks)UnityMcpBridge/Editor/Tools/MenuItems/MenuItemExecutor.cs.meta(1 hunks)UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs(1 hunks)UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs.meta(1 hunks)UnityMcpBridge/UnityMcpServer~/src/server.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py(2 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py(0 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py(1 hunks)
💤 Files with no reviewable changes (2)
- UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py
- UnityMcpBridge/Editor/Tools/ExecuteMenuItem.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-03T16:00:55.812Z
Learnt from: dsarno
PR: CoplayDev/unity-mcp#0
File: :0-0
Timestamp: 2025-09-03T16:00:55.812Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
UnityMcpBridge/Editor/MCPForUnityBridge.cs
🧬 Code graph analysis (10)
UnityMcpBridge/Editor/Tools/MenuItems/ManageMenuItem.cs (3)
UnityMcpBridge/Editor/Helpers/Response.cs (1)
Response(10-61)UnityMcpBridge/Editor/Tools/MenuItems/MenuItemExecutor.cs (2)
MenuItemExecutor(13-65)Execute(25-64)UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs (5)
MenuItemsReader(14-104)List(29-52)List(66-82)Exists(87-103)RefreshCommand(57-61)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/MenuItemsReaderTests.cs (1)
UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs (5)
MenuItemsReader(14-104)RefreshCommand(57-61)List(29-52)List(66-82)Exists(87-103)
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
UnityMcpBridge/Editor/Tools/MenuItems/ManageMenuItem.cs (2)
ManageMenuItem(13-48)HandleCommand(18-47)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/MenuItemExecutorTests.cs (1)
UnityMcpBridge/Editor/Tools/MenuItems/MenuItemExecutor.cs (2)
MenuItemExecutor(13-65)Execute(25-64)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection(351-371)async_send_command_with_retry(410-422)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/ManageMenuItemTests.cs (1)
UnityMcpBridge/Editor/Tools/MenuItems/ManageMenuItem.cs (1)
ManageMenuItem(13-48)
UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs (5)
UnityMcpBridge/Editor/Windows/MCPForUnityEditorWindow.cs (1)
MenuItem(48-52)UnityMcpBridge/Editor/Helpers/Response.cs (2)
Response(10-61)Success(18-33)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/ManageMenuItemTests.cs (1)
JObject(9-9)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/MenuItemExecutorTests.cs (1)
JObject(9-9)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/MenuItemsReaderTests.cs (1)
JObject(11-11)
UnityMcpBridge/Editor/Tools/CommandRegistry.cs (1)
UnityMcpBridge/Editor/Tools/MenuItems/ManageMenuItem.cs (2)
ManageMenuItem(13-48)HandleCommand(18-47)
UnityMcpBridge/Editor/Tools/MenuItems/MenuItemExecutor.cs (2)
UnityMcpBridge/Editor/Helpers/Response.cs (2)
Response(10-61)Success(18-33)UnityMcpBridge/Editor/Tools/ExecuteMenuItem.cs (1)
ExecuteMenuItem(13-128)
UnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py (2)
manage_menu_item(14-54)register_manage_menu_item_tools(10-54)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py
15-15: Unused function argument: ctx
(ARG001)
🪛 markdownlint-cli2 (0.17.2)
README.md
45-45: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🔇 Additional comments (18)
UnityMcpBridge/Editor/Tools/MenuItems/ManageMenuItem.cs.meta (1)
1-11: Unity meta: OK to commit.New MonoImporter meta looks standard; no issues.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems.meta (1)
1-9: Folder meta: OK.Folder asset GUID registered; nothing actionable.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/MenuItemsReaderTests.cs.meta (1)
1-11: Test meta: OK.Standard MonoImporter fields; ship it.
UnityMcpBridge/Editor/Tools/MenuItems.meta (1)
1-9: Folder meta: OK.Matches typical Unity folder meta.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/ManageMenuItemTests.cs.meta (1)
2-2: No GUID-coupled references found.Only the meta file contains the new GUID; safe to proceed.
UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs.meta (1)
1-11: Unity meta: OK.No concerns.
UnityMcpBridge/Editor/Tools/MenuItems/MenuItemExecutor.cs.meta (1)
1-11: Unity meta: OK.All default fields present; good to go.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/MenuItemExecutorTests.cs.meta (1)
1-11: Unity .meta addition — LGTMGUID and importer settings look fine.
UnityMcpBridge/Editor/Tools/CommandRegistry.cs (1)
4-4: Needed namespace import — LGTMRequired for ManageMenuItem reference.
UnityMcpBridge/Editor/MCPForUnityBridge.cs (1)
16-16: Import for menu items tooling — LGTMUnityMcpBridge/UnityMcpServer~/src/tools/__init__.py (1)
10-10: Swap to manage_menu_item registration — LGTMUnityMcpBridge/Editor/Tools/MenuItems/ManageMenuItem.cs (1)
18-47: Action routing and error handling — LGTMClear delegation to execute/list/exists/refresh with standardized responses.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/MenuItemsReaderTests.cs (2)
13-22: Good coverage of refresh path and response shape.
Assertions match Response.Success contract and message content.
80-87: Error path assertion reads well.
Message substring target is stable and aligned with Response.Error.UnityMcpBridge/UnityMcpServer~/src/server.py (1)
55-76: Docs update aligns with new manage_menu_item capabilities.
Clear guidance and cautions about refresh; good.TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/ManageMenuItemTests.cs (2)
11-18: Unknown action test is precise and helpful.
Catches routing regressions early.
20-27: List routing happy-path looks good.
Covers the common case.UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs (1)
29-44: Deterministic scan with de-dupe and ordinal sort.
Solid use of TypeCache, Distinct, and StringComparer.Ordinal. Good error hardening.
| // Basic blacklist to prevent execution of disruptive menu items. | ||
| private static readonly HashSet<string> _menuPathBlacklist = new HashSet<string>( | ||
| StringComparer.OrdinalIgnoreCase) | ||
| { | ||
| "File/Quit", | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Blacklist is too narrow—add cross‑platform quit variants and a few high‑risk entries.
Current block only catches "File/Quit". On Windows/macOS, common variants include "File/Exit" and "Unity/Quit Unity". Consider adding a few other disruptive items (e.g., "Edit/Preferences...", "File/Build Settings...", "Assets/Import Package/Custom Package...") or make the list configurable.
Apply minimally safe additions:
private static readonly HashSet<string> _menuPathBlacklist = new HashSet<string>(
StringComparer.OrdinalIgnoreCase)
{
"File/Quit",
+ "File/Exit",
+ "Unity/Quit Unity",
+ "Edit/Preferences...",
+ "File/Build Settings...",
+ "Window/Layouts/Save Layout",
+ "Window/Package Manager",
+ "Assets/Import Package/Custom Package..."
};
Will do! |
I'll check that one! When testing I checked NuGet and another package, and some basic Unity Window items. At least for my own curiosity, I'll definitely have a look. Thanks for testing! I'll fix the merge requests, do your suggestions and merge it in |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
README.md (1)
45-45: Document all supported actions and tighten the example.Expand the description to include
existsandrefresh, and format the example path as code for clarity.- * `manage_menu_item`: Execute and list Unity Editor menu items (e.g., execute "File/Save Project"). + * `manage_menu_item`: Execute, list, check existence, and refresh Unity Editor menu items (e.g., execute `File/Save Project`).Nit: markdownlint may flag MD007 (list indentation). Consider normalizing indentation for this list or adding a local disable comment if intentional.
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py (2)
34-47: Pre-validate action and required parameters client-side.Short-circuiting invalid requests improves UX and avoids unnecessary Unity roundtrips. Also only include
refreshwhen true to keep payload minimal.action = (action or "").lower() if not action: return {"success": False, "error": "action is required (execute|list|exists|refresh)"} - # Prepare parameters for the C# handler - params_dict: dict[str, Any] = { - "action": action, - "menuPath": menu_path, - "search": search, - "refresh": refresh, - } - # Remove None values - params_dict = {k: v for k, v in params_dict.items() if v is not None} + # Validate supported actions and required args + allowed = {"execute", "list", "exists", "refresh"} + if action not in allowed: + return {"success": False, "error": f"invalid action '{action}' (expected one of: execute|list|exists|refresh)"} # noqa: E501 + if action in {"execute", "exists"} and not menu_path: + return {"success": False, "error": f"menu_path is required for action '{action}'"} + + # Prepare parameters for the C# handler + params_dict: dict[str, Any] = { + "action": action, + "menuPath": menu_path, + "search": search, + } + if refresh: + params_dict["refresh"] = True + # Remove None values + params_dict = {k: v for k, v in params_dict.items() if v is not None}
55-55: Use a consistent error key in fallback path.Else branch should return
error(notmessage) to match the docstring and other tools.- return result if isinstance(result, dict) else {"success": False, "message": str(result)} + return result if isinstance(result, dict) else {"success": False, "error": str(result)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(1 hunks)UnityMcpBridge/Editor/MCPForUnityBridge.cs(2 hunks)UnityMcpBridge/Editor/Tools/MenuItems/ManageMenuItem.cs(1 hunks)UnityMcpBridge/Editor/Tools/MenuItems/MenuItemExecutor.cs(1 hunks)UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs(1 hunks)UnityMcpBridge/UnityMcpServer~/src/server.py(3 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- UnityMcpBridge/Editor/MCPForUnityBridge.cs
- UnityMcpBridge/Editor/Tools/MenuItems/MenuItemExecutor.cs
- UnityMcpBridge/Editor/Tools/MenuItems/ManageMenuItem.cs
- UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs
- UnityMcpBridge/UnityMcpServer~/src/server.py
🧰 Additional context used
🧬 Code graph analysis (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection(351-371)async_send_command_with_retry(410-422)
🪛 markdownlint-cli2 (0.17.2)
README.md
45-45: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py
16-16: Unused function argument: ctx
(ARG001)
🔇 Additional comments (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py (1)
13-15: Confirm decorator order yields expected telemetry coverage.Current order applies
telemetry_toolfirst, thenmcp.tool. Ensure this captures tool execution as intended; if you want to include FastMCP’s wrapper timings, swap the order.
More info to LLMs + better validation
It's only ever useful in the context of listing menu items
| @telemetry_tool("manage_menu_item") | ||
| async def manage_menu_item( | ||
| ctx: Context, | ||
| action: Annotated[Literal["execute", "list", "exists"], "One of 'execute', 'list', 'exists'"], |
There was a problem hiding this comment.
@dsarno @Scriptwonder @justinpbarnett I did a test to ensure it works, but I'll update the other MCP definitions with literals instead of just strings. Should be clearer to us and LLMs what's allowed + removes some validation on the value of action
There was a problem hiding this comment.
If we had the need for it, we can also add Pydantic for better input validation:
https://gofastmcp.com/servers/tools#advanced-metadata-with-field
|
Merging in as @dsarno checked it out! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py (1)
19-19: Silence Ruff ARG001 for unused ctx without changing the tool signature.This param is required by FastMCP; silence the linter.
Apply either one (both also fine):
- ctx: Context, + ctx: Context, # noqa: ARG001 - required by FastMCP tool interfaceor:
) -> dict[str, Any]: """Manage Unity menu items (execute/list/exists). Args: ctx: The MCP context. @@ """ + # ctx is required by FastMCP but unused in this handler + del ctx # noqa: F841
🧹 Nitpick comments (6)
README.md (1)
45-45: Tighten copy and fix markdownlint MD007 (unordered list indentation).
- Wording: remove the semicolon and minor tweak for clarity.
- MD007: either normalize list indentation to 0 spaces for this block or disable the rule around the list.
Apply this minimal edit now (and optionally align surrounding bullets similarly):
- * `manage_menu_item`: List Unity Editor menu items; and check for their existence or execute them (e.g., execute "File/Save Project"). +* `manage_menu_item`: List Unity Editor menu items, check existence, or execute them (e.g., execute "File/Save Project").Option B (if you prefer to keep indentation): wrap the list with
<!-- markdownlint-disable MD007 -->/<!-- markdownlint-enable MD007 -->.UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py (2)
20-27: Fail fast when menu_path is missing for execute/exists.Avoid a round-trip to Unity for obvious parameter errors.
action: Annotated[Literal["execute", "list", "exists"], "One of 'execute', 'list', 'exists'"], menu_path: Annotated[str | None, "Menu path for 'execute' or 'exists' (e.g., 'File/Save Project')"] = None, @@ ) -> dict[str, Any]: @@ - # Prepare parameters for the C# handler + # Pre-validate required params for specific actions + if action in ("execute", "exists") and not menu_path: + return {"success": False, "error": "menu_path is required for 'execute' and 'exists' actions."} + + # Prepare parameters for the C# handler
56-57: Align error payload key with the rest of the server (“error”, not “message”).Non-dict results are unlikely, but keep response shape consistent.
- return result if isinstance(result, dict) else {"success": False, "message": str(result)} + return result if isinstance(result, dict) else {"success": False, "error": str(result)}UnityMcpBridge/Editor/Tools/MenuItems/ManageMenuItem.cs (1)
16-17: Remove “refresh” from valid actions (it’s a parameter, not an action).Python tool only allows execute|list|exists; current comments and errors mention “refresh” as an action, which is misleading.
- /// Routes actions: execute, list, exists, refresh + /// Routes actions: execute, list, exists @@ - return Response.Error("Action parameter is required. Valid actions are: execute, list, exists, refresh."); + return Response.Error("Action parameter is required. Valid actions are: execute, list, exists."); @@ - return Response.Error($"Unknown action: '{action}'. Valid actions are: execute, list, exists, refresh."); + return Response.Error($"Unknown action: '{action}'. Valid actions are: execute, list, exists.");If you did intend to support a “refresh” action, add a case here and expose a
RefreshCommand()in MenuItemsReader. Happy to draft that as well.Also applies to: 23-24, 36-38
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/MenuItemsReaderTests.cs (1)
69-86: Add a test to exercise the ‘refresh’ flag path.This ensures the cache rebuild pathway is covered.
Here’s a minimal pattern that toggles refresh without relying on specific contents:
+ [Test] + public void List_WithRefresh_DoesNotThrow_AndReturnsArray() + { + var res = MenuItemsReader.List(new JObject { ["refresh"] = true }); + var jo = JObject.FromObject(res); + Assert.IsTrue((bool)jo["success"]); + Assert.AreEqual(JTokenType.Array, jo["data"].Type); + }UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs (1)
92-94: Speed up Exists() with a HashSet cache (optional).For larger menus, Contains on a List is O(n). Keep a HashSet alongside the sorted list for O(1) existence checks. Consider OrdinalIgnoreCase if you want case-insensitive lookups.
- private static List<string> _cached; + private static List<string> _cached; + private static HashSet<string> _cachedSet; @@ - _cached = methods + _cached = methods ... .ToList(); + _cachedSet = new HashSet<string>(_cached, StringComparer.Ordinal); return _cached; @@ - bool exists = (_cached ?? new List<string>()).Contains(menuPath); + bool exists = _cachedSet != null + ? _cachedSet.Contains(menuPath) + : (_cached ?? new List<string>()).Contains(menuPath); return Response.Success($"Exists check completed for '{menuPath}'.", new { exists });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
README.md(1 hunks)TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs.meta(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/MenuItemsReaderTests.cs(1 hunks)TestProjects/UnityMCPTests/ProjectSettings/Packages/com.unity.testtools.codecoverage/Settings.json(1 hunks)UnityMcpBridge/Editor/AssemblyInfo.cs.meta(1 hunks)UnityMcpBridge/Editor/Helpers/PackageDetector.cs.meta(1 hunks)UnityMcpBridge/Editor/Tools/MenuItems/ManageMenuItem.cs(1 hunks)UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs(1 hunks)UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- TestProjects/UnityMCPTests/Assets/Scripts/TestAsmdef.meta
- TestProjects/UnityMCPTests/Assets/Scripts/LongUnityScriptClaudeTest.cs.meta
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs.meta
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs.meta
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-04T01:01:11.927Z
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.
Applied to files:
UnityMcpBridge/Editor/AssemblyInfo.cs.meta
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
PR: CoplayDev/unity-mcp#0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs.meta
🧬 Code graph analysis (4)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/MenuItemsReaderTests.cs (1)
UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs (4)
MenuItemsReader(14-95)List(29-52)List(57-73)Exists(78-94)
UnityMcpBridge/Editor/Tools/MenuItems/ManageMenuItem.cs (4)
UnityMcpBridge/Editor/Helpers/Response.cs (1)
Response(10-61)UnityMcpBridge/Editor/Tools/MenuItems/MenuItemExecutor.cs (2)
MenuItemExecutor(13-65)Execute(25-64)UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs (4)
MenuItemsReader(14-95)List(29-52)List(57-73)Exists(78-94)UnityMcpBridge/Editor/Helpers/McpLog.cs (1)
McpLog(6-30)
UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs (2)
UnityMcpBridge/Editor/Helpers/McpLog.cs (1)
McpLog(6-30)UnityMcpBridge/Editor/Helpers/Response.cs (2)
Response(10-61)Success(18-33)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py (1)
UnityMcpBridge/UnityMcpServer~/src/unity_connection.py (2)
get_unity_connection(351-371)async_send_command_with_retry(410-422)
🪛 markdownlint-cli2 (0.17.2)
README.md
45-45: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🪛 Ruff (0.12.2)
UnityMcpBridge/UnityMcpServer~/src/tools/manage_menu_item.py
19-19: Unused function argument: ctx
(ARG001)
🔇 Additional comments (7)
TestProjects/UnityMCPTests/ProjectSettings/Packages/com.unity.testtools.codecoverage/Settings.json (1)
2-3: LGTM: Adds standard Unity Code Coverage metadata.m_Name/m_Path look correct for this asset; no functional concerns.
Please confirm this file is intentionally versioned—Unity can regenerate Package Settings across editor versions/OS, which might create noisy diffs in CI/PRs.
UnityMcpBridge/Editor/Helpers/PackageDetector.cs.meta (1)
2-11: Meta importer normalization looks correct.GUID preserved and MonoImporter block is standard for C# scripts. No action needed.
UnityMcpBridge/Editor/AssemblyInfo.cs.meta (1)
2-11: Consistent .meta format for C# script.MonoImporter block added with stable GUID. Good to go.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MenuItems/MenuItemsReaderTests.cs (1)
13-30: Nice coverage on sorting and basic success shape.Validates ordering and payload form; looks good.
UnityMcpBridge/Editor/Tools/MenuItems/MenuItemsReader.cs (1)
54-73: API shape and filtering behavior LGTM.Search is case-insensitive, results are Response-wrapped, and refresh flag is honored. Nice.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs.meta (2)
3-11: LGTM: Standard MonoImporter metadata for a test scriptDefault values, executionOrder remains 0, and no unexpected references. Safe/inert for tests.
2-2: GUID unique — confirm intentionalityRan duplicate-GUID check — no duplicates found; TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs.meta contains guid: c15ba6502927e4901a43826c43debd7c.
Confirm this GUID was intentionally left unchanged; if not, restore the original .meta or update references that depend on it.
2 participants
This allows LLMs to correctly execute menu items, particularly for 3rd party tools.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores