Move Get commands to editor resources + Run Python tests every update #368
Conversation
- Reorganized instructions from flat bullet list into categorized workflow sections - Emphasized critical script management workflow with numbered steps - Improved readability and scannability for AI agents using the MCP server It doesn't make sense to repeat the fucnction tools, they're already parsed
- Implemented resources for querying active tool, editor state, prefab stage, selection, and open windows - Added project configuration resources for layers and project metadata - Organized new resources into Editor and Project namespaces for better structure
- Expanded guidance to include scripts created by any tool, not just manage_script - Added "etc" to tools examples for better clarity
…flow - Removed reload_domain tool as Unity automatically recompiles scripts when modified - Updated script management instructions to rely on editor_state polling and console checking instead of manual domain reload - Simplified workflow by removing unnecessary manual recompilation step
- Moved all test files from root tests/ to MCPForUnity/UnityMcpServer~/src/tests/integration/ for better organization - Added conftest.py with telemetry and dependency stubs to simplify test setup - Removed redundant path manipulation and module loading code from individual test files
- Run tests on all branches instead of only main - Add pull request trigger to catch issues before merge - Maintain path filtering to run only when relevant files change
- Configured automated testing on push and pull requests using pytest - Set up uv for dependency management and Python 3.10 environment - Added test results artifact upload for debugging failed runs
- Changed installation commands from pip to uv pip for better dependency management - Updated test running instructions to use uv run pytest - Added examples for running integration and unit tests separately
…llation - Added path filters to only trigger tests when Python source or workflow files change - Split dependency installation into sync and dev install steps for better clarity - Fixed YAML indentation for improved readability
|
Caution Review failedThe pull request is closed. WalkthroughRefactors editor state access by moving state queries out of ManageEditor into dedicated C# and Python resource handlers, adds/updates CI workflows, overhauls test bootstrap to use shared helpers, and normalizes test/tool mode literals to "EditMode"/"PlayMode". Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Server as MCP Server
participant PyRes as Python Resource (run_tests / project_* etc.)
participant Unity as Unity Editor
Client->>Server: request resource (e.g. unity://editor/state)
Server->>PyRes: dispatch resource handler
PyRes->>Unity: send command "get_editor_state" (async, with retry)
alt success
Unity-->>PyRes: dict payload (editor metadata)
PyRes-->>Server: structured MCPResponse (Pydantic -> JSON)
Server-->>Client: 200 + response
else failure
Unity-->>PyRes: MCPResponse or error
PyRes-->>Server: pass-through MCPResponse
Server-->>Client: error payload
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (14)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_improved_anchor_matching.py (2)
71-71: Remove redundant import.The
remodule is already imported at the top level (line 5). This local import is unnecessary and inconsistent with the stated goal of removing in-test imports.Apply this diff:
- import re - anchor_pattern = r"\s*}\s*$"
134-145: Fix broken test result logic.The test functions don't return success/failure status, so
success1,success2, andsuccess3will all beNone. The condition at line 145 will always evaluate toFalse, incorrectly reporting failures even when all tests pass.Apply this diff to fix the test runner:
success1 = test_improved_anchor_matching() + print("✓ test_improved_anchor_matching passed") print("\n" + "="*60) print("Comparing old vs new behavior...") success2 = test_old_vs_new_matching() + print("✓ test_old_vs_new_matching passed") print("\n" + "="*60) print("Testing _apply_edits_locally with improved matching...") success3 = test_apply_edits_with_improved_matching() + print("✓ test_apply_edits_with_improved_matching passed") print("\n" + "="*60) - if success1 and success2 and success3: - print("🎉 ALL TESTS PASSED! Improved anchor matching is working!") - else: - print("💥 Some tests failed. Need more work on anchor matching.") + print("🎉 ALL TESTS PASSED! Improved anchor matching is working!")Alternatively, if you want to preserve the success tracking logic, modify each test function to return
Trueat the end.
🧹 Nitpick comments (11)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_script_uri.py (1)
17-29: Consider simplifying to use a plain dict.The
DummyMCPinstance is created but only used as a dictionary to store filtered tools—itstooldecorator method is never utilized. This could be simplified for clarity.Apply this diff to simplify:
def _register_tools(): - mcp = DummyMCP() + tools_dict = {} # Import the tools module to trigger decorator registration import tools.manage_script # trigger decorator registration # Get the registered tools from the registry from registry import get_registered_tools registered_tools = get_registered_tools() # Add all script-related tools to our dummy MCP for tool_info in registered_tools: tool_name = tool_info['name'] if any(keyword in tool_name for keyword in ['script', 'apply_text', 'create_script', 'delete_script', 'validate_script', 'get_sha']): - mcp.tools[tool_name] = tool_info['func'] - return mcp.tools + tools_dict[tool_name] = tool_info['func'] + return tools_dictMCPForUnity/UnityMcpServer~/src/server.py (1)
162-192: Consider mentioning thereload_domaintool in Script Management workflow.The instructions are well-organized and provide clear guidance. However, the Script Management section references checking compilation status via
read_consoleand pollingeditor_state, but doesn't mention the newreload_domaintool that was added in this PR (Server/tools/reload_domain.py). Consider adding a reference to this tool in the workflow.Example addition after line 173:
- After creating or modifying scripts (by your own tools or the `manage_script` tool) use `read_console` to check for compilation errors before proceeding +- Optionally trigger domain reload explicitly with `reload_domain` to force recompilation - Only after successful compilation can new components/types be usedMCPForUnity/UnityMcpServer~/src/tests/integration/test_script_tools.py (1)
4-6: Remove unused direct imports.The direct imports of
manage_script_moduleandmanage_asset_module(lines 5-6) are not used anywhere in the test file. The setup functions still perform their own imports at lines 23 and 38 to trigger decorator registration. These direct imports can be removed to eliminate confusion.Apply this diff:
from tests.integration.test_helpers import DummyContext -import tools.manage_script as manage_script_module -import tools.manage_asset as manage_asset_moduleMCPForUnity/UnityMcpServer~/src/tests/integration/conftest.py (1)
30-33: Consider simplifying decorator signature.The
telemetry_toolstub decorator acceptsdargsanddkwargsbut doesn't use them. While this is intentional for a stub, using*args, **kwargswould be clearer and eliminate the static analysis warning.-def telemetry_tool(*dargs, **dkwargs): +def telemetry_tool(*args, **kwargs): def _wrap(fn): return fn return _wrapREADME-zh.md (1)
41-50: Fix list indentation for markdown consistency.The unordered list items use 2-space indentation when they should have 0 indentation according to markdown best practices.
Apply this diff to fix the indentation:
- * `execute_menu_item`: 执行 Unity 编辑器菜单项（例如，"File/Save Project"）。 - * `manage_asset`: 执行资源操作（导入、创建、修改、删除等）。 +* `execute_menu_item`: 执行 Unity 编辑器菜单项（例如，"File/Save Project"）。 +* `manage_asset`: 执行资源操作（导入、创建、修改、删除等）。 * `manage_editor`: 控制和查询编辑器的状态和设置。 - * `manage_gameobject`: 管理游戏对象：创建、修改、删除、查找和组件操作。 +* `manage_gameobject`: 管理游戏对象：创建、修改、删除、查找和组件操作。 * `manage_scene`: 管理场景（加载、保存、创建、获取层次结构等）。 - * `manage_script`: 管理 C# 脚本（创建、读取、更新、删除）。 +* `manage_script`: 管理 C# 脚本（创建、读取、更新、删除）。 * `manage_shader`: 执行着色器 CRUD 操作（创建、读取、修改、删除）。 - * `read_console`: 获取控制台消息或清除控制台。 - * `reload_domain`: 重新加载 Unity 域。 - * `run_test`: 在 Unity 编辑器中运行测试。 +* `read_console`: 获取控制台消息或清除控制台。 +* `reload_domain`: 重新加载 Unity 域。 +* `run_test`: 在 Unity 编辑器中运行测试。 * `apply_text_edits`: 具有前置条件哈希和原子多编辑批次的精确文本编辑。MCPForUnity/UnityMcpServer~/src/tests/integration/test_find_in_file_minimal.py (1)
4-4: Excellent refactor to use centralized test helpers.The removal of complex import-time setup and sys.path manipulation in favor of importing DummyContext from test_helpers improves test maintainability and aligns with the broader PR goal of standardizing test infrastructure.
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_asset_param_coercion.py (1)
10-12: Remove or prefix unused parameters.The
cmdandloopparameters are defined but never used in the fake function.Apply this diff to fix the linter warnings:
- async def fake_async_send(cmd, params, loop=None): + async def fake_async_send(_cmd, params, _loop=None): captured["params"] = params return {"success": True, "data": {}}README.md (1)
43-52: Consider consistent list indentation.The static analysis tool expects 0-space indentation for unordered list items, but the current 2-space indentation is used. While this doesn't affect rendering in most Markdown parsers, you may want to align with the linter's expectations for consistency.
Apply this diff if you want to match the linter's style:
- * `execute_menu_item`: Executes Unity Editor menu items (e.g., "File/Save Project"). - * `manage_asset`: Performs asset operations (import, create, modify, delete, etc.). - * `manage_editor`: Controls and queries the editor's state and settings. - * `manage_gameobject`: Manages GameObjects: create, modify, delete, find, and component operations. - * `manage_scene`: Manages scenes (load, save, create, get hierarchy, etc.). - * `manage_script`: Manages C# scripts (create, read, update, delete). - * `manage_shader`: Performs shader CRUD operations (create, read, modify, delete). - * `read_console`: Gets messages from or clears the console. - * `reload_domain`: Reloads the Unity domain. - * `run_test`: Runs a tests in the Unity Editor. +* `execute_menu_item`: Executes Unity Editor menu items (e.g., "File/Save Project"). +* `manage_asset`: Performs asset operations (import, create, modify, delete, etc.). +* `manage_editor`: Controls and queries the editor's state and settings. +* `manage_gameobject`: Manages GameObjects: create, modify, delete, find, and component operations. +* `manage_scene`: Manages scenes (load, save, create, get hierarchy, etc.). +* `manage_script`: Manages C# scripts (create, read, update, delete). +* `manage_shader`: Performs shader CRUD operations (create, read, modify, delete). +* `read_console`: Gets messages from or clears the console. +* `reload_domain`: Reloads the Unity domain. +* `run_test`: Runs a tests in the Unity Editor.MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_gameobject_param_coercion.py (1)
8-10: Consider prefixing unused parameter with underscore.The
cmdparameter is unused but required for signature compatibility withsend_command_with_retry. Convention suggests prefixing with_.Apply this diff:
- def fake_send(cmd, params): + def fake_send(_cmd, params):MCPForUnity/Editor/Resources/Editor/ActiveTool.cs (1)
55-62: Simplify helper method - condition is always true when called.
GetActiveToolName()is only invoked whencustomToolActiveis true (line 21), so the check on line 57 is redundant and line 61 is unreachable.Apply this diff to simplify:
public static string GetActiveToolName() { - if (UnityEditor.Tools.current == Tool.Custom) - { - return "Unknown Custom Tool"; - } - return UnityEditor.Tools.current.ToString(); + return "Unknown Custom Tool"; }Alternatively, if you want the helper to be more general-purpose, remove the conditional call on line 21:
- string activeToolName = customToolActive ? EditorTools.GetActiveToolName() : toolName; + string activeToolName = EditorTools.GetActiveToolName();Then adjust the helper to handle all cases.
MCPForUnity/UnityMcpServer~/src/resources/selection.py (1)
27-34: Avoid shared mutable defaults in SelectionDataLine 27’s list fields (and the
datadefault on Line 33) currently reuse the same list/model instance across responses. Pydantic copies in many cases, but sticking aField(default_factory=...)here removes any doubt—especially once we start mutatingobjectsorassetGUIDsdownstream. The same tweak onSelectionResponse.datakeeps the nested model safe too. Consider:-from pydantic import BaseModel +from pydantic import BaseModel, Field @@ - objects: list[SelectionObjectInfo] = [] - gameObjects: list[SelectionGameObjectInfo] = [] - assetGUIDs: list[str] = [] + objects: list[SelectionObjectInfo] = Field(default_factory=list) + gameObjects: list[SelectionGameObjectInfo] = Field(default_factory=list) + assetGUIDs: list[str] = Field(default_factory=list) @@ - data: SelectionData = SelectionData() + data: SelectionData = Field(default_factory=SelectionData)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (69)
.github/workflows/python-tests.yml(1 hunks).github/workflows/unity-tests.yml(1 hunks)MCPForUnity/Editor/Resources/Editor.meta(1 hunks)MCPForUnity/Editor/Resources/Editor/ActiveTool.cs(1 hunks)MCPForUnity/Editor/Resources/Editor/ActiveTool.cs.meta(1 hunks)MCPForUnity/Editor/Resources/Editor/EditorState.cs(1 hunks)MCPForUnity/Editor/Resources/Editor/EditorState.cs.meta(1 hunks)MCPForUnity/Editor/Resources/Editor/PrefabStage.cs(1 hunks)MCPForUnity/Editor/Resources/Editor/PrefabStage.cs.meta(1 hunks)MCPForUnity/Editor/Resources/Editor/Selection.cs(1 hunks)MCPForUnity/Editor/Resources/Editor/Selection.cs.meta(1 hunks)MCPForUnity/Editor/Resources/Editor/Windows.cs(1 hunks)MCPForUnity/Editor/Resources/Editor/Windows.cs.meta(1 hunks)MCPForUnity/Editor/Resources/Project.meta(1 hunks)MCPForUnity/Editor/Resources/Project/Layers.cs(1 hunks)MCPForUnity/Editor/Resources/Project/Layers.cs.meta(1 hunks)MCPForUnity/Editor/Resources/Project/ProjectInfo.cs(1 hunks)MCPForUnity/Editor/Resources/Project/ProjectInfo.cs.meta(1 hunks)MCPForUnity/Editor/Resources/Project/Tags.cs(1 hunks)MCPForUnity/Editor/Resources/Project/Tags.cs.meta(1 hunks)MCPForUnity/Editor/Resources/Tests/GetTests.cs(1 hunks)MCPForUnity/Editor/Tools/ManageEditor.cs(6 hunks)MCPForUnity/UnityMcpServer~/src/pyproject.toml(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/active_tool.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/editor_state.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/layers.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/menu_items.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/prefab_stage.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/project_info.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/selection.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/tags.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/windows.py(1 hunks)MCPForUnity/UnityMcpServer~/src/server.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/conftest.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_edit_normalization_and_noop.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_edit_strict_and_warnings.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_find_in_file_minimal.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_get_sha.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_improved_anchor_matching.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_asset_param_coercion.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_gameobject_param_coercion.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_script_uri.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_read_console_truncate.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_read_resource_minimal.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_resources_api.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_script_tools.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_telemetry_endpoint_validation.py(2 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_telemetry_queue_worker.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_telemetry_subaction.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_transport_framing.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_validate_script_summary.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/pytest.ini(1 hunks)README-zh.md(1 hunks)README.md(1 hunks)Server/server.py(1 hunks)Server/tools/reload_domain.py(1 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs(0 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs(0 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/CommandRegistryTests.cs(0 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs(0 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs(0 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsTests.cs(2 hunks)TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialMeshInstantiationTests.cs(0 hunks)docs/CUSTOM_TOOLS.md(2 hunks)docs/README-DEV.md(1 hunks)pytest.ini(0 hunks)tests/conftest.py(0 hunks)tests/test_manage_asset_param_coercion.py(0 hunks)tests/test_manage_gameobject_param_coercion.py(0 hunks)
💤 Files with no reviewable changes (10)
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/MCPToolParameterTests.cs
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ComponentResolverTests.cs
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/CommandRegistryTests.cs
- tests/test_manage_gameobject_param_coercion.py
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/AIPropertyMatchingTests.cs
- tests/conftest.py
- pytest.ini
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MaterialMeshInstantiationTests.cs
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGameObjectTests.cs
- tests/test_manage_asset_param_coercion.py
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
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.
📚 Learning: 2025-10-13T13:27:23.040Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 316
File: TestProjects/UnityMCPTests/Assets/Tests/EditMode/Resources.meta:1-8
Timestamp: 2025-10-13T13:27:23.040Z
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.
Applied to files:
MCPForUnity/Editor/Resources/Editor.metaMCPForUnity/Editor/Resources/Project/ProjectInfo.cs.metaMCPForUnity/Editor/Resources/Project.metaMCPForUnity/UnityMcpServer~/src/tests/integration/test_edit_strict_and_warnings.pyMCPForUnity/Editor/Resources/Project/Layers.cs.metaMCPForUnity/Editor/Resources/Project/ProjectInfo.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsTests.csMCPForUnity/Editor/Resources/Editor/ActiveTool.cs.metaMCPForUnity/UnityMcpServer~/src/tests/integration/test_read_resource_minimal.pyMCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_script_uri.py
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
MCPForUnity/UnityMcpServer~/src/resources/menu_items.pyServer/server.pyMCPForUnity/Editor/Resources/Editor/ActiveTool.cs.metaMCPForUnity/UnityMcpServer~/src/server.py
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
docs/README-DEV.mdMCPForUnity/UnityMcpServer~/src/tests/pytest.iniServer/server.pyMCPForUnity/UnityMcpServer~/src/tests/integration/test_edit_strict_and_warnings.pyMCPForUnity/UnityMcpServer~/src/tests/integration/conftest.pyMCPForUnity/UnityMcpServer~/src/resources/active_tool.pyMCPForUnity/UnityMcpServer~/src/resources/project_info.pyMCPForUnity/UnityMcpServer~/src/tests/integration/test_validate_script_summary.pyMCPForUnity/UnityMcpServer~/src/pyproject.tomlMCPForUnity/UnityMcpServer~/src/tests/integration/test_read_resource_minimal.pyMCPForUnity/UnityMcpServer~/src/tests/integration/test_script_tools.pyMCPForUnity/UnityMcpServer~/src/tests/integration/test_get_sha.pyMCPForUnity/UnityMcpServer~/src/server.pyMCPForUnity/UnityMcpServer~/src/tests/integration/test_read_console_truncate.pyMCPForUnity/Editor/Tools/ManageEditor.csMCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_script_uri.pyMCPForUnity/UnityMcpServer~/src/tests/integration/test_edit_normalization_and_noop.pyMCPForUnity/UnityMcpServer~/src/tests/integration/test_find_in_file_minimal.pyMCPForUnity/UnityMcpServer~/src/tests/integration/test_resources_api.py
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 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:
MCPForUnity/Editor/Resources/Project/ProjectInfo.cs.metaMCPForUnity/Editor/Resources/Project.meta
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 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:
MCPForUnity/Editor/Resources/Editor/Selection.csMCPForUnity/Editor/Resources/Project/ProjectInfo.csMCPForUnity/Editor/Resources/Editor/ActiveTool.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsTests.csMCPForUnity/Editor/Tools/ManageEditor.cs
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Applied to files:
MCPForUnity/UnityMcpServer~/src/server.py
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
Applied to files:
MCPForUnity/UnityMcpServer~/src/server.py
🧬 Code graph analysis (24)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_edit_strict_and_warnings.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)
MCPForUnity/UnityMcpServer~/src/tests/integration/conftest.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_telemetry_subaction.py (1)
_noop(19-20)
MCPForUnity/Editor/Resources/Editor/Selection.cs (1)
MCPForUnity/Editor/Helpers/Response.cs (2)
Response(10-61)Success(18-33)
MCPForUnity/Editor/Resources/Project/ProjectInfo.cs (1)
MCPForUnity/Editor/Helpers/Response.cs (2)
Response(10-61)Success(18-33)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_asset_param_coercion.py (2)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)MCPForUnity/UnityMcpServer~/src/tests/integration/test_script_tools.py (1)
run(168-179)
MCPForUnity/Editor/Resources/Editor/ActiveTool.cs (2)
MCPForUnity/Editor/Tools/ManageEditor.cs (1)
HandleCommand(25-127)MCPForUnity/Editor/Helpers/Response.cs (2)
Response(10-61)Success(18-33)
MCPForUnity/Editor/Resources/Project/Layers.cs (3)
MCPForUnity/Editor/Resources/Project/Tags.cs (2)
McpForUnityResource(11-26)HandleCommand(14-25)MCPForUnity/Editor/Tools/ManageEditor.cs (1)
HandleCommand(25-127)MCPForUnity/Editor/Helpers/Response.cs (2)
Response(10-61)Success(18-33)
MCPForUnity/Editor/Resources/Editor/Windows.cs (2)
MCPForUnity/Editor/Tools/ManageEditor.cs (1)
HandleCommand(25-127)MCPForUnity/Editor/Helpers/Response.cs (2)
Response(10-61)Success(18-33)
MCPForUnity/Editor/Resources/Editor/PrefabStage.cs (2)
MCPForUnity/Editor/Tools/ManageEditor.cs (1)
HandleCommand(25-127)MCPForUnity/Editor/Helpers/Response.cs (2)
Response(10-61)Success(18-33)
MCPForUnity/Editor/Resources/Editor/EditorState.cs (2)
MCPForUnity/Editor/Resources/Editor/Selection.cs (2)
McpForUnityResource(12-51)HandleCommand(15-50)MCPForUnity/Editor/Helpers/Response.cs (2)
Response(10-61)Success(18-33)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_validate_script_summary.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)
Server/tools/reload_domain.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
info(3-4)
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsTests.cs (2)
MCPForUnity/Editor/Resources/Editor/PrefabStage.cs (1)
HandleCommand(14-41)MCPForUnity/Editor/Tools/ManageEditor.cs (1)
HandleCommand(25-127)
MCPForUnity/Editor/Resources/Project/Tags.cs (3)
MCPForUnity/Editor/Resources/Project/Layers.cs (2)
McpForUnityResource(12-38)HandleCommand(17-37)MCPForUnity/Editor/Tools/ManageEditor.cs (1)
HandleCommand(25-127)MCPForUnity/Editor/Helpers/Response.cs (2)
Response(10-61)Success(18-33)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_read_resource_minimal.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_script_tools.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_gameobject_param_coercion.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_get_sha.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_read_console_truncate.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)
MCPForUnity/Editor/Tools/ManageEditor.cs (1)
MCPForUnity/Editor/Helpers/McpLog.cs (2)
McpLog(6-32)Error(28-31)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_script_uri.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_edit_normalization_and_noop.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_find_in_file_minimal.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_resources_api.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)
🪛 markdownlint-cli2 (0.18.1)
README.md
43-43: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
44-44: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
45-45: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
46-46: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
47-47: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
48-48: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
49-49: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
README-zh.md
43-43: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
44-44: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
45-45: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
46-46: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
47-47: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
48-48: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
49-49: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🪛 Ruff (0.14.3)
MCPForUnity/UnityMcpServer~/src/resources/layers.py
8-8: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
MCPForUnity/UnityMcpServer~/src/tests/integration/conftest.py
30-30: Unused function argument: dargs
(ARG001)
30-30: Unused function argument: dkwargs
(ARG001)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_asset_param_coercion.py
10-10: Unused function argument: cmd
(ARG001)
10-10: Unused function argument: loop
(ARG001)
MCPForUnity/UnityMcpServer~/src/resources/windows.py
26-26: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_gameobject_param_coercion.py
8-8: Unused function argument: cmd
(ARG001)
MCPForUnity/UnityMcpServer~/src/resources/tags.py
8-8: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test in editmode
🔇 Additional comments (46)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_improved_anchor_matching.py (1)
5-7: LGTM! Static imports are cleaner.The move from dynamic module loading to static imports simplifies the test setup and improves maintainability.
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_script_uri.py (2)
3-3: LGTM! Clean refactor to centralized test helper.The import of
DummyContextfromtests.integration.test_helpersaligns well with the PR objective to refactor test infrastructure and use centralized test helpers. This simplifies the test file and promotes reusability.
42-44: No issues found - the monkeypatch is correctly targeted.The import statement
import unity_connection(line 9) combined with all call sites accessing the function asunity_connection.send_command_with_retrymeans the monkeypatch targetingunity_connection.send_command_with_retryat the module level will work correctly and intercept the mocked function calls as intended.MCPForUnity/Editor/Resources/Editor.meta (1)
2-2: LGTM!Standard Unity metadata GUID update. Unity manages these identifiers automatically when assets are reorganized.
MCPForUnity/UnityMcpServer~/src/tests/pytest.ini (1)
1-8: LGTM!Standard pytest configuration with conventional test discovery patterns and useful markers for integration and unit tests. This enables selective test execution and follows pytest best practices.
MCPForUnity/Editor/Resources/Project.meta (1)
1-8: LGTM!Standard Unity folder metadata file. This accompanies the new Project resources structure introduced in this PR.
MCPForUnity/UnityMcpServer~/src/tests/integration/test_telemetry_subaction.py (1)
9-9: Good refactoring!Removing sys.path manipulation in favor of direct parent package imports improves test maintainability and aligns with Python packaging best practices. The remaining module stubbing logic is appropriate for test isolation.
.github/workflows/unity-tests.yml (1)
6-15: Good workflow optimization!Adding path filters ensures Unity tests only run when relevant files change, reducing unnecessary CI resource usage while maintaining test coverage for all branches.
MCPForUnity/Editor/Resources/Editor/PrefabStage.cs.meta (1)
1-11: LGTM!Standard Unity MonoImporter metadata file for the new PrefabStage.cs editor resource.
docs/CUSTOM_TOOLS.md (1)
32-32: Import path verified as correct.The simplified import
from fastmcp import Contextis the official public API for fastmcp>=2.12.5, as specified in your project's pyproject.toml. This import path is already used consistently throughout the codebase (13+ files), confirming the documentation update aligns with current implementation.MCPForUnity/UnityMcpServer~/src/tests/integration/test_read_console_truncate.py (1)
1-78: LGTM! Clean refactor to centralized test helpers.The migration from dynamic module loading to using the centralized
DummyContextfromtests.integration.test_helpersimproves test maintainability and reduces complexity. The test logic correctly validates both full and truncated console reading with proper assertions.MCPForUnity/Editor/Resources/Project/Layers.cs (1)
1-39: LGTM! Consistent resource implementation.The implementation correctly follows the established MCP resource pattern (matching Tags.cs) and properly handles Unity's 32-layer system by filtering out unnamed layers. The error handling and response format are appropriate.
MCPForUnity/UnityMcpServer~/src/resources/menu_items.py (1)
10-14: LGTM! Standardized resource naming.The update to
name="menu_items"aligns with the PR-wide resource naming standardization while maintaining backward compatibility through the unchanged function name and URI.Server/tools/reload_domain.py (1)
1-24: LGTM! Well-documented domain reload tool.The implementation follows the established MCP tool pattern with a comprehensive description that clearly explains when and why to use domain reload. The async implementation and return handling are correct.
docs/README-DEV.md (1)
1-287: LGTM! Comprehensive documentation improvements.The documentation updates successfully migrate from pip to uv-based workflows while adding valuable new sections on testing, deployment, stress testing, and CI workflows. The command examples are correct and the structure is clear and well-organized.
Server/server.py (1)
162-188: Update instructions to reflect the active plugin's guidance or deprecate the legacy server file.The inconsistency is confirmed.
Server/server.pyexplicitly instructs users to "ALWAYS callreload_domainimmediately," whileMCPForUnity/UnityMcpServer~/src/server.pyuses a different approach—checkingread_consolefor errors and pollingeditor_state.isCompilingto verify compilation completion. Since MCPForUnity is the maintained plugin, either:
- Align
Server/server.pyto match the MCPForUnity guidance (removing the explicitreload_domainrequirement), or- Clearly mark
Server/server.pyas deprecated or legacy to avoid user confusionMCPForUnity/UnityMcpServer~/src/resources/windows.py (1)
1-37: LGTM! Consistent resource implementation.The resource endpoint follows the established pattern for Unity MCP resources: strongly-typed Pydantic models with sensible defaults, async command delegation, and conditional response wrapping. The implementation is clean and consistent with other editor resources in this PR.
Note: The static analysis warning (RUF012) about the mutable default on line 26 is a false positive—Pydantic handles mutable defaults correctly through its field system.
MCPForUnity/UnityMcpServer~/src/resources/prefab_stage.py (1)
1-29: LGTM! Well-structured prefab stage resource.The implementation correctly models the prefab editing context with appropriate optional fields for when no prefab is open. The resource description clearly communicates the
isOpen=falsebehavior, and the pattern is consistent with other Unity editor resources.MCPForUnity/UnityMcpServer~/src/tests/integration/conftest.py (1)
1-48: Good test infrastructure setup.The conftest properly stubs out telemetry and external dependencies for integration testing, and the multiple environment variables ensure telemetry is disabled across different naming conventions.
MCPForUnity/UnityMcpServer~/src/resources/active_tool.py (1)
1-37: LGTM! Clean active tool resource implementation.The resource properly models the Unity editor's active tool state, including transform handle data. The Vector3 model provides a reusable type for 3D coordinates, and the overall structure follows the established pattern for Unity MCP resources.
MCPForUnity/UnityMcpServer~/src/resources/layers.py (1)
1-19: LGTM! Concise and well-documented layers resource.The resource description helpfully directs users to read layer data before using modification tools. The dict structure appropriately maps Unity's 0-31 layer indices to names.
Note: The static analysis warning (RUF012) on line 8 is a false positive—Pydantic correctly handles mutable defaults.
MCPForUnity/UnityMcpServer~/src/tests/integration/test_transport_framing.py (1)
26-26: Good cleanup of import handling.The removal of explicit sys.path manipulation in favor of standard Python imports simplifies the test infrastructure. The change aligns with the broader test refactoring in this PR.
MCPForUnity/UnityMcpServer~/src/tests/integration/test_telemetry_endpoint_validation.py (1)
10-11: LGTM! Simplified import approach improves test reliability.Removing dynamic path manipulation and relying on standard import resolution makes the test more portable and less fragile. This change aligns well with the PR's goal of improving test infrastructure.
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_asset_param_coercion.py (1)
7-29: LGTM!The test correctly verifies that pagination parameters are coerced from strings to integers when calling
manage_asset. The test structure is clear and assertions are appropriate.MCPForUnity/UnityMcpServer~/src/tests/integration/test_resources_api.py (1)
3-3: LGTM!The simplified import using
DummyContextfrom the centralized test helpers improves maintainability and removes import-time side effects from dynamic module loading.MCPForUnity/UnityMcpServer~/src/tests/integration/test_edit_strict_and_warnings.py (1)
1-1: LGTM!The migration to using
DummyContextfrom centralized test helpers simplifies the test file by removing in-test environment stubbing and dynamic module loading.MCPForUnity/UnityMcpServer~/src/tests/integration/test_get_sha.py (1)
1-1: LGTM!The import simplification aligns with the broader test infrastructure refactor, replacing dynamic module loading with static imports from centralized test helpers.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsTests.cs (2)
55-55: LGTM!The explicit type qualification for
UnityEditor.SceneManagement.PrefabStageis clear and correct.Also applies to: 127-127
59-59: LGTM!The test correctly exercises the new resource-based architecture for prefab stage information retrieval. The change from
ManageEditor.HandleCommandto the dedicatedPrefabStage.HandleCommandaligns with the PR's objective to consolidate editor state querying into resource endpoints.MCPForUnity/UnityMcpServer~/src/tests/integration/test_validate_script_summary.py (1)
1-1: LGTM!The import simplification removes dynamic module loading in favor of static imports from centralized test helpers, aligning with the broader test infrastructure modernization in this PR.
README.md (1)
43-52: LGTM!The updated tools list accurately reflects the new capabilities added in this PR, including the resource-based architecture for editor state queries. The documentation is clear and comprehensive.
MCPForUnity/UnityMcpServer~/src/tests/integration/test_telemetry_queue_worker.py (1)
6-6: LGTM! Simplified import improves test maintainability.The direct import of
telemetryreplaces the previous dynamic module loading approach, making the test setup clearer and more maintainable.MCPForUnity/Editor/Resources/Editor/Windows.cs (1)
16-57: LGTM! Well-structured implementation with appropriate error handling.The
HandleCommandmethod properly:
- Uses
Resources.FindObjectsOfTypeAll<EditorWindow>()to discover all open windows- Implements dual-level error handling (per-window and top-level)
- Collects comprehensive metadata (title, type, focus state, position, instanceID)
- Returns standardized Response objects consistent with other resource endpoints
MCPForUnity/Editor/Resources/Project/Tags.cs (1)
14-25: LGTM! Simple and consistent implementation.The
HandleCommandmethod correctly retrieves tags usingInternalEditorUtility.tagsand returns standardized Response objects. The implementation follows the same pattern asLayers.csand other resource endpoints.MCPForUnity/UnityMcpServer~/src/tests/integration/test_edit_normalization_and_noop.py (2)
1-1: LGTM! Centralized test helper import.The import of
DummyContextfrom the centralizedtests.integration.test_helpersmodule is a good refactoring that replaces dynamic module loading with static imports, improving test maintainability.
12-24: LGTM! Clear test setup with explicit tool registration.The
setup_tools()function provides a clear and explicit way to register tools for testing, replacing the previous dynamic loading approach. The function properly imports tools and retrieves registered tools from the registry.MCPForUnity/UnityMcpServer~/src/resources/tags.py (1)
11-19: LGTM! Clean resource endpoint implementation.The
get_tags()function is properly decorated and implements the standard pattern for MCP resources:
- Uses
async_send_command_with_retryfor Unity communication- Conditionally returns
TagsResponseor raw response based on response type- Includes descriptive metadata for the resource
MCPForUnity/UnityMcpServer~/src/tests/integration/test_read_resource_minimal.py (1)
4-4: LGTM! Test helper consolidation improves maintainability.The shift from inline test stubs to a shared
DummyContexthelper reduces duplication and simplifies test setup.MCPForUnity/Editor/Resources/Editor/PrefabStage.cs (1)
14-41: Well-structured resource implementation.The method follows the established pattern with proper error handling, null checks, and standardized response formatting.
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_gameobject_param_coercion.py (1)
5-29: Test correctly validates parameter coercion and mapping.The test ensures that the
tagparameter maps tosearchTermand that boolean-like strings are properly propagated to the C# side. The flexible assertions accommodate different coercion behaviors.MCPForUnity/Editor/Resources/Editor/EditorState.cs (1)
15-38: LGTM! Clean implementation of editor state resource.The method correctly gathers dynamic editor state from Unity APIs and returns it in a standardized format with proper error handling.
MCPForUnity/Editor/Resources/Project/ProjectInfo.cs (1)
16-39: LGTM! Project info retrieval is well-implemented.The method correctly derives project metadata from Unity APIs, normalizes paths for cross-platform compatibility, and follows the standardized response pattern.
MCPForUnity/Editor/Resources/Editor/Selection.cs (1)
15-50: LGTM! Comprehensive selection information gathering.The method thoroughly captures selection details using LINQ projections and null-safe access patterns. Well-structured and follows the established resource pattern.
MCPForUnity/Editor/Resources/Editor/ActiveTool.cs (1)
14-49: Well-structured active tool resource.The method correctly retrieves tool information including custom tool detection, pivot settings, and handle transforms. The response format is comprehensive and follows the established pattern.
MCPForUnity/UnityMcpServer~/src/resources/editor_state.py (1)
1-32: LGTM! Clean Python resource implementation.The module correctly defines Pydantic models with appropriate defaults, uses modern Python type hints (3.10+ union syntax), and implements the async resource pattern with proper response handling.
MCPForUnity/Editor/Tools/ManageEditor.cs (1)
136-158: Case-insensitive tool parsing is a solid UX winLine 136’s
Enum.TryParsewithignoreCase: truekeeps the API tolerant of the mixed‑case strings we routinely see coming from clients. Nice touch.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Changed default mode from "edit" to "EditMode" in C# code - Updated Python tool to use "EditMode" and "PlayMode" instead of lowercase variants
- Changed absolute imports to relative imports in integration tests for better package structure - Removed test packages from pyproject.toml package list
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_get_sha.py (1)
45-45: Remove leftover comment.This comment appears to be a note from refactoring and doesn't add value to the current code.
- # No need to patch tools.manage_script; it now calls unity_connection.send_command_with_retryMCPForUnity/UnityMcpServer~/src/tests/integration/test_script_tools.py (1)
161-166: Remove the redundant function.globals patching; monkeypatch.setattr is sufficient.The
manage_assetfunction uses a direct import (from unity_connection import async_send_command_with_retry), so the function's globals and the module namespace are the same. The dual patching is redundant—monkeypatch.setattralone handles the patch correctly. The comment about "dynamically loaded module alias" no longer applies after the PR migrated to direct imports.Simplify to:
import tools.manage_asset as tools_manage_asset monkeypatch.setattr(tools_manage_asset, "async_send_command_with_retry", fake_async)MCPForUnity/UnityMcpServer~/src/tests/integration/test_read_console_truncate.py (1)
15-27: Remove unnecessary DummyMCP instance creation in setup_tools().All test setup_tools() functions follow an inconsistent pattern: they create a
DummyMCPinstance but never invoke its decorator mechanism. Instead, tools are registered globally via the@mcp_for_unity_tooldecorator and retrieved from the global registry. Themcpinstance serves only as a container and adds confusion to the code.Refactor to eliminate the unnecessary instance creation—either return the filtered registry directly or use a plain dict without creating DummyMCP.
🧹 Nitpick comments (15)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_edit_normalization_and_noop.py (4)
4-9: Consider removing unused decorator functionality or clarifying its purpose.The
DummyMCP.tooldecorator is defined but never invoked anywhere in the test file. Thesetup_tools()function creates aDummyMCPinstance but populatesmcp.toolsdirectly from the registry rather than using the decorator. Additionally, line 8 combines multiple statements on one line, reducing readability.If the decorator is not needed for this test file, simplify to:
class DummyMCP: - def __init__(self): self.tools = {} - - def tool(self, *args, **kwargs): - def deco(fn): self.tools[fn.__name__] = fn; return fn - return deco + def __init__(self): + self.tools = {}If the decorator will be used in the future, improve readability:
def tool(self, *args, **kwargs): - def deco(fn): self.tools[fn.__name__] = fn; return fn + def deco(fn): + self.tools[fn.__name__] = fn + return fn return deco
12-24: Extract duplicated setup logic into a reusable helper function.The setup logic in
setup_tools()(lines 12-24) is duplicated intest_atomic_multi_span_and_relaxed()(lines 92-102) with only the filter keywords differing. This violates the DRY principle and makes maintenance harder.Consider extracting a parameterized helper:
def setup_tools_with_filter(keywords): """Set up tools filtered by the given keywords.""" mcp = DummyMCP() # Import the tools module to trigger decorator registration import tools.manage_script import tools.script_apply_edits # Get the registered tools from the registry from registry import get_registered_tools tools = get_registered_tools() # Add filtered tools to our dummy MCP for tool_info in tools: tool_name = tool_info['name'] if any(keyword in tool_name for keyword in keywords): mcp.tools[tool_name] = tool_info['func'] return mcp.tools def setup_tools(): return setup_tools_with_filter([ 'script', 'apply_text', 'create_script', 'delete_script', 'validate_script', 'get_sha' ])Then in
test_atomic_multi_span_and_relaxed, replace lines 92-102 with:tools_struct_dict = setup_tools_with_filter(['script_apply', 'apply_edits'])
22-22: Split long line for better readability.Line 22 exceeds typical Python line length guidelines, making it harder to read in code reviews and narrow editor windows.
Apply this diff to improve readability:
- if any(keyword in tool_name for keyword in ['script', 'apply_text', 'create_script', 'delete_script', 'validate_script', 'get_sha']): + keywords = ['script', 'apply_text', 'create_script', + 'delete_script', 'validate_script', 'get_sha'] + if any(keyword in tool_name for keyword in keywords): mcp.tools[tool_name] = tool_info['func']
67-67: Remove or clarify incomplete comment.The comment on line 67 appears incomplete or doesn't add meaningful context to the code.
Consider removing it or expanding it to be more descriptive:
- # last call is apply_text_edits + # Verify the last call sent the normalized index-based editsor simply remove it if it doesn't provide value:
- # last call is apply_text_edits -MCPForUnity/UnityMcpServer~/src/tests/integration/test_validate_script_summary.py (2)
15-27: Consider moving imports to module level for consistency.The
setup_tools()function uses runtime imports (lines 18, 20-21), which contradicts the PR's stated goal of shifting to static imports. Additionally, the string-based keyword filtering on line 25 is fragile and could break if tool names change.Consider this refactor:
+import tools.manage_script +from registry import get_registered_tools + + def setup_tools(): mcp = DummyMCP() - # Import the tools module to trigger decorator registration - import tools.manage_script - # Get the registered tools from the registry - from registry import get_registered_tools registered_tools = get_registered_tools() - # Add all script-related tools to our dummy MCP + # Filter for script-related tools + script_keywords = ['script', 'apply_text', 'create_script', 'delete_script', 'validate_script', 'get_sha'] for tool_info in registered_tools: tool_name = tool_info['name'] - if any(keyword in tool_name for keyword in ['script', 'apply_text', 'create_script', 'delete_script', 'validate_script', 'get_sha']): + if any(keyword in tool_name for keyword in script_keywords): mcp.tools[tool_name] = tool_info['func'] return mcp.tools
30-32: Add safety check for tool existence.The test accesses
tools["validate_script"]without verifying the tool was registered. If the tool isn't found, this will raise an unclearKeyErrorrather than a descriptive test failure.Consider adding an assertion:
def test_validate_script_returns_counts(monkeypatch): tools = setup_tools() + assert "validate_script" in tools, "validate_script tool not registered" validate_script = tools["validate_script"]MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_asset_param_coercion.py (3)
10-12: Consider capturingcmdand using underscore prefix for unused parameters.The static analysis correctly identifies that
cmdandloopare unused. While this is intentional (the mock must match the real function signature), you could improve the code by:
- Prefixing intentionally unused parameters with underscores (
_cmd,_loop)- Capturing the
cmdparameter for consistency with similar tests (seetest_script_tools.pylines 167-178)Apply this diff to improve clarity:
- async def fake_async_send(cmd, params, loop=None): + async def fake_async_send(_cmd, params, _loop=None): + captured["cmd"] = _cmd captured["params"] = params return {"success": True, "data": {}}
26-28: Consider adding assertion for thecmdparameter.For consistency with similar integration tests (e.g.,
test_script_tools.pylines 167-178) and more complete verification, consider asserting that the correct command was sent.Add this assertion:
assert result == {"success": True, "data": {}} + assert captured["cmd"] == "manage_asset" assert captured["params"]["pageSize"] == 50 assert captured["params"]["pageNumber"] == 2
30-34: Remove excessive empty lines.The file ends with five consecutive empty lines, which is unnecessary. Standard practice is to have at most one or two empty lines at the end of a file.
MCPForUnity/UnityMcpServer~/src/tests/integration/test_get_sha.py (2)
4-12: Consider simplifying DummyMCP.The
DummyMCPclass defines atool()decorator method that's never used. Sincesetup_tools()only uses thetoolsdict as a container, consider replacing the class with a simple dict.-class DummyMCP: - def __init__(self): - self.tools = {} - - def tool(self, *args, **kwargs): - def deco(fn): - self.tools[fn.__name__] = fn - return fn - return decoThen update line 16:
- mcp = DummyMCP() + tools_dict = {}And line 26:
- mcp.tools[tool_name] = tool_info['func'] + tools_dict[tool_name] = tool_info['func']And line 27:
- return mcp.tools + return tools_dict
25-25: String-based tool filtering is fragile.The keyword matching on line 25 could inadvertently match unintended tools or miss renamed tools. Consider using explicit tool names or a more robust filtering mechanism.
- if any(keyword in tool_name for keyword in ['script', 'apply_text', 'create_script', 'delete_script', 'validate_script', 'get_sha']): + if tool_name in {'apply_text', 'create_script', 'delete_script', 'validate_script', 'get_sha'}:MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_script_uri.py (2)
17-29: Consider using a pytest fixture for _register_tools.The
_register_tools()function is called in every test, which repeats the import and filtering logic. Converting it to a pytest fixture would improve efficiency and follow pytest best practices.Apply this diff to refactor as a fixture:
-def _register_tools(): +@pytest.fixture +def registered_tools(): mcp = DummyMCP() # Import the tools module to trigger decorator registration import tools.manage_script # trigger decorator registration # Get the registered tools from the registry from registry import get_registered_tools registered_tools = get_registered_tools() # Add all script-related tools to our dummy MCP for tool_info in registered_tools: tool_name = tool_info['name'] if any(keyword in tool_name for keyword in ['script', 'apply_text', 'create_script', 'delete_script', 'validate_script', 'get_sha']): mcp.tools[tool_name] = tool_info['func'] return mcp.toolsThen update test signatures (example):
def test_split_uri_unity_path(monkeypatch, registered_tools): test_tools = registered_tools # rest of test...Note: The keyword-based filtering on line 27 is brittle and will break if tool naming conventions change. This is acceptable for integration tests, but be aware that tool renames may require updates here.
56-109: LGTM: Comprehensive URI format coverage.The parametrized test effectively covers multiple file:// URL formats including localhost, Windows paths, URL encoding (line 59), and paths outside the Assets directory (line 65). The plain path test provides good coverage for the common case.
Optional enhancement: Consider adding test cases for error scenarios such as:
- Invalid URI formats
- Missing or malformed components
- URIs that should fail validation
This would help ensure robust error handling in the URI parsing logic.
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_gameobject_param_coercion.py (1)
8-10: Prefer underscore prefix for intentionally unused parameters.The
cmdparameter is intentionally unused but required to match the signature ofsend_command_with_retry. Using_cmdis more idiomatic and signals to both readers and linters that this is deliberate.Apply this diff:
- def fake_send(cmd, params): + def fake_send(_cmd, params): captured["params"] = params return {"success": True, "data": {}}MCPForUnity/UnityMcpServer~/src/tests/integration/test_read_console_truncate.py (1)
4-12: Consider moving DummyMCP to test_helpers to avoid duplication.The
DummyMCPclass defines a decorator pattern but the decorator is never used in this file. Thesetup_toolsfunction creates aDummyMCPinstance but then imports tools from the actual registry instead of using the mock's decorator for registration. If this class is used across multiple integration tests (as suggested by the AI summary), consider moving it totest_helpers.pyalongsideDummyContextto centralize test utilities and avoid duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
MCPForUnity/Editor/Resources/Editor/PrefabStage.cs(1 hunks)MCPForUnity/Editor/Tools/RunTests.cs(1 hunks)MCPForUnity/UnityMcpServer~/src/pyproject.toml(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/tags.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_edit_normalization_and_noop.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_edit_strict_and_warnings.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_find_in_file_minimal.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_get_sha.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_asset_param_coercion.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_gameobject_param_coercion.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_script_uri.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_read_console_truncate.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_read_resource_minimal.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_resources_api.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_script_tools.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_validate_script_summary.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/run_tests.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- MCPForUnity/UnityMcpServer~/src/tests/integration/test_resources_api.py
- MCPForUnity/UnityMcpServer~/src/tests/integration/test_read_resource_minimal.py
- MCPForUnity/UnityMcpServer~/src/tests/integration/test_find_in_file_minimal.py
- MCPForUnity/Editor/Resources/Editor/PrefabStage.cs
- MCPForUnity/UnityMcpServer~/src/tests/integration/test_edit_strict_and_warnings.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_script_uri.pyMCPForUnity/UnityMcpServer~/src/tests/integration/test_read_console_truncate.pyMCPForUnity/UnityMcpServer~/src/tests/integration/test_validate_script_summary.pyMCPForUnity/UnityMcpServer~/src/tests/integration/test_script_tools.pyMCPForUnity/UnityMcpServer~/src/pyproject.tomlMCPForUnity/UnityMcpServer~/src/tests/integration/test_edit_normalization_and_noop.pyMCPForUnity/UnityMcpServer~/src/tests/integration/test_get_sha.py
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 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:
MCPForUnity/UnityMcpServer~/src/pyproject.toml
🧬 Code graph analysis (8)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_script_uri.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_read_console_truncate.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_validate_script_summary.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_script_tools.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_edit_normalization_and_noop.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_get_sha.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_asset_param_coercion.py (2)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)MCPForUnity/UnityMcpServer~/src/tests/integration/test_script_tools.py (1)
run(168-179)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_gameobject_param_coercion.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(1-10)
🪛 Ruff (0.14.3)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_asset_param_coercion.py
10-10: Unused function argument: cmd
(ARG001)
10-10: Unused function argument: loop
(ARG001)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_gameobject_param_coercion.py
8-8: Unused function argument: cmd
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test in editmode
- GitHub Check: Test in editmode
🔇 Additional comments (8)
MCPForUnity/UnityMcpServer~/src/pyproject.toml (1)
40-40: ✓ Test packages properly excluded from distribution.This correctly addresses the critical issue flagged in previous reviews. The
packageslist now properly excludes"tests"and"tests.integration", ensuring test code won't be bundled in wheel distributions. pytest will continue to discover and run tests from the source tree during CI/CD workflows without needing them packaged.MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_asset_param_coercion.py (1)
1-4: LGTM!The imports are appropriate for this integration test. The use of relative imports for test helpers and direct module imports for the code under test follows good testing practices.
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_script_uri.py (2)
3-3: LGTM: Consolidated test helper import.The change to import
DummyContextfromtest_helperseliminates duplication and improves maintainability across integration tests.
32-54: LGTM: Unity URI parsing test is well-structured.The test correctly verifies that
unity://URIs are parsed into the expected name and path parameters. The monkeypatch usage is appropriate for isolating the URI parsing logic from the actual Unity connection.MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_gameobject_param_coercion.py (2)
1-2: LGTM!The imports are clean and appropriate for the integration test.
14-25: LGTM!The test correctly invokes
manage_gameobjectwith string parameters to verify coercion behavior, and the response assertions appropriately handle wrapper diagnostics.MCPForUnity/UnityMcpServer~/src/tests/integration/test_read_console_truncate.py (1)
57-78: No issues found. The test correctly validates client-side stacktrace filtering.The
fake_sendfunction returns a response with the"stacktrace"field (line 67), and the assertion expects it to be stripped from the final response (lines 76-77). This is correct because the tool performs client-side filtering to strip stacktrace fields wheninclude_stacktrace=False. The test properly validates this behavior.MCPForUnity/UnityMcpServer~/src/resources/tags.py (1)
1-9: LGTM! Mutable default issue resolved.The
Field(default_factory=list)pattern correctly addresses the mutable default value concern raised in the previous review. This is the proper Pydantic approach for list fields.
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_gameobject_param_coercion.py
Show resolved
Hide resolved
MCPForUnity/UnityMcpServer~/src/tests/integration/test_script_tools.py
Outdated
Show resolved
Hide resolved
…tools.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
It's already covered by pushes
- Removed reload_domain tool as Unity handles script recompilation automatically - Updated documentation to reflect automatic compilation workflow - Simplified script management workflow instructions in server description
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
.github/workflows/unity-tests.yml (1)
6-6: Expanded branch trigger to all branches with path filtering.Good change. Running tests on all branches (via
"**") combined with the specific path filters (TestProjects/UnityMCPTests/, MCPForUnity/Editor/, and the workflow file itself) ensures tests catch regressions early without running unnecessarily on unrelated changes.Operational note: This will increase GitHub Actions minutes usage since the workflow now runs on every branch that touches the filtered paths, not just main. Ensure your team is aware of this to monitor monthly usage.
README-zh.md (1)
41-49: Fix markdown linting violations for tool list indentation.Markdownlint MD007 is triggered when list items are not indented by the configured number of spaces (default: 2), but in this case the linter expects 0-indent while the actual is 2. Since this tool list is nested within a
<details>tag for visual readability, you have two options:
- Remove the leading 2-space indentation to comply with the linter defaults (list would start flush-left within the details tag).
- Configure the linter to allow indented lists via
.markdownlint.jsonusing thestart_indentedparameter or adjust the indent rule configuration.Choose the approach that aligns with your project's markdown style guide.
- <details open> - <summary><strong> 可用工具 </strong></summary> - - 您的大语言模型可以使用以下功能： - - * `execute_menu_item`: 执行 Unity 编辑器菜单项（例如，"File/Save Project"）。 - * `manage_asset`: 执行资源操作（导入、创建、修改、删除等）。 +<details open> + <summary><strong> 可用工具 </strong></summary> + + 您的大语言模型可以使用以下功能： + +* `execute_menu_item`: 执行 Unity 编辑器菜单项（例如，"File/Save Project"）。 +* `manage_asset`: 执行资源操作（导入、创建、修改、删除等）。 * `manage_editor`: 控制和查询编辑器的状态和设置。 - * `manage_gameobject`: 管理游戏对象：创建、修改、删除、查找和组件操作。 +* `manage_gameobject`: 管理游戏对象：创建、修改、删除、查找和组件操作。 * `manage_scene`: 管理场景（加载、保存、创建、获取层次结构等）。 - * `manage_script`: 管理 C# 脚本（创建、读取、更新、删除）。 +* `manage_script`: 管理 C# 脚本（创建、读取、更新、删除）。 * `manage_shader`: 执行着色器 CRUD 操作（创建、读取、修改、删除）。 - * `read_console`: 获取控制台消息或清除控制台。 - * `run_test`: 在 Unity 编辑器中运行测试。 +* `read_console`: 获取控制台消息或清除控制台。 +* `run_test`: 在 Unity 编辑器中运行测试。 * `apply_text_edits`: 具有前置条件哈希和原子多编辑批次的精确文本编辑。MCPForUnity/UnityMcpServer~/src/resources/layers.py (1)
6-8: Consider usingField(default_factory=dict)for the mutable default.The static analysis tool correctly flags the mutable default value. While this may work correctly if
MCPResponseis a Pydantic model, best practice is to useField(default_factory=dict)to make the intent explicit and avoid potential issues.Apply this diff:
+from pydantic import Field + class LayersResponse(MCPResponse): """Dictionary of layer indices to layer names.""" - data: dict[int, str] = {} + data: dict[int, str] = Field(default_factory=dict)Based on the static analysis hint.
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_asset_param_coercion.py (3)
10-10: Consider prefixing unused parameters with underscore.The
cmdandkwargsparameters are unused in the mock function. While acceptable in test mocks, you can silence the linter warning by prefixing them with underscores.Apply this diff:
- async def fake_async_send(cmd, params, **kwargs): + async def fake_async_send(_cmd, params, **_kwargs): captured["params"] = params return {"success": True, "data": {}}
26-28: Consider adding edge case tests for invalid inputs.The assertions correctly validate successful coercion. Consider adding separate test cases to verify behavior with invalid inputs like non-numeric strings, negative numbers, or zero values to ensure proper error handling.
Would you like me to generate additional test cases for edge cases?
30-34: Remove excessive trailing blank lines.The file has 5 trailing blank lines. Most style guides recommend ending with a single newline or none.
MCPForUnity/UnityMcpServer~/src/server.py (1)
175-203: Well-structured guidance for MCP clients.The refactored instructions section is clear, concise, and actionable. The breakdown into focused sections (Resources vs Tools, Script Management, Scene Setup, etc.) makes it easy for MCP clients to understand key workflows and best practices.
Consider renaming the "Important Workflows:" heading on line 177 to something more accurate like "Key Guidelines:" or "Essential Concepts:", since the content covers general guidelines and concepts rather than specific workflows.
MCPForUnity/UnityMcpServer~/src/resources/active_tool.py (1)
7-11: LGTM! Consider centralizing Vector3 if reused.The Vector3 model is straightforward and correctly implemented. If this type is used across multiple resources, consider moving it to a shared models module to avoid duplication.
📜 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 (25)
.github/workflows/python-tests.yml(1 hunks).github/workflows/unity-tests.yml(1 hunks)MCPForUnity/UnityMcpServer~/src/pyproject.toml(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/active_tool.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/editor_state.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/layers.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/menu_items.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/prefab_stage.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/project_info.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/selection.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/tags.py(1 hunks)MCPForUnity/UnityMcpServer~/src/resources/tests.py(2 hunks)MCPForUnity/UnityMcpServer~/src/resources/windows.py(1 hunks)MCPForUnity/UnityMcpServer~/src/server.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/conftest.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_instance_routing_comprehensive.py(0 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_instance_targeting_resolution.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_asset_json_parsing.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_asset_param_coercion.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tests/integration/test_script_tools.py(1 hunks)MCPForUnity/UnityMcpServer~/src/tools/run_tests.py(1 hunks)README-zh.md(1 hunks)README.md(1 hunks)Server/server.py(4 hunks)Server/tools/run_tests.py(1 hunks)
💤 Files with no reviewable changes (1)
- MCPForUnity/UnityMcpServer~/src/tests/integration/test_instance_routing_comprehensive.py
🚧 Files skipped from review as they are similar to previous changes (4)
- MCPForUnity/UnityMcpServer~/src/resources/tags.py
- MCPForUnity/UnityMcpServer~/src/tools/run_tests.py
- .github/workflows/python-tests.yml
- MCPForUnity/UnityMcpServer~/src/pyproject.toml
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-13T13:41:00.086Z
Learnt from: JohanHoltby
Repo: CoplayDev/unity-mcp PR: 309
File: MCPForUnity/Editor/Helpers/ServerInstaller.cs:478-508
Timestamp: 2025-10-13T13:41:00.086Z
Learning: In the MCPForUnityTools feature (MCPForUnity/Editor/Helpers/ServerInstaller.cs), the design intentionally forces users to have only one .py file per MCPForUnityTools folder to keep file tracking simple. Package-style tools (subdirectories with __init__.py) are not supported.
Applied to files:
MCPForUnity/UnityMcpServer~/src/resources/project_info.pyMCPForUnity/UnityMcpServer~/src/tests/integration/test_script_tools.pyServer/server.pyMCPForUnity/UnityMcpServer~/src/tests/integration/conftest.pyMCPForUnity/UnityMcpServer~/src/server.pyMCPForUnity/UnityMcpServer~/src/resources/active_tool.py
📚 Learning: 2025-10-03T22:11:46.002Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 301
File: docs/CUSTOM_TOOLS.md:54-62
Timestamp: 2025-10-03T22:11:46.002Z
Learning: In Unity MCP, the `description` parameter in the `mcp_for_unity_tool` decorator is technically optional but should always be included as a best practice. Without it, there's a higher chance that MCP clients will not parse the tool correctly. All Unity MCP tools should include the description in the decorator for compatibility.
Applied to files:
Server/server.pyMCPForUnity/UnityMcpServer~/src/server.py
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Applied to files:
Server/server.pyMCPForUnity/UnityMcpServer~/src/server.py
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
Applied to files:
MCPForUnity/UnityMcpServer~/src/server.py
📚 Learning: 2025-09-04T01:01:11.927Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 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:
MCPForUnity/UnityMcpServer~/src/server.py
🧬 Code graph analysis (8)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_asset_json_parsing.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(16-55)
Server/tools/run_tests.py (1)
MCPForUnity/UnityMcpServer~/src/tools/run_tests.py (1)
RunTestsResponse(38-39)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_script_tools.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(16-55)
Server/server.py (3)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (2)
info(36-37)warning(39-40)MCPForUnity/UnityMcpServer~/src/unity_connection.py (1)
get_connection(567-607)Server/unity_connection.py (1)
get_connection(567-607)
MCPForUnity/UnityMcpServer~/src/tests/integration/conftest.py (1)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_telemetry_subaction.py (1)
_noop(19-20)
MCPForUnity/UnityMcpServer~/src/resources/tests.py (2)
Server/resources/tests.py (2)
get_tests(24-34)GetTestsResponse(19-20)MCPForUnity/UnityMcpServer~/src/models.py (1)
MCPResponse(6-10)
MCPForUnity/UnityMcpServer~/src/resources/menu_items.py (2)
Server/resources/menu_items.py (2)
get_menu_items(18-33)GetMenuItemsResponse(9-10)MCPForUnity/UnityMcpServer~/src/models.py (1)
MCPResponse(6-10)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_asset_param_coercion.py (2)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_helpers.py (1)
DummyContext(16-55)MCPForUnity/UnityMcpServer~/src/tests/integration/test_script_tools.py (1)
run(166-177)
🪛 markdownlint-cli2 (0.18.1)
README-zh.md
43-43: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
44-44: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
45-45: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
46-46: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
47-47: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
48-48: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
49-49: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
README.md
43-43: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
44-44: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
45-45: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
46-46: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
47-47: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
48-48: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
49-49: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🪛 Ruff (0.14.3)
MCPForUnity/UnityMcpServer~/src/resources/layers.py
8-8: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
MCPForUnity/UnityMcpServer~/src/tests/integration/conftest.py
30-30: Unused function argument: dargs
(ARG001)
30-30: Unused function argument: dkwargs
(ARG001)
MCPForUnity/UnityMcpServer~/src/resources/windows.py
26-26: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_asset_param_coercion.py
10-10: Unused function argument: cmd
(ARG001)
10-10: Unused function argument: kwargs
(ARG001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test in editmode
🔇 Additional comments (34)
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_asset_json_parsing.py (1)
7-7: LGTM: Clean refactor to relative import.The change from absolute to relative import for
DummyContextfollows Python best practices for package-local imports and aligns with the broader test refactoring pattern in this PR.MCPForUnity/UnityMcpServer~/src/tests/integration/test_instance_targeting_resolution.py (3)
1-1: LGTM! Clean import refactoring.Replacing path manipulation with a relative import is the right approach and follows Python packaging best practices.
4-44: Well-structured integration test.The test clearly verifies session-scoped instance resolution with good arrange/act/assert separation and comprehensive assertions.
46-79: Good coverage of the default case.This test effectively verifies fallback behavior when no active instance is configured, ensuring the tool handles both scenarios correctly.
MCPForUnity/UnityMcpServer~/src/resources/layers.py (1)
11-19: LGTM! Clean and consistent resource implementation.The implementation follows the established pattern for Unity resources. The URI scheme, description, and error handling are appropriate. The description correctly references Unity's 32-layer limit (indices 0-31) and helpfully points users to related tools.
MCPForUnity/UnityMcpServer~/src/tests/integration/test_manage_asset_param_coercion.py (2)
1-4: LGTM!The imports are minimal and necessary for the test.
16-24: LGTM!The test execution correctly validates parameter coercion by passing string values for
page_sizeandpage_number.Server/server.py (1)
111-112: LGTM! Log formatting improves readability.The log message line-wrapping changes are stylistically clean and follow Python conventions for handling long lines. No semantic changes to the logged content.
Also applies to: 117-118, 131-132, 251-252
MCPForUnity/UnityMcpServer~/src/tests/integration/test_script_tools.py (1)
4-4: LGTM! Clean refactor to relative import.The change from absolute to relative import is appropriate for package-internal test helpers and aligns with the test infrastructure reorganization.
MCPForUnity/UnityMcpServer~/src/tests/integration/conftest.py (4)
1-15: LGTM! Good test isolation setup.The defensive approach of setting multiple telemetry-related environment variables ensures telemetry is properly disabled during testing. Clear documentation explains the test context and execution instructions.
17-27: LGTM! Appropriate telemetry stub.The stub provides all necessary no-op implementations to prevent file I/O during test imports. Using
setdefaultcorrectly avoids overwriting any existing module registration.
29-35: LGTM! Stub decorator correctly designed.The
telemetry_tooldecorator intentionally accepts but doesn't use arguments - this is correct for a no-op stub that matches the real decorator's signature without performing any operations. The static analysis warnings about unused arguments are false positives in this context.
37-66: LGTM! Proper module hierarchy for fastmcp stubs.The stub module structure correctly mirrors the real fastmcp package hierarchy. The minimal
_DummyContextstub here serves a different purpose than the fullDummyContextmock in test_helpers.py - the former enables imports while the latter provides actual test functionality. The naming distinction (leading underscore) appropriately reflects this difference.MCPForUnity/UnityMcpServer~/src/resources/menu_items.py (1)
18-18: LGTM! Improved type accuracy.The updated return type annotation correctly reflects the runtime behavior at line 33, where the function can return either a
GetMenuItemsResponse(when the response is a dict) or anMCPResponse(for error cases). This aligns with the broader pattern applied across multiple resource endpoints mentioned in the PR summary.MCPForUnity/UnityMcpServer~/src/resources/tests.py (1)
24-24: LGTM: Return type annotations correctly reflect runtime behavior.The addition of
| MCPResponseto the return types aligns with the actual runtime behavior where these functions can return either a typed response or anMCPResponseobject on error (lines 34, 54). This mirrors the pattern used by other MCP resource endpoints in the codebase.Also applies to: 41-41
Server/tools/run_tests.py (2)
49-51: LGTM: Type annotations correctly tightened.The changes improve type precision:
timeout_secondsannotation correctly separates the base type (int | str) from the nullable wrapper (| None)- Return type
RunTestsResponseaccurately reflects the actual return value (line 77)
47-48: Remove this review comment—the concern is based on a false premise.The script output confirms that old lowercase values (
"edit","play") are not used for the test mode parameter anywhere in the codebase. The matches found ("action": "edit", case "play") belong to entirely different parameters in unrelated functions (manage_editor,script_apply_edits). The test mode parameter is consistently updated across all files with the new title case values ("EditMode","PlayMode"): inrun_tests.py,tests.py, and the corresponding C# implementations (RunTests.cs,GetTests.cs).Likely an incorrect or invalid review comment.
MCPForUnity/UnityMcpServer~/src/resources/windows.py (2)
7-26: LGTM! Clean data model design.The Pydantic models are well-structured with appropriate defaults and clear documentation. The static analysis hint (RUF012) about the mutable default on line 26 is a false positive—Pydantic's BaseModel handles mutable defaults correctly by creating new instances for each object.
29-37: LGTM! Resource endpoint follows established patterns.The decorator configuration and response handling logic are correct and consistent with other MCP resources in the codebase.
MCPForUnity/UnityMcpServer~/src/resources/editor_state.py (2)
7-21: LGTM! Comprehensive editor state model.The data models properly capture editor runtime state with appropriate types and defaults. The nullable
activeObjectNamefield correctly handles cases where no object is selected.
24-32: LGTM! Well-documented dynamic resource.The resource description appropriately indicates this is dynamic state that should be refreshed frequently, which helps users understand the data's volatility.
MCPForUnity/UnityMcpServer~/src/resources/active_tool.py (2)
14-26: LGTM! Comprehensive tool state model.The ActiveToolData model properly captures editor tool state and transform handle information with appropriate defaults.
29-37: LGTM! Clear and helpful resource definition.The resource is well-configured with concrete examples in the description that help users understand what data to expect.
MCPForUnity/UnityMcpServer~/src/resources/project_info.py (2)
7-18: LGTM! Well-defined project configuration model.The data models appropriately capture static project information with sensible string fields and defaults.
21-29: LGTM! Appropriately categorized static resource.The resource correctly uses the
unity://project/namespace and the description appropriately indicates the data is static, which helps users understand caching behavior.MCPForUnity/UnityMcpServer~/src/resources/prefab_stage.py (4)
1-4: LGTM!Imports are clean and follow the established patterns in the codebase.
7-13: LGTM!The model structure is sound with sensible defaults. The camelCase field naming aligns with Unity/C# conventions for seamless interoperability.
16-18: LGTM!Clean response wrapper following the established MCPResponse pattern.
26-29: Code structure is correct. Error handling is appropriately layered.Schema verification confirms the C# response structure (
{success, data: {isOpen, assetPath}}) aligns with the Python models. Pydantic automatically converts the nesteddatadict toPrefabStageData, and theisinstancecheck on line 29 properly distinguishes between success responses (dict) and error responses (MCPResponse). The C# handler's try-catch block provides the error handling layer, and allPrefabStageDatafields have safe defaults for optional values.MCPForUnity/UnityMcpServer~/src/resources/selection.py (5)
1-4: LGTM!The imports are clean and follow the project's conventions for MCP resource modules.
32-34: LGTM!The response model follows the established MCPResponse pattern used throughout the codebase.
7-17: ****The field difference between the two models is intentional and correctly aligns with the underlying C# implementation. The C#
Selection.csfile (lines 27-39) shows thatSelection.objectsincludes atypefield (line 29), whileSelection.gameObjectsdoes not. The Python models accurately mirror this distinction—SelectionObjectInfocorresponds to general objects with atypefield, andSelectionGameObjectInfocorresponds to GameObjects without one. This reflects Unity's API design where GameObjects have an implicit type.Likely an incorrect or invalid review comment.
37-45: Verify error handling strategy for model validation at endpoint level.The review comment's concern is technically valid. While
async_send_command_with_retrycatches exceptions at the communication layer, anyValidationErrorfromSelectionResponse(**response)would occur after that try-except block and wouldn't be caught. This pattern is consistent across all 9+ endpoints in the codebase, suggesting it may be intentional.However, without seeing explicit middleware or MCP framework exception handlers, the error handling strategy at the endpoint level is unclear. Confirm whether:
- The MCP framework implicitly handles unhandled exceptions from resource functions
- Unity responses are guaranteed to contain required fields for
SelectionResponseconstruction- Or if explicit error handling should be added (e.g.,
try: SelectionResponse(**response) except ValidationError: return MCPResponse(success=False, error=...))
20-29: No changes needed—the default value is correct.Selection.activeInstanceID returns 0 when nothing is selected, which matches the Python model's default of
activeInstanceID: int = 0. The C# code directly assigns fromUnityEditor.Selection.activeInstanceID, confirming this behavior is intentional and correct. The field typeint(non-nullable) is appropriate since0is a valid, defined sentinel value rather than representing absence.
- Updated all resource handlers to accept Context parameter for Unity instance routing - Replaced direct async_send_command_with_retry calls with async_send_with_unity_instance wrapper - Added imports for get_unity_instance_from_context and async_send_with_unity_instance helpers
- Added new tools: manage_prefabs, create_script, delete_script, get_sha - Added new resources: editor state, windows, project info, layers, and tags - Clarified manage_script as compatibility router with recommendation to use newer edit tools - Fixed run_test to run_tests for consistency
- Changed function signature from synchronous to async - Added await keywords to ctx.info() and ctx.error() calls to properly handle async context methods
The initial goal of these changes were to add a single domain reload functionality, but that didn't work out. I did some planned movement to resources detailed in #287. I also re-worked how we run tests. They're part of the backend code and they run every time we push code up. This will ensure that all contributors don't unintentionally break things. Similarly, Unity tests are run as well on every push
Summary by CodeRabbit
New Features
Improvements
Breaking Changes